sql: run planHooks in a new tracing span #76084
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
For some reason, plan hooks run in a new goroutine; that goroutine communicates results through a channel, that's adapted to look like a planNode by the hookFnNode. Before this patch, that goroutine was capturing the caller's ctx, and hence the caller's tracing span. This was leading to span-use-after-Finish because the goroutine in question was sometimes outliving the caller. Although not written anywhere, I think the expectation is that generally the goroutine will not outlive the the hookFnNode by much, since hookFnNode.Next() is supposed to be consumed fully. Still, at the very least, there were races related to ctx cancellation, as the hookFnNode listens for cancellation in parallel with the goroutine running (which is probably a bad idea). This patch fixes ths span use-after-finish by giving the goroutine a new span. I took the opportunity to give all the plan hooks names, and use that as the span name. Since these hooks are all weirdos, it seems like a good idea to particularly reflect them in the trace, regardless of the bug that's being fixed. Fixes cockroachdb#75425 Release note: None
andreimatei
requested review from
msbutler and
samiskin
and removed request for
a team
|
This change is |
msbutler
approved these changes
Feb 4, 2022
yuzefovich
approved these changes
Feb 5, 2022
There was a problem hiding this comment.
Nice! It's definitely a better approach than deriving a separate tracing span whenever a new "child" DistSQL flow is spun up.
Reviewed 11 of 11 files at r1, all commit messages.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @samiskin)
|
bors r+ |
|
Build succeeded: |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
For some reason, plan hooks run in a new goroutine; that goroutine
communicates results through a channel, that's adapted to look like a
planNode by the hookFnNode. Before this patch, that goroutine was
capturing the caller's ctx, and hence the caller's tracing span. This
was leading to span-use-after-Finish because the goroutine in question
was sometimes outliving the caller. Although not written anywhere, I
think the expectation is that generally the goroutine will not outlive
the the hookFnNode by much, since hookFnNode.Next() is supposed to be
consumed fully. Still, at the very least, there were races related to
ctx cancellation, as the hookFnNode listens for cancellation in parallel
with the goroutine running (which is probably a bad idea).
This patch fixes ths span use-after-finish by giving the goroutine a new
span. I took the opportunity to give all the plan hooks names, and use
that as the span name. Since these hooks are all weirdos, it seems like
a good idea to particularly reflect them in the trace, regardless of the
bug that's being fixed.
Fixes #75425
Release note: None