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

[Feature] Add experiment zapslog package to integrate with slog #1246

Merged
merged 14 commits into from
Mar 17, 2023

Conversation

knight42
Copy link
Contributor

Ref #1179

This PR provides a implementation of slog.Handler in a separate experiment exp/zapslog module as suggested by @abhinav.

Signed-off-by: Jian Zeng <anonymousknight96@gmail.com>
@CLAassistant
Copy link

CLAassistant commented Mar 11, 2023

CLA assistant check
All committers have signed the CLA.

@knight42
Copy link
Contributor Author

/cc @abhinav PTAL

Copy link
Collaborator

@abhinav abhinav left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this, @knight42!
I've taken an initial pass over it, including some thinking-out-loud.

There are a couple open questions here that may merit some discussion, so I've tagged other maintainers.

exp/zapslog/example_test.go Outdated Show resolved Hide resolved
exp/zapslog/example_test.go Outdated Show resolved Hide resolved
exp/zapslog/example_test.go Outdated Show resolved Hide resolved
exp/zapslog/example_test.go Outdated Show resolved Hide resolved
exp/zapslog/go.mod Outdated Show resolved Hide resolved
exp/zapslog/slog.go Outdated Show resolved Hide resolved
exp/zapslog/slog.go Outdated Show resolved Hide resolved
exp/zapslog/slog.go Outdated Show resolved Hide resolved
Comment on lines 70 to 73
return convertAttrToField(slog.Attr{
Key: attr.Key,
Value: attr.Value.Resolve(),
})
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will resolve the LogValuer eagerly, which will hurt anyone using LogValuer to defer expensive operations.

Ideally, we want to use a wrapper struct that calls LogValue() at serialization time,
but that may not be 100% straightforward because a LogValuer doesn't know if it's a group or a primitive type until calling LogValue(),
whereas Zap wants to know that in advance.
This may require a lazy field-equivalent in Zap.
We can defer designing that to a follow-up.
CC @mway @prashantv @sywhang

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 on a adding a lazy-field equivalent in zap separately.

exp/zapslog/slog.go Outdated Show resolved Hide resolved
Signed-off-by: Jian Zeng <anonymousknight96@gmail.com>
Signed-off-by: Jian Zeng <anonymousknight96@gmail.com>
Signed-off-by: Jian Zeng <anonymousknight96@gmail.com>
We don't need a extra struct to save the Group Attr, only a list of Attr
is sufficient.

Signed-off-by: Jian Zeng <anonymousknight96@gmail.com>
Signed-off-by: Jian Zeng <anonymousknight96@gmail.com>
Comment on lines 98 to 106
ent := zapcore.Entry{
Level: convertSlogLevel(record.Level),
Time: record.Time,
Message: record.Message,
// FIXME: do we need to set the following fields?
// LoggerName:
// Caller:
// Stack:
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc @abhinav . Do we need to complete the Entry?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those three fields are optional. We can start without them.
I can see Caller and LoggerName being filled in the future.
(We could probably fill the Caller from Record.PC,
but doing that cheaply adds some complexity so we can do without for now.)

However, this does bring up the need for customization.
A plain NewHandler may not be sufficient for that.

I've pushed a change to your branch with the following new API:

type HandlerOptions struct{ LoggerName string }

func (HandlerOptions) New(zapcore.Core) *Handler

The func NewHandler(..) just calls this now to get default options.
This mirrors the JSONHandler, NewJSONHandler, HandlerOptions setup.

This leaves us room for future customization llke:
opting into adding a caller name, custom level mapping, etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! The HandlerOptions is commonly used for customization, though I thought we might want to consistently follow the functional options pattern in zap.

I can see Caller and LoggerName being filled in the future.
(We could probably fill the Caller from Record.PC,

After some trial & error, I found I could add the Caller in this PR as well, plz see the commit 1ae5945

@codecov
Copy link

codecov bot commented Mar 16, 2023

Codecov Report

Merging #1246 (656ca5e) into master (85c4932) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #1246   +/-   ##
=======================================
  Coverage   98.23%   98.23%           
=======================================
  Files          49       49           
  Lines        3231     3231           
=======================================
  Hits         3174     3174           
  Misses         49       49           
  Partials        8        8           

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Signed-off-by: Jian Zeng <anonymousknight96@gmail.com>
Signed-off-by: Jian Zeng <anonymousknight96@gmail.com>
Signed-off-by: Jian Zeng <anonymousknight96@gmail.com>
Signed-off-by: Jian Zeng <anonymousknight96@gmail.com>
Copy link
Member

@mway mway left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for implementing this!

exp/zapslog/doc.go Outdated Show resolved Hide resolved
exp/zapslog/example_test.go Outdated Show resolved Hide resolved
exp/zapslog/example_test.go Outdated Show resolved Hide resolved
exp/zapslog/example_test.go Outdated Show resolved Hide resolved
@mway
Copy link
Member

mway commented Mar 16, 2023

Thinking about it more, as long as we address the import name comment, we can worry about the other nits later.

Signed-off-by: Jian Zeng <anonymousknight96@gmail.com>
Signed-off-by: Jian Zeng <anonymousknight96@gmail.com>
@knight42
Copy link
Contributor Author

@mway Hi I have addressed your comments, PTAL

Copy link
Member

@mway mway left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

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

Successfully merging this pull request may close these issues.

5 participants