Skip to content
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

Open
4 of 7 tasks
abhinav opened this issue Aug 23, 2023 · 9 comments
Open
4 of 7 tasks

zapslog stabilization tracking issue #1333

abhinav opened this issue Aug 23, 2023 · 9 comments

Comments

@abhinav
Copy link
Collaborator

abhinav commented Aug 23, 2023

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:

  • Support for a custom mapping of slog.Level to zap.Level
abhinav pushed a commit that referenced this issue Sep 6, 2023
Improving test coverage on slog package.

Refs #1333
abhinav added a commit that referenced this issue Feb 5, 2024
…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]>
@arukiidou
Copy link
Contributor

I would like to contribute zapslog GA.

zapslog: Add a zapslog.Core that writes Zap logs to a slog.Handler
We may want to rename zapslog.Options to HandlerOptions when we do this.

Can you tell us more about this?

@abhinav
Copy link
Collaborator Author

abhinav commented Feb 6, 2024

I would like to contribute zapslog GA.

zapslog: Add a zapslog.Core that writes Zap logs to a slog.Handler
We may want to rename zapslog.Options to HandlerOptions when we do this.

Can you tell us more about this?

Hello! Yes!

Okay, so right now, zapslog takes a Zap core, and returns a slog handler. func NewHandler(zapcore.Core) slog.Handler.
That's for applications that are already using Zap, and want to connect to a library that's using slog, to have the library feed their slog logs into their Zap 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, func NewCore(slog.Handler) zapcore.Core.

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 Option type to HandlerOption because we'd need another Option type (CoreOption) for the NewCore function.

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.

@arukiidou
Copy link
Contributor

arukiidou commented Feb 6, 2024

That way we could move zapslog to the root module without having to change anything, and easier to migrate from exp for users.

@abhinav
Copy link
Collaborator Author

abhinav commented Feb 6, 2024

Yeah, I think following the rename, we can defer adding NewCore to when we need it.
That PR looks good! Thanks!

abhinav pushed a commit that referenced this issue Feb 6, 2024
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]>
@arukiidou

This comment was marked as resolved.

@arukiidou
Copy link
Contributor

arukiidou commented Feb 8, 2024

@mblaschke
Copy link

any estimation when this will be finished?

@mblaschke
Copy link

any progress?

@hikarukin
Copy link

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

4 participants