Refactor logging #61
Refactor logging #61
Conversation
|
This is fine for refactoring, but I don't see an API for initializing Nodekit with another logger. |
| } | ||
| } | ||
|
|
||
| trace(message: string): void; |
There was a problem hiding this comment.
do we use trace and debug somewhere?
There was a problem hiding this comment.
#60 - will be used here. We're gonna have to merge this one first and rebase the opentracing one on top of this.
|
|
||
| export interface NodekitLogger { | ||
| info(message: string): void; | ||
| info(extra: Dict | undefined, message: string): void; |
There was a problem hiding this comment.
Shouldn't message be first argument (for consistency with context class loggers)? Or it would be better to be consistent with pino logger class methods?
There was a problem hiding this comment.
The intention was to be consistent with pino first. I think it makes sense to do this rather than reorder the arguments of every call to the logger.
|
You probably should coordinate this PR with #60 and decide what should be merged first. |
Yea, we discussed it and agreed that this one should be merged first. |
|
@melikhov-dev I've added a config option for passing a ready-made logger and added tests for it. Please take a look again. |
|
@main-kun LGTM |
NodekitLoggerinterface to remove direct dependency onpino.Loggerand allow creating context with other loggersPinoLoggeras a default logger implementationlogWarnmethod toAppContextto support warn level logginglogErrorto matchlogWarn