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.

Sign up for GitHub

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

Jump to bottom

zapslog stabilization tracking issue #1333

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

zapslog stabilization tracking issue #1333

abhinav opened this issue Aug 23, 2023 · 10 comments

Comments

@abhinav
Copy link
Collaborator

abhinav commented Aug 23, 2023 •
edited
Loading

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 abhinav mentioned this issue Aug 23, 2023
Open
abhinav pushed a commit that referenced this issue Sep 6, 2023
@zekth
test: improve slog coverage (#1347)
9a36792 
Improving test coverage on slog package.

Refs #1333
@dennwc dennwc mentioned this issue Jan 31, 2024
Merged
@arukiidou arukiidou mentioned this issue Feb 2, 2024
Merged
abhinav added a commit that referenced this issue Feb 5, 2024
@arukiidou @abhinav
zapslog: fix all with slogtest, support inline group, ignore empty gr…
35ded09 
…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 arukiidou mentioned this issue Feb 6, 2024
Merged
@arukiidou
Copy link
Contributor

arukiidou commented Feb 6, 2024 •
edited
Loading

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
@arukiidou
zapslog: rename Option to HandlerOption (#1411)
7db06bc 
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 arukiidou mentioned this issue Feb 7, 2024
Open
@arukiidou

This comment was marked as resolved.

Sign in to view
@arukiidou arukiidou mentioned this issue Feb 7, 2024
Draft
@arukiidou
Copy link
Contributor

arukiidou commented Feb 8, 2024 •
edited
Loading

@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.

@nick4fake
Copy link

What is the current state in terms of stability?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@abhinav @nick4fake @mblaschke @arukiidou @hikarukin