log,server: avoid global variables to log/trace server IDs

Prior to this change, the server identifiers (cluster ID, node ID etc)
were stored in global variables in the `log` package.
This was problematic when a single process contains multiple servers,
e.g. in tests, `demo` and multi-tenant CockroachDB.

This change switches the mechanism to use identifiers stored in the
go context. The disadvantage is that the server IDs are not any
more logged at the beginning of each log file (since a given log file
could report data from multiple servers).

Release note (cli change): The server identifiers (cluster ID, node
ID, tenant ID, instance ID) are not any more duplicated at the start
of every new log file (during log file rotations). They are now only
logged when known during server start-up.
(The copy of the identifiers is still included in per-event envelopes
for the various `json` output logging formats.)

pkg/ccl/backupccl/backup_test.go

@@ -6857,9 +6857,6 @@ func TestBackupRestoreInsideTenant(t *testing.T) {
 	defer log.Scope(t).Close(t)
 
 	makeTenant := func(srv serverutils.TestServerInterface, tenant uint64) (*sqlutils.SQLRunner, func()) {
-		// Prevent a logging assertion that the server ID is initialized multiple times.
-		log.TestingClearServerIdentifiers()
-
 		_, conn := serverutils.StartTenant(t, srv, base.TestTenantArgs{TenantID: roachpb.MakeTenantID(tenant)})
 		cleanup := func() { conn.Close() }
 		return sqlutils.MakeSQLRunner(conn), cleanup
@@ -7015,16 +7012,11 @@ func TestBackupRestoreTenant(t *testing.T) {
 	systemDB.Exec(t, `BACKUP system.users TO 'nodelocal://1/users'`)
 	systemDB.CheckQueryResults(t, `SELECT manifest->>'tenants' FROM [SHOW BACKUP 'nodelocal://1/users' WITH as_json]`, [][]string{{"[]"}})
 
-	// Prevent a logging assertion that the server ID is initialized multiple times.
-	log.TestingClearServerIdentifiers()
-
 	_, conn11 := serverutils.StartTenant(t, srv, base.TestTenantArgs{TenantID: roachpb.MakeTenantID(11)})
 	defer conn11.Close()
 	tenant11 := sqlutils.MakeSQLRunner(conn11)
 	tenant11.Exec(t, `CREATE DATABASE foo; CREATE TABLE foo.baz(i int primary key); INSERT INTO foo.baz VALUES (111), (211)`)
 
-	log.TestingClearServerIdentifiers()
-
 	_, conn20 := serverutils.StartTenant(t, srv, base.TestTenantArgs{TenantID: roachpb.MakeTenantID(20)})
 	defer conn20.Close()
 	tenant20 := sqlutils.MakeSQLRunner(conn20)
@@ -7084,8 +7076,6 @@ func TestBackupRestoreTenant(t *testing.T) {
 			[][]string{{`100`, `0`, `0`, `0`}},
 		)
 
-		log.TestingClearServerIdentifiers()
-
 		ten10Stopper := stop.NewStopper()
 		_, restoreConn10 := serverutils.StartTenant(
 			t, restoreTC.Server(0), base.TestTenantArgs{
@@ -7130,8 +7120,6 @@ func TestBackupRestoreTenant(t *testing.T) {
 			[][]string{{`10`, `true`, `{"id": "10", "state": "ACTIVE"}`}},
 		)
 
-		log.TestingClearServerIdentifiers()
-
 		_, restoreConn10 = serverutils.StartTenant(
 			t, restoreTC.Server(0), base.TestTenantArgs{TenantID: roachpb.MakeTenantID(10), Existing: true},
 		)
@@ -7171,8 +7159,6 @@ func TestBackupRestoreTenant(t *testing.T) {
 			[][]string{{`10`, `true`, `{"id": "10", "state": "ACTIVE"}`}},
 		)
 
-		log.TestingClearServerIdentifiers()
-
 		_, restoreConn10 := serverutils.StartTenant(
 			t, restoreTC.Server(0), base.TestTenantArgs{TenantID: roachpb.MakeTenantID(10), Existing: true},
 		)
@@ -7202,8 +7188,6 @@ func TestBackupRestoreTenant(t *testing.T) {
 			},
 		)
 
-		log.TestingClearServerIdentifiers()
-
 		_, restoreConn10 := serverutils.StartTenant(
 			t, restoreTC.Server(0), base.TestTenantArgs{TenantID: roachpb.MakeTenantID(10), Existing: true},
 		)
@@ -7213,8 +7197,6 @@ func TestBackupRestoreTenant(t *testing.T) {
 		restoreTenant10.CheckQueryResults(t, `select * from foo.bar`, tenant10.QueryStr(t, `select * from foo.bar`))
 		restoreTenant10.CheckQueryResults(t, `select * from foo.bar2`, tenant10.QueryStr(t, `select * from foo.bar2`))
 
-		log.TestingClearServerIdentifiers()
-
 		_, restoreConn11 := serverutils.StartTenant(
 			t, restoreTC.Server(0), base.TestTenantArgs{TenantID: roachpb.MakeTenantID(11), Existing: true},
 		)
@@ -7233,8 +7215,6 @@ func TestBackupRestoreTenant(t *testing.T) {
 
 		restoreDB.Exec(t, `RESTORE TENANT 10 FROM 'nodelocal://1/t10' AS OF SYSTEM TIME `+ts1)
 
-		log.TestingClearServerIdentifiers()
-
 		_, restoreConn10 := serverutils.StartTenant(
 			t, restoreTC.Server(0), base.TestTenantArgs{TenantID: roachpb.MakeTenantID(10), Existing: true},
 		)
@@ -7253,8 +7233,6 @@ func TestBackupRestoreTenant(t *testing.T) {
 
 		restoreDB.Exec(t, `RESTORE TENANT 20 FROM 'nodelocal://1/t20'`)
 
-		log.TestingClearServerIdentifiers()
-
 		_, restoreConn20 := serverutils.StartTenant(
 			t, restoreTC.Server(0), base.TestTenantArgs{TenantID: roachpb.MakeTenantID(20), Existing: true},
 		)

pkg/ccl/changefeedccl/helpers_test.go

@@ -410,7 +410,6 @@ func startTestCluster(t testing.TB) (serverutils.TestClusterInterface, *gosql.DB
 func startTestTenant(
 	t testing.TB, options feedTestOptions,
 ) (serverutils.TestServerInterface, *gosql.DB, func()) {
-	log.TestingClearServerIdentifiers()
 	ctx := context.Background()
 
 	kvServer, _, cleanup := startTestFullServer(t, options)

pkg/ccl/importccl/import_stmt_test.go

@@ -6868,9 +6868,6 @@ func TestImportInTenant(t *testing.T) {
 	defer conn10.Close()
 	t10 := sqlutils.MakeSQLRunner(conn10)
 
-	// Prevent a logging assertion that the server ID is initialized multiple times.
-	log.TestingClearServerIdentifiers()
-
 	// Setup a few tenants, each with a different table.
 	_, conn11 := serverutils.StartTenant(t, tc.Server(0), base.TestTenantArgs{TenantID: roachpb.MakeTenantID(11)})
 	defer conn11.Close()

pkg/ccl/kvccl/kvtenantccl/tenant_upgrade_test.go

@@ -26,7 +26,6 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/testutils/sqlutils"
 	"github.com/cockroachdb/cockroach/pkg/testutils/testcluster"
 	"github.com/cockroachdb/cockroach/pkg/util/leaktest"
-	"github.com/cockroachdb/cockroach/pkg/util/log"
 	"github.com/cockroachdb/cockroach/pkg/util/stop"
 	"github.com/stretchr/testify/require"
 )
@@ -90,8 +89,6 @@ func TestTenantUpgrade(t *testing.T) {
 			TestingKnobs: base.TestingKnobs{},
 			Settings:     settings,
 		}
-		// Prevent a logging assertion that the server ID is initialized multiple times.
-		log.TestingClearServerIdentifiers()
 		tenant, err := tc.Server(0).StartTenant(ctx, tenantArgs)
 		require.NoError(t, err)
 		return connectToTenant(t, tenant.SQLAddr())
@@ -130,7 +127,6 @@ func TestTenantUpgrade(t *testing.T) {
 		// Restart the tenant and ensure that the version is correct.
 		cleanup()
 		{
-			log.TestingClearServerIdentifiers()
 			tenantServer, err := tc.Server(0).StartTenant(ctx, base.TestTenantArgs{
 				TenantID: roachpb.MakeTenantID(initialTenantID),
 				Existing: true,
@@ -156,7 +152,6 @@ func TestTenantUpgrade(t *testing.T) {
 		// Restart the new tenant and ensure it has the right version.
 		cleanup()
 		{
-			log.TestingClearServerIdentifiers()
 			tenantServer, err := tc.Server(0).StartTenant(ctx, base.TestTenantArgs{
 				TenantID: roachpb.MakeTenantID(postUpgradeTenantID),
 				Existing: true,
@@ -286,8 +281,6 @@ func TestTenantUpgradeFailure(t *testing.T) {
 			},
 			Settings: settings,
 		}
-		// Prevent a logging assertion that the server ID is initialized multiple times.
-		log.TestingClearServerIdentifiers()
 		return &tenantInfo{tenantArgs: &tenantArgs,
 			v2onMigrationStopper: v2onMigrationStopper}
 	}
@@ -334,7 +327,6 @@ func TestTenantUpgradeFailure(t *testing.T) {
 		// Restart the tenant and ensure that the version is correct.
 		cleanup()
 		{
-			log.TestingClearServerIdentifiers()
 			tca, cleanup := startAndConnectToTenant(t, tenantInfo)
 			defer cleanup()
 			initialTenantRunner = sqlutils.MakeSQLRunner(tca)

pkg/ccl/serverccl/statusccl/BUILD.bazel

@@ -16,7 +16,6 @@ go_library(
         "//pkg/testutils/serverutils",
         "//pkg/testutils/sqlutils",
         "//pkg/util/httputil",
-        "//pkg/util/log",
         "//pkg/util/protoutil",
         "@com_github_stretchr_testify//require",
     ],

pkg/ccl/serverccl/statusccl/tenant_grpc_test.go

@@ -91,7 +91,6 @@ func TestTenantGRPCServices(t *testing.T) {
 	sqlRunner.Exec(t, "CREATE TABLE test (id int)")
 	sqlRunner.Exec(t, "INSERT INTO test VALUES (1)")
 
-	log.TestingClearServerIdentifiers()
 	tenant2, connTenant2 := serverutils.StartTenant(t, server, base.TestTenantArgs{
 		TenantID:     tenantID,
 		Existing:     true,
@@ -110,7 +109,6 @@ func TestTenantGRPCServices(t *testing.T) {
 		require.Contains(t, string(body), "INSERT INTO test VALUES")
 	})
 
-	log.TestingClearServerIdentifiers()
 	tenant3, connTenant3 := serverutils.StartTenant(t, server, base.TestTenantArgs{
 		TenantID:     roachpb.MakeTenantID(11),
 		TestingKnobs: testingKnobs,

pkg/ccl/serverccl/statusccl/tenant_test_utils.go

@@ -24,7 +24,6 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/testutils/serverutils"
 	"github.com/cockroachdb/cockroach/pkg/testutils/sqlutils"
 	"github.com/cockroachdb/cockroach/pkg/util/httputil"
-	"github.com/cockroachdb/cockroach/pkg/util/log"
 	"github.com/cockroachdb/cockroach/pkg/util/protoutil"
 	"github.com/stretchr/testify/require"
 )
@@ -50,7 +49,6 @@ func newTestTenant(
 	tenantParams.Existing = existing
 	tenantParams.TestingKnobs = knobs
 
-	log.TestingClearServerIdentifiers()
 	tenant, tenantConn := serverutils.StartTenant(t, server, tenantParams)
 	sqlDB := sqlutils.MakeSQLRunner(tenantConn)
 	status := tenant.StatusServer().(serverpb.SQLStatusServer)

pkg/ccl/sqlproxyccl/proxy_handler_test.go

@@ -1160,7 +1160,6 @@ func newDirectoryServer(
 			return nil, status.Error(codes.NotFound, "tenant not found")
 		}
 
-		log.TestingClearServerIdentifiers()
 		tenantStopper := tenantdirsvr.NewSubStopper(tdsStopper)
 		ten, err := srv.StartTenant(ctx, base.TestTenantArgs{
 			Existing:      true,

pkg/ccl/sqlproxyccl/tenant/directory_test.go

@@ -486,7 +486,6 @@ func destroyTenant(tc serverutils.TestClusterInterface, id roachpb.TenantID) err
 func startTenant(
 	ctx context.Context, srv serverutils.TestServerInterface, id uint64,
 ) (*tenantdirsvr.Process, error) {
-	log.TestingClearServerIdentifiers()
 	tenantStopper := tenantdirsvr.NewSubStopper(srv.Stopper())
 	t, err := srv.StartTenant(
 		ctx,

pkg/ccl/streamingccl/streamingest/stream_ingestion_job_test.go

@@ -69,9 +69,6 @@ SET CLUSTER SETTING changefeed.experimental_poll_interval = '10ms'
 `)
 	require.NoError(t, err)
 
-	// Prevent a logging assertion that the server ID is initialized multiple times.
-	log.TestingClearServerIdentifiers()
-
 	// Start the destination server.
 	hDest, cleanupDest := streamingtest.NewReplicationHelper(t, base.TestServerArgs{})
 	defer cleanupDest()

pkg/ccl/testccl/sqlccl/temp_table_clean_test.go

@@ -20,7 +20,6 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/testutils/serverutils"
 	"github.com/cockroachdb/cockroach/pkg/testutils/sqlutils"
 	"github.com/cockroachdb/cockroach/pkg/util/leaktest"
-	"github.com/cockroachdb/cockroach/pkg/util/log"
 	"github.com/cockroachdb/cockroach/pkg/util/stop"
 	"github.com/cockroachdb/cockroach/pkg/util/syncutil"
 )
@@ -100,7 +99,6 @@ func TestTenantTempTableCleanup(t *testing.T) {
 			},
 		},
 	)
-	log.TestingClearServerIdentifiers()
 	tenantStoppers := []*stop.Stopper{stop.NewStopper(), stop.NewStopper()}
 	_, tenantPrimaryDB := serverutils.StartTenant(t, tc.Server(0),
 		base.TestTenantArgs{
@@ -118,16 +116,14 @@ func TestTenantTempTableCleanup(t *testing.T) {
 	tenantSQL.Exec(t, "SET experimental_enable_temp_tables = 'on'")
 	tenantSQL.Exec(t, "set cluster setting sql.temp_object_cleaner.cleanup_interval='1 seconds'")
 	tenantSQL.Exec(t, "CREATE TEMP TABLE temp_table (x INT PRIMARY KEY, y INT);")
-	// Prevent a logging assertion that the server ID is initialized multiple times.
-	log.TestingClearServerIdentifiers()
+
 	_, tenantSecondDB := serverutils.StartTenant(t, tc.Server(1),
 		base.TestTenantArgs{
 			Existing: true,
 			TenantID: serverutils.TestTenantID(),
 			Settings: settings,
 			Stopper:  tenantStoppers[1],
 		})
-	log.TestingClearServerIdentifiers()
 	tenantSecondSQL := sqlutils.MakeSQLRunner(tenantSecondDB)
 	tenantSecondSQL.CheckQueryResults(t, "SELECT table_name FROM [SHOW TABLES]",
 		[][]string{
@@ -160,8 +156,6 @@ func TestTenantTempTableCleanup(t *testing.T) {
 	// Enable our hook to allow the database to be
 	// brought up.
 	pause()
-	// Prevent a logging assertion that the server ID is initialized multiple times.
-	log.TestingClearServerIdentifiers()
 	// Once we restart the tenant, no sessions should exist
 	// so all temporary tables should be cleaned up.
 	tenantStoppers[0] = stop.NewStopper()

pkg/cli/democluster/BUILD.bazel

@@ -30,7 +30,6 @@ go_library(
         "//pkg/server/serverpb",
         "//pkg/server/status",
         "//pkg/settings",
-        "//pkg/sql",
         "//pkg/sql/catalog/catalogkeys",
         "//pkg/sql/catalog/catconstants",
         "//pkg/sql/distsql",

pkg/cli/democluster/demo_cluster.go

@@ -37,7 +37,6 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/server/serverpb"
 	"github.com/cockroachdb/cockroach/pkg/server/status"
 	"github.com/cockroachdb/cockroach/pkg/settings"
-	"github.com/cockroachdb/cockroach/pkg/sql"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/catalogkeys"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/catconstants"
 	"github.com/cockroachdb/cockroach/pkg/sql/distsql"
@@ -406,7 +405,6 @@ func (c *transientCluster) Start(
 					StartingHTTPPort: c.demoCtx.HTTPPort - 2,
 					Locality:         c.demoCtx.Localities[i],
 					TestingKnobs: base.TestingKnobs{
-						TenantTestingKnobs: &sql.TenantTestingKnobs{DisableLogTags: true},
 						Server: &server.TestingKnobs{
 							ContextTestingKnobs: rpc.ContextTestingKnobs{
 								ArtificialLatencyMap: latencyMap,

pkg/cli/start.go

@@ -745,9 +745,8 @@ If problems persist, please see %s.`
 				buf.Printf("cluster name:\t%s\n", baseCfg.ClusterName)
 			}
 
-			// Remember the cluster ID for log file rotation.
+			// Report the server identifiers.
 			clusterID := s.ClusterID().String()
-			log.SetNodeIDs(clusterID, int32(nodeID))
 			buf.Printf("clusterID:\t%s\n", clusterID)
 			buf.Printf("nodeID:\t%d\n", nodeID)
 

pkg/server/tenant.go

@@ -281,17 +281,6 @@ func StartTenant(
 	// The InstanceID subsystem is not available until `preStart`.
 	args.rpcContext.NodeID.Set(ctx, roachpb.NodeID(s.SQLInstanceID()))
 
-	if knobs, ok := baseCfg.TestingKnobs.TenantTestingKnobs.(*sql.TenantTestingKnobs); !ok || !knobs.DisableLogTags {
-		// Register the server's identifiers so that log events are
-		// decorated with the server's identity. This helps when gathering
-		// log events from multiple servers into the same log collector.
-		//
-		// We do this only here, as the identifiers may not be known before this point.
-		clusterID := args.rpcContext.ClusterID.Get().String()
-		log.SetNodeIDs(clusterID, 0 /* nodeID is not known for a SQL-only server. */)
-		log.SetTenantIDs(args.TenantID.String(), int32(s.SQLInstanceID()))
-	}
-
 	externalUsageFn := func(ctx context.Context) multitenant.ExternalUsage {
 		userTimeMillis, _, err := status.GetCPUTime(ctx)
 		if err != nil {

pkg/sql/exec_util.go

@@ -1344,10 +1344,6 @@ type TenantTestingKnobs struct {
 	// OverrideTokenBucketProvider allows a test-only TokenBucketProvider (which
 	// can optionally forward requests to the real provider).
 	OverrideTokenBucketProvider func(origProvider kvtenant.TokenBucketProvider) kvtenant.TokenBucketProvider
-
-	// DisableLogTags can be set to true to cause the tenant server to avoid
-	// setting any global log tags for cluster id or node id.
-	DisableLogTags bool
 }
 
 var _ base.ModuleTestingKnobs = &TenantTestingKnobs{}

pkg/sql/logictest/logic.go

@@ -1542,9 +1542,6 @@ func (t *logicTest) newCluster(serverArgs TestServerArgs, opts []clusterOpt) {
 				Existing:          i > 0,
 			}
 
-			// Prevent a logging assertion that the server ID is initialized multiple times.
-			log.TestingClearServerIdentifiers()
-
 			tenant, err := t.cluster.Server(i).StartTenant(context.Background(), tenantArgs)
 			if err != nil {
 				t.rootT.Fatalf("%+v", err)

pkg/sql/sqltestutils/BUILD.bazel

@@ -30,7 +30,6 @@ go_library(
         "//pkg/testutils/skip",
         "//pkg/testutils/sqlutils",
         "//pkg/util/cloudinfo",
-        "//pkg/util/log",
         "//pkg/util/protoutil",
         "//pkg/util/treeprinter",
         "@com_github_cockroachdb_datadriven//:datadriven",

pkg/sql/sqltestutils/telemetry.go

@@ -34,7 +34,6 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/testutils/skip"
 	"github.com/cockroachdb/cockroach/pkg/testutils/sqlutils"
 	"github.com/cockroachdb/cockroach/pkg/util/cloudinfo"
-	"github.com/cockroachdb/cockroach/pkg/util/log"
 	"github.com/cockroachdb/cockroach/pkg/util/treeprinter"
 	"github.com/cockroachdb/datadriven"
 	"github.com/cockroachdb/errors"
@@ -156,9 +155,6 @@ func (tt *telemetryTest) Start(t *testing.T, serverArgs []base.TestServerArgs) {
 	tt.serverDB = tt.cluster.ServerConn(0)
 	tt.prepareCluster(tt.serverDB)
 
-	// Prevent a logging assertion that the server ID is initialized multiple times.
-	log.TestingClearServerIdentifiers()
-
 	tt.tenant, tt.tenantDB = serverutils.StartTenant(tt.t, tt.server, base.TestTenantArgs{
 		TenantID:                    serverutils.TestTenantID(),
 		AllowSettingClusterSettings: true,

pkg/util/log/BUILD.bazel

@@ -37,6 +37,7 @@ go_library(
         "log_flush.go",
         "redact.go",
         "registry.go",
+        "server_ident.go",
         "sinks.go",
         "stderr_redirect.go",
         "stderr_redirect_unix.go",