-
Notifications
You must be signed in to change notification settings - Fork 4.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
grpclog: change logger to avoid Printf when the logger is io.Discard #7471
base: master
Are you sure you want to change the base?
Conversation
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #7471 +/- ##
==========================================
+ Coverage 81.87% 82.00% +0.13%
==========================================
Files 373 373
Lines 37822 37853 +31
==========================================
+ Hits 30967 31043 +76
+ Misses 5563 5524 -39
+ Partials 1292 1286 -6
|
463f9b1
to
ae6fd53
Compare
bf693df
to
bd9603c
Compare
bd9603c
to
2da6dee
Compare
2242738
to
1184eb7
Compare
This PR is labeled as requiring an update from the reporter, and no update has been received after 6 days. If no update is provided in the next 7 days, this issue will be automatically closed. |
Sorry, I was on vacation last week. I'll try to address the comments sometime this week |
This PR is labeled as requiring an update from the reporter, and no update has been received after 6 days. If no update is provided in the next 7 days, this issue will be automatically closed. |
2bf7f34
to
accdbaf
Compare
3931f73
to
b711721
Compare
d3198be
to
459a5cb
Compare
Changes the logger to not call Printf when the logger is io.Discard. Also changes the logger constructor to use io.Discard instead of io.MultiWriter(io.Discard, io.Discard), which is necessary to detect all levels being discarded. Fixes grpc#7463
459a5cb
to
16b93cd
Compare
N.B. I made a few changes since the last review, adding var sprintf = fmt.Sprintf
var exit = os.Exit in loggerv2.go, to make it possible to test that Testing Sprintf not being called on io.Discard was previously done via the closure. After removing the closure, this allows the test to change the var to test. For the exit, the Github ARM runner started breaking with the external call this PR was previously doing to test log.Fatal/os.Exit (a la https://go.dev/talks/2014/testing.slide#23), IMO the tests are valuable, but I realise it's a bit unusual, I can remove the vars and corresponding tests if you don't agree. |
Changes the logger to not call Printf when the logger is io.Discard.
Also changes the logger constructor to use
io.Discard
instead ofio.MultiWriter(io.Discard, io.Discard)
, which is necessary to detect all levels being discarded.This is a significant performance improvement for disabled logs, see #7463 for details.
Notes:
log.Printf
itself, but I don't think that's possible, because the JSON option needs to Printf and then escape the result after interpolating, which I don't think is possible with what's exposed by the std (possibly sans some overly complex io.Writer interceptors).log.Printf
for the non-JSON, but there's not much point if we can't use it for the JSON, and it would incur an unnecessary atomic call.io.Discard
logic could be made faster (e.g. with more nested if's), and could do smarter things around the discard/multiwriter. I wrote it this way for readability, and because I don't think the performance is significant. But I'm happy to change it.Fixes #7463
RELEASE NOTES: