OpenTelemetry TraceIdRatioBased sampler requirements following OTEP 235 #4166
Conversation
|
Feedback from the OTel Spec SIG meeting discussion cc/ @jsuereth:
Update: 68fa270 |
|
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
…ication into jmacd/otep235
…ication into jmacd/otep235
This reduces the number of lines of diff in PR 4166, which replaces the entire `tracestate-probability-sampling.md` file with new contents. Part of #4166. ## Changes Move a file, place a link to it and explain that a change is in progress.
|
@kalyanaj @PeterF778 @oertl @kentquirk Please take another look at this PR, especially the file |
|
@open-telemetry/specs-trace-approvers @open-telemetry/specs-approvers @open-telemetry/technical-committee this PR has reached consensus in the Sampling SIG, we have multiple prototypes implemented, and we are looking for final approvals. |
|
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
Co-authored-by: J. Kalyana Sundaram <kalyanaj@microsoft.com>
…ication into jmacd/otep235
|
@open-telemetry/specs-trace-approvers @open-telemetry/specs-approvers @open-telemetry/technical-committee this PR has reached consensus in the Sampling SIG, we have multiple prototypes implemented, and we are looking for final approvals. |
| SDKs or even different versions of the same language SDKs may produce inconsistent | ||
| results for the same input. | ||
| The `TraceIdRatioBased` sampler implements simple, ratio-based probability sampling using randomness features specified in the [W3C Trace Context Level 2][W3CCONTEXTMAIN] Candidate Recommendation. | ||
| OpenTelemetry follows W3C Trace Context Level 2, which specifies 56 bits of randomness, in making use of 56 bits of information for probabilistic sampling decisions. |
There was a problem hiding this comment.
I'll add this here to be consistent, as @dyladan points out in the other PR, https://github.com/open-telemetry/opentelemetry-specification/pull/4162/files#r1747452247, this is a problem for those who the max safe integer is 2^53 - 1. So this matches the w3c spec but, assuming this is a requirement of JS, seems like a pretty big issue that should be addressed there and changed and then done here before this gets merged -- obv unless it is deemed it can't be changed at the w3c level anymore.
There was a problem hiding this comment.
I added a remark there that I think this will not be a problem. It is possible to represent the randomness value and threshold value as a byte slice or hexadecimal string. Either way, lexicographical comparisons are possible without using large unsigned integer values.
|
|
||
| [PKGSAMPLING]: https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/main/pkg/sampling/README.md | ||
|
|
||
| OpenTelemetry SDKs are recommended to use 4 digits of precision by default. The following table shows values computed by the method above for 1-in-N probability sampling, with precision 3, 4, and 5. |
There was a problem hiding this comment.
This is a little bit arbitrary. When I updated the OTel-Collector's processor/probabilisticsamplerprocessor I found an existing hash function that used 14 bits of information; in that case, 4 hex-digits of precision was enough to convey that sampler's decision w/o loss.
Compare and contrast: the TraceIdRatioBased sampler in the existing specification doesn't say what form of floating point manipulation is used to get to a ratio, though it recommends 6 decimal places of precision when printing that number in the description.
We could analyze the relative error to make this decision (@oertl what do you think?), however there's an argument that it matters less than you might think. When we interpret a threshold value, it is an exact representation of the sampling process. Sampling probabilities represented by thresholds are always exact--when you see a span with the threshold you have a corresponding, exact rational number telling you its probability (i.e., (2^56 - T)/(2^56)).
The question of accuracy and precision here applies to the translation from an input representation (likely a percentage, OTel doesn't specify this) to the exact threshold that will be used. If users are turning sampling probabilities into integer adjusted counts, this precision determines the error that will be introduced. I think 3 digits yields too much error (e.g., 1/100 sampling w/ adjusted counts of 100.05 is a 0.05% error) and 4 digits is acceptable (e.g., 1/100 sampling w/ adjusted counts of 99.9997 is a 0.003% error). @tsloughter what do you think?
…ication into jmacd/otep235
| SDKs or even different versions of the same language SDKs may produce inconsistent | ||
| results for the same input. | ||
| The `TraceIdRatioBased` sampler implements simple, ratio-based probability sampling using randomness features specified in the [W3C Trace Context Level 2][W3CCONTEXTMAIN] Candidate Recommendation. | ||
| OpenTelemetry follows W3C Trace Context Level 2, which specifies 56 bits of randomness, in making use of 56 bits of information for probabilistic sampling decisions. |
There was a problem hiding this comment.
I added a remark there that I think this will not be a problem. It is possible to represent the randomness value and threshold value as a byte slice or hexadecimal string. Either way, lexicographical comparisons are possible without using large unsigned integer values.
| The `TraceIdRatioBased` sampler MUST ignore the parent `SampledFlag`. | ||
| For respecting the parent `SampledFlag`, see the `ParentBased` sampler specified below. |
There was a problem hiding this comment.
Question for Sampling SIG @oertl @PeterF778 @kalyanaj @kentquirk --
How important is it to you that we add specification to the ParentBased sampler, which in the prototype here adds validation for incoming contexts? To me this is not a big priority, but if so, we could specify that ParentBased samplers used in non-root contexts are meant to validate that the incoming TraceContext/TraceState sampled flag is consistent with the threshold/randomness and/or explicit randomness setting, given the TraceID, and if they are inconsistent, unset the threshold value.
There was a problem hiding this comment.
IMO, we should not change information explicitly set by other services or users. We can't foresee which cases we, or our users, will have for this field in the future, and forcing it to be cleared might bite us. I'd say that SDKs should just ignore inconsistent values, acting as if they weren't set but propagating down what was received.
| The `TraceIdRatioBased` GetDescription MUST return a string of the form `"TraceIdRatioBased{RATIO}"` | ||
| with `RATIO` replaced with the Sampler instance's trace sampling ratio | ||
| represented as a decimal number. The precision of the number SHOULD follow | ||
| implementation language standards and SHOULD be high enough to identify when | ||
| Samplers have different ratios. For example, if a TraceIdRatioBased Sampler | ||
| had a sampling ratio of 1 to every 10,000 spans it could return | ||
| `"TraceIdRatioBased{0.000100}"` as its description. |
There was a problem hiding this comment.
Note I left this as-is for compatibility purposes. I'd be happy also to say that this was never defined as a stable string, and that we should extend the TraceIdRatioBased sampler's description with the actually configured threshold (which can vary according to precision).
There was a problem hiding this comment.
Is there a reason to define it as a stable string now?
| @@ -87,6 +87,7 @@ formats is required. Implementing more than one format is optional. | |||
| | [Built-in `SpanProcessor`s implement `ForceFlush` spec](specification/trace/sdk.md#forceflush-1) | | | + | | + | + | + | + | + | + | + | | | |||
| | [Attribute Limits](specification/common/README.md#attribute-limits) | X | | + | | + | + | + | + | | | | | | |||
| | Fetch InstrumentationScope from ReadableSpan | | | + | | + | | | + | | | | | | |||
| | TraceIdRatioBased implements OpenTelemetry tracestate `th` field | | | | | | | | | | | | | | |||
There was a problem hiding this comment.
Same question as the other PR: if this is required, shouldn't there be a couple of implementations lined up before the spec change is merged?
| (in combination with [`ParentBased`](#parentbased)) because different language | ||
| SDKs or even different versions of the same language SDKs may produce inconsistent | ||
| results for the same input. | ||
| The `TraceIdRatioBased` sampler implements simple, ratio-based probability sampling using randomness features specified in the [W3C Trace Context Level 2][W3CCONTEXTMAIN] Candidate Recommendation. |
There was a problem hiding this comment.
I feel very bad for this comment, but does this file currently have a word wrap at around 80 characters? I personally prefer not to force line wraps and let people configure their editors to their preferences, but I prefer consistency even more.
| The `TraceIdRatioBased` sampler MUST ignore the parent `SampledFlag`. | ||
| For respecting the parent `SampledFlag`, see the `ParentBased` sampler specified below. |
There was a problem hiding this comment.
IMO, we should not change information explicitly set by other services or users. We can't foresee which cases we, or our users, will have for this field in the future, and forcing it to be cleared might bite us. I'd say that SDKs should just ignore inconsistent values, acting as if they weren't set but propagating down what was received.
|
|
||
| ##### `TraceIdRatioBased` sampler algorithm | ||
|
|
||
| A Trace configured with sampling threshold `T`, a 56-bit unsigned number corresponding with the sampling ratio, has `ShouldSample()` called for a trace having randomness value `R`, a 56-bit unsigned random number. |
There was a problem hiding this comment.
I'm having trouble parsing this section. Can we simplify it?
Here's a suggestion which might still need some improvement:
| A Trace configured with sampling threshold `T`, a 56-bit unsigned number corresponding with the sampling ratio, has `ShouldSample()` called for a trace having randomness value `R`, a 56-bit unsigned random number. | |
| Given a trace with a sampling threshold `T` and a randomness value `R` (typically, the 7 rightmost bytes of the trace ID), when `ShouldSample()` is called, it checks whether `R >= T` and returns `RECORD_AND_SAMPLE`, otherwise returns `DROP`. |
But I think you might have the case in mind where R is not set yet and we are at the root span. In that case, the first "trace" would be "tracer". Related question: is this here supposed to replace the OTEP? I like how we have it in the OTEP:
The R value MUST be derived as follows:
- If the key rv is present in the Tracestate header, then R = rv.
- Else if the Random Trace ID Flag is true in the traceparent header, then R is the lowest-order 56 bits of the trace-id.
- Else R MUST be generated as a random value in the range [0, (2**56)-1] and added to the Tracestate header with key rv.
| The `TraceIdRatioBased` GetDescription MUST return a string of the form `"TraceIdRatioBased{RATIO}"` | ||
| with `RATIO` replaced with the Sampler instance's trace sampling ratio | ||
| represented as a decimal number. The precision of the number SHOULD follow | ||
| implementation language standards and SHOULD be high enough to identify when | ||
| Samplers have different ratios. For example, if a TraceIdRatioBased Sampler | ||
| had a sampling ratio of 1 to every 10,000 spans it could return | ||
| `"TraceIdRatioBased{0.000100}"` as its description. |
There was a problem hiding this comment.
Is there a reason to define it as a stable string now?
|
|
||
| ### Sampling Probability | ||
|
|
||
| Sampling probability is the likelihood that a span will be *kept*. Each participant can choose a different sampling probability for each span. For example, if the sampling probability is 0.25, around 25% of the spans will be kept. |
There was a problem hiding this comment.
I'm starting to think that you mean "tracer" here instead of "participant", potentially being "collector" when this is made not by a tracer. So, participant is "a tracer or a Collector" ?
|
|
||
| Sampling probability is the likelihood that a span will be *kept*. Each participant can choose a different sampling probability for each span. For example, if the sampling probability is 0.25, around 25% of the spans will be kept. | ||
|
|
||
| Sampling probability is valid in the range 2^-56 through 1. Note that the zero value is not defined and that "never" sampling is not a form of probability sampling. |
There was a problem hiding this comment.
2^-56 might seem a bit random for the non-initiated: would it be worth saying that this so that we have 7 bytes, matching the 7 bytes we get from the "randomness" (typically the 7 rightmost bytes from the trace ID)?
|
|
||
| Similarly, if the sampling probability is 1% (drop 99% of spans), the rejection threshold with 5 digits of precision would be (1-0.01) * 2^56 = 4458562600304640 = 0xfd70a00000000. | ||
|
|
||
| We refer to this rejection threshold conceptually as `T`. We represent it using the key `th`. This must be propagated in both the `tracestate` header and in the TraceState attribute of each span. |
There was a problem hiding this comment.
| We refer to this rejection threshold conceptually as `T`. We represent it using the key `th`. This must be propagated in both the `tracestate` header and in the TraceState attribute of each span. | |
| We refer to this rejection threshold conceptually as `T`. We represent it using the key `th`. This must be propagated in both the `tracestate` header and in the TraceState attribute of each span. In the example above, the `th` key has `fd70a00000000` as the value. |
|
|
||
| This proposal supports two sources of randomness: | ||
|
|
||
| - **A custom source of randomness**: This proposal allows for a *random* (or pseudo-random) 56-bit value. We refer to this as `rv`. This can be generated and propagated through the `tracestate` header and the tracestate attribute in each span. |
There was a problem hiding this comment.
I think I commented this elsewhere, but when should I, as a user, should consider having a custom source of randomness?
|
|
||
| If `R` >= `T`, *keep* the span, else *drop* the span. | ||
|
|
||
| `T` represents the maximum threshold that was applied in all previous consistent sampling stages. If the current sampling stage applies a greater threshold value than any stage before, it MUST update (increase) the threshold correspondingly. |
There was a problem hiding this comment.
Perhaps this comes later, but the OTEP also mentions that this cannot be lowered, only increased.
There was a problem hiding this comment.
I just came to the part where it says that it can be lowered at head samplers, but not for downstream samplers. This statement here might need to be adjusted then.
| - The `th` key MUST be defined with a value corresponding to the sampling probability the sampler used. | ||
| - The `rv` value, if present on the input TraceState, MUST be defined and equal to the incoming span context's `rv` value, including the root context. | ||
|
|
||
| Trace SDKs are responsible for for synthesizing `rv` values in the OpenTelemetry TraceState root span contexts. |
There was a problem hiding this comment.
| Trace SDKs are responsible for for synthesizing `rv` values in the OpenTelemetry TraceState root span contexts. | |
| Trace SDKs are responsible for synthesizing `rv` values in the OpenTelemetry TraceState root span contexts. |
|
|
||
| If `R` >= `T`, *keep* the span, else *drop* the span. | ||
|
|
||
| `T` represents the maximum threshold that was applied in all previous consistent sampling stages. If the current sampling stage applies a greater threshold value than any stage before, it MUST update (increase) the threshold correspondingly. |
There was a problem hiding this comment.
I just came to the part where it says that it can be lowered at head samplers, but not for downstream samplers. This statement here might need to be adjusted then.
|
|
||
| The original TraceIdRatioBased sampler specification gave a workaround for the underspecified behavior, that it was safe to use for root spans: "It is recommended to use this sampler algorithm only for root spans (in combination with [`ParentBased`](./sdk.md#parentbased)) because different language SDKs or even different versions of the same language SDKs may produce inconsistent results for the same input." | ||
|
|
||
| To avoid inconsistency during this transition, users SHOULD follow this guidance until all TraceIdRatioBased samplers used in a system have been upgraded to the modern `TraceIdRatioBased` specification based on W3C Trace Context Level 2 randomness. After all `TraceIdRatioBased` samplers have been upgraded, it is safe to use `TraceIdRatioBased` sampler without also using the `ParentBased` sampler. |
There was a problem hiding this comment.
How can users assess that they reached this? Should we keep a table, showing from which versions which SDKs support the new spec?
|
|
||
| ### Converting floating-point probability to threshold value | ||
|
|
||
| Threshold values are encoded with trailing zeros removed, which allows for variable precision. This can be accompolished by rounding, and there are several practical way to do this with built-in string formatting libraries. |
There was a problem hiding this comment.
| Threshold values are encoded with trailing zeros removed, which allows for variable precision. This can be accompolished by rounding, and there are several practical way to do this with built-in string formatting libraries. | |
| Threshold values are encoded with trailing zeros removed, which allows for variable precision. This can be accomplished by rounding, and there are several practical ways to do this with built-in string formatting libraries. |
|
|
||
| Threshold values are encoded with trailing zeros removed, which allows for variable precision. This can be accompolished by rounding, and there are several practical way to do this with built-in string formatting libraries. | ||
|
|
||
| With up to 56 bits of precision available, implementations that use built-in floating point number support will be limited by the precision of the underlying number support. If the language supports IEEE 754-2008-standard hexadecimal floating point, for example in Golang, |
There was a problem hiding this comment.
The last statement sounds a bit strange.
| A downstream sampler, in contrast, may output a given ended Span with a *modified* trace state, complying with following rules: | ||
|
|
||
| - If the chosen sampling probability is 1, the sampler MUST NOT modify any existing `th`, nor set any `th`. | ||
| - Otherwise, the chosen sampling probability is in `(0, 1)`. In this case the sampler MUST output the span with a `th` equal to `max(input th, chosen th)`. In other words, `th` MUST NOT be decreased (as it is not possible to retroactively adjust an earlier stage's sampling probability), and it MUST be increased if a lower sampling probability was used. This case represents the common case where a downstream sampler is reducing span throughput in the system. |
There was a problem hiding this comment.
| - Otherwise, the chosen sampling probability is in `(0, 1)`. In this case the sampler MUST output the span with a `th` equal to `max(input th, chosen th)`. In other words, `th` MUST NOT be decreased (as it is not possible to retroactively adjust an earlier stage's sampling probability), and it MUST be increased if a lower sampling probability was used. This case represents the common case where a downstream sampler is reducing span throughput in the system. | |
| - Otherwise, the chosen sampling probability is in `[0, 1)`. In this case the sampler MUST output the span with a `th` equal to `max(input th, chosen th)`. In other words, `th` MUST NOT be decreased (as it is not possible to retroactively adjust an earlier stage's sampling probability), and it MUST be increased if a lower sampling probability was used. This case represents the common case where a downstream sampler is reducing span throughput in the system. |
Fixes #1413.
Changes
Updates Trace SDK and TraceState handling specifications with OTEP 235 sampling thresholds. This PR depends on #4162 to introduce the concept of Trace Randomness. This PR is the second part of two, it focuses on thresholds.
TraceIdRatioBasedalgorithm section. The existing TODO implies this is not a breaking change.TraceIdRatioBasedconstructionTraceIdRatioBaseddescription (leave unmodified).The content of OTEP 235 was revised for clarity by @kalyanaj in open-telemetry/oteps#261. I've heavily copied from the final text in that still-unmerged OTEP. I introduced new content explaining how to compute thresholds from probabilities with use of variable precision, referring to the OTel Collector-Contrib
pkg/samplingreference implementation. The new (Golang) demonstration code is validated here, https://go.dev/play/p/7eLM6FkuoA5.A proof of concept for this specification along with #4162 can be found in open-telemetry/opentelemetry-go#5645.
Part of #3602.
Product of the Sampling SIG members @kentquirk @kalyanaj @oertl @PeterF778 and myself.
CHANGELOG.mdspec-compliance-matrix.md