sql: fix and simplify session tracing span mgmt #72122
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
|
This change is |
andreimatei
force-pushed
the
tracing.use-after-finish-session-tracing
branch
4 times, most recently
from
4840d61 to
ffaef58
Compare
irfansharif
approved these changes
Oct 28, 2021
Release note: None
Before this patch, session tracing was broken in two ways: - it leaked a span, corresponding to the transaction that was ongoing when session tracing is initiated. Session tracing was replacing that span in the connection's tracking, and so nobody was ever Finish()ing it. - it exposed a span to use-after-finish. This is the span that replaced the span discussed above. When session tracing is initiated in a transaction, it overwrote the txn's span. When session tracing was finished, it finished that span. But, in case tracing is finished in the same txn as the one it started in, then that span is still referenced by the connection. This patch fixes these issues and also simplifies how session tracing works. We do away with the special handling of the transaction during which tracing starts. Instead, we just give this transaction a span inheriting from the same recording span that future transactions will inherit from. The SessionTracing struct gets out of the business of owning any spans. Release note: None
andreimatei
force-pushed
the
tracing.use-after-finish-session-tracing
branch
from
ffaef58 to
a6558fd
Compare
andreimatei
commented
Nov 4, 2021
There was a problem hiding this comment.
bors r+
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @abarganier)
|
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.
Before this patch, session tracing was broken in two ways:
when session tracing is initiated. Session tracing was replacing that
span in the connection's tracking, and so nobody was ever Finish()ing
it.
the span discussed above. When session tracing is initiated in a
transaction, it overwrote the txn's span. When session tracing was
finished, it finished that span. But, in case tracing is finished
in the same txn as the one it started in, then that span is still
referenced by the connection.
This patch fixes these issues and also simplifies how session tracing
works. We do away with the special handling of the transaction during
which tracing starts. Instead, we just give this transaction a span
inheriting from the same recording span that future transactions will
inherit from. The SessionTracing struct gets out of the business of
owning any spans.
Release note: None