Probability Samplers based on W3C Trace Context Level 2 by jmacd · Pull Request #3910 · open-telemetry/opentelemetry-specification

jmacd · 2024-02-27T23:20:55Z

Fixes #1413.
Fixes #3602.

Changes

Updates Trace SDK and Propagator specifications with complete support for consistent probability sampling as described in OTEP 235.

Related issues 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 Introduce sampling package as reference implementation for OTEP 235 opentelemetry-collector-contrib#29720
CHANGELOG.md TODO
spec-compliance-matrix.md TODO

jmacd · 2024-02-27T23:22:54Z

@kentquirk @oertl @PeterF778 @kalyanaj Please take a look.
I believe a bit more prototyping will be required, so I leave this open as a draft PR.
I am looking to prototype an OTel-Go SDK change to the TraceIdRatioBased sampler, and I will be continuing to revive open-telemetry/opentelemetry-collector-contrib#24811 in the coming week. Cheers!

jmacd · 2024-02-27T23:32:55Z

@kentquirk @oertl @PeterF778 @kalyanaj
TL;DR I found about half of the text in the old, experimental specification to be overkill and removed a lot of it. If you see something you'll greatly miss, we can bring it back. @kentquirk if you want to throw out the old, experimental specification and start from scratch, I'm open to that. I don't think we need to extend the specification with all the pseudocode from the OTEP, readers can refer back to that document for details.

oertl · 2024-02-28T09:30:54Z

+`traceparent` and `tracestate` contexts fields specified in [W3C Trace
+Context Level 2](https://www.w3.org/TR/trace-context-2/).
+
+When injecting and extracting trace context to ro from a carrier, the


Suggested change

When injecting and extracting trace context to ro from a carrier, the

When injecting and extracting trace context to or from a carrier, the

oertl · 2024-02-28T09:32:28Z

 #### TraceIdRatioBased

-* The `TraceIdRatioBased` MUST ignore the parent `SampledFlag`. To respect the
+The `TraceIdRatioBased` MUST ignore the parent `SampledFlag`. To respect the


Suggested change

The `TraceIdRatioBased` MUST ignore the parent `SampledFlag`. To respect the

The `TraceIdRatioBased` sampler MUST ignore the parent `SampledFlag`. To respect the

oertl · 2024-02-28T09:35:12Z

+* A rejection threshold is calculated, expressing as an integer how
+  many out of 2**56 trace IDs should be sampled.
+* The threshold is encoded as a "T-value", expressing the threshold
+  using up to 5 hexadecimal digits of precision.


Suggested change

using up to 5 hexadecimal digits of precision.

using up to 14 hexadecimal digits of precision.

oertl · 2024-02-28T09:45:50Z

-Samplers SHOULD unset `p` when the invariant between the `sampled`,
-`p`, and `r` values is violated before using the `tracestate` to make
-a sampling decision or interpret adjusted count.
+Samplers SHOULD unset T-value (by erasing the `tv` field) when the


Suggested change

Samplers SHOULD unset T-value (by erasing the `tv` field) when the

Samplers SHOULD unset T-value (by erasing the `th` field) when the

oertl · 2024-02-28T09:46:12Z

 ```
-base10(r) = 3
-base10(p) = 2
+tracestate: ot=tv:c


Suggested change

tracestate: ot=tv:c

tracestate: ot=th:c

oertl · 2024-02-28T09:48:36Z

+return a string of the form `"TraceIdRatioBased{RATIO;tv:TVALUE}"`
+with `RATIO` the configured probability and `TVALUE` replaced by the


Suggested change

return a string of the form `"TraceIdRatioBased{RATIO;tv:TVALUE}"`

with `RATIO` the configured probability and `TVALUE` replaced by the

return a string of the form `"TraceIdRatioBased{RATIO;th:THRESHOLD}"`

with `RATIO` the configured probability and `THRESHOLD` replaced by the

oertl · 2024-02-28T09:59:10Z

+Here, R-value ce929d0e0e4736 indicates that a consistent probability
+sampler configured with probability greater than approximately 19.3%,


Suggested change

Here, R-value ce929d0e0e4736 indicates that a consistent probability

sampler configured with probability greater than approximately 19.3%,

The set sampled flag together with the R-value being ce929d0e0e4736 indicate that the parent used a consistent probability sampler configured with probability greater than approximately 19.3%,

oertl · 2024-02-28T11:50:28Z

+T-value is not set, consistent with an unsampled context.  In this
+case, the parent context could have been sampled at less than 19.3%
+probability or used the `AlwaysOff` sampler.


In this case we know that the parent was not consistently sampled. If any other sampling mechanism is used, the probability could be any value. Mentioning 19.3% here, is therefore a little bit confusing.

oertl · 2024-02-28T11:56:49Z

+To combine a consistent probability Sampler decision with a
+non-probability Sampler decision, T-value should be set according to
+the probability sampler(s).  If no probability sampler decides to
+sample, no T-value should be set.


Suggested change

To combine a consistent probability Sampler decision with a

non-probability Sampler decision, T-value should be set according to

the probability sampler(s). If no probability sampler decides to

sample, no T-value should be set.

If the sampling decision is the result of multiple samplers that are not all consistent samplers, no T-value should be set.

oertl · 2024-02-28T11:59:48Z

+The `TraceIdRatioBased` and `ParentBased` samplers can be used as
+delegates of another sampler, for conditioning the choice of sampler
+on span and other fixed attributes.  However, for adjusted counts to
+be trustworthy, the choice of non-root sampler cannot be conditioned
+on the parent's sampled trace flag or the OpenTelemetry tracestate
+R-value or T-value, as these decisions would lead to incorrect


Suggested change

The `TraceIdRatioBased` and `ParentBased` samplers can be used as

delegates of another sampler, for conditioning the choice of sampler

on span and other fixed attributes. However, for adjusted counts to

be trustworthy, the choice of non-root sampler cannot be conditioned

on the parent's sampled trace flag or the OpenTelemetry tracestate

R-value or T-value, as these decisions would lead to incorrect

The `TraceIdRatioBased` and `ParentBased` samplers can be used as

delegates of another sampler, for conditioning the choice of sampler

on span and other fixed attributes. However, for adjusted counts to

be trustworthy, the choice of non-root sampler cannot be conditioned

on the parent's sampled trace flag or the OpenTelemetry tracestate

R-value, as these decisions would lead to incorrect

Choosing the T-value based on the parent T-value is fine. This is exactly what is done for parent-based sampling.

PeterF778 · 2024-02-28T16:55:16Z

+- TraceFlags (8 bits)
+- TraceState (string)
+
+Propagators MUST NOT assume that bits 2-7 (6 most significant bits) will be zero.


For clarity, let's say "bits 2-7 of TraceFlags".

PeterF778 · 2024-02-28T17:00:05Z

+
+* Ratio values are restricted to the range `2**-56` through 1
+* A rejection threshold is calculated, expressing as an integer how
+  many out of 2**56 trace IDs should be sampled.


"sampled" => "dropped"

PeterF778 · 2024-02-28T21:36:09Z

-and r-value are independent settings, each can be meaningfully set
-without the other present.
+The [Sampler API](sdk.md#sampler) is responsible for setting the
+`sampled` and `random` flags and the `tracestate`.


This is problematic. A sampler should not change the random flag at all. This flag must be set when the TraceID is created which is ahead of making the sampling decision for the root span.

PeterF778 · 2024-02-28T21:41:27Z

-bits.
+Head Samplers (i.e., trace SDKs) SHOULD by default generate the 56
+random bits specified for TraceIDs by the W3C Trace Context Level 2,
+and set the Random trace flag, when generating new contexts.


According to the SDK specification, the TraceID is already generated even when sampling ROOT spans. This needs more discussions.

kentquirk

I'm a fan of this. I think you've done a good job here and based on what I see, I don't think we need to be rewriting it from scratch. Our past history leads me to believe that might be a long process. Let's push forward!

kentquirk · 2024-02-29T02:14:11Z

+### W3C Trace Context Requirements
+
+A W3C Trace Context propagator is expected to implement the
+`traceparent` and `tracestate` contexts fields specified in [W3C Trace


Suggested change

`traceparent` and `tracestate` contexts fields specified in [W3C Trace

`traceparent` and `tracestate` context fields specified in [W3C Trace

kentquirk · 2024-02-29T02:15:09Z

+- TraceFlags (8 bits)
+- TraceState (string)
+
+Propagators MUST NOT assume that bits 2-7 (6 most significant bits) will be zero.


Suggested change

Propagators MUST NOT assume that bits 2-7 (6 most significant bits) will be zero.

Propagators MUST NOT assume that bits 2-7 of TraceFlags (the 6 most significant bits) will be zero.

kentquirk · 2024-02-29T02:18:44Z

+- [Sampled](https://www.w3.org/TR/trace-context/#sampled-flag)
+- [Random](https://www.w3.org/TR/trace-context-2/#random-trace-id-flag)


Suggested change

- [Sampled](https://www.w3.org/TR/trace-context/#sampled-flag)

- [Random](https://www.w3.org/TR/trace-context-2/#random-trace-id-flag)

- [Sampled (value 0x1)](https://www.w3.org/TR/trace-context/#sampled-flag)

- [Random (value 0x2)](https://www.w3.org/TR/trace-context-2/#random-trace-id-flag)

I think it would help to know which bits we're talking about.

kentquirk · 2024-02-29T02:21:59Z

+* A rejection threshold is calculated, expressing as an integer how
+  many out of 2**56 trace IDs should be sampled.


Suggested change

* A rejection threshold is calculated, expressing as an integer how

many out of 2**56 trace IDs should be sampled.

* A rejection threshold is calculated, expressing as an integer the number

out of 2**56 trace IDs that should be dropped.

kentquirk · 2024-02-29T02:24:21Z

+This states that the SDK should fill least significant 7 bytes (i.e., 56
+bits) of the TraceID are genuinely random or pseudo-random., so they
+can be used for probability sampling.


Suggested change

This states that the SDK should fill least significant 7 bytes (i.e., 56

bits) of the TraceID are genuinely random or pseudo-random., so they

can be used for probability sampling.

This states that the SDK should fill the least significant 7 bytes (i.e., 56

bits) of the TraceID with bits that are genuinely random or pseudo-random,

so that they can be used for probability sampling.

kentquirk · 2024-02-29T02:31:56Z

-decimal value is greater than 63 before using the `tracestate` to make
-a sampling decision or interpret adjusted count.
+These points, taken together, means there will be a long delay before
+the Random trace flag functions as intended in ther OpenTelemetry


Suggested change

the Random trace flag functions as intended in ther OpenTelemetry

the Random trace flag functions as intended in the OpenTelemetry

kentquirk · 2024-02-29T02:32:51Z


-##### Example: Probability sampled context
+Tail Samplers SHOULD ignore the W3C Trace Context Level 2 Random flag
+when interpreting span data.  This is a compromise 


Suggested change

when interpreting span data. This is a compromise

when interpreting span data. This is a compromise.

kentquirk · 2024-02-29T02:33:14Z

+when interpreting span data.  This is a compromise 

-Consider a trace context with the following headers:
+When there is an explicit R-value set, the Sampler will anyway


Suggested change

When there is an explicit R-value set, the Sampler will anyway

When there is an explicit R-value set, the Sampler will

kalyanaj · 2024-02-29T04:45:26Z

+* Sampler decisions are made by comparing the Trace ID randomness
+  against the rejection threshold.
+* When Sampled, T-value is included in the [OpenTelemetry TraceState
+  header](./tracestate-handling.md), identified by sub-key `th`,


Should we update the tracestate-handling.md to include the examples for the sub-key 'th'? Right now, it seems to be referring to the earlier p values and r value examples.

kalyanaj · 2024-02-29T04:48:08Z

+  using up to 5 hexadecimal digits of precision.
+* Sampler decisions are made by comparing the Trace ID randomness
+  against the rejection threshold.
+* When Sampled, T-value is included in the [OpenTelemetry TraceState


It will be good to rephrase this to an active voice of what implementations must do. For example, something like: "When sampled, an implementation MUST update the tracestate header to specify the T-value. <+ any additional details>"

github-actions · 2024-03-12T03:17:12Z

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

github-actions · 2024-03-20T03:16:44Z

Closed as inactive. Feel free to reopen if this PR is still being worked on.

github-actions · 2024-04-05T03:17:11Z

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

github-actions · 2024-04-13T03:15:37Z

Closed as inactive. Feel free to reopen if this PR is still being worked on.

jmacd added 5 commits February 15, 2024 07:59

WIP

598bbd0

one less TODO in sdk.md 😂

98e76fd

Move description

b15fc05

more wip

ea4ab98

draft probability sampling complete

b81f41f

oertl reviewed Feb 28, 2024

View reviewed changes

PeterF778 reviewed Feb 28, 2024

View reviewed changes

kentquirk reviewed Feb 29, 2024

View reviewed changes

kalyanaj reviewed Feb 29, 2024

View reviewed changes

wip

0ec6cb1

This was referenced Mar 6, 2024

Add OpenTelemetry sampling conventions open-telemetry/semantic-conventions#793

Closed

Propagate 8 bits of W3C trace context flags open-telemetry/opentelemetry-go#5030

Closed

github-actions Bot added the Stale label Mar 12, 2024

github-actions Bot closed this Mar 20, 2024

jmacd removed the Stale label Mar 28, 2024

jmacd reopened this Mar 28, 2024

github-actions Bot added the Stale label Apr 5, 2024

github-actions Bot closed this Apr 13, 2024

jmacd mentioned this pull request Apr 24, 2024

Project Tracking: Sampling #4012

Closed

	When injecting and extracting trace context to ro from a carrier, the
	When injecting and extracting trace context to or from a carrier, the

	The `TraceIdRatioBased` MUST ignore the parent `SampledFlag`. To respect the
	The `TraceIdRatioBased` sampler MUST ignore the parent `SampledFlag`. To respect the

	using up to 5 hexadecimal digits of precision.
	using up to 14 hexadecimal digits of precision.

	Samplers SHOULD unset T-value (by erasing the `tv` field) when the
	Samplers SHOULD unset T-value (by erasing the `th` field) when the

		return a string of the form `"TraceIdRatioBased{RATIO;tv:TVALUE}"`
		with `RATIO` the configured probability and `TVALUE` replaced by the

		Here, R-value ce929d0e0e4736 indicates that a consistent probability
		sampler configured with probability greater than approximately 19.3%,

	Here, R-value ce929d0e0e4736 indicates that a consistent probability
	sampler configured with probability greater than approximately 19.3%,
	The set sampled flag together with the R-value being ce929d0e0e4736 indicate that the parent used a consistent probability sampler configured with probability greater than approximately 19.3%,

	`traceparent` and `tracestate` contexts fields specified in [W3C Trace
	`traceparent` and `tracestate` context fields specified in [W3C Trace

	Propagators MUST NOT assume that bits 2-7 (6 most significant bits) will be zero.
	Propagators MUST NOT assume that bits 2-7 of TraceFlags (the 6 most significant bits) will be zero.

		- [Sampled](https://www.w3.org/TR/trace-context/#sampled-flag)
		- [Random](https://www.w3.org/TR/trace-context-2/#random-trace-id-flag)

		* A rejection threshold is calculated, expressing as an integer how
		many out of 2**56 trace IDs should be sampled.

	the Random trace flag functions as intended in ther OpenTelemetry
	the Random trace flag functions as intended in the OpenTelemetry

	when interpreting span data. This is a compromise
	when interpreting span data. This is a compromise.

	When there is an explicit R-value set, the Sampler will anyway
	When there is an explicit R-value set, the Sampler will

Conversation

jmacd commented Feb 27, 2024

Changes

Uh oh!

jmacd commented Feb 27, 2024

Uh oh!

jmacd commented Feb 27, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kentquirk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kalyanaj Feb 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Mar 12, 2024

Uh oh!

github-actions Bot commented Mar 20, 2024

Uh oh!

github-actions Bot commented Apr 5, 2024

Uh oh!

github-actions Bot commented Apr 13, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

kalyanaj Feb 29, 2024 •

edited

Loading