OpenTelemetry TraceIdRatioBased sampler requirements following OTEP 235

[jmacd]

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.

• Revise TraceIdRatioBased algorithm section. The existing TODO implies this is not a breaking change.
• Change text about TraceIdRatioBased construction
• Move text about TraceIdRatioBased description (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/sampling reference 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.

• Related issues also Ignored transactions and consistent probability sampling #3307, Requesting clarification for non-probabilistic samplers #2253, Refactor the built-in Samplers for probability sampling #2179, Multi-policy sampling #2113, Change default trace-id format to be similar to AWS X-Ray (use timestamp) #1947,Composite Sampler #1844
• OTEP: Threshold propagation in trace state oteps#235
• Links to the prototypes: https://github.com/open-telemetry/opentelemetry-collector-contrib/tree/main/processor/probabilisticsamplerprocessor, Prototype for W3C Trace Context Level 2 support in TraceIDRatioBased sampler opentelemetry-go#5645
• CHANGELOG.md
• spec-compliance-matrix.md

[jmacd]

Feedback from the OTel Spec SIG meeting discussion cc/ @jsuereth:

• Please add a migration guide to explain how transitioning samplers will work; in particular, it's not safe to begin using non-root independent sampling until TraceIdRatioBased samplers are replaced everywhere in a trace. Until then, only safe to continue using ParentBased sampling w/ root TraceIdRatioBased decision.

Update: 68fa270

[github-actions]

This PR was marked stale due to lack of activity. It will be closed in 7 days.

[jmacd]

@kalyanaj @PeterF778 @oertl @kentquirk Please take another look at this PR, especially the file tracestate-probability-sampling.md which now reads as a new file, not as a major rewrite. The contents are derived from open-telemetry/oteps#261.

[jmacd]

@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.

[github-actions]

This PR was marked stale due to lack of activity. It will be closed in 7 days.

[jmacd]

@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.

[jmacd]

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).

[jpkrohling]

Is there a reason to define it as a stable string now?

[jmacd]

We have explicitly stated that Description need not be a stable string to allow Samplers to vary dynamically, and I can't see why it should be defined as stable anyway. If anyone has been parsing this value to determine the effective sampling probability, leaving it as-is will be good, but they should begin using the encoded threshold instead.

[jpkrohling]

Partial review, will try to complete by tomorrow.

[jpkrohling]

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?

[jmacd]

I have a shared my draft, open-telemetry/opentelemetry-go#5645, and @oertl has already merged an equivalent sampler in the Java contrib repository. (I would add that the OTel-Collector-Contrib probabilistic sampler processor acts as a near-prototype.)

[jpkrohling]

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.

[jmacd]

There is inconsistent style in this file, and I feel like reviewers have asked for me to do it both ways. I'll do anything you want!!

[jpkrohling]

I'm fine leaving it as is, we (you?) can send a PR afterwards having it consistent :-)

[jpkrohling]

I'm having trouble parsing this section. Can we simplify it?

Here's a suggestion which might still need some improvement:

Suggested change

	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.

[yuanyuanzhao3]

A Trace configured with sampling threshold T

Do you mean "A Tracer configured with sampling threshold T"? Note the typo?

It is also not entirely clear to me whether the better term is "A sampler".

[jmacd]

The way that R is calculated is meant to be part of #4162. We ran into a difficulty after the OTEP (235) merged which is discussed in the subsequent and unmerged (but widely approved by Sampling SIG members) open-telemetry/oteps#261. The Random flag isn't useful in all cases as a signal for this purpose, so we have instead a "presumption of trace randomness". Root tracers should use either use the correct randomness or support user-supplied rv randomness values (optional). Non-root tracers should check for rv otherwise assume a random TraceID.

[jmacd]

How about

Given a Sampler configured with a sampling threshold `T` and Context with randomness value `R` (typically, the 7 rightmost bytes of the trace ID), when `ShouldSample()` is called, it uses the expression `R >= T` to decide whether to return `RECORD_AND_SAMPLE` or `DROP`. 

* If randomness value (R) is greater or equal to the rejection threshold (T), meaning when (R >= T), return `RECORD_AND_SAMPLE`, otherwise, return `DROP`.
* When (R >= T), the OpenTelemetry TraceState SHOULD be modified to include the key-value `th:T` for rejection threshold value (T), as specified for the [OpenTelemetry TraceState `th` sub-key][TRACESTATEHANDLING].

[jpkrohling]

Is there a reason to define it as a stable string now?

[jpkrohling]

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" ?

[jmacd]

Yes. I've added ", which includes a set of tracers and collectors" to the first use of "participants". This term originated in https://github.com/open-telemetry/oteps/pull/261/files.

[jpkrohling]

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)?

[jmacd]

I added "The value 56 appearing in this expression corresponds with 7 bytes of randomness (i.e., 56 bits) which are specified for W3C Trace Context Level 2 TraceIDs. ".

[jpkrohling]

I think I commented this elsewhere, but when should I, as a user, should consider having a custom source of randomness?

[jmacd]

This is meant to be part of #4162 which focuses on randomness. It writes "To enable sampling in this and other situations where TraceIDs lack sufficient randomness,"

However, I tried to stay away from the advanced use-cases some might mention. If you have a reason to use independent trace IDs and still want them to sample consistently, this is what you'd choose.

[jpkrohling]

Perhaps this comes later, but the OTEP also mentions that this cannot be lowered, only increased.

[jpkrohling]

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.

[jmacd]

Probability can be lowered after the fact by re-sampling with a higher threshold, but not raised after the fact (by a lower threshold). I'll look for any inconsistencies.

[jpkrohling]

Other than my previous comments, LGTM!

[jpkrohling]

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.

[jpkrohling]

How can users assess that they reached this? Should we keep a table, showing from which versions which SDKs support the new spec?

[jmacd]

Another way they can do this is to wait for all spans to have the W3C trace Random flag set across a system. How does that sound?

[jpkrohling]

The last statement sounds a bit strange.

[jmacd]

Yes. Fixed:

One way to encode thresholds uses the IEEE 754-2008-standard hexadecimal floating point representation as a simple solution.  For example, in Golang,

[jpkrohling]

Suggested change

- 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.

[jmacd]

We can't represent 0-probability sampling with a threshold. The corresponding threshold is out-of-range on purpose. If you want 0-probability sampling, you can simply not export a span, or you can export a span w/o the th value set, which says "unknown sampling probability".

[yuanyuanzhao3]

A Trace configured with sampling threshold T

Do you mean "A Tracer configured with sampling threshold T"? Note the typo?

It is also not entirely clear to me whether the better term is "A sampler".

[PeterF778]

The phrase "parent span might have been dropped" is not correct in this context. According to our specs, if the parent span was dropped, there wouldn't be parent's T-value available (we clear th value when dropping a span). I suggest saying "Downstream samplers may decide to drop the parent span".

[jmacd]

I read this section and it has me a bit confused. I propose to cut a lot of text, since I'm not sure it was answering real questions.

[PeterF778]

Nit: wouldn't this code result in an empty string if the threshold is zero?

[jmacd]

Yes. I propose

if tvalue == 0: 
  add_otel_trace_state('tv:0')
else:
  h = hex(tvalue).rstrip('0')
  # remove leading 0x
  add_otel_trace_state('tv:'+h[2:])