sql: use stmt's span for exec stats propagation#61532
sql: use stmt's span for exec stats propagation#61532craig[bot] merged 1 commit intocockroachdb:masterfrom
Conversation
|
This change is |
asubiotto
left a comment
There was a problem hiding this comment.
Reviewed 2 of 2 files at r1.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @RaduBerinde and @yuzefovich)
pkg/sql/instrumentation.go, line 169 at r1 (raw file):
ih.savePlanForStats = appStats.shouldSaveLogicalPlanDescription(fingerprint, implicitTxn) if sp := tracing.SpanFromContext(ctx); sp == nil {
Should we make this a util.CrdbTestBuild assertion instead? I'm not comfortable introducing this panic.
595312a to
c214a15
Compare
yuzefovich
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/sql/instrumentation.go, line 169 at r1 (raw file):
Previously, asubiotto (Alfonso Subiotto Marqués) wrote…
Should we make this a
util.CrdbTestBuildassertion instead? I'm not comfortable introducing this panic.
Done.
c214a15 to
25fa2cf
Compare
asubiotto
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/sql/instrumentation.go, line 174 at r2 (raw file):
panic(errors.AssertionFailedf("the context doesn't have a tracing span")) } return ctx, false
Sorry for being so nitpicky, but why are we returning early here? Granted that a nil span is unexpected, but it seems nicer to fallback to the code below. If we don't want to collect bundles etc... we'll return below, and if we do, we'll create our own span. If this is nil for some reason and a customer is trying to get a debug zip, it's going to be confusing and annoying when the bundle is empty.
Previously, when sampling the statement, we would always create a new tracing span. However, there is another span that we can use instead: we always create a tracing span for each statement in `connExecutor.execCmd`. That span is not used directly for anything and is needed because the transactions expect that a span is present in their context. This commit utilizes the present tracing span for the sampling purposes which gives us a performance boost (some benchmarks show that this eliminates about a quarter of the performance overhead with "always on" sampling"). Release justification: low-risk update to new functionality. Release note: None
25fa2cf to
842d79b
Compare
yuzefovich
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/sql/instrumentation.go, line 174 at r2 (raw file):
Previously, asubiotto (Alfonso Subiotto Marqués) wrote…
Sorry for being so nitpicky, but why are we returning early here? Granted that a nil span is unexpected, but it seems nicer to fallback to the code below. If we don't want to collect bundles etc... we'll return below, and if we do, we'll create our own span. If this is nil for some reason and a customer is trying to get a debug zip, it's going to be confusing and annoying when the bundle is empty.
Done. Simply "falling back" to the code below didn't work, and I didn't want to introduce some code path to handle a case that should never happen in the first place.
asubiotto
left a comment
There was a problem hiding this comment.
Reviewed 2 of 2 files at r3.
Reviewable status:
|
TFTR! bors r+ |
|
Build succeeded: |
This is a "by-hand" revert of 842d79b and motivates the next commit. To reduce the memory overhead for tracing, statements will stop using real spans unless absolutely required (for e.g. when SET TRACING = on). The intent behind cockroachdb#61532 was to reduce the overhead of sampled statements by not creating an additional new span when one was already available (the statement's span). Well, with the next commit, the statement's span will be a no-op one, so when we know that we're sampling we'll want create a new span. In the end, with respect to spans created, we'll be doing no worse for sampled statements relative to cockroachdb#61532. Release note: None
This is a "by-hand" revert of 842d79b and motivates the next commit. The only "by-hand" bits comes from wanting to retain the comments added in that commit. To reduce the memory overhead for tracing, statements will stop using real spans unless absolutely required (for e.g. when SET TRACING = on). The intent behind cockroachdb#61532 was to reduce the overhead of sampled statements by not creating an additional new span when one was already available (the statement's span). Well, with the next commit, the statement's span will be a no-op one, so when we know that we're sampling we'll want create a new span. In the end, with respect to spans created, we'll be doing no worse for sampled statements relative to cockroachdb#61532. Release note: None
This is a "by-hand" revert of 842d79b and motivates the next commit. The only "by-hand" bits comes from wanting to retain the comments added in that commit. To reduce the memory overhead for tracing, statements will stop using real spans unless absolutely required (for e.g. when SET TRACING = on). The intent behind cockroachdb#61532 was to reduce the overhead of sampled statements by not creating an additional new span when one was already available (the statement's span). Well, with the next commit, the statement's span will be a no-op one, so when we know that we're sampling we'll want create a new span. In the end, with respect to spans created, we'll be doing no worse for sampled statements relative to cockroachdb#61532. Release note: None
Previously, when sampling the statement, we would always create a new
tracing span. However, there is another span that we can use instead:
we always create a tracing span for each statement in
connExecutor.execCmd. That span is not used directly for anything andis needed because the transactions expect that a span is present in
their context. This commit utilizes the present tracing span for the
sampling purposes which gives us a performance boost (some benchmarks
show that this eliminates about a quarter of the performance overhead
with "always on" sampling").
Addresses: #59379.
Release justification: low-risk update to new functionality.
Release note: None