tracing,sql: remove option to bypass the tracing registry #59315
Comments
irfansharif
added
C-bug
|
Was there a specific test that was failing? I assume this is specific to an edge case in the draining code path, but knowing which test will make it clear how to reproduce and what a solution might look like. |
|
I don't remember off-hand, but I think |
|
Just to make sure this is clear, this is a must-fix for 21.1 as otherwise the value of the tracing work is severely diminished - if SQL spans are not in the registry, we get a lot less insight from the registry. |
|
Heads up that #59992 introduces a testing knob that can be used from logic tests which works around the problem. That means we should be good for testing, but we (SQL exec 😄) still need to look into this in the stability period. |
1. Require admin role for `crdb_internal.node_inflight_trace_spans` 2. Add num_structured column to that table (this can be seen as temporary for testing; we can remove it once cockroachdb#55733 is fully addressed and payloads can be introspected separately). 3. Add workaround for failure to finish sql txn spans (cockroachdb#59315) for use in logic tests, which together with 2.) and 4.) enables a better `contention_event` test. 4. Add `crdb_internal.trace_id` builtin. Release note: None
|
Confirming that we're planning on taking a look at this during the stability period next milestone. |
1. Require admin role for `crdb_internal.node_inflight_trace_spans` 2. Add num_structured column to that table (this can be seen as temporary for testing; we can remove it once cockroachdb#55733 is fully addressed and payloads can be introspected separately). 3. Add workaround for failure to finish sql txn spans (cockroachdb#59315) for use in logic tests, which together with 2.) and 4.) enables a better `contention_event` test. 4. Add `crdb_internal.trace_id` builtin. Release note: None
1. Require admin role for `crdb_internal.node_inflight_trace_spans` 2. Add num_structured column to that table (this can be seen as temporary for testing; we can remove it once cockroachdb#55733 is fully addressed and payloads can be introspected separately). 3. Add workaround for failure to finish sql txn spans (cockroachdb#59315) for use in logic tests, which together with 2.) and 4.) enables a better `contention_event` test. 4. Add `crdb_internal.trace_id` builtin. Release note: None
1. Require admin role for `crdb_internal.node_inflight_trace_spans` 2. Add num_structured column to that table (this can be seen as temporary for testing; we can remove it once cockroachdb#55733 is fully addressed and payloads can be introspected separately). 3. Add workaround for failure to finish sql txn spans (cockroachdb#59315) for use in logic tests, which together with 2.) and 4.) enables a better `contention_event` test. 4. Add `crdb_internal.trace_id` builtin. Release note: None
In #58902 we introduced a leak-detection for un-Finish()ed tracing.Spans. We did this by making sure that on server shutdown the tracing registry was empty (all newly started spans create an entry there, and remove themselves when finished). In doing so we observed that spans created within
txnState.resetForNewSQLTxnaren't Finish()-ed during server shut down. We introduced atracing.WithBypassRegistry()for that codepath, with the intention of addressing it pre-21.1. It'd be nice to figure out how we could ensure that the Spans are finished explicitly so that we're able to grab a hold of them in the registry (which will then let us introspect them).The text was updated successfully, but these errors were encountered: