Added logger abstraction with default implementation #134
Conversation
There was a problem hiding this comment.
Left some inline comments. Furthermore:
- please squash redundant commits, such as (but not limited to)
feat(logger): fixed logger arguments types - for the types/interfaces/enums I think they should be combined in
types.tsinstead of splitting many tiny definitions across many files - finally, I think the enum method for defining channels is kinda limiting. For example, when a plugin wants to log something, we should be able to see what plugin it is, or better yet, which file is logging! In other words, this should be easily extendible by different subparts of the SDK and different plugins and even user-defined plugins of course!
Please actually compare other libs and logging methods, as I think log4js seems a bit ummm old/hacky. A quick look at it shows that it was started for the browser and was ported to node. I have personally used winston before, and I think pino was recommended as well, please compare them first
| @@ -34,6 +35,7 @@ | |||
| "@typescript-eslint/eslint-plugin": "^2.20.0", | |||
| "@typescript-eslint/parser": "^1.5.0", | |||
| "@zerollup/ts-transform-paths": "^1.7.17", | |||
| "app-root-path": "^3.0.0", | |||
There was a problem hiding this comment.
To define path to the default place to save logs here
| @@ -45,11 +47,12 @@ | |||
| "fs-extra": "^8.1.0", | |||
| "jest": "^26.4.2", | |||
| "jolocom-lib": "^5.2.1", | |||
| "log4js": "^6.3.0", | |||
There was a problem hiding this comment.
have you compared this against other options for logging libraries? Why did you choose log4js in specific?
serg-temchenko
force-pushed
the
feat/logger
branch
from
f3e5283 to
713b620
Compare
serg-temchenko
force-pushed
the
feat/logger
branch
from
713b620 to
bed558a
Compare
About 'squash' - IMO it's ok to have full history of changes, but ok, I squashed those commits... About ' types/interfaces/enums' in one file - not just the logic must be decoupled and encapsulated, but the structure as well, I don't see some profit to put everything in one file (like classes/types/interfaces etc.) it's definitely the 'bad smell' which brings confusions and bad structure in overall - but ok, I put everything in one file... About 'enum method for defining channels' - this is the definition of predefined configuration where we already know about the channels and the type of logs to store, additionally I added 'customConfig' property to be configured from the client app so when there will be defined 'service management system' all we need is just iterate over all 'categories' and create instances of logger channels objects and request it from any part of the app/plugin and use... About 'which file is logging' - I assume the channel definition in log record is more than enough to understand which plugin/part of the code relates to this log.... in case if we logging errors - stack trace could be taken from catched error. About 'comparing libs' - not sure that I understand argue about 'old/hacky' (winston actually older than log4js). IMO defining/discussing/suggesting of the lib to use must be done on the planing stage but not when implementation is done if there is some preference... but about comparing libs I answered here, please take a look. |
|
hey @serg-temchenko sorry missed this comment!
I think we need to revist this conversation within the team, as it results in a lot of git log chatter as PRs go through the review cycle. Best case condition is squash the commits and keep the relevant parts of history in the squash commit message body.
I think it's also a matter of convenience, balance between lots of small files vs. a few giant files :D
Awesome that should work
I had quickly gone through some of the log4js code and saw that it's this ancient originally browser-based thing, and thought there are definitely better modern alternatives, unfortunately I'm not up-to-date about logging frameworks :D All in all though the important bit is having a logger interface, and 'defaultLogger' is currently just the "log4js" implementation. I think it should use a mechanism like the storage plugin, be configurable during SDK initialization, and the 'defaultLogger' can be renamed to |
Related to PR #129
Work which already done with defining places to log in Agent and in InteractionManager/Interaction currently stashed and will wait that moment when service manager will be integrated and
DefaultLoggerinstance will be available to use...