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.

Sign up for GitHub

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

Jump to bottom

Stderr threshold issue fix #221

Closed
wants to merge 3 commits into from
Closed

Stderr threshold issue fix #221

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1d97b27
Added alsotostderrthreshold options and fixed StderrThreshold issue
Mar 9, 2021
6f2fbaa
new parameter inside the test (test needs to be fixed)
Mar 9, 2021
28c2619
test in this way
Mar 11, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
22 changes: 11 additions & 11 deletions integration_tests/klog_test.go
View file
Open in desktop
Expand Up @@ -98,31 +98,31 @@ func TestDestinationsWithDifferentFlags(t *testing.T) {
"everything disabled": {
// Nothing, including the trace on fatal, is showing anywhere

flags: []string{"-logtostderr=false", "-alsologtostderr=false", "-stderrthreshold=1000"},
flags: []string{"-logtostderr=false", "-alsologtostderr=false", "-stderrthreshold=0", "-alsologtostderrthreshold=1000"},

notExpectedOnStderr: allLogREs,
},
"everything disabled but low stderrthreshold": {
// Everything above -stderrthreshold, including the trace on fatal, will
// be logged to stderr, even if we set -logtostderr to false.

flags: []string{"-logtostderr=false", "-alsologtostderr=false", "-stderrthreshold=1"},
flags: []string{"-logtostderr=false", "-alsologtostderr=false", "-stderrthreshold=0", "-alsologtostderrthreshold=1"},

expectedOnStderr: res{warningLogRE, errorLogRE, stackTraceRE},
notExpectedOnStderr: res{infoLogRE},
},
"with logtostderr only": {
// Everything, including the trace on fatal, goes to stderr

flags: []string{"-logtostderr=true", "-alsologtostderr=false", "-stderrthreshold=1000"},
flags: []string{"-logtostderr=true", "-alsologtostderr=false", "-stderrthreshold=0", "-alsologtostderrthreshold=1000"},

expectedOnStderr: allLogREs,
},
"with log file only": {
// Everything, including the trace on fatal, goes to the single log file

logfile: true,
flags: []string{"-logtostderr=false", "-alsologtostderr=false", "-stderrthreshold=1000"},
flags: []string{"-logtostderr=false", "-alsologtostderr=false", "-stderrthreshold=0", "-alsologtostderrthreshold=1000"},

expectedLogFile: true,

Expand All @@ -133,7 +133,7 @@ func TestDestinationsWithDifferentFlags(t *testing.T) {
// Everything, including the trace on fatal, goes to the log files in the log dir

logdir: true,
flags: []string{"-logtostderr=false", "-alsologtostderr=false", "-stderrthreshold=1000"},
flags: []string{"-logtostderr=false", "-alsologtostderr=false", "-stderrthreshold=0", "-alsologtostderrthreshold=1000"},

expectedLogDir: true,

Expand All @@ -145,7 +145,7 @@ func TestDestinationsWithDifferentFlags(t *testing.T) {
// Everything, including the trace on fatal, goes to the log files in the log dir

logdir: true,
flags: []string{"-logtostderr=false", "-alsologtostderr=false", "-stderrthreshold=1000", "-one_output=true"},
flags: []string{"-logtostderr=false", "-alsologtostderr=false", "-stderrthreshold=0", "-alsologtostderrthreshold=1000", "-one_output=true"},

expectedLogDir: true,

Expand All @@ -158,7 +158,7 @@ func TestDestinationsWithDifferentFlags(t *testing.T) {
// ignored, nothing goes to the log files in the log dir.

logdir: true,
flags: []string{"-logtostderr=true", "-alsologtostderr=false", "-stderrthreshold=1000"},
flags: []string{"-logtostderr=true", "-alsologtostderr=false", "-stderrthreshold=0", "-alsologtostderrthreshold=1000"},

expectedOnStderr: allLogREs,
},
Expand All @@ -168,7 +168,7 @@ func TestDestinationsWithDifferentFlags(t *testing.T) {

logdir: true,
logfile: true,
flags: []string{"-logtostderr=false", "-alsologtostderr=false", "-stderrthreshold=1000"},
flags: []string{"-logtostderr=false", "-alsologtostderr=false", "-stderrthreshold=0", "-alsologtostderrthreshold=1000"},

expectedLogFile: true,

Expand All @@ -179,7 +179,7 @@ func TestDestinationsWithDifferentFlags(t *testing.T) {
// Everything, including the trace on fatal, goes to the single log file
// AND to stderr.

flags: []string{"-alsologtostderr=true", "-logtostderr=false", "-stderrthreshold=1000"},
flags: []string{"-alsologtostderr=true", "-logtostderr=false", "-stderrthreshold=0", "-alsologtostderrthreshold=1000"},
logfile: true,

expectedLogFile: true,
Expand All @@ -192,7 +192,7 @@ func TestDestinationsWithDifferentFlags(t *testing.T) {
// log dir AND to stderr.

logdir: true,
flags: []string{"-alsologtostderr=true", "-logtostderr=false", "-stderrthreshold=1000"},
flags: []string{"-alsologtostderr=true", "-logtostderr=false", "-stderrthreshold=0", "-alsologtostderrthreshold=1000"},

expectedLogDir: true,

Expand All @@ -205,7 +205,7 @@ func TestDestinationsWithDifferentFlags(t *testing.T) {
// log dir AND to stderr.

logdir: true,
flags: []string{"-alsologtostderr=true", "-logtostderr=false", "-stderrthreshold=1000", "-one_output=true"},
flags: []string{"-alsologtostderr=true", "-logtostderr=false", "-stderrthreshold=0", "-alsotostderrthreshold=1000", "-one_output=true"},

expectedLogDir: true,

Expand Down
28 changes: 18 additions & 10 deletions klog.go
View file
Open in desktop
Expand Up @@ -43,7 +43,10 @@
// Logs are written to standard error instead of to files.
// -alsologtostderr=false
// Logs are written to standard error as well as to files.
// -stderrthreshold=ERROR
// -stderrthreshold=INFO
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this might have to land in new release because it's a breaking change.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please check the change inside the code. This is to maintain the pre-existing behavior.
However, there are no problems to land in a release.

// Log events at or above this severity are logged to standard
// error.
// -alsologtostderrthreshold=ERROR
// Log events at or above this severity are logged to standard
// error as well as to files.
// -log_dir=""
Expand Down Expand Up @@ -91,10 +94,10 @@ import (
"github.com/go-logr/logr"
)

// severity identifies the sort of log: info, warning etc. It also implements
// the flag.Value interface. The -stderrthreshold flag is of type severity and
// should be modified only through the flag.Value interface. The values match
// the corresponding constants in C++.
// severity identifies the sort of log: info, warning etc. It also implements the
// flag.Value interface. The -stderrthreshold and -alsologtostderrthreshold flags are
// of type severity and should be modified only through the flag.Value interface.
// The values match the corresponding constants in C++.
type severity int32 // sync/atomic int32

// These constants identify the log levels in order of increasing severity.
Expand Down Expand Up @@ -403,7 +406,8 @@ type flushSyncWriter interface {

// init sets up the defaults and runs flushDaemon.
func init() {
logging.stderrThreshold = errorLog // Default stderrThreshold is ERROR.
logging.stderrThreshold = infoLog // Default stderrThreshold is ERROR.
logging.alsoToStderrThreshold = errorLog // Default alsoToStderrThreshold is ERROR.
logging.setVState(0, nil, false)
logging.logDir = ""
logging.logFile = ""
Expand Down Expand Up @@ -436,6 +440,7 @@ func InitFlags(flagset *flag.FlagSet) {
flagset.BoolVar(&logging.oneOutput, "one_output", logging.oneOutput, "If true, only write logs to their native severity level (vs also writing to each lower severity level)")
flagset.BoolVar(&logging.skipLogHeaders, "skip_log_headers", logging.skipLogHeaders, "If true, avoid headers when opening log files")
flagset.Var(&logging.stderrThreshold, "stderrthreshold", "logs at or above this threshold go to stderr")
flagset.Var(&logging.alsoToStderrThreshold, "alsologtostderrthreshold", "logs at or above this threshold go to stderr as well as files")
flagset.Var(&logging.vmodule, "vmodule", "comma-separated list of pattern=N settings for file-filtered logging")
flagset.Var(&logging.traceLocation, "log_backtrace_at", "when logging hits line file:N, emit a stack trace")
}
Expand All @@ -454,7 +459,8 @@ type loggingT struct {
alsoToStderr bool // The -alsologtostderr flag.

// Level flag. Handled atomically.
stderrThreshold severity // The -stderrthreshold flag.
stderrThreshold severity // The -stderrthreshold flag.
alsoToStderrThreshold severity // The -alsologtostderrthreshold flag.

// freeList is a list of byte buffers, maintained under freeListMu.
freeList *buffer
Expand Down Expand Up @@ -915,9 +921,11 @@ func (l *loggingT) output(s severity, log logr.Logger, buf *buffer, file string,
log.Info(string(data))
}
} else if l.toStderr {
os.Stderr.Write(data)
if s >= l.stderrThreshold.get() {
os.Stderr.Write(data)
}
} else {
if alsoToStderr || l.alsoToStderr || s >= l.stderrThreshold.get() {
if alsoToStderr || l.alsoToStderr || s >= l.alsoToStderrThreshold.get() {
os.Stderr.Write(data)
}

Expand Down Expand Up @@ -968,7 +976,7 @@ func (l *loggingT) output(s severity, log logr.Logger, buf *buffer, file string,
// Dump all goroutine stacks before exiting.
trace := stacks(true)
// Write the stack trace for all goroutines to the stderr.
if l.toStderr || l.alsoToStderr || s >= l.stderrThreshold.get() || alsoToStderr {
if l.toStderr || l.alsoToStderr || s >= l.alsoToStderrThreshold.get() || alsoToStderr {
os.Stderr.Write(trace)
}
// Write the stack trace for all goroutines to the files.
Expand Down
27 changes: 14 additions & 13 deletions klog_test.go
View file
Open in desktop
Expand Up @@ -1427,19 +1427,20 @@ func (l *testLogr) WithValues(...interface{}) logr.Logger {

// existedFlag contains all existed flag, without KlogPrefix
var existedFlag = map[string]struct{}{
"log_dir": {},
"add_dir_header": {},
"alsologtostderr": {},
"log_backtrace_at": {},
"log_file": {},
"log_file_max_size": {},
"logtostderr": {},
"one_output": {},
"skip_headers": {},
"skip_log_headers": {},
"stderrthreshold": {},
"v": {},
"vmodule": {},
"log_dir": {},
"add_dir_header": {},
"alsologtostderr": {},
"alsologtostderrthreshold": {},
"log_backtrace_at": {},
"log_file": {},
"log_file_max_size": {},
"logtostderr": {},
"one_output": {},
"skip_headers": {},
"skip_log_headers": {},
"stderrthreshold": {},
"v": {},
"vmodule": {},
}

// KlogPrefix define new flag prefix
Expand Down