-
Notifications
You must be signed in to change notification settings - Fork 2.5k
feat: optional logger #3618
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
feat: optional logger #3618
Conversation
This commit introduces an optional logger parameter to various structs. This enhancement allows users to provide custom logging implementations.
An issue will remain for direct calls to internal.Logger.Printf, as the call depth cannot be adjusted there without changing the function signature.
cb7a509 to
8bf1376
Compare
4b03ae0 to
f2406a6
Compare
| legacyLoggerWithLevel.Errorf(ctx, format, v...) | ||
| return | ||
| } | ||
| cl.logger.ErrorContext(ctx, format, v...) |
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.
Use ErrorContext etc. will cause inaccurate source log attr.
Should use cl.logger.Handler().Handleinstead when wrapping, see slog godoc Example (Wrapping).
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.
@ccoVeille Maybe you would like to check this.
@wzy9607 thank you for pointing this out
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 feel like there is an error here, yes
it should be
cl.logger.ErrorContext(cl.printfToStructured(ctx, format, v...))I will fix it.
I created the printToStructured to be able to handle the right format
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 have fixed the issue in #3622
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.
maybe the fix was lost when you force pushed? Can we just add a test with slog that it will work as expected?
|
I feel like this PR can be closed now, as #3622 replaces it |
|
closed in favour of #3622 |
Continue the work on #3560 opened by @ccoVeille