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

test: migrate slog benchmark from golang.org/x/exp/slog to log/slog #1363

Merged
merged 5 commits into from
Sep 23, 2023

Conversation

chenyanchen
Copy link
Contributor

Bechmark test log/slog.

@CLAassistant
Copy link

CLAassistant commented Sep 22, 2023

CLA assistant check
All committers have signed the CLA.

@chenyanchen
Copy link
Contributor Author

Did you have uniform benchmark environment? If you do, please update benchmark result in README.

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.

Hey, @chenyanchen. Thanks for this PR. This makes sense, and .

However, I think this could benefit from a small change:
What you've done in b.Run("slog" is great.
I think we need a second b.Run("slog/LogAttrs" that is the same, except it uses Logger.LogAttrs and the old fakeSlogFields() list.
This will provide a comparison of both, slog with any, and slog with slog.Attr.

Would you be willing to make that change?
If not, one of us will try to get to it as soon as we have some time to do it.

@codecov
Copy link

codecov bot commented Sep 22, 2023

Codecov Report

Merging #1363 (fb96ef2) into master (4ddf010) will increase coverage by 0.14%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1363      +/-   ##
==========================================
+ Coverage   98.28%   98.42%   +0.14%     
==========================================
  Files          53       53              
  Lines        3493     3493              
==========================================
+ Hits         3433     3438       +5     
+ Misses         50       46       -4     
+ Partials       10        9       -1     

see 1 file with indirect coverage changes

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

@chenyanchen
Copy link
Contributor Author

It's very thoughtful of you.

The slog Logger.LogAttrs benchmark been added.

@chenyanchen
Copy link
Contributor Author

Did you have uniform benchmark environment? If you do, please update benchmark result in README.

If you do not ask for specific hardware, I can do benchmarks on my M1 Mac.

Adjusts the benchmark name to match the convention set by, Zap.Sugar.
Updates the README generation script to add an entry for this.
@abhinav
Copy link
Collaborator

abhinav commented Sep 22, 2023

Did you have uniform benchmark environment? If you do, please update benchmark result in README.

If you do not ask for specific hardware, I can do benchmarks on my M1 Mac.

Hey, we do not have a uniform benchmark environment.
The benchmarks are updated with whatever machine someone was on at the time they ran the benchmarks.
It hasn't been a problem because the absolute numbers are less important than the relative numbers, which tend to follow the same trend regardless of where we're running the benchmarks.

@chenyanchen
Copy link
Contributor Author

Benchmars has been updated. The results looks as expected.

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.

It's weird that there's no difference between slog with any and slog with LogAttrs, but that's something to investigate later.

Thanks!

@abhinav abhinav merged commit 2f09c51 into uber-go:master Sep 23, 2023
6 checks passed
@chenyanchen chenyanchen deleted the test/benchmark/slog branch September 24, 2023 01:57
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.

None yet

3 participants