8000 Fields helper accepts both map and slice by hn8 · Pull Request #352 · rs/zerolog · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Fields helper accepts both map and slice #352

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
8000
Merged
merged 3 commits into from
Sep 8, 2021
Merged

Fields helper accepts both map and slice #352

merged 3 commits into from
Sep 8, 2021

Conversation

hn8
Copy link
Contributor
@hn8 hn8 commented Aug 31, 2021 •
edited
Loading

Useful for logging wrappers like zerologr to reuse the same code for Fields helper (which requires map). No extra allocation for variadic arguments or slice of key/value pairs. It avoids stack copy overhead from variadic arguments (as in Zap ...Fields)

@hn8
Add KeyValues helper
7e5c6f7 
No extra allocation for variadic arguments or slice of key/value pairs
hn8 added a commit to go-logr/zerologr that referenced this pull request Aug 31, 2021
@hn8
PoC for zerolog.KeyValues
a9b7416 
rs/zerolog#352
@rs
Copy link
Owner
rs commented Sep 7, 2021

I'm not a big fan of the name of the method. Ideally it should be Fields but it is taken by the map form. I also wonder if a variadic argument wouldn't be more friendly for this method (not sure about the allocation difference).

What I have in mind (but convinced by any of them):

  • Fields(interface) that would handle both map and array forms via type inference. I generally don't like type-less APIs, but here it is more a convenience method so perhaps it's ok
  • Fields(interface…) same as above but either it's a single map arg or a variadic key, value… It might be very confusing.
  • FieldList([]interface) isn't much more convincing than KeyValues

Ideas are welcome.

@hn8 hn8 changed the title Add KeyValues helper Fields helper accepts both map and slice Sep 8, 2021
@hn8
Copy link
Contributor Author
hn8 commented Sep 8, 2021 •
edited
Loading

@rs Pushed the change to go with first option Fields(interface). It is less confusing than variadic version, also has less overhead. Tried to optimize logr Zap wrapper before, learned that passing arguments with variadic spread down to call stack does hurt its performance. Does it look good for you?

@rs
Copy link
Owner
rs commented Sep 8, 2021

Looks good. Could you please add some tests and an example?

@rs rs merged commit 65adfd8 into rs:master Sep 8, 2021
@hn8
Copy link
Contributor Author
hn8 commented Sep 8, 2021

@rs Would you please create a new git tag (for go mod)? Thanks!

@rs
Copy link
Owner
rs commented Sep 9, 2021

done

hn8 added a commit to go-logr/zerologr that referenced this pull request Sep 10, 2021
@hn8
Move logic to zerolog
71c14ca 
rs/zerolog#352
@hn8 hn8 mentioned this pull request Oct 21, 2021
Merged
@tonglil
Copy link
tonglil commented Jan 6, 2022

Just wondering, is there any way Fields() can detect errors and call Err(err) to print the stacktrace for https://github.com/rs/zerolog#error-logging-with-stacktrace?

Example: https://go.dev/play/p/ulHc0WIqlC0

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

Successfully merging this pull request may close these issues.
3 participants
@hn8 @rs @tonglil
0