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 inline the Attrs of a group with an empty key #1402

Closed
arukiidou opened this issue Jan 27, 2024 · 2 comments
Closed
Assignees

Comments

@arukiidou
Copy link
Contributor

arukiidou commented Jan 27, 2024

Describe the bug

slogtest cause errors:

missing key "c": a Handler should inline the Attrs of a group with an empty key (zap/exp/zapslog/slogtest.go:126)

actual logs:

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

To Reproduce
run testing/slogtest

f: func(l *slog.Logger) {
	l.Info("msg", "a", "b", slog.Group("", slog.String("c", "d")), "e", "f")
},

Expected behavior

so that:

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

Additional context
none.

@arukiidou
Copy link
Contributor Author

arukiidou commented Jan 30, 2024

I think that might fix with the following approach

case slog.KindGroup:
return zap.Object(attr.Key, groupObject(attr.Value.Group()))

to

	case slog.KindGroup:
		if attr.Key == "" {
			return zap.Inline(groupObject(attr.Value.Group()))
		}
		return zap.Object(attr.Key, groupObject(attr.Value.Group()))

What do you think? @abhinav @sywhang

arukiidou added a commit to arukiidou/zap that referenced this issue Feb 2, 2024
Signed-off-by: junya koyama <arukiidou@yahoo.co.jp>
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
Thanks, @arukiidou

@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