Skip to content

Commit

Permalink
klog.Fatal: revert dumping of all stack backtraces to stderr
Browse files Browse the repository at this point in the history
This is a revert of kubernetes#79.

Dumping the stack backtraces of all goroutines to stderr is not useful for
processes which have many goroutines. The console buffer overflows and the
original error which got dumped earlier is no longer visible. Even in CI
systems where stderr is captured the full dump is often not useful.

This was pointed out as a potential problem during the original PR review but
only got more attention after the updated klog got merged into Kubernetes and
developers there started to see the longer output.
  • Loading branch information
pohly committed Jun 13, 2022
1 parent ea35802 commit c977919
Show file tree
Hide file tree
Showing 2 changed files with 16 additions and 10 deletions.
15 changes: 9 additions & 6 deletions integration_tests/klog_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -91,9 +91,10 @@ func TestDestinationsWithDifferentFlags(t *testing.T) {
notExpectedInDir map[int]res
}{
"default flags": {
// Everything, including the trace on fatal, goes to stderr
// Everything, EXCEPT the trace on fatal, goes to stderr

expectedOnStderr: allLogREs,
expectedOnStderr: res{infoLogRE, warningLogRE, errorLogRE, fatalLogRE},
notExpectedOnStderr: res{stackTraceRE},
},
"everything disabled": {
// Nothing, including the trace on fatal, is showing anywhere
Expand All @@ -112,11 +113,12 @@ func TestDestinationsWithDifferentFlags(t *testing.T) {
notExpectedOnStderr: res{infoLogRE},
},
"with logtostderr only": {
// Everything, including the trace on fatal, goes to stderr
// Everything, EXCEPT the trace on fatal, goes to stderr

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

expectedOnStderr: allLogREs,
expectedOnStderr: res{infoLogRE, warningLogRE, errorLogRE, fatalLogRE},
notExpectedOnStderr: res{stackTraceRE},
},
"with log file only": {
// Everything, including the trace on fatal, goes to the single log file
Expand Down Expand Up @@ -154,13 +156,14 @@ func TestDestinationsWithDifferentFlags(t *testing.T) {
notExpectedInDir: defaultNotExpectedInDirREs,
},
"with log dir and logtostderr": {
// Everything, including the trace on fatal, goes to stderr. The -log_dir is
// Everything, EXCEPT the trace on fatal, goes to stderr. The -log_dir is
// ignored, nothing goes to the log files in the log dir.

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

expectedOnStderr: allLogREs,
expectedOnStderr: res{infoLogRE, warningLogRE, errorLogRE, fatalLogRE},
notExpectedOnStderr: res{stackTraceRE},
},
"with log file and log dir": {
// Everything, including the trace on fatal, goes to the single log file.
Expand Down
11 changes: 7 additions & 4 deletions klog.go
Original file line number Diff line number Diff line change
Expand Up @@ -924,12 +924,15 @@ func (l *loggingT) output(s severity.Severity, log *logr.Logger, buf *buffer.Buf
OsExit(1)
}
// 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 {
os.Stderr.Write(trace)
// First, make sure we see the trace for the current goroutine on standard error.
// If -logtostderr has been specified, the loop below will do that anyway
// as the first stack in the full dump.
if !l.toStderr {
os.Stderr.Write(stacks(false))
}

// Write the stack trace for all goroutines to the files.
trace := stacks(true)
logExitFunc = func(error) {} // If we get a write error, we'll still exit below.
for log := severity.FatalLog; log >= severity.InfoLog; log-- {
if f := l.file[log]; f != nil { // Can be nil if -logtostderr is set.
Expand Down

0 comments on commit c977919

Please sign in to comment.