-
Notifications
You must be signed in to change notification settings - Fork 36
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: resolved issue in setLogger function with winston logger #1522
Conversation
refs INSTA-24448
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pre-approving
@@ -174,6 +174,10 @@ function setLoggerLevel(_logger, level) { | |||
*/ | |||
function isPinoLogger(_logger) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’m still a bit concerned about this function. Would suggest revisiting this logic to ensure it accurately targets Pino logger without potentially matching other logging libraries that have similar properties.
typeof _logger === 'object' && | ||
typeof _logger.child === 'function' && | ||
typeof _logger.level === 'string' && | ||
typeof _logger.bindings === 'function' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The bindings method is unique to Pino .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For me still its too generic. Other logging libraries might also implement similar methods. Otherwise, if you’re confident that no other logger implements it in the same way as Pino, then it could be fine for now.
refs INSTA-24448
refs #1521