jolocom-sdk
jolocom-sdk copied to clipboard
Added logger abstraction with default implementation
- Added logger abstraction
- Added default logger implementation
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 DefaultLogger
instance will be available to use...
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.ts
instead 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 usedwinston
before, and I thinkpino
was recommended as well, please compare them first
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!
About 'squash' - IMO it's ok to have full history of changes, but ok, I squashed those commits...
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.
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...
I think it's also a matter of convenience, balance between lots of small files vs. a few giant files :D
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...
Awesome that should work
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.
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 looking at the article you shared, consistency wise bunyan looks interesting because of the constant 0% drops
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 JolocomLoggerLog4js
in a separate package. It is very useful to maintain the structure so it remains easy to transfer to the monorepo