release-20.2: util/log: conditionally include the tenant+server IDs on every line#58708
Conversation
|
This change is |
|
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 |
tbg
left a comment
There was a problem hiding this comment.
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.
00aee5f to
dfb01b6
Compare
60e315b to
301152a
Compare
andy-kimball
left a comment
There was a problem hiding this comment.
Reviewable status:
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
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/server/testserver.go, line 718 at r1 (raw file):
Previously, andy-kimball (Andy Kimball) wrote…
I don't think
sqlServerOptionalKVArgsis 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
sqlInstanceIDmust always be accessed atomically.
Done.
d9a1bca to
9837545
Compare
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
9837545 to
ebbc531
Compare
|
@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. |
|
Will do. Sleep well @knz! |
andy-kimball
left a comment
There was a problem hiding this comment.
Reviewable status:
|
I just merged it. Thanks @knz for getting it over finish line. |
58275: roachprod: Support AWS GP3 drives. r=petermattis a=miretskiy Add support for spinning up VMs with GP3 drives. Make it possible to specify multiple, possibly different EBS volumes for each aws instance. Release Notes: None 58862: server,cli: ensure the server identifiers are properly logged in SQL pds r=itsbilal a=knz This is a forward-port of a change (#58708) we've discovered was necessary in v20.2. Release note: None Co-authored-by: Yevgeniy Miretskiy <yevgeniy@cockroachlabs.com> Co-authored-by: Raphael 'kena' Poss <knz@thaumogen.net>
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_IDSis set to atrue-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