Add OTEL_SAMPLING_PROBABILITY env variable definition

[tigrannajaryan]

Fixes #1105

[arminru]

Thanks, that was quick, @tigrannajaryan 🙂

[jkwatson]

Does setting this env var imply setting a ParentOrElse sampler with an underlying TraceIdRatioBased sampler? Or something else?

[tigrannajaryan]

I'll let language maintainers tell what is needed here, I don't know how sampling in SDKs works.

[jkwatson]

I think it should do what I suggested, but I'd love other opinions. And, we should definitely document what's expected. :)

[tigrannajaryan]

@jkwatson can you please suggest the exact edit to use on this line? I think it will fit best in the Notes column.

[arminru]

OTEL_TRACEID_RATIO_(BASED_)SAMPLER_PROBABILITY is quite a bulky term but would probably be the most expressive one.
In the notes column, we could link to /~https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/trace/sdk.md#traceidratiobased.

[jkwatson]

@arminru would you still expect that the TraceIdRatioBased sampler would be wrapped with a ParentOrElse with that name?

[arminru]

@jkwatson I would not expect that variable to affect which samplers are set up in which arrangement but if a TraceIdRatioBased was set up automatically based on other configuration, this would be the probability it would use. Does that make sense?

[jkwatson]

Given that, it seems strange to have an env-var to control one facet of a sampler, but no environment variable to actual specify a sampler!

I think we should hold off on this addition until we have a way to specify a sampler via env vars, so that this env var makes sense in that context.

[jmacd]

We might want to add metrics and logs sampling later, should we name this OTEL_TRACE_SAMPLING_PROBABILITY?

[carlosalberto]

I think we should hold off on this addition until we have a way to specify a sampler via env vars, so that this env var makes sense in that context.

@jkwatson Makes sense. I think we ought to add an OTEL_SAMPLER env var with a few pre cooked values (to keep it simple for now), i.e. always_on, always_off, traceidratio, parentbased_traceidratio.

Opinions?

[jkwatson]

I think we should hold off on this addition until we have a way to specify a sampler via env vars, so that this env var makes sense in that context.

@jkwatson Makes sense. I think we ought to add an OTEL_SAMPLER env var with a few pre cooked values (to keep it simple for now), i.e. always_on, always_off, traceidratio, parentbased_traceidratio.

Opinions?

that makes sense to me, yes.

[arminru]

@carlosalberto opened #1136 for adding OTEL_TRACE_SAMPLER, please take a look at that one as well.

[carlosalberto]

#1136 has been merged. @tigrannajaryan please rebase we can try to wrap this up 😉

[tigrannajaryan]

#1136 has been merged. @tigrannajaryan please rebase we can try to wrap this up 😉

Done.

[carlosalberto]

It looks we should go with @jmacd's suggestion and rename this to OTEL_TRACE_SAMPLING_PROBABILITY? @tigrannajaryan

[carlosalberto]

@open-telemetry/specs-approvers Please review.

[tigrannajaryan]

Renamed to OTEL_TRACE_SAMPLING_PROBABILITY.
I am still looking for a recommendation on what to put in the Notes (if anything).

[carlosalberto]

@Oberon00 @jkwatson as you seem to be more familiar with Sampling needs, maybe want to drop your opinion on what should be stated here?

[Oberon00]

I don't understand what the default value means here, as discussed in #985. Is this the default to be used if sampler is traceidratio?

[Oberon00]

If we have two variables we need to document what happens when e.g. OTEL_TRACE_SAMPLING_PROBABILITY is set but OTEL_TRACE_SAMPLER is not set (should the documented default parentbased_always_on be used or should it "smartly" use traceidratio? Or if it is explicitly set to something else than traceidratio or parentbased_traceidratio.

Alternatively, we might consider having only one variable with a bit more complex values like OTEL_TRACE_SAMPLER=traceidratio?ratio=0.25 (I imagine using querystring syntax would be useful here since many languages support parsing that out of the box).

[cijothomas]

Can we say OTEL_TRACE_SAMPLING_PROBABILITY is evaluated only if OTEL_TRACE_SAMPLER is set to traceidratio?

[Oberon00]

Or parentbased_traceidratio.

What if the tracer is configured in code? Should it somehow pick up the value? I guess not.

[carlosalberto]

@Oberon00 If I understood correctly, this is a value used mostly for auto-instrumentation, at least for now (similar to OTEL_PROPAGATORS).

[Oberon00]

If that is the intention, it should be documented. I would not be surprised if some SDKs respond to these environment variables out of the box.

[cijothomas]

@Oberon00 If I understood correctly, this is a value used mostly for auto-instrumentation, at least for now (similar to OTEL_PROPAGATORS).

This was not mentioned anywhere that this is only for auto-instrumentation. I was planning to make SDK (.NET), respond to these env variables by default.

[jmacd]

If I understood correctly, this is a value used mostly for auto-instrumentation, at least for now (similar to OTEL_PROPAGATORS).

This is functionality that auto-instrumentation can use, I agree, but it's functionality that I want for manual instrumentation too. I think of making every SDK implement these variables for configuring manual instrumentation is a post-GA item, something covered by the so-called "distros" or "launchers" that give you an easy way to install the SDK configured by the environment.

[yurishkuro]

What if the tracer is configured in code? Should it somehow pick up the value? I guess not.

The convention in the Jaeger SDKs is:

• there is a Configuration struct
• it is used to construct the tracer
• it can be populated programmatically (default values result in default behavior), in some cases from config files
• it does not take env vars into account by default. To enable configuration via env vars, one must invoke cfg = cfg.FromEnv() explicitly on an existing config object, e.g. after setting the default parameters programmatically.

One downside of this configuration mechanism is that it must accounts for a number of optional features and, as a result, forces all dependencies to be included. E.g. in Java we have several modules (core, thrift, zipkin, etc.), and a top-level client module that implements the configuration and depends on all of the other modules (whether you end up using them or not).

[carlosalberto]

I think of making every SDK implement these variables for configuring manual instrumentation is a post-GA item,

Sounds good to me. For now the only components required to support this would be auto-instrumentation components.

[Oberon00]

The name should be

Suggested change

	| OTEL_TRACE_SAMPLING_PROBABILITY| Sampling probability, a number in the [0..1] range, e.g. 0.25| 1 | |
	| OTEL_TRACE_SAMPLER_TRACEIDRATIO_RATIO| Sampling ratio (probability), a number in the [0..1] range, e.g. 0.25| 1 | |

to match the name traceidratio that is used below for OTEL_TRACE_SAMPLER and to have some logical hierarchy that is easy to understand and extensible if we add configuration for other samplers.

The current name has no obvious connection to traceidratio, which I would find very confusing as a new user.

[jmacd]

I support this renaming. The intention for this environment variable is to configure only the traceidratio sampler, as I see it.

[carlosalberto]

+1

[jkwatson]

@Oberon00 @jkwatson as you seem to be more familiar with Sampling needs, maybe want to drop your opinion on what should be stated here?

I'm more concerned with having clear instructions for implementors in this case.

[carlosalberto]

Closed in favor of #1190