[Single Span Sampling] Consider traces rejected solely by priority sampling as rejected#2131
Merged
marcotc merged 3 commits intoJul 28, 2022
Conversation
marcotc
force-pushed
the
single-span-fix-priority-sample
branch
2 times, most recently
from
July 6, 2022 20:57
a7073ed to
282196b
Compare
Codecov Report
@@ Coverage Diff @@
## feature-single-span-sampling #2131 +/- ##
=============================================================
Coverage 97.53% 97.53%
=============================================================
Files 1047 1047
Lines 54232 54256 +24
=============================================================
+ Hits 52896 52920 +24
Misses 1336 1336
📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more |
marcotc
force-pushed
the
single-span-fix-priority-sample
branch
from
July 9, 2022 00:16
282196b to
f926088
Compare
ivoanjo
reviewed
Jul 11, 2022
Co-authored-by: Ivo Anjo <[email protected]>
ivoanjo
approved these changes
Jul 12, 2022
Comment on lines
-33
to
+36
| return if trace_op.sampled? | ||
| return if trace_op.sampled? && trace_op.priority_sampled? |
Member
There was a problem hiding this comment.
This change is incredibly important, and incredibly subtle. The description you wrote on the PR description is quite helpful in understanding why this is done this way -- can I convince you to copy paste it here as a comment, so it stays closer to the code?
Member
There was a problem hiding this comment.
(I still kinda think the longer version in the PR description deserves a life as a comment right here above this line where it applies)
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
3 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.
The tracer flushes all traces that have not been dropped by the internal sampler (
TraceOperation#sampled == falsemeans dropped internally). This includes traces that might be dropped due to their Priority Sampling status downstream.For Single Span Sampling, we need to guarantee that Single Span Sampled spans are kept and thus cannot trust that all spans flushed by the tracer are kept, unless it's both selected by the internal sampler (
TraceOperation#sampled == true) the Priority Sampling status are set to keep (TraceOperation#sampling_priority > 0).This logic is different from the logic used to flush traces through the transport: the tracer wants to send all traces that have not been rejected internally OR traces with Priority Sampling > 0. For Single Span Sampling, we only want to apply Single Span Sampling to spans that we know are going to be dropped, either internally or by the agent. This means that there's an intersection of states where these two processes diverge: when the tracer sends the trace to the agent, but it's dropped by priority sampling.
This PR ensures that this intersecting state is correctly handled for Single Span Sampling.
Single Span Sampling is moving the tracer more towards making independent sampling decisions, and thus having a bit more complicated logic, which currently resides in the agent.