slogr: add SlogSink #211
slogr: add SlogSink #211
Conversation
|
See go-logr/zapr#60 for a proper implementation of the proposed |
There was a problem hiding this comment.
super small nits. I get it now. I wonder if we should explain somewhere why this is like this. Specifically, that Enabled is in both logr.Logger and slog.Handler interfaces. This design allows people who already have a logr.Logger to use slog-aware implementations and slog-unaware implementations in the exact same way, without sacrificing functionality.
slogr/slogr_test.go
Outdated
|
|
||
| // slogSink is only used through slog and thus doesn't need to implement the | ||
| // normal LogSink methods. | ||
| type slogSink struct { |
There was a problem hiding this comment.
Can we rename this to include "test" so it is less ambiguous wrt SlogSink
slogr/slogr.go
Outdated
| return handler | ||
| } | ||
|
|
||
| // SlogSink is an interface that a LogSink can implement to support logging |
There was a problem hiding this comment.
...is an optional interface...
There was a problem hiding this comment.
Done.
I wonder if we should explain somewhere why this is like this.
I added more comments to the SlogSink interface about this.
A LogSink may implement this additional interface if it wants to support logging through slog better. Doing so addresses several issues: - stack unwinding gets avoided in favor of logging the pre-recorded PC - proper grouping of key/value pairs via WithGroup - verbosity levels > slog.LevelInfo can be recorded - less overhead A LogSink implementation which wants to provide a `New*Handler() slog.Handler` function can either implement a second type (required because the prototype of the Enabled method conflicts) or use `logr.NewSlogHandler`. The latter is better because then conversion back-and-forth works. The overhead for the additional wrapper should be minimal.
|
LGTM |
A LogSink may implement this additional interface if it wants to support logging through slog better. Doing so addresses several issues:
A LogSink implementation which wants to provide a
New*Handler() slog.Handlerfunction can either implement a second type (required because the prototype of the Enabled method conflicts) or uselogr.NewSlogHandler. The latter is better because then conversion back-and-forth works. The overhead for the additional wrapper should be minimal.