diff --git a/pkg/util/log/flags.go b/pkg/util/log/flags.go index 9392366fc6f6..1c2fca1b5943 100644 --- a/pkg/util/log/flags.go +++ b/pkg/util/log/flags.go @@ -43,31 +43,18 @@ func init() { &logging.showLogs, &logging.vmoduleConfig.mu.vmodule, ) - // package is imported but not further initialized. - defaultConfig := logconfig.DefaultConfig() - if err := defaultConfig.Validate(nil /* no default directory */); err != nil { + // By default, we use and apply the test configuration. + // This can be overridden to use output to file in tests + // using TestLogScope. + cfg, err := getTestConfig(nil /* output to files disabled */) + if err != nil { panic(err) } - - // Default stderrThreshold to log everything to the process' - // external stderr (OrigStderr). - defaultConfig.Sinks.Stderr.Filter = severity.INFO - // Ensure all channels go to stderr. - defaultConfig.Sinks.Stderr.Channels.Channels = logconfig.SelectAllChannels() - // We also don't capture internal writes to fd2 by default: - // let the writes go to the external stderr. - defaultConfig.CaptureFd2.Enable = false - // Since we are letting writes go to the external stderr, - // we cannot keep redaction markers there. - *defaultConfig.Sinks.Stderr.Redactable = false - // Remove all sinks other than stderr. - defaultConfig.Sinks.FluentServers = nil - defaultConfig.Sinks.FileGroups = nil - - if _, err := ApplyConfig(defaultConfig); err != nil { + if _, err := ApplyConfig(cfg); err != nil { panic(err) } + // Reset the "active' flag so that the main commands can reset the // configuration. logging.mu.active = false diff --git a/pkg/util/log/fluent_client_test.go b/pkg/util/log/fluent_client_test.go index b553a7062f59..f4feb056f34b 100644 --- a/pkg/util/log/fluent_client_test.go +++ b/pkg/util/log/fluent_client_test.go @@ -28,7 +28,7 @@ import ( func TestFluentClient(t *testing.T) { defer leaktest.AfterTest(t)() - sc := Scope(t) + sc := ScopeWithoutShowLogs(t) defer sc.Close(t) serverAddr, cleanup, fluentData := servePseudoFluent(t) diff --git a/pkg/util/log/test_log_scope.go b/pkg/util/log/test_log_scope.go index a62f9144b9b0..6dc0c4735fc5 100644 --- a/pkg/util/log/test_log_scope.go +++ b/pkg/util/log/test_log_scope.go @@ -67,7 +67,7 @@ type tShim interface { // restrictions. func Scope(t tShim) *TestLogScope { if logging.showLogs { - return (*TestLogScope)(nil) + return newLogScope(t, false /* use files */) } scope := ScopeWithoutShowLogs(t) @@ -85,20 +85,16 @@ func Scope(t tShim) *TestLogScope { // directory. // When the scope ends, the previous configuration is restored. // -// ScopeWithoutShowLogs() is only valid if logging was not yet -// active. If it was, the test fails with an assertion error. -// The motivation for this restriction is to simplify reasoning by -// users of this facility: if a user has set up their code so that -// logging goes to files already, they are signaling that they want -// logging to go there and not elsewhere. In that case, it is -// undesirable to come along with a new random directory and take -// logging over there. -// // ScopeWithoutShowLogs() does not enable redirection of internal // stderr writes to files. Tests that wish to use that facility should // call the other APIs in these package after setting up a // TestLogScope. func ScopeWithoutShowLogs(t tShim) (sc *TestLogScope) { + t.Helper() + return newLogScope(t, true /* use files */) +} + +func newLogScope(t tShim, useFiles bool) (sc *TestLogScope) { t.Helper() sc = &TestLogScope{} @@ -117,7 +113,7 @@ func ScopeWithoutShowLogs(t tShim) (sc *TestLogScope) { if cl := logging.testingFd2CaptureLogger; cl != nil { // Temporarily give up the previous internal fd2 capture. We'll set // up a new one with the new configuration below. - // The original will e restored in the Close() function. + // The original will be restored in the Close() function. if err := cl.getFileSink().relinquishInternalStderr(); err != nil { // This should not fail. If it does, some caller messed up by // switching over stderr redirection to a different file sink @@ -140,35 +136,21 @@ func ScopeWithoutShowLogs(t tShim) (sc *TestLogScope) { } }() - tempDir, err := ioutil.TempDir("", "log"+fileutil.EscapeFilename(t.Name())) - if err != nil { - t.Fatal(err) - } - // Remember the directory name for the Close() function. - sc.logDir = tempDir - - // Create a fresh configuration. - cfg := logconfig.DefaultConfig() - - if skip.UnderBench() { - // Avoid logging anything to stderr, to avoid polluting the output - // of benchmarks. This is necessary because 'go test' unhelpfully - // merges stdout and stderr writes together. - cfg.Sinks.Stderr.Filter = severity.NONE - } else { - // Normal case: make all logged errors/fatal calls go to the - // external stderr, in addition to the log file. - cfg.Sinks.Stderr.Filter = severity.ERROR + var fileDir *string + if useFiles { + tempDir, err := ioutil.TempDir("", "log"+fileutil.EscapeFilename(t.Name())) + if err != nil { + t.Fatal(err) + } + // Remember the directory name for the Close() function. + sc.logDir = tempDir + fileDir = &sc.logDir } - // Disable the internal fd2 capture to file, to ensure that panic - // objects get reported to stderr. - cfg.CaptureFd2.Enable = false - // We cannot enable redactable markers on stderr when fd2 capture is - // disabled. - bf := false - cfg.Sinks.Stderr.Redactable = &bf - if err := cfg.Validate(&sc.logDir); err != nil { + // Obtain the standard test configuration, with the configured + // destination directory. + cfg, err := getTestConfig(fileDir) + if err != nil { t.Fatal(err) } @@ -183,10 +165,58 @@ func ScopeWithoutShowLogs(t tShim) (sc *TestLogScope) { t.Fatal(err) } - t.Logf("test logs captured to: %s", tempDir) + if useFiles { + t.Logf("test logs captured to: %s", *fileDir) + } return sc } +// getTestConfig initialize the logging configuration to parameters +// suitable for use in tests. +func getTestConfig(fileDir *string) (testConfig logconfig.Config, err error) { + testConfig = logconfig.DefaultConfig() + + if err := testConfig.Validate(fileDir); err != nil { + return testConfig, err + } + + if fileDir == nil { + // File output is disabled; we use stderr for everything. + + // All messages go to stderr. + testConfig.Sinks.Stderr.Filter = severity.INFO + // Ensure all channels go to stderr. + testConfig.Sinks.Stderr.Channels.Channels = logconfig.SelectAllChannels() + // Remove all sinks other than stderr. + testConfig.Sinks.FluentServers = nil + testConfig.Sinks.FileGroups = nil + } else { + // Output to files enabled. + + // Even though we use file output, make all logged errors/fatal + // calls go to the external stderr, in addition to the log file. + testConfig.Sinks.Stderr.Filter = severity.ERROR + } + + if skip.UnderBench() { + // Avoid logging anything to stderr, to avoid polluting the output + // of benchmarks. This is necessary because 'go test' unhelpfully + // merges stdout and stderr writes together. + // + // This overrides any default set above. + testConfig.Sinks.Stderr.Filter = severity.NONE + } + + // Disable the internal fd2 capture to file, to ensure that panic + // objects get reported to stderr. + testConfig.CaptureFd2.Enable = false + // Since we are letting writes go to the external stderr, + // we cannot keep redaction markers there. + *testConfig.Sinks.Stderr.Redactable = false + + return testConfig, nil +} + // GetDirectory retrieves the log directory for this scope. func (l *TestLogScope) GetDirectory() string { return l.logDir @@ -214,7 +244,7 @@ func (l *TestLogScope) Rotate(t tShim) { // deleted, unless the test has failed and the directory is non-empty. func (l *TestLogScope) Close(t tShim) { t.Helper() - if l == nil || l.logDir == "" { + if l == nil { // Never initialized. return } @@ -223,29 +253,31 @@ func (l *TestLogScope) Close(t tShim) { // Ensure any remaining logs are written to files. Flush() - defer func() { - // Check whether there is something to remove. - emptyDir, err := isDirEmpty(l.logDir) - if err != nil { - t.Fatal(err) - } - inPanic := calledDuringPanic() - if (t.Failed() && !emptyDir) || inPanic { - // If the test failed or there was a panic, we keep the log - // files for further investigation. - if inPanic { - fmt.Fprintln(OrigStderr, "\nERROR: a panic has occurred!\n"+ - "Details cannot be printed yet because we are still unwinding.\n"+ - "Hopefully the test harness prints the panic below, otherwise check the test logs.\n") + if l.logDir != "" { + defer func() { + // Check whether there is something to remove. + emptyDir, err := isDirEmpty(l.logDir) + if err != nil { + t.Fatal(err) } - fmt.Fprintln(OrigStderr, "test logs left over in:", l.logDir) - } else { - // Clean up. - if err := os.RemoveAll(l.logDir); err != nil { - t.Error(err) + inPanic := calledDuringPanic() + if (t.Failed() && !emptyDir) || inPanic { + // If the test failed or there was a panic, we keep the log + // files for further investigation. + if inPanic { + fmt.Fprintln(OrigStderr, "\nERROR: a panic has occurred!\n"+ + "Details cannot be printed yet because we are still unwinding.\n"+ + "Hopefully the test harness prints the panic below, otherwise check the test logs.\n") + } + fmt.Fprintln(OrigStderr, "test logs left over in:", l.logDir) + } else { + // Clean up. + if err := os.RemoveAll(l.logDir); err != nil { + t.Error(err) + } } - } - }() + }() + } if l.cleanupFn != nil { l.cleanupFn()