Add JaegerRemoteSampler spec

[pavolloffay]

Signed-off-by: Pavol Loffay p.loffay@gmail.com

This PR defines JaegerRemoteSampler it is basically a follow-up after #1791.

Jaeger remote sampler is already available in:

• Collector: https://github.com/open-telemetry/opentelemetry-collector-contrib/tree/main/receiver/jaegerreceiver#remote-sampling
• Java: https://github.com/open-telemetry/opentelemetry-java/tree/main/sdk-extensions/jaeger-remote-sampler
• Go: soon Add Jaeger remote sampler opentelemetry-go-contrib#936

cc) @jmacd @carlosalberto @willarmiros

Changes

[willarmiros]

cc @bhautikpip as well

[william-tran]

There's discussion in the Otel Sampling SIG about aligning on an OTel native remote sampler, and as a user I'd still want to see that happen as I'm looking for features like matching Samplers to any attributes instead of just service and operation name. But I see this as a way to help Jaeger users transition to OTel; while addressing the need to evolve remote sampling to enable more fine grained control can continue to happen.

[pavolloffay]

'd still want to see that happen as I'm looking for features like matching Samplers to any attributes instead of just service and operation name. But I see this as a way to help Jaeger users transition to OTel; while addressing the need to evolve remote sampling to enable more fine grained control can continue to happen.

+1. This effort is mostly to enable migration. I do agree that more generic remote sampling is needed in the long run.

[github-actions]

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

[pavolloffay]

Can somebody merge the PR? The GH actions bot wants to close it

[yurishkuro]

@pavolloffay would merging it introduce a contradiction since OTEL has no rate limiting sampler?

[jmacd]

@pavolloffay would merging it introduce a contradiction since OTEL has no rate limiting sampler?

I don't mind this contradiction. In the terminology of #2047, RateLimited is some sort of non-probability Sampler and not very specific. I understand that often RateLimited is implemented by a LeakyBucket sampler, which has 2 (?) parameters. Perhaps replace "RateLimited" by "rate-limited" and file an issue to define the LeakyBucket rate-limited sampler in a future iteration?

[yurishkuro]

Perhaps replace "RateLimited" by "rate-limited"

Seems reasonable. I do think implementing rate limiter will be needed, especially to guarantee minimal level of sampling of rare endpoints, so this would be an added requirement for SDKs that want to support Jaeger's remote sampling policy format.

often RateLimited is implemented by a LeakyBucket sampler, which has 2 (?) parameters

the algorithm has two parameters, but the sampler usually has only one, the rate of traces / sec sampled (could be fractional).

[carlosalberto]

Left a non-blocking comment, otherwise LGTM

[anuraaga]

Perhaps replace "RateLimited" by "rate-limited" and file an issue to define the LeakyBucket rate-limited sampler in a future iteration?

#1769 is the issue for a rate limiting sampler. My impression from the thread is it was rejected for addition to OTel since it doesn't support probabilistic sampling 😃

[jmacd]

My impression from the thread is it was rejected for addition to OTel since it doesn't support probabilistic sampling

Right. Probability sampling is preferable to non-probability sampling. Speaking as a vendor, we are not interested in receiving sampled spans without being able to count them. #2047 specifies how to mix these two kinds of Sampler, and I consider this to admit specifications for non-probability samplers when it merges. When that happens, the user that @yurishkuro describes, who wants to combine probability sampling with a minimum-rate could have options:

(a) A leaky bucket sampler (for minimum-rate sampling) composed with a probability sampler (for Span-to-metrics)
(a) An adaptive sampler that adjusts a simple probability sampler to maintain a soft rate limit, in which case all spans are countable but the rate limit is not guaranteed.

Given these choices, I'd choose (b) and I expect the complexity of the adaptive logic to be less than the complexity of the leaky bucket algorithm. This is why I suggested we write "rate-limited" instead of naming a specific algorithm, because naming a specific algorithm requires, well, specifying it. Do you want to write the specification for LeakyBucket? I'd prefer to write the adaptive sampler specification.

[carlosalberto]

@anuraaga @jmacd should we follow the discussion either in #1769 and merge this? Or have a blocking request?

[carlosalberto]

@pavolloffay This PR needs to be updated ;(

[pavolloffay]

@carlosalberto PR rebased and renamed RateLimitting to rate-limited per @jmacd request.