-
Notifications
You must be signed in to change notification settings - Fork 21
first pass at implementing a zap log handler for slog #1433
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: main
Are you sure you want to change the base?
Conversation
pkg/logger/v2/logger.go
Outdated
@@ -0,0 +1,169 @@ | |||
package v2 |
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.
package v2 | |
package logger |
Level slog.Leveler | ||
|
||
// (optional) Logger helps convert existing zap.Logger to slog.Logger. | ||
Logger *zap.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 think we should target a zapcore.Core
, no?
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 only reason I used this was to allow users to drop in an existing logger. I thought it might make the conversion easier.
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.
Which users though? I think we want to hide these details as much as possible from typical logging users. The more interesting "user" is the core logger package, which needs to use this internally to stay aligned.
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 played around with using a zapcore.Core
instead, but I needed to work around errors. If we use zap.Logger
, we can directly pass the logger from v1 over to v2 and if it's nil we can grab a global logger with zap.L()
. This allows us to return a slog.Logger
without an error and cleans up the interface a little.
We can use a zapcore.Core
instance instead, but our interface would then be New() (*slog.Logger, error)
. Maybe that's not so bad, but it would be nice to be able to drop the error handling if possible.
pkg/logger/v2/logger.go
Outdated
if hndlr, ok := handler.(ZapHandler); ok { | ||
return slog.New(hndlr.WithName(name)) | ||
} | ||
|
||
return logger | ||
} | ||
|
||
type ZapHandler interface { | ||
WithName(string) slog.Handler | ||
} |
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 think we could inline this and maybe keep it unexported until there is a need to expose it? I was worried more about multiple internal types than external.
if hndlr, ok := handler.(ZapHandler); ok { | |
return slog.New(hndlr.WithName(name)) | |
} | |
return logger | |
} | |
type ZapHandler interface { | |
WithName(string) slog.Handler | |
} | |
if hndlr, ok := handler.(interface { withName(string) slog.Handler }); ok { | |
return slog.New(hndlr.WithName(name)) | |
} | |
return 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.
That works. I had it this way to run some tests, but it's not necessary.
5af1c5a
to
3e932a4
Compare
Requires
Supports