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.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add sampler API, use in SDK tracer #225
Add sampler API, use in SDK tracer #225
Changes from all commits
496320d
21dd081
9ac4516
5e0370a
bc8782d
9a8f3c1
4264f16
567a7da
e743fdc
d90a8bc
7e0330b
a0978b2
7b1fccb
9b935ad
8de632d
644cf9d
6f0a33d
0de147e
c3efeb0
b24a465
235d74f
3594e32
1a4f96b
d6127b0
0e370ad
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to the W3 spec, you should use the high ("left") 8 bytes instead:
But I wonder if we can find a more robust way to maybe randomly mix some bits here and there together.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I considered this but decided to stick to the OC behavior (or at least the intended behavior: this also fixes a rounding/OBO bug). FWIW checking the high bytes also seems more correct to me, but in practice -- if people are using short trace IDs in the wild -- sampling every request seems worse than sampling based on the non-random part.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you're right about this. I suggested it as a change to the spec in open-telemetry/opentelemetry-specification#331.
Another fun benefit of checking bytes high-to-low is that the sampling decision should be mostly consistent between samplers that check different numbers of bytes. Unlike checking low-to-high where every incremental bit is effectively another coin toss. Ultimately we'll probably just put this number in the spec, but it's a neat property in any case. :D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI - keep an eye on https://github.com/w3c/trace-context/pull/344/files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's revisit this once open-telemetry/opentelemetry-specification#331 and w3c/trace-context#344 are resolved.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If rate is less than
0.0000000000000000000271
, bound will be rounded to 0 which means it never gets sampled.Do we consider to round up or this is just too pedantic?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually it is
0.0000000000000000000813
since bound is rounded to1
(and all-zero trace id is illegal according to the TraceContext spec). 😄There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I used
round
intentionally here for what I thought were pedantic reasons: this way each of the2^64
sampling rate "buckets" are split exactly in half:and etc.
I think if you pass in a sampling rate
<= 2^-65
it's more correct for us to make the actual rate0
than2^-64
.This doesn't exactly work because float precision is worse than
2^-64
for values close to1
, but it works near0
and is at least theoretically correct (or should be, there may still be bugs here).Oh that's interesting, so the trace ID space is
[0x1, 0xf...f]
, not[0x0, 0xf...f]
. So would you expect this line to be:I think that'd give us the same behavior as above, but always sample trace ID
0x0
.Should we have special handling for
0x0
trace IDs?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm also happy to leave this as-is and treat
0x0
as a regular ID here, but treat it differently at span creation time. Up to you. :DThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have any preference here as I cannot think of a scenario where people would need such low but non-zero sample rate (unless someone is instrumenting SETI@home). 😆
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something I missed above: there are and
2^64 - 1
trace IDs besides0x0
that end in0x0000000000000000
. If we want to always/never sample the all-zero trace ID we have to make that decision before masking.