-
Notifications
You must be signed in to change notification settings - Fork 2.5k
feat: add optional logger wherever possible #3622
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
base: master
Are you sure you want to change the base?
Conversation
03ddd12 to
fa95d65
Compare
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 following Jit checks failed to run:
- secret-detection-trufflehog
- static-code-analysis-semgrep-pro
#jit_bypass_commit in this PR to bypass, Jit Admin privileges required.
More info in the Jit platform.
93fa6c4 to
c963331
Compare
|
I apparently failed to keep your GPG signature @ndyakov, it sounds logic as I used history rewritting to fix an issue in one of the commit I did earlier. Feel free to pull, sign your commits, and push again. My fork is now all set for maintainers edits |
ndyakov
left a comment
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.
Overall looks good to me, can we just make sure this one is addressed (preferably add a simple test):
#3618 (comment)
Here the difference I applied to an old commit. func (cl *LoggerWrapper) Errorf(ctx context.Context, format string, v ...any) {
if cl == nil || cl.logger == nil {
legacyLoggerWithLevel.Errorf(ctx, format, v...)
return
}
- cl.logger.ErrorContext(ctx, format, v...)
+ cl.logger.ErrorContext(cl.printfToStructured(ctx, format, v...))
}I can add a test, but adding a test about a logger is never fun. Maybe it can be done in a separate PR. I'm suggesting that, but if you are OK to wait for me to add the tests a few more days, maybe a week. I can do it. |
|
@ccoVeille No problem, I will prepare a beta without the logger, you can write tests and resolve conflicts in the next week and then we will include it. |
c963331 to
1f95e53
Compare
|
@ndyakov please review again. I feel the system is better now.
EDIT: this is needed if a LoggerWrapper is provided directly |
1f95e53 to
3fe1541
Compare
|
Apparently, I broke something. It's too late here, but the code is close to what is needed. |
|
I feel like I have to move out the change about fixing the panic and sprintf issue |
Moved to |
3fe1541 to
a5a1f38
Compare
|
I squashed everything in one commit even yours @ndyakov. I was done to deal with conflicts when someone was merging something 🙄 |
|
The PR is still in DRAFT mode because I have no clear status on what is the minimal Go version for go-redis As stated in #3631, right now the Except that the code can be reviewed. But if the minimal version is moved to something at least Go 1.21, my implementation could be simplified a lot as I would be able to delete everything about |
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. While most of the changes comes from ccoVeille's work, Nedyalko Dyakov made some changes in other files to ensure the legacy logger is replaced with the new logger. Co-authored-by: Nedyalko Dyakov <[email protected]>
a5a1f38 to
f92e566
Compare
This commit introduces an optional logger parameter to various structs.
This enhancement allows users to provide custom logging implementations.
Fixes #3558
Note
This is a follow-up of #3560 that was merged in a branch (not
master) and #3618 that was updated by @ndyakov to add changes on it.The MR is now ready to be merged.