release-20.2: util/log: conditionally include the tenant+server IDs on every line

[knz]

Fixes #58705.

For CC security logging we want the ability to route the logging
events from the files where they are written into a centralized
logging collector.

However this routing is done line-by-line. To enable log aggregation
across multiple clusters, or multiple nodes, we need to disambiguate
which log entries come from which cluster and which node.

This patch accommodates this requirement by adding the cluster ID and,
for tenant servers, the tenant and SQL instance ID, on every output
line when the env var COCKROACH_ALWAYS_LOG_SERVER_IDS is set to a
true-ish value.

Note: this feature is unneeded in v21.1 because in that version JSON
logging is available and that already includes the server identity
bits.

Release note: None

[cockroach-teamcity]

This change is 

[knz]

NB: The PR is draft still because this needs testing with SQL tenant servers, not just KV pods.

Also hoping to get input from @tbg about where to best position the log.Set calls. Is the change in testserver.go adequate?

[tbg]

Reviewed 4 of 4 files at r1.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andy-kimball and @knz)

---

pkg/server/testserver.go, line 718 at r1 (raw file):

	}

	// Inform the logging package of the cluster identifiers.

Yeah, this seems fine. There's a chance of a few log lines going out before this line, which I'm sure you're aware of. But the clusterID isn't known at startup, so this is hard to avoid.

[andy-kimball]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @adityamaru, @andy-kimball, @knz, and @tbg)

---

pkg/server/testserver.go, line 718 at r1 (raw file):

Previously, tbg (Tobias Grieger) wrote…

Yeah, this seems fine. There's a chance of a few log lines going out before this line, which I'm sure you're aware of. But the clusterID isn't known at startup, so this is hard to avoid.

I don't think sqlServerOptionalKVArgs is supposed to be available for SQL-only nodes, is it?

// sqlServerOptionalKVArgs are the arguments supplied to newSQLServer which are
// only available if the SQL server runs as part of a KV node.

I think you want to use s.SQLInstanceID() to get it instead.

---

pkg/util/log/clog.go, line 86 at r2 (raw file):

	// The following identifiers are reported when enabled.
	tenantID      syncutil.AtomicString
	sqlInstanceID int32

I'd beef up the comment to specify that sqlInstanceID must always be accessed atomically.

[knz]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @adityamaru, @andy-kimball, and @tbg)

---

pkg/server/testserver.go, line 718 at r1 (raw file):

Previously, andy-kimball (Andy Kimball) wrote…

I don't think sqlServerOptionalKVArgs is supposed to be available for SQL-only nodes, is it?

// sqlServerOptionalKVArgs are the arguments supplied to newSQLServer which are
// only available if the SQL server runs as part of a KV node.

I think you want to use s.SQLInstanceID() to get it instead.

Done.

---

pkg/util/log/clog.go, line 86 at r2 (raw file):

Previously, andy-kimball (Andy Kimball) wrote…

I'd beef up the comment to specify that sqlInstanceID must always be accessed atomically.

Done.

[knz]

@logston I've been baby sitting this PR to finish its CI and it's taking me beyond working hours. I think it will all right in the end but the teamcity agents get restarted (probably because of some transient issues). Feel free to merge this when it's green. I'm signing off for the day.

[logston]

Will do. Sleep well @knz!

[andy-kimball]

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @adityamaru, @andy-kimball, and @tbg)

[andy-kimball]

I just merged it. Thanks @knz for getting it over finish line.