feat: fix stderrthreshold not honored when logtostderr is set (#212) + two new flags#432
Conversation
|
This issue is currently awaiting triage. If klog contributors determine this is a relevant issue, they will accept it by applying the The 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-sigs/prow repository. |
d9db7d9 to
c7c4d9d
Compare
|
@harshanarayana @thockin @dims @pohly @mengjiao-liu I'm tagging you to get your feedback. @aramase @neolit123 @yagonobre @yuzhiquan @serathius @mtaufen @DirectXMan12 @vincepri @shsjshentao @dims @justinsb I'm tagging you too because you were involved in the discussion and attempted to resolve this issue several years ago. |
Add two new flags to allow severity filtering with logtostderr while maintaining full backward compatibility: - legacy_stderr_threshold_behavior (default: true): Controls whether stderrthreshold is honored when logtostderr=true - alsologtostderrthreshold (default: INFO): Provides filtering control when alsologtostderr=true This fix allows users to opt-in to severity filtering when logging to stderr, addressing a long-standing issue where all logs would be written to stderr regardless of severity level. The default behavior remains unchanged for backward compatibility. Ref: #212
c7c4d9d to
08e6e8b
Compare
pohly
left a comment
There was a problem hiding this comment.
I'm undecided whether it's worth fixing this. But at least now it's not a breaking change, so I am okayish with trying. I haven't looked at all code changes yet.
|
@pohly, do you have any news about this PR? |
|
@pierluigilenoci: it took you more than two years to come up with this PR since we last discussed the issue. It's not urgent, is it? So please understand that reviewers are not immediately jumping on reviewing this. For example, I am on vacation. |
|
@pohly Thanks for the heads-up, and enjoy your vacation! You’re right—it took me a long time to open this PR. I just hadn’t had the time or mental bandwidth to tackle a change this complex earlier. Now that I’ve finally put together a solution, I’m confident it’s non-breaking and a net positive: logs would finally be handled in accordance with the flags. No urgency at all—please take your time. When you or other reviewers are available, I’d appreciate any feedback. I’m happy to adjust the implementation, add tests, or split things up if that helps. |
|
@pohly, would you be able to take a look at this PR in the following days? |
|
@pohly, do you think you'll have time to review my PR? |
|
I can't promise anything at this point. IMHO it has low priority and there's always something else that needs my attention more urgently. |
pohly
left a comment
There was a problem hiding this comment.
Some minor nits. I don't see any problem with the logic, but there's always a risk that I am missing something. Therefore I'm still unsure whether it really makes sense to extend klog. We've long said that it's feature set is frozen.
@pierluigilenoci: are you really going to use the new flags or did you just submit the PR to get closure on a years old issue?
@aramase, @deyanp, @tallclair, @kcone, @nyejon: you all commented on #212 at some point. Does it really still make sense to fix that issue?
Co-authored-by: Patrick Ohly <patrick.ohly@intel.com>
Co-authored-by: Patrick Ohly <patrick.ohly@intel.com>
The next step, at least for me, would be to fix the tools I use that are driving me crazy with this bug. And of course, if @aramase agree, to fix this Azure/secrets-store-csi-driver-provider-azure#387 |
- Move alsologtostderrthreshold initialization next to stderrThreshold at the beginning of init() for better organization - Simplify conditional logic in output() by combining conditions with || operators to avoid redundant .get() calls and improve readability These changes don't affect functionality, only improve code organization and efficiency as suggested in the PR review.
|
@pohly I've implemented both of your suggested changes in commit 39c4c76:
All tests continue to pass. Thanks for the review feedback! |
|
@pohly I wanted to share some thoughts on how to frame this PR. While I understand the concern about adding features to a frozen codebase, I'd like to suggest viewing this more as a bug fix rather than a new feature. The current behavior where The approach I've taken ensures zero breaking changes through the opt-in I believe that once klog consumers discover this capability exists, many will adopt it naturally because it solves a real pain point—excessive log noise when using stderr output. The fact that this issue has persisted for years and keeps resurfacing in discussions suggests there's genuine need for this functionality. In essence, we're not adding new features to klog—we're fixing a long-standing bug in a backward-compatible way that finally makes the existing flags work as users would intuitively expect them to. Thank you for taking the time to review this carefully. I appreciate your thoughtful feedback! |
Replace the deprecated k8s.io/klog import with k8s.io/klog/v2 and configure legacy_stderr_threshold_behavior and stderrthreshold flags so that klog properly honors the stderrthreshold setting. Ref: kubernetes/klog#212, kubernetes/klog#432 Signed-off-by: Pierluigi Lenoci <pierluigi.lenoci@gmail.com>
Replace all imports of k8s.io/klog with k8s.io/klog/v2, update go.mod dependencies, and configure stderrthreshold to ensure log messages are properly written to stderr. Ref: kubernetes/klog#212, kubernetes/klog#432 Signed-off-by: Pierluigi Lenoci <pierluigi.lenoci@gmail.com>
Replace all imports of k8s.io/klog with k8s.io/klog/v2 across all non-vendor Go source files (33 files), update go.mod dependencies, and configure stderrthreshold to ensure log messages are properly written to stderr. Ref: kubernetes/klog#212, kubernetes/klog#432 Signed-off-by: Pierluigi Lenoci <pierluigi.lenoci@gmail.com>
Migrate from k8s.io/klog (v1) to k8s.io/klog/v2 v2.140.0. Opt into the fixed stderrthreshold behavior by setting legacy_stderr_threshold_behavior=false after klog.InitFlags(). Ref: kubernetes/klog#212, kubernetes/klog#432 Signed-off-by: Pierluigi Lenoci <pierluigi.lenoci@gmail.com>
Migrate from k8s.io/klog (v1) to k8s.io/klog/v2 v2.140.0. Opt into the fixed stderrthreshold behavior by setting legacy_stderr_threshold_behavior=false after klog.InitFlags(). Ref: kubernetes/klog#212, kubernetes/klog#432 Signed-off-by: Pierluigi Lenoci <pierluigi.lenoci@gmail.com>
- Replace all `k8s.io/klog` (v1) imports with `k8s.io/klog/v2` across 91 non-vendor Go source files - Fix klog v1→v2 API change: `klog.V(level)` now returns `Verbose` instead of `bool`, update to use `.Enabled()` in podman.go - Upgrade `k8s.io/klog/v2` from v2.100.1 to v2.140.0 - Configure `legacy_stderr_threshold_behavior=false` and `stderrthreshold=INFO` flags after `klog.InitFlags()` so that klog properly honors the stderr threshold setting - Regenerate vendor/ directory Ref: kubernetes/klog#212, kubernetes/klog#432 Signed-off-by: Pierluigi Lenoci <pierluigi.lenoci@gmail.com>
Replace deprecated k8s.io/klog with k8s.io/klog/v2 and opt into the fixed stderrthreshold behavior introduced in klog/v2 v2.140.0. Without this fix, the -stderrthreshold flag is silently ignored when -logtostderr=true (klog's default), making it impossible to filter log output by severity. Also fix klog.V() boolean check to use klog.V().Enabled() for klog/v2 API compatibility. See: kubernetes/klog#432 Signed-off-by: Pierluigi Lenoci <pierluigi.lenoci@gmail.com>
Opt into the fixed klog behavior by setting legacy_stderr_threshold_behavior=false after klog.InitFlags(). Ref: kubernetes/klog#212, kubernetes/klog#432 Signed-off-by: Pierluigi Lenoci <pierluigi.lenoci@gmail.com>
|
@pierluigilenoci I suggest only referring back to this in the PR bodies instead of the commits, it's really hard to follow the history of this thread. |
Description
This PR fixes a long-standing issue (#212) where the
-stderrthresholdflag is completely ignored when-logtostderr=true, preventing users from filtering logs by severity level when logging to stderr.The solution introduces two new flags while maintaining 100% backward compatibility through opt-in behavior.
What type of PR is this?
/kind feature
/kind bug
What this PR does / why we need it
Problem
When
-logtostderr=trueis set, klog writes all logs to stderr regardless of the-stderrthresholdsetting. This causes excessive noise in log outputs and makes it impossible to filter logs by severity, which is particularly problematic for applications that want to reduce log verbosity (see #212 and Azure/secrets-store-csi-driver-provider-azure#387).Solution
This PR adds two new flags:
-legacy_stderr_threshold_behavior(default:true)true(default): Maintains current behavior - all logs go to stderr when-logtostderr=truefalse: New behavior - honors-stderrthresholdeven when-logtostderr=true-alsologtostderrthreshold(default:INFO)-alsologtostderr=trueAdditional Fix
Fixed a bug in
severityValue.Set()where it was hardcoded to always setlogging.stderrThreshold, preventing multipleseverityValueinstances from working independently.Usage Examples
Enable severity filtering (new behavior):
./myapp -logtostderr=true \ -legacy_stderr_threshold_behavior=false \ -stderrthreshold=ERRORResult: Only ERROR and FATAL messages appear in stderr.
Filter logs when mirroring to stderr:
./myapp -logtostderr=false \ -alsologtostderr=true \ -alsologtostderrthreshold=WARNING \ -log_dir=/var/log/myappResult: All logs in files, only WARNING+ to stderr.
Which issue(s) this PR fixes
Fixes #212
Special notes for your reviewer
Does this PR introduce a user-facing change?
Additional documentation
See the example in
examples/stderr_threshold_fix/main.gofor a practical demonstration.Migration Guide
For end users: No action required. Default behavior unchanged.
To opt into severity filtering:
-legacy_stderr_threshold_behavior=false-stderrthreshold=ERROR(or WARNING) to filter logsRelated PRs/Issues