-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Conversation
3699e9e
to
68a48fd
Compare
Did you have uniform benchmark environment? If you do, please update benchmark result in README. |
There was a problem hiding this 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 Report
@@ 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 |
fix: use disabled slog for BenchmarkDisabledAddingFields
It's very thoughtful of you. The |
If you do not ask for specific hardware, I can do benchmarks on my |
Adjusts the benchmark name to match the convention set by, Zap.Sugar. Updates the README generation script to add an entry for this.
Hey, we do not have a uniform benchmark environment. |
…d .. && make updatereadme
Benchmars has been updated. The results looks as expected. |
There was a problem hiding this 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!
Bechmark test
log/slog
.