colexec: derive a tracing span in materializer when collecting stats#81995
colexec: derive a tracing span in materializer when collecting stats#81995craig[bot] merged 1 commit intocockroachdb:masterfrom
Conversation
|
This change is |
msirek
left a comment
There was a problem hiding this comment.
Nice fix!
Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @cucaroach and @yuzefovich)
pkg/sql/colexec/materializer.go line 257 at r2 (raw file):
} else { ctx = m.StartInternalNoSpan(ctx) }
The code changes look good, but I have a question. The RowSource interface says: "RowSources that consume other RowSources are expected to Start() their inputs. " Is there are reason that Materializer.Start doesn't call Start on its input?
yuzefovich
left a comment
There was a problem hiding this comment.
The reproduction here requires a lot of data, so I think it's ok to merge without a regression test. There isn't really any downside to this change too (having an extra tracing span incurs some overhead, but when we're collecting stats, we don't really care about that).
TFTR!
bors r+
Reviewable status:
pkg/sql/colexec/materializer.go line 257 at r2 (raw file):
Previously, msirek (Mark Sirek) wrote…
The code changes look good, but I have a question. The
RowSourceinterface says: "RowSources that consume other RowSources are expected to Start() their inputs. " Is there are reason thatMaterializer.Startdoesn't callStarton itsinput?
Materializer is an adapter from the colexecop.Operator interface to the RowSource interface. Operator.Init is roughly equivalent to RowSource.Start, and a few lines below, wrapped in a panic catcher, we do call Init on the input. The adapter in the opposite direction (from the RowSource to Operator) is the Columnarizer, and Columnarizer.Init calls input.Start.
msirek
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/sql/colexec/materializer.go line 257 at r2 (raw file):
Previously, yuzefovich (Yahor Yuzefovich) wrote…
Materializeris an adapter from thecolexecop.Operatorinterface to theRowSourceinterface.Operator.Initis roughly equivalent toRowSource.Start, and a few lines below, wrapped in a panic catcher, we do callIniton the input. The adapter in the opposite direction (from theRowSourcetoOperator) is theColumnarizer, andColumnarizer.Initcallsinput.Start.
👍
|
Build failed (retrying...): |
|
Build failed (retrying...): |
|
Looks like the newly added tracing span is being used after it was finished, I'll need to look into that. bors r- |
|
Canceled. |
7fee8d2 to
fd0d5d3
Compare
yuzefovich
left a comment
There was a problem hiding this comment.
I had to make some tweaks to how the materializer is closed. Please take another look.
The problem was that some of the closers would try to log an event into the already finished tracing span (which happens automatically in InternalClose, which was previously done before closing the closers), so this would lead to use-after-finish crash with tracing infra. Some examples of such closers are rowexec.orderedAggregator.close, rowexec.hashAggregator.close, colexec.externalSorter.Close.
Reviewable status:
pkg/sql/colexec/materializer.go line 257 at r2 (raw file):
Previously, msirek (Mark Sirek) wrote…
👍
Done.
msirek
left a comment
There was a problem hiding this comment.
Thanks for the explanation. The updated code looks good.
Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status:
|
This change doesn't seem worth backporting given that it's not exactly trivial, so I think we'll just merge it on master. TFTRs! bors r+ |
|
Build failed: |
|
Hm, looks like this change slows down the tests somehow so that they time out (or more likely to time out). I'll need to take a closer look at that. |
c88ee56 to
220b6ff
Compare
ab41ee5 to
243bb42
Compare
f91d5da to
530db9c
Compare
If we don't do this, then the stats would be attached to the span of the materializer's user, and if that user itself has a lot of payloads to attach (e.g. a joinReader attaching the KV keys it looked up), then the stats might be dropped based on the maximum size of structured payload per tracing span of 10KiB (see `tracing.maxStructuredBytesPerSpan`). Deriving a separate span guarantees that the stats won't be dropped. This required some changes to make a test - that makes too many assumptions about tracing infra - work. Release note: None
|
I think I figured it out - Since I only modified the testing knobs behavior since I got LGTMs, I don't think extra reviews are needed. The only failure I saw on CI is #85934 which is present on master. bors r+ |
|
👎 Rejected by label |
|
bors r+ |
|
Build failed (retrying...): |
|
Build succeeded: |
If we don't do this, then the stats would be attached to the span of
the materializer's user, and if that user itself has a lot of
payloads to attach (e.g. a joinReader attaching the KV keys it looked
up), then the stats might be dropped based on the maximum size of
structured payload per tracing span of 10KiB (see
tracing.maxStructuredBytesPerSpan). Deriving a separate spanguarantees that the stats won't be dropped.
This required some changes to make a test - that makes too many
assumptions about tracing infra - work.
Release note: None