Skip to content
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

stderrthreshold not honored when logtostderr is set #212

Open
aramase opened this issue Feb 11, 2021 · 49 comments
Open

stderrthreshold not honored when logtostderr is set #212

aramase opened this issue Feb 11, 2021 · 49 comments
Labels
kind/feature Categorizes issue or PR as related to a new feature. lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness.

Comments

@aramase
Copy link
Member

aramase commented Feb 11, 2021

/kind bug

What steps did you take and what happened:
[A clear and concise description of what the bug is.]
We're using the klog logging framework in Secrets Store CSI Driver. When trying to set -stderrthreshold=ERROR with -logtostderr=true, we're seeing all the logs are being printed. Is this the expected behavior?

Users want to only log the ERROR but that doesn't seem like an option today? Is there any configuration that we're missing.

cc @tam7t

What did you expect to happen:

Anything else you would like to add:
[Miscellaneous information that will assist in solving the issue.]

@k8s-ci-robot k8s-ci-robot added the kind/bug Categorizes issue or PR as related to a bug. label Feb 11, 2021
@aramase
Copy link
Member Author

aramase commented Feb 11, 2021

Found this PR (#31) which adds the severity check when logtostderr is set. This was however reverted #50

@pierluigilenoci
Copy link

@pohly @dims could you please take a look?

@pierluigilenoci
Copy link

@DirectXMan12 @justinsb @neolit123 @mtaufen could you please take a look?

@pierluigilenoci
Copy link

@pohly @yuzhiquan @munnerz could you please take a look?

@pierluigilenoci
Copy link

Who needs to be tagged to get feedback?

@jayunit100
@hoegaarden
@andyxning
@yagonobre
@vincepri
@detiber
@thockin
@tallclair
@piosz
@brancz
@lavalamp

@lavalamp
Copy link
Member

lavalamp commented Mar 4, 2021

Everyone is extremely overloaded. This looks like it may be much more important to you than perhaps to some other people.

The description of the reversion PR is pretty clear about what went wrong. I recommend sending a PR that adds what you need while not triggering the problems mentioned there.

@pierluigilenoci
Copy link

@lavalamp I can understand that you are all loaded with work but this seems a pretty big bug for a logging library and it seems incredible to me that in two years no one has been able to fix it.

@dims
Copy link
Member

dims commented Mar 9, 2021

@pierluigilenoci please re-read exactly what @lavalamp said. that's what you will hear from any one of us. good luck!

@fejta-bot
Copy link

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-contributor-experience at kubernetes/community.
/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jun 7, 2021
@pierluigilenoci
Copy link

/remove-lifecycle stale

@k8s-ci-robot k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jun 8, 2021
@deyanp
Copy link

deyanp commented Jul 28, 2021

@lavalamp @dims any chance to get the PR of @pierluigilenoci approved? Sorry for jumping in, but I am affected by a pod for Azure AKS which is using this library and generating high log storage costs per month ...

@dims
Copy link
Member

dims commented Jul 28, 2021

@deyanp this is configurable from the command line as well as programatically, so sorry i don't see why the application developer using this library is unable to set this in one of those ways? can you please explain more?

@deyanp
Copy link

deyanp commented Aug 4, 2021

@dims , I cannot explain more, as I am not a contributor to the client library/application in question (using klog) - https://github.com/Azure/secrets-store-csi-driver-provider-azure ... My understanding is that the contributors of (e.g. @aramase ) are stating the bug is in klog ...

@jwmajors81
Copy link

I came across this same issue today when using the cluster-autoscaler. I concur with what others have stated here about the fact that stderrthreshold was not honored when logtostderr was set to true. However, it is honored when logtostderr is set to false. In addition, if you want to log to stderr then you can set logtostderr to false and then set stderrthreshold to a value that is less than ERROR (ex: INFO) and the logs will show up in stderr.

      - command:
        - ./cluster-autoscaler
        - --v=4
        - --stderrthreshold=info
        - --logtostderr=false

The code that is responsible for this logic can be found at https://github.com/kubernetes/klog/blob/main/klog.go#L938

This may not fulfill all use cases for individuals who have posted here, but I hope others find this information helpful.

@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Mark this issue or PR as fresh with /remove-lifecycle stale
  • Mark this issue or PR as rotten with /lifecycle rotten
  • Close this issue or PR with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Dec 27, 2021
@pierluigilenoci
Copy link

/remove-lifecycle stale

@k8s-ci-robot k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jan 3, 2022
@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Mark this issue or PR as fresh with /remove-lifecycle stale
  • Mark this issue or PR as rotten with /lifecycle rotten
  • Close this issue or PR with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

@pohly
Copy link

pohly commented Apr 4, 2022

There is currently no way to configure Klog to send log to stderr with only the ERROR level.

That's the key takeaway: you want a new mode of operation, one which currently isn't supported. That's fair.

But as with any other feature request the question then becomes whether that feature is useful enough to enough users to justify extra complexity or (if that turns out to be necessary) breaking other users. The bar for the latter is very high.

@nyejon
Copy link

nyejon commented Apr 4, 2022

Can one add an option in addition to the existing ones, which provides the expected functionality, while deprecating the current way of doing it?

Keep the flags independent of each other, logtostderr, logtofiles, stderrthreshold, logtofilesthreshold.

@pohly
Copy link

pohly commented Apr 4, 2022

A new -always-apply-stderrthreshold boolean with default false could be added that changes the traditional behavior of "filter stderr only when writing to file" to "filter stderr in all cases".

Just to be crystal clear: the goal is to write only errors and warnings to stderr and completely suppress all info log entries?

@pohly
Copy link

pohly commented Apr 4, 2022

while deprecating the current way of doing it?

I'm not sure about deprecation. It implies that users must take action and that we then eventually really will change the code. I for one don't have the motivation for that and dealing with upset users.

@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Mark this issue or PR as fresh with /remove-lifecycle stale
  • Mark this issue or PR as rotten with /lifecycle rotten
  • Close this issue or PR with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jul 3, 2022
@pierluigilenoci
Copy link

This is still an issue

@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Mark this issue or PR as fresh with /remove-lifecycle rotten
  • Close this issue or PR with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

@k8s-ci-robot k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Aug 3, 2022
@pierluigilenoci
Copy link

/remove-lifecycle rotten

@k8s-ci-robot k8s-ci-robot removed the lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. label Aug 3, 2022
@tallclair
Copy link
Member

/lifecycle frozen

@k8s-ci-robot k8s-ci-robot added the lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. label Aug 24, 2022
@pierluigilenoci
Copy link

/remove-lifecycle frozen

@k8s-ci-robot k8s-ci-robot removed the lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. label Aug 26, 2022
@pierluigilenoci
Copy link

@tallclair this is still a huge bug that nobody want to face 😉

@tallclair
Copy link
Member

/lifecycle frozen

@pierluigilenoci lifecycle frozen prevents the bug from being auto-closed. It doesn't mean we're stopping work on it. It's an acknowledgement that this is a real bug that should be fixed and not ignored...

@k8s-ci-robot k8s-ci-robot added the lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. label Sep 7, 2022
@pohly
Copy link

pohly commented Sep 7, 2022

@pierluigilenoci has already started working on this and - as far as I know - intends to finish that work. I'm not aware of others who care enough about it to address this issue.

@pohly
Copy link

pohly commented Sep 7, 2022

Also note that I clarified the current behavior in #319, so there should be no misunderstanding anymore about what the current implementation does.

@pohly
Copy link

pohly commented Sep 7, 2022

/kind feature

It works as originally implemented and (now) as documented, it just doesn't do something that would be useful to some users (filter by severity when writing to stderr).

@k8s-ci-robot k8s-ci-robot added the kind/feature Categorizes issue or PR as related to a new feature. label Sep 7, 2022
@pohly
Copy link

pohly commented Sep 7, 2022

/remove-kind bug

@k8s-ci-robot k8s-ci-robot removed the kind/bug Categorizes issue or PR as related to a bug. label Sep 7, 2022
@kcone
Copy link

kcone commented Nov 21, 2023

This is important to me as well. I think that it should be important to many other people.

FWIW, it seems that the aws-ebs-csi-driver uses klog, which reports all messages as errors. (This is only one example.)
Because of this, we have clients who filter out all messages from this driver, which is bad for several reasons.
The more noise that comes from these things, the more logs we have, the more clients filter (or don't) and the less observability we have in our systems. If this isn't a bug, then it at least is strongly contributing to an anti-pattern.

Props to those of you supporting this project; I know it's a largely thankless effort. :)

@pohly
Copy link

pohly commented Nov 21, 2023

FWIW, it seems that the aws-ebs-csi-driver uses klog, which reports all messages as errors.

Can you explain further? If the driver uses e.g. klog.Infof, it's not printing the message as error.

@pierluigilenoci
Copy link

This bug has been documented and has become a feature. 😉

I had started implementing a solution in the past, but I stopped when I realized that it was only possible to do so (based on my skills and knowledge) by introducing breaking changes. And so I got stuck. 😞

I hope one day to find the time to try again, but I can't promise that. 🙏🏻

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature Categorizes issue or PR as related to a new feature. lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness.
Projects
None yet
Development

Successfully merging a pull request may close this issue.