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: Handler should not output groups for an empty Record #1401

Closed
arukiidou opened this issue Jan 27, 2024 · 3 comments
Closed

zapslog: Handler should not output groups for an empty Record #1401

arukiidou opened this issue Jan 27, 2024 · 3 comments
Assignees

Comments

@arukiidou
Copy link
Contributor

arukiidou commented Jan 27, 2024

Describe the bug

slogtest cause errors:

unexpected key "H": a Handler should not output groups for an empty Record (zap/exp/zapslog/slogtest.go:165)

actual logs:

zap\exp\zapslog\logger.go:130: 2024-01-28T01:08:54.927+0900	INFO	msg	{"a": "b", "G": {"c": "d", "H": {}}}

To Reproduce
run slogtest

f: func(l *slog.Logger) {
	l.With("a", "b").WithGroup("G").With("c", "d").WithGroup("H").Info("msg")
},

Expected behavior

so that:

zap\exp\zapslog\logger.go:130: 2024-01-28T01:08:54.927+0900	INFO	msg	{"a": "b", "G": {"c": "d"}}

Additional context
none.

@arukiidou

This comment was marked as outdated.

arukiidou added a commit to arukiidou/zap that referenced this issue Feb 2, 2024
Signed-off-by: junya koyama <arukiidou@yahoo.co.jp>
@arukiidou
Copy link
Contributor Author

arukiidou commented Feb 3, 2024

Solution

  • When WithGroup is called, the handler "hold" the group
    • When group has not empty, "hold" is released. following:
      • With called (except slog.Attr is empty)
      • When Log called with args.
      • When LogAttrs called (except slog.Attr is empty)
    • If not, don't output the "hold" group.

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 <arukiidou@yahoo.co.jp>
Co-authored-by: Abhinav Gupta <mail@abhinavg.net>
@abhinav
Copy link
Collaborator

abhinav commented Feb 5, 2024

Resolved by #1408

@abhinav abhinav closed this as completed Feb 5, 2024
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

3 participants