Better span sampler #903
Open
Better span sampler #903
Conversation
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
solnic
force-pushed
the
new-span-sampler
branch
4 times, most recently
from
5a841bb to
01a7066
Compare
This updates OpenTelemetry.Sampler so that it uses `traces_sample_rate` for sampling spans before they get sent to the span processor. This way we're not processing spans when they are dropped during sampling, which was previously the case as the Client was responsible for sampling just before attemping to send a transaction.
Previously it incorrectly referred to "parent" where in reality we're dealing with spans from the same trace and respect sampling decisions that were already made for a given trace. This is not the same thing as parent/child spans that we're handling in the SpanProcessor as this type of relationship is established post-sampling.
whatyouhide
approved these changes
Jun 6, 2025
Comment on lines
+84
to
+100
| cond do | ||
| sample_rate == 0.0 -> | ||
| tracestate = build_tracestate(sample_rate, 1.0, false) | ||
| {:drop, [], tracestate} | ||
|
|
||
| sample_rate == 1.0 -> | ||
| tracestate = build_tracestate(sample_rate, 0.0, true) | ||
| {:record_and_sample, [], tracestate} | ||
|
|
||
| true -> | ||
| random_value = :rand.uniform() | ||
| sampled = random_value < sample_rate | ||
|
|
||
| tracestate = build_tracestate(sample_rate, random_value, sampled) | ||
| decision = if sampled, do: :record_and_sample, else: :drop | ||
| {decision, [], tracestate} | ||
| end |
There was a problem hiding this comment.
Refactoring suggestion:
{sampled, random_value} =
cond do
sample_rate == 0.0 -> {false, 1.0}
sample_rate == 1.0 -> {true, 0.0}
true -> random_value = :rand.uniform(); {random_value < sample_rate, random_value}
end
tracestate = build_tracestate(sample_rate, random_value, sampled)
decision = if sampled, do: :record_and_sample, else: :drop
{decision, [], tracestate}
|
just noting the first change that is necessary, but we can defer this to the future because we don't have trace propagation implemented right now:
|
sl0thentr0py
requested changes
Jun 6, 2025
| span_ctx -> Span.span_id(span_ctx) | ||
| end | ||
|
|
||
| transaction = |
There was a problem hiding this comment.
this is a useless allocation, can you just make a new method on ClientReport that just takes a single record_discarded_event(reason, data_category)?
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.
This updates OpenTelemetry.Sampler so that it uses
traces_sample_ratefor sampling spans before they get sent to the span processor.This way we're not processing spans when they are
dropped during sampling, which was previously the
case as the Client was responsible for sampling
just before attempting to send a transaction.
This is based on:
#skip-changelog