server: unify and harden crdb_internal.cluster_inflight_traces #106326
Conversation
|
This change is |
yuzefovich
force-pushed
the
mt-traces-followup
branch
4 times, most recently
from
6e4235f
to
6fe5563
Compare
This commit unifies population of `crdb_internal.cluster_inflight_traces` between single-tenant and multi-tenant modes - from now on, in all deployment modes we use all available SQL instances to fetch span recordings from. The trace collector has been refactored accordingly. Additionally, the trace collector is hardened to ignore any errors that are encountered when retrieving recordings from a single SQL instance (previously, it would fail if we encountered an error when dialing the SQL instance or when receiving GetSpanRecordingResponse, and now we will log a warning and proceed silently). The rationale for making this change is that instances might no longer be "ready" when the iteration reaches them, so this should make the virtual table generation more bullet-proof. Release note: None
yuzefovich
force-pushed
the
mt-traces-followup
branch
from
6fe5563
to
5f0cd35
Compare
yuzefovich
requested review from
dhartunian,
knz and
a team
and removed request for
a team
There was a problem hiding this comment.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @dhartunian and @knz)
pkg/sql/crdb_internal.go line 1928 at r1 (raw file):
CREATE TABLE crdb_internal.cluster_inflight_traces ( trace_id INT NOT NULL, -- The trace's ID. node_id INT NOT NULL, -- The node's ID.
Do we want to rename node_id to instance_id? I'm leaning towards "no" given that "instance" seems to be internal-only concept.
There was a problem hiding this comment.
Reviewable status:
a discussion (no related file):
Overall this LGTM.
I agree with not renaming the node_id column for now. Until the operator experience for MT clusters is further understood, SQL instance ID does currently seem like an internal concept.
I suppose an alternative to completely dropping errors when dialing SQL instances, would be to send a notice or something to indicate that you might be viewing partial results. But, since we include the node_id in the output and since this is an internal function, I'm not sure that is worth it.
This was exactly my thinking. TFTR! bors r+ |
|
Build succeeded: |
This commit unifies population of
crdb_internal.cluster_inflight_tracesbetween single-tenant and multi-tenant modes - from now on, in all deployment modes we use all available SQL instances to fetch span recordings from. The trace collector has been refactored accordingly.Additionally, the trace collector is hardened to ignore any errors that are encountered when retrieving recordings from a single SQL instance (previously, it would fail if we encountered an error when dialing the SQL instance or when receiving GetSpanRecordingResponse, and now we will log a warning and proceed silently). The rationale for making this change is that instances might no longer be "ready" when the iteration reaches them, so this should make the virtual table generation more bullet-proof.
Epic: CRDB-26691
Touches: #100826
Release note: None