-
Notifications
You must be signed in to change notification settings - Fork 218
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 is useless if logtostderr is set #31
Conversation
I signed it |
/approve |
os.Stderr.Write(data) | ||
if s >= l.stderrThreshold.get() { | ||
os.Stderr.Write(data) | ||
} | ||
} else { | ||
if alsoToStderr || l.alsoToStderr || s >= l.stderrThreshold.get() { | ||
os.Stderr.Write(data) |
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.
I think we should make alsologtostderr
behave the same way?
I agree with this change, but this is a behavioural change. I don't know if it will catch anyone out, as the new behaviour feels more correct. We should ensure this makes the release notes though! |
I agree as well. I think the previous behavior was more confusing, and I've talked to plenty of people that assume it works this way before trying it (I assumed this as well at some point). |
@shsjshentao can you please update the /approve |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dims, shsjshentao 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 |
thanks @shsjshentao |
stderrthreshold is useless if logtostderr is set.
This pr can ignore logs which log level is under stderrthreshold.
Like:
flag.Set("logtostderr","true")
flag.Set("stderrthreshold","ERROR")
Does this PR introduce a user-facing change?: