-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
zapslog stabilization tracking issue #1333
Comments
Improving test coverage on slog package. Refs #1333
…oup. (#1408) This change adds a test based on testing/slogtest that verifies compliance with the slog handler contract (a draft of this was available in #1335), and fixes all resulting issues. The two remaining issues were: - `Group("", attrs)` should inline the new fields instead of creating a group with an empty name. This was fixed with the use of `zap.Inline`. - Groups without any attributes should not be created. That is, `logger.WithGroup("foo").Info("bar")` should not create an empty "foo" namespace (`"foo": {}`). This was fixed by keeping track of unapplied groups and applying them the first time a field is serialized. Following this change, slogtest passes as expected. Refs #1333 Resolves #1334, #1401, #1402 Supersedes #1263, #1335 ### TESTS - passed. arukiidou#1 - This also works in Go 1.22 --------- Signed-off-by: junya koyama <[email protected]> Co-authored-by: Abhinav Gupta <[email protected]>
I would like to contribute zapslog GA.
Can you tell us more about this? |
Hello! Yes! Okay, so right now, zapslog takes a Zap core, and returns a slog handler. The proposal was for the reverse: your application is using slog, and you want to use a library that uses Zap, so you want Zap to feed into your slog as the backend. Basically, Thinking about it out loud, though I don't know if this is actually necessary. @mway @prashantv @sywhang @r-hang What do y'all think? On the second point: if this was added, then we'd want to rename the One thing we could do is defer implementing NewCore until the need presents itself, but rename Option to HandlerOption anyway so that if/when that happens, we have the "option" name free. |
That way we could move zapslog to the root module without having to change anything, and easier to migrate from exp for users. |
Yeah, I think following the rename, we can defer adding |
Refs #1333 Rename `Option` to `HandlerOption` to avoid future conflicts with [zap.Option](https://pkg.go.dev/go.uber.org/zap#Option). There is no change in functionality. This is a breaking change to zapslog, but the package is currently marked experimental. This will allow us to stabilize it. --------- Signed-off-by: junya koyama <[email protected]>
This comment was marked as resolved.
This comment was marked as resolved.
|
any estimation when this will be finished? |
any progress? |
Hi there, just a quick question. Were any performance tests done with zapslog to see if there is any performance degradation? I know that at least some initial tests done with slog and zerolog showed quite a dramatic impact due to support for slogHandler. |
This issue tracks everything we need to do before zapslog can be moved to the main go.uber.org/zap module.
Future features: Things we can add to it without risk after the initial availability:
The text was updated successfully, but these errors were encountered: