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
klog 2.20.0, logr v1.1.0, zapr v1.1.0 #104103
Conversation
This is WIP because klog doesn't have a formal release yet and because there are known test failures in staging/src/k8s.io/component-base/logs/json. |
This is failing because it uses |
Can you use `logr.Discard()` instead, if you need a valid Logger?
…On Tue, Aug 3, 2021 at 10:33 AM Patrick Ohly ***@***.***> wrote:
This is failing because it uses Logger{} to denote an uninitialized logger instead of the former nil, but that then cause nil pointer access because klog does not (and cannot!) detect that such a struct is unusable.
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications on the go with GitHub Mobile for iOS or Android.
|
/remove-sig api-machinery |
@kubernetes/dep-approvers: @serathius finished the technical review, now we need approval for the modified dependencies (external zapr instead of internal fork, updated dependencies). |
Just noticed some left-over comment and a formatting issue, force-pushed. |
This replaces the experimental logr v0.4 with the stable v1.1.0 release. This is a breaking API change for some users because: - Comparing logr.Logger against nil is not possible anymore: it's now a struct instead of an interface. Code which allows a nil logger should switch to *logr.Logger as type. - Logger implementations must be updated in lockstep. Instead of updating the forked zapr code in json.go, directly using the original go-logr/zapr is simpler and avoids duplication of effort. The updated zapr supports logging of numeric verbosity. Error messages don't have a verbosity (= always get logged), so "v" is not getting added to them anymore. Source code logging for panic messages got fixed so that it references the code with the invalid log call, not the json.go implementation. Finally, zapr includes additional information in its panic messages ("zap field", "ignored key", "invalid key").
/retest |
/approve |
/triage accepted |
@@ -149,57 +163,63 @@ func TestKlogIntegration(t *testing.T) { | |||
fun: func() { | |||
klog.Error("test ", 1) | |||
}, | |||
format: `{"ts":%f,"caller":"json/klog_test.go:%d","msg":"test 1\n","v":0}`, |
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.
is it expected that these no longer output "v":0
?
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.
Yes, verbosity for error didn't have any semantic meaning. It was always supposed to be equal zero, but we decided to drop it in zapr.
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.
Yes. Only info messages have a verbosity level and are subjected to filtering by verbosity. Error messages are always printed and therefore conceptually don't have verbosity. In other words, V(3).Error
is the same as V(0).Error
.
are there any benchmarks/performance tests on the structured logging to demonstrate this replumbing didn't noticeably affect resource usage? |
dependency mechanics lgtm, just a couple questions |
I ran the
So performance gets slightly better. |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dims, liggitt, pohly The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Just a note about using benchstat. You should look at value of For example looks like there is a small regression for InfoLoggerInfo-36, but with p=0.246, benchstat doesn't show delta as there is not enough runs to be sure. I would recommend increasing number of runs to 10. |
/retest Unit test flake ( |
/retest gce test flake ( |
What type of PR is this?
/kind cleanup
/kind api-change
What this PR does / why we need it:
This replaces the experimental logr v0.4 with the stable v1.1.0
release. This is a breaking API change for some users because:
it's now a struct instead of an interface. Code which
allows a nil logger must switch to *logr.Logger as type.
Instead of updating the forked zapr code in json.go, directly using
the original go-logr/zapr is simpler and avoids duplication of effort.
Which issue(s) this PR fixes:
Fixes #104097
Special notes for your reviewer:
The updated zapr supports logging of numeric verbosity. Error messages
don't have a verbosity (= always get logged), so "v" is not getting
added to them anymore.
Source code logging for panic messages got fixed so that it references
the code with the invalid log call, not the json.go implementation.
Finally, zapr includes additional information in its panic
messages ("zap field", "ignored key", "invalid key").
Does this PR introduce a user-facing change?