stop, kvclient: include DistSender partial batches in traces#67713
Merged
craig[bot] merged 3 commits intocockroachdb:masterfrom Jul 20, 2021
Merged
stop, kvclient: include DistSender partial batches in traces#67713craig[bot] merged 3 commits intocockroachdb:masterfrom
craig[bot] merged 3 commits intocockroachdb:masterfrom
Conversation
Implement RunLimitedAsyncTask in terms of RunAsyncTask. This also unifies the behavior w.r.t the provided taskName - one of the two methods was prepending an "[async]" and the other wasn't. Release note: None
Add a new stoppper.RunAsyncTaskEx(), which takes an optional semaphore argument. The new method uses an extensible set of options, as more options are sure to come in the future. Release note: None
Before this patch, since fairly recently, the spans created for Stopper.RunAsyncTask[Ex]() were not included in the recording of the caller's span because these spans were created with the ForkSpan(). This patch adds an option to RunAsyncEx to make the task's span a child of the caller's, and thus to be included in the caller's recording. This option is used by the DistSender when sending partial batch requests, therefore re-including these requests in higher-level traces (as they used to be). The reason why async tasks were detached from the caller's span in cockroachdb#59815 was because of concerns about parent and children spans with very different lifetimes becoming tied through a trace, delaying the garbage collection of the whole trace until the last span in it finishes. This patch gives the caller of RunAsyncTaskEx control over this behavior; in the particular case of the DistSender, the traces are not too long lived and the DistSender waits for the async tasks it spawns to finish. In such circumstances, tying the allocations of the child spans to the whole trace should be fine. Release note (general change): A recent release removed parts of some queries from the debugging traces of those queries. This information (i.e. the execution of some low-level RPCs) has been re-included in the traces.
Member
|
This change is |
Contributor
aayushshah15
left a comment
There was a problem hiding this comment.
Thanks!
Reviewable status:
complete! 1 of 0 LGTMs obtained (waiting on @tbg)
tbg
approved these changes
Jul 20, 2021
Member
tbg
left a comment
There was a problem hiding this comment.
Didn't do a thorough review since this is code I've already looked at on the original PR.
Reviewed 1 of 1 files at r1, 10 of 10 files at r2, 7 of 7 files at r3.
Reviewable status:
andreimatei
commented
Jul 20, 2021
Contributor
Author
andreimatei
left a comment
There was a problem hiding this comment.
bors r+
Reviewable status:
Contributor
|
Build succeeded: |
Contributor
|
@andreimatei how do we feel about backporting this to v21.1? |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
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.
These are the first few patches from #66387 - extracting the non-contentious prefix dealing with DistSender tracing.
Before this patch, since fairly recently, the spans created for
Stopper.RunAsyncTaskEx were not included in the recording of the
caller's span because these spans were created with the ForkSpan(). This
patch adds an option to RunAsyncEx to make the task's span a child of
the caller's, and thus to be included in the caller's recording. This
option is used by the DistSender when sending partial batch requests,
therefore re-including these requests in higher-level traces (as they
used to be).
The reason why async tasks were detached from the caller's span
in #59815 was because of concerns about parent and children spans with
very different lifetimes becoming tied through a trace, delaying the
garbage collection of the whole trace until the last span in it
finishes. This patch gives the caller of RunAsyncTaskEx control over
this behavior; in the particular case of the DistSender, the traces are
not too long lived and the DistSender waits for the async tasks it
spawns to finish. In such circumstances, tying the allocations of the
child spans to the whole trace should be fine.
Release note (general change): A recent release removed parts of some
queries from the debugging traces of those queries. This information
(i.e. the execution of some low-level RPCs) has been re-included in the
traces.