Honor stderrthreshold when logtostderr is enabled#2109
Honor stderrthreshold when logtostderr is enabled#2109pierluigilenoci wants to merge 2 commits intoGoogleContainerTools:mainfrom
Conversation
After calling klog.InitFlags, opt into the fixed stderrthreshold behavior added in klog v2.140.0 so that severe-enough messages are still mirrored to stderr when logtostderr is set. Reference: kubernetes/klog#212 Signed-off-by: Pierluigi Lenoci <pierluigi.lenoci@gmail.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
|
Hi @pierluigilenoci. Thanks for your PR. I'm waiting for a GoogleContainerTools member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
cc @sdowell for review |
There was a problem hiding this comment.
Code Review
This pull request updates the k8s.io/klog/v2 dependency to v2.140.0 and configures several entry points to opt into the new stderrthreshold behavior by disabling legacy_stderr_threshold_behavior. The vendored klog code includes significant refactoring of key-value serialization to handle deduplication and adds a new WithHeader option to the textlogger. Feedback across multiple files indicates that errors returned by flag.Set are being ignored, which could lead to silent configuration failures and is inconsistent with existing error handling in the codebase.
cmd/junit-report/main.go
Outdated
| _ = flag.Set("legacy_stderr_threshold_behavior", "false") | ||
| _ = flag.Set("stderrthreshold", "INFO") |
There was a problem hiding this comment.
The error returned by flag.Set is being ignored. If setting these flags fails (for example, if the klog version is incorrect and the flags don't exist), the application will continue with incorrect logging behavior silently. This defeats the purpose of this change. The error should be handled, probably by logging and exiting, similar to how other errors are handled in this application.
if err := flag.Set("legacy_stderr_threshold_behavior", "false"); err != nil {
klog.Fatalf("Failed to set flag %q: %v", "legacy_stderr_threshold_behavior", err)
}
if err := flag.Set("stderrthreshold", "INFO"); err != nil {
klog.Fatalf("Failed to set flag %q: %v", "stderrthreshold", err)
}
cmd/nomos/nomos.go
Outdated
| _ = fs.Set("legacy_stderr_threshold_behavior", "false") | ||
| _ = fs.Set("stderrthreshold", "INFO") |
There was a problem hiding this comment.
The error returned by fs.Set is being ignored. If setting these flags fails, the application will silently fall back to the old logging behavior, which is undesirable. The error should be handled to ensure the logging is configured as expected. A fatal log on error would be appropriate here.
| _ = fs.Set("legacy_stderr_threshold_behavior", "false") | |
| _ = fs.Set("stderrthreshold", "INFO") | |
| if err := fs.Set("legacy_stderr_threshold_behavior", "false"); err != nil { | |
| klog.Fatalf("Failed to set flag %q: %v", "legacy_stderr_threshold_behavior", err) | |
| } | |
| if err := fs.Set("stderrthreshold", "INFO"); err != nil { | |
| klog.Fatalf("Failed to set flag %q: %v", "stderrthreshold", err) | |
| } |
| _ = flag.Set("legacy_stderr_threshold_behavior", "false") | ||
| _ = flag.Set("stderrthreshold", "INFO") |
There was a problem hiding this comment.
The errors returned by flag.Set are being ignored. This is inconsistent with the error handling for flag.Set("v", "1") just a few lines below. To ensure logging is configured correctly and to maintain consistency, these errors should be handled.
if err := flag.Set("legacy_stderr_threshold_behavior", "false"); err != nil {
klog.Fatalf("Failed to set flag %q: %v", "legacy_stderr_threshold_behavior", err)
}
if err := flag.Set("stderrthreshold", "INFO"); err != nil {
klog.Fatalf("Failed to set flag %q: %v", "stderrthreshold", err)
}
pkg/util/log/setup.go
Outdated
| _ = flag.Set("legacy_stderr_threshold_behavior", "false") | ||
| _ = flag.Set("stderrthreshold", "INFO") |
There was a problem hiding this comment.
The errors returned by flag.Set are being ignored, which is inconsistent with the error handling for flag.Set("logtostderr", "true") that follows. To ensure robust initialization, these errors should be handled, for example by calling klog.Fatal(err).
| _ = flag.Set("legacy_stderr_threshold_behavior", "false") | |
| _ = flag.Set("stderrthreshold", "INFO") | |
| if err := flag.Set("legacy_stderr_threshold_behavior", "false"); err != nil { | |
| klog.Fatal(err) | |
| } | |
| if err := flag.Set("stderrthreshold", "INFO"); err != nil { | |
| klog.Fatal(err) | |
| } |
|
@googlebot I signed it. |
Address review feedback by wrapping flag.Set() calls with proper error checking instead of silently discarding errors. Signed-off-by: Pierluigi Lenoci <pierluigi.lenoci@gmail.com>
|
I've addressed the review feedback — all Could an org member run |
|
/ok-to-test |
Summary
klog.InitFlags, opt into the fixedstderrthresholdbehavior introduced in klog v2.140.0k8s.io/klog/v2from v2.130.1 to v2.140.0cmd/nomos/nomos.go(uses namedfsflagset)cmd/junit-report/main.gopkg/util/log/setup.gopkg/resourcegroup/controllers/log/setup.goMotivation
When
logtostderris enabled (the default), klog historically suppressed thestderrthresholdflag, meaning thatWARNINGandERRORmessages could not be selectively filtered. klog v2.140.0 introduced new flags (legacy_stderr_threshold_behavior) to opt into the corrected behavior wherestderrthresholdis properly honored.Reference: kubernetes/klog#212
Test plan
stderrthresholdflag can be used to control which severity levels appear on stderr