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. |
…metry#4168) 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 open-telemetry#4166. ## Changes Move a file, place a link to it and explain that a change is in progress.
This reverts commit e6dc409.
specification/trace/sdk.md
Outdated
| * For root spans, always sample a new context. | ||
| * For child spans, take the decision of the parent context. | ||
|
|
||
| By using the ParentBased sampler by default, users can change sampling |
There was a problem hiding this comment.
Nit: this should be an "aside" or non-normative callout. I'm not sure we have precedence here, but could you move this to some different markdown structure, perhaps > quote ?
|
|
||
| When a TraceIdRatioBased Sampler makes a decision for a non-root Span | ||
| using TraceID randomness, but the Trace random flag was not set, the | ||
| SDK SHOULD issue a one-time warning statement in its log with a |
There was a problem hiding this comment.
nit: one-time warning - You may need to include criteria of what consistutes a "time", i.e. is it once per line of code, once per span name...
I think this is a bit too vague to implement consistently.
|
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
…ication into jmacd/otep235
|
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
|
Closed as inactive. Feel free to reopen if this PR is still being worked on. |
|
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
|
Closed as inactive. Feel free to reopen if this PR is still being worked on. |
…ication into jmacd/otep235
specification/trace/sdk.md
Outdated
| 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]. |
There was a problem hiding this comment.
Note there is a simple change needed here, following #4321. The TraceIDRatio sampler is a composable sampler in the sense defined there, where it simply returns its T value as the intent.
|
Is this ready for mainstream review? |
…ication into jmacd/otep235
|
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
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