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.Handler: Test with slogtest, fix all failures #1334

Closed
2 of 4 tasks
Tracked by #1333
abhinav opened this issue Aug 23, 2023 · 5 comments · Fixed by #1408
Closed
2 of 4 tasks
Tracked by #1333

zapslog.Handler: Test with slogtest, fix all failures #1334

abhinav opened this issue Aug 23, 2023 · 5 comments · Fixed by #1408

Comments

@abhinav
Copy link
Collaborator

abhinav commented Aug 23, 2023 •
edited

zapslog.Handler is not yet tested with slogtest on master.
We should add a test for that and address any problems that occur.

#1335 attempts to add such a test. Pending failures:
@abhinav abhinav mentioned this issue Aug 23, 2023
Open
7 tasks
@abhinav abhinav changed the title Test with slogtest, fix all failures zapslog: Test with slogtest, fix all failures Aug 23, 2023
@abhinav abhinav changed the title zapslog: Test with slogtest, fix all failures zapslog.Handler: Test with slogtest, fix all failures Aug 23, 2023
@abhinav abhinav mentioned this issue Aug 23, 2023
Closed
abhinav added a commit to abhinav/zap that referenced this issue Aug 23, 2023
@abhinav
zapslog: Test with slogtest
93b166a 
Adds a test for zapslog that verifies its behavior
with slogtest's TestHandler function.
This verifies compliance with the slog logging contract.

Resolves uber-go#1334
@abhinav abhinav mentioned this issue Aug 23, 2023
Closed
abhinav added a commit to abhinav/zap that referenced this issue Aug 23, 2023
@abhinav
zapslog: Test with slogtest
724d7fa 
Adds a test for zapslog that verifies its behavior
with slogtest's TestHandler function.
This verifies compliance with the slog logging contract.

Resolves uber-go#1334
abhinav added a commit to abhinav/zap that referenced this issue Sep 1, 2023
@abhinav
zapslog: Test with slogtest
29d3c8d 
Adds a test for zapslog that verifies its behavior
with slogtest's TestHandler function.
This verifies compliance with the slog logging contract.

Resolves uber-go#1334
@abhinav abhinav mentioned this issue Sep 1, 2023
Merged
abhinav added a commit to abhinav/zap that referenced this issue Sep 1, 2023
@abhinav
zapslog: Handler should ignore an empty Attr
910c4cd 
Per the slog.Handler contract,
handlers should not log attributes that are equal to the zero value.
This is equivalent to Zap's `zap.Skip()`.

Discovered by uber-go#1335
Refs uber-go#1334
sywhang pushed a commit that referenced this issue Sep 1, 2023
@abhinav
zapslog: Handler should ignore an empty Attr (#1344)
e9f4bda 
Per the slog.Handler contract,
handlers should not log attributes that are equal to the zero value.
This is equivalent to Zap's `zap.Skip()`.

Discovered by #1335
Refs #1334
@SoulPancake
Copy link

Can I help with this or is it done? @abhinav

@abhinav
Copy link
Collaborator Author

abhinav commented Sep 6, 2023

Hey, @SoulPancake. Thanks for your interest!
This isn't done yet. I've created #1335 with the test, but I haven't had time to fix the failures (save #1344).
Looks like @mway made an attempt to fix one of the failures in (#1263) but hasn't had time to follow up.
If you have interest in helping with this, please do go ahead.

Thanks!

@justinhwang
Copy link
Contributor

Hey @abhinav, as far as "Handler should ignore a zero Record.Time" - isn't this up to each individual Core to determine? We could update the ioCore w/ jsonEncoder (i.e. have a zero check in jsonEncoder.EncodeEntry) to be compliant which would take care of the error from #1335 but zapslog wouldn't always be compliant no?

@abhinav
Copy link
Collaborator Author

abhinav commented Oct 4, 2023

@justinhwang Yeah, that's a fair point. I think it makes sense that the official encoders that come with Zap support that behavior, and we should possibly consider documenting that encoder implementations are highly encouraged to skip writing the time for a log entry if it's a zero value.

@justinhwang justinhwang mentioned this issue Oct 4, 2023
Merged
abhinav added a commit to abhinav/zap that referenced this issue Oct 5, 2023
@abhinav
zapslog: Test with slogtest
32fc0d2 
Adds a test for zapslog that verifies its behavior
with slogtest's TestHandler function.
This verifies compliance with the slog logging contract.

Resolves uber-go#1334
mway pushed a commit that referenced this issue Oct 5, 2023
@justinhwang @abhinav
zapcore: Ignore nil Entry.Time in encoders (#1369)
87577d8 
Per the comment on `Encoder.EncodeEntry`, any fields that are empty
including fields on the `Entry` type should be omitted. Omit the `Time`
field when we have empty time.
This also aligns with slog.Handler contract.

Refs #1334
Discovered by #1335

---------

Co-authored-by: Abhinav Gupta <mail@abhinavg.net>
abhinav added a commit to abhinav/zap that referenced this issue Oct 5, 2023
@abhinav
zapslog: Test with slogtest
d064fd7 
Adds a test for zapslog that verifies its behavior
with slogtest's TestHandler function.
This verifies compliance with the slog logging contract.

Resolves uber-go#1334
This was referenced Jan 27, 2024
Closed
Closed
@arukiidou
Copy link
Contributor

@abhinav

arukiidou added a commit to arukiidou/zap that referenced this issue Feb 2, 2024
@arukiidou
zapslog: Test with slogtest, fix all failures uber-go#1334
f09e134 
Signed-off-by: junya koyama <arukiidou@yahoo.co.jp>
@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 <arukiidou@yahoo.co.jp>
Co-authored-by: Abhinav Gupta <mail@abhinavg.net>
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
4 participants
@abhinav @justinhwang @arukiidou @SoulPancake