[Feature] Add experiment zapslog package to integrate with slog
#1246
Conversation
Signed-off-by: Jian Zeng <[email protected]>
|
|
|
/cc @abhinav PTAL |
exp/zapslog/slog.go
Outdated
| return convertAttrToField(slog.Attr{ | ||
| Key: attr.Key, | ||
| Value: attr.Value.Resolve(), | ||
| }) |
There was a problem hiding this comment.
This will resolve the LogValuer eagerly, which will hurt anyone using LogValuer to defer expensive operations.
Ideally, we want to use a wrapper struct that calls LogValue() at serialization time,
but that may not be 100% straightforward because a LogValuer doesn't know if it's a group or a primitive type until calling LogValue(),
whereas Zap wants to know that in advance.
This may require a lazy field-equivalent in Zap.
We can defer designing that to a follow-up.
CC @mway @prashantv @sywhang
There was a problem hiding this comment.
+1 on a adding a lazy-field equivalent in zap separately.
Signed-off-by: Jian Zeng <[email protected]>
Signed-off-by: Jian Zeng <[email protected]>
Signed-off-by: Jian Zeng <[email protected]>
We don't need a extra struct to save the Group Attr, only a list of Attr is sufficient. Signed-off-by: Jian Zeng <[email protected]>
Signed-off-by: Jian Zeng <[email protected]>
exp/zapslog/slog.go
Outdated
| ent := zapcore.Entry{ | ||
| Level: convertSlogLevel(record.Level), | ||
| Time: record.Time, | ||
| Message: record.Message, | ||
| // FIXME: do we need to set the following fields? | ||
| // LoggerName: | ||
| // Caller: | ||
| // Stack: | ||
| } |
There was a problem hiding this comment.
cc @abhinav . Do we need to complete the Entry?
There was a problem hiding this comment.
Those three fields are optional. We can start without them.
I can see Caller and LoggerName being filled in the future.
(We could probably fill the Caller from Record.PC,
but doing that cheaply adds some complexity so we can do without for now.)
However, this does bring up the need for customization.
A plain NewHandler may not be sufficient for that.
I've pushed a change to your branch with the following new API:
type HandlerOptions struct{ LoggerName string }
func (HandlerOptions) New(zapcore.Core) *HandlerThe func NewHandler(..) just calls this now to get default options.
This mirrors the JSONHandler, NewJSONHandler, HandlerOptions setup.
This leaves us room for future customization llke:
opting into adding a caller name, custom level mapping, etc.
There was a problem hiding this comment.
Thank you! The HandlerOptions is commonly used for customization, though I thought we might want to consistently follow the functional options pattern in zap.
I can see Caller and LoggerName being filled in the future.
(We could probably fill the Caller from Record.PC,
After some trial & error, I found I could add the Caller in this PR as well, plz see the commit 1ae5945
Codecov Report
@@ Coverage Diff @@
## master #1246 +/- ##
=======================================
Coverage 98.23% 98.23%
=======================================
Files 49 49
Lines 3231 3231
=======================================
Hits 3174 3174
Misses 49 49
Partials 8 8 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
Signed-off-by: Jian Zeng <[email protected]>
Signed-off-by: Jian Zeng <[email protected]>
Signed-off-by: Jian Zeng <[email protected]>
Signed-off-by: Jian Zeng <[email protected]>
|
Thinking about it more, as long as we address the import name comment, we can worry about the other nits later. |
Signed-off-by: Jian Zeng <[email protected]>
Signed-off-by: Jian Zeng <[email protected]>
|
@mway Hi I have addressed your comments, PTAL |
Ref #1179
This PR provides a implementation of
slog.Handlerin a separate experimentexp/zapslogmodule as suggested by @abhinav.