Skip to content
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 probability sampler details #331

Closed
wants to merge 5 commits into from
Closed
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
89 changes: 87 additions & 2 deletions specification/sdk-tracing.md
Original file line number Diff line number Diff line change
Expand Up @@ -122,8 +122,93 @@ These are the default samplers implemented in the OpenTelemetry SDK:

#### Probability Sampler algorithm

TODO: Add details about how the probability sampler is implemented as a function
of the `TraceID`.
The `ProbabilitySampler` makes a determistic sampling decision for each span
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Basing the algorithm on trace id should not be a requirement. It's an optimization that is possible in some languages, but could be pretty expensive in others, considering that trace id is just a blob of bytes that needs to be interpreted as a number.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is not a performance optimization (at least not primarily). Using the trace ID as the only input (and the same algorithm) ensures that all spans of the trace have the same sampling decision, which is required to avoid broken traces.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would assume that the root span of a trace might use a probability sampler, and all the descendants would use the sampled bit from context, so there's only one decision, and I agree with @yurishkuro.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd also expect the sampler to respect the sampled bit by default, but we say here that it should be possible to configure the sampler to apply to all spans, or any span with a remote parent.

L119 actually says the default behavior should be to make a new sampling decision for spans with remote parents, but I don't know the motivation for this, and think there's a good argument for restricting this to root spans by default.

One benefit of this sampling algorithm is that lower-probability samplers always sample a subset of traces of higher-probability samplers. Because of this, each service could set its own sampling rate and we'd still get complete traces for a percentage of requests equal to the lowest sampling rate. For example, if we have three services with sampling rates .3, .2, and .1 respectively, we'd get complete traces for .1 of requests. With independent random samplers we'd get traces for .006 of requests.

Even if we didn't want to make it possible to change the sampling rate along the call chain, there are some clear benefits to making the sampling decision deterministic. E.g. checking that telemetry is working.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are good arguments. 👍

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

an integer comparison to check if sampling makes sense

As I understand it, @yurishkuro's complaint is that we have to convert the byte array into an int to do the comparison. FWIW an implementation could compute the bound as a byte array instead and do a left-to-right bytewise comparison (see this gist for an example in python), but it's not clear that this would be faster than converting to an int, especially if the language boxes the bytes to do the comparison.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps ProbabilitySampler should be renamed DeterministicProbabilitySampler to indicate that it's based on the TraceID?

DeterministicProbabilitySampler is a mouthful. I'd prefer PercentageSampler or RateSampler, but I think ProbabilitySampler is a fine description, and has the benefit of having the same name in OpenCensus.

I would not be willing to draw statistical interpretations from a body of spans using deterministic sampling from clients in written with different PRNGs.

I can think of two obvious risks for people doing statistical inference on sampled traces: one is that trace IDs aren't even nominally randomly distributed. For example, if the trace ID includes a timestamp fragment, the sampled request rate might look cyclical or bursty even if the actual request rate is constant. The other is that the service itself handles requests differently depending on the trace ID.

But these are bugs in the service or instrumentation, not problems of insufficient randomness.

AFAICT we don't actually need strong statistical randomness here. We just need the "random" part of trace ID to be roughly uniformly distributed over all possible values, and not to be correlated with the stats we're monitoring (latency, error rate, etc.).

The fact that PRNGs are more P than R doesn't seem unusually bad in this context. And in any case a RandomProbabilitySampler would suffer from the same problems.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about if sampler does not depend on the trace-id at all? There is no need to consistently select the same trace for sampling as the sampling decision is mostly made when root span is created.
Sampler can simply increment an atomic counter by 1 for every root span is created. If sampling rate is 0.01 then trace is sampled if the count is multiple of 100. Counter can be initialized to a random value once, so across different instance/process they are not synchronized. This sounds more like RateSampler though.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I addressed this above in #331 (comment). We want to be able to make new sampling decisions for remote parent spans, which is why it's helpful to sample consistently.

Now that I read your description, RateSampler is clearly a better name for that than the behavior I'm describing.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RateSampler would imply time dependency, like rate limiting, this is more of a RatioSampler.

based on the span's trace ID. It samples a fixed percentage of all spans
following a user-supplied sampling rate.

According to the [W3C Trace Context
spec](https://www.w3.org/TR/trace-context/#trace-id), vendor-supplied trace IDs
may include both random and non-random components. To avoid sampling based on
the non-random component, the sampler should consider only the leftmost portion
of the trace ID. Implementations MAY allow the user to configure the number of
bits of the trace ID that the sampler considers. We define this number as
the sampler's *precision*.

A trace ID is a 16-byte array. We define the *truncated trace ID* to be the
leftmost precision-many bits of the trace ID:

```
truncated_trace_id = traceID[0:ceil(precision / 8))]
```

Where:

- `precision` is the number of bits of the trace ID to consider, in `[1, 128]`
- `ceil(float f)` returns the smallest integer greater than `f`
- `a[l:h]` is the slice operator: it returns the subsequence of `a` from index
`l` to `h` inclusive

This is equivalent to converting the trace ID to an unsigned integer assuming
big-endian byte order, and shifting to remove the unused bits:

```
truncated_trace_id = (int) traceID >> (128 - precision)
```

The `ProbabilitySampler` should only sample traces with truncated trace IDs
less than a certain value. We define this value as the sampler's *bound*:

```
bound = round(rate * pow(2, precision))
```

Where:

- `round(float f)` rounds `f` to the nearest integer
- `rate` is the sampling rate, in `[0.0, 1.0]`
- `pow(a, b)` is the exponentiation operator: `a` to the power of `b`

Note that this value doesn't depend on the trace to be sampled. Implementations
should generally compute this once, not on every sampling decision.

The sampling decision for a trace is given by:

```
to_sample = truncated_trace_id < bound
```

Note that the effective sampling rate is the number closest to `rate` that can
be expressed as a multiple of `2^-precision`. As a consequence, it's not
possible to set arbitrarily low sampling rates, even on platforms that support
arbitrary-precision arithmetic.

A `ProbabilitySampler` with rate `0.0` MUST NOT choose to sample any traces,
even if every bit of the truncated trace ID is `0`. Similarly, a
`ProbabilitySampler` with rate `1.0` MUST choose to sample all traces, even if
every bit of the the truncated trace ID is `1`.

**Example:**

Consider a `ProbabilitySampler` with rate `.25` and 16 bit precision.

First, find the lowest truncated trace ID that will not be sampled. This number
represents the 25th percentile of the range of possible values:

```
.25 * 2^16 = 0x4000
```

Make a sampling decision for a given trace by comparing this number to the
leftmost 16 bits of its 128 bit trace ID:

```
trunc_tid = (int) traceID >> (128 - 16)
to_sample = trunc_tid < 0x4000
```

This sampler should sample traces with truncated trace ID in the range
`[0x0000, 0x4000)`. Assuming `x` is uniformly distributed over `[0x0000,
0xffff]`, this should account for 25% of all traces.

## Tracer Creation

Expand Down