Add new tail sampling processor policy: probabilistic

[yvrhdn]

Description:
This adds a new tail sampling policy that samples a percentage of traces.

[jpkrohling]

Why would I use this instead of the probabilistic sampling processor?
https://github.com/open-telemetry/opentelemetry-collector/tree/main/processor/probabilisticsamplerprocessor

[yvrhdn]

Oh, wasn't aware of the probabilistic sampling processor (for some reason) 🤦🏻 I'll take a look at it and make sure our terminology is consistent.

I think the main difference is when the sampling decision happens. If you only want percentage or probabilistic sampling it doesn't make sense to use this tail sampling processor.
But combining this with other tail sampling policies makes it more meaningful. For instance:

    policies:
      [
          {
            name: all-errors,
            type: status_code,
            status_code: {status_codes: [ERROR]}
          },
          {
            name: half-of-remaining,
            type: percentage,
            percentage: {percentage: 0.5}
          },
     ]

This pipeline would sample all traces with status code error and 50% of the remaining traces. Using the probabilistic sampling processor you risk dropping traces with errors.

[jpkrohling]

Good point. Make sure to mention that in the readme then.

[joe-elliott]

Is there a difference between this and just putting the probabilistic sampling processor after tail sampling at 50%?

[yvrhdn]

The result will be slightly different: putting the probabilistic sampling processor after the tail sampling processor will filter what the tail sampling processor samples.
The percentage sampler as implemented here will filter what is not sampled by another policy.

So for instance a pipeline:

tail sampling (sample all errors) -> probabilistic (at 50%)

Result: 50% of traces with error
Why:

• the tail sampler drops every non-error
• the probabilistic sampler drops 50% of what the tail sampler returns

While the following:

tail sampling (sampler all errors -> sample 50%)

Result: all traces with error + 50% of traces without

[github-actions]

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

[github-actions]

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

[yvrhdn]

Not stale, I've been busy with some other stuff but I will continue working on this soon 🤞🏻

[github-actions]

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

[yvrhdn]

Looking at how the probabilistic sampling processor is implemented, I went for a very similar approach: instead of keeping a counter we can sample based upon the hashed trace ID. This is stateless (easier code) and allows the tail sampling processor to run distributed (i.e. if you have multiple collectors behind the Trace ID aware load-balancing exporter).

This does not take into account already sampled traces, so the effective sampling percentage might be higher if other policies sample a lot. But the processor has the same implementation using sampling.priority.

Example:

policies:
  latency:        samples 10% of traces
  probabilistic:  samples 25% of traces

effective sample percentage: 10% + (25% of remaining 90%) = 32,5%

I dislike that I basically had to copy a large part of the probabilistic sampling processor. Maybe we can extract the hashing and sampling part into a shared library? otel-go also does trace ID-based sampling and might also profit from this.

[jpkrohling]

Is adding the probabilistic sampling policy at the head of the chain considered a configuration error? If so, should a warning be generated?

[yvrhdn]

Hmm, after thinking some more this isn't actually true: the order does not matter because the tail sampling processor will always run all policies. If one of the policies returns Sampled it will keep the trace. I was thinking too much of this as a pipeline.

[jpkrohling]

instead of keeping a counter we can sample based upon the hashed trace ID

Jaeger does something very similar under the name of downsampling at the storage plugin: https://github.com/jaegertracing/jaeger/blob/c5642b708be9e2577cc1c494889ae97946ccc78a/storage/spanstore/downsampling_writer.go#L131-L140

It uses fnv64a, which would be my preference for this one here as well.

Maybe we can extract the hashing and sampling part into a shared library? otel-go also does trace ID-based sampling and might also profit from this.

Great idea. I think we are using CRC for the load balancing exporter, but we should settle for the same across the board. My choice would be fnv instead of Murmur, though. Would you prefer to work on the refactoring first, and then change this PR, or to get this PR merged first, and do the refactoring later?

[yvrhdn]

It uses fnv64a, which would be my preference for this one here as well.

Sounds good 👍 The implementation will be easier as well since it's in the standard library.

Would you prefer to work on the refactoring first, and then change this PR, or to get this PR merged first, and do the refactoring later?

How about I switch this sampler to fnv64a, then we can (finally) merge this PR and look at refactoring/combining logic?

[jpkrohling]

Sounds good to me.

[yvrhdn]

Not sure why CI is failing, might be a performance issue.

Tests work locally but take 13s, I'm guessing the runners are slower than my machine.

➜ go test -race ./...
ok  	github.com/open-telemetry/opentelemetry-collector-contrib/processor/tailsamplingprocessor	1.911s
ok  	github.com/open-telemetry/opentelemetry-collector-contrib/processor/tailsamplingprocessor/internal/idbatcher	0.848s
ok  	github.com/open-telemetry/opentelemetry-collector-contrib/processor/tailsamplingprocessor/internal/sampling	13.687s

I'll lower the size of the test.

[bogdandrutu]

Please rebase

[yvrhdn]

Done 👍 Would like me to squash the commits or does the repo squash on merge?