From 9e0d30c1e9c384068f3bdb2c494162219b413c32 Mon Sep 17 00:00:00 2001
From: Miral Gadani <miral@cockroachlabs.com>
Date: Wed, 13 Sep 2023 01:19:30 +0000
Subject: [PATCH] roachtest: default logging to file instead of stderr

Previously, stderr/out would be cluttered by unuseful log statements
from various components like `testutils` and `sarama`, which are
configured to utilise the pkg/util/log. This is, by default, set to
use a stderr sink. It makes parsing the TC roachtest build log
especially cumbersome..

This PR introduces a roachtest specific configuration which filters all
but FATAL events to stderr, and preserves what was previously being
written, by including a file sink that writes to `artifactsDir`.

Epic: none
Release note: None
---
 pkg/cmd/roachtest/BUILD.bazel |  2 ++
 pkg/cmd/roachtest/main.go     | 42 ++++++++++++++++++++++++++---------
 2 files changed, 33 insertions(+), 11 deletions(-)

diff --git a/pkg/cmd/roachtest/BUILD.bazel b/pkg/cmd/roachtest/BUILD.bazel
index 0d1b2e0a9f6a..586fe2a2a091 100644
--- a/pkg/cmd/roachtest/BUILD.bazel
+++ b/pkg/cmd/roachtest/BUILD.bazel
@@ -41,6 +41,8 @@ go_library(
         "//pkg/util/httputil",
         "//pkg/util/leaktest",
         "//pkg/util/log",
+        "//pkg/util/log/logconfig",
+        "//pkg/util/log/logpb",
         "//pkg/util/quotapool",
         "//pkg/util/randutil",
         "//pkg/util/stop",
diff --git a/pkg/cmd/roachtest/main.go b/pkg/cmd/roachtest/main.go
index e88599bfd79e..0c2b6d0fd5cc 100644
--- a/pkg/cmd/roachtest/main.go
+++ b/pkg/cmd/roachtest/main.go
@@ -34,6 +34,9 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/roachprod/vm"
 	"github.com/cockroachdb/cockroach/pkg/util/allstacks"
 	"github.com/cockroachdb/cockroach/pkg/util/leaktest"
+	"github.com/cockroachdb/cockroach/pkg/util/log"
+	"github.com/cockroachdb/cockroach/pkg/util/log/logconfig"
+	"github.com/cockroachdb/cockroach/pkg/util/log/logpb"
 	"github.com/cockroachdb/cockroach/pkg/util/stop"
 	"github.com/cockroachdb/cockroach/pkg/util/timeutil"
 	"github.com/cockroachdb/errors"
@@ -48,19 +51,21 @@ import (
 // details. Also, if the exit codes here change, they need to updated
 // on that script accordingly.
 
-// ExitCodeTestsFailed is the exit code that results from a run of
-// roachtest in which the infrastructure worked, but at least one
-// test failed.
-const ExitCodeTestsFailed = 10
+const (
+	// ExitCodeTestsFailed is the exit code that results from a run of
+	// roachtest in which the infrastructure worked, but at least one
+	// test failed.
+	ExitCodeTestsFailed = 10
 
-// ExitCodeClusterProvisioningFailed is the exit code that results
-// from a run of roachtest in which some clusters could not be
-// created due to errors during cloud hardware allocation.
-const ExitCodeClusterProvisioningFailed = 11
+	// ExitCodeClusterProvisioningFailed is the exit code that results
+	// from a run of roachtest in which some clusters could not be
+	// created due to errors during cloud hardware allocation.
+	ExitCodeClusterProvisioningFailed = 11
 
-// runnerLogsDir is the dir under the artifacts root where the test runner log
-// and other runner-related logs (i.e. cluster creation logs) will be written.
-const runnerLogsDir = "_runner-logs"
+	// runnerLogsDir is the dir under the artifacts root where the test runner log
+	// and other runner-related logs (i.e. cluster creation logs) will be written.
+	runnerLogsDir = "_runner-logs"
+)
 
 // Only used if passed otherwise refer to ClusterSpec.
 // If a new flag is added here it should also be added to createFlagsOverride().
@@ -423,6 +428,20 @@ runner itself.
 	}
 }
 
+// This diverts all the default non fatal logging to a file in `baseDir`. This is particularly
+// useful in CI, where without this, stderr/stdout are cluttered with logs from various
+// packages used in roachtest like sarama and testutils.
+func setLogConfig(baseDir string) {
+	logConf := logconfig.DefaultStderrConfig()
+	logConf.Sinks.Stderr.Filter = logpb.Severity_FATAL
+	if err := logConf.Validate(&baseDir); err != nil {
+		panic(err)
+	}
+	if _, err := log.ApplyConfig(logConf); err != nil {
+		panic(err)
+	}
+}
+
 type cliCfg struct {
 	args                   []string
 	count                  int
@@ -499,6 +518,7 @@ func runTests(register func(registry.Registry), cfg cliCfg, benchOnly bool) erro
 		return errors.Newf("--debug-always is only allowed when running a single test")
 	}
 
+	setLogConfig(cfg.artifactsDir)
 	runnerDir := filepath.Join(cfg.artifactsDir, runnerLogsDir)
 	runnerLogPath := filepath.Join(
 		runnerDir, fmt.Sprintf("test_runner-%d.log", timeutil.Now().Unix()))