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

Configuring a sampler from an environment variable #679

Open
mwear opened this issue Jun 30, 2020 · 6 comments
Open

Configuring a sampler from an environment variable #679

mwear opened this issue Jun 30, 2020 · 6 comments
Labels
area:sampling Related to trace sampling area:sdk Related to the SDK release:after-ga Not required before GA release, and not going to work on before GA spec:trace Related to the specification/trace directory

Comments

@mwear
Copy link
Member

mwear commented Jun 30, 2020

Sampler configuration

A comment on #666 expressed the desire to be able to configure a sampler by environment variable. I've opened this issue as a way to discuss how this should work.

A possible solution

Configuring a propagator via environment variable requires taking a string and looking up or creating a propagator instance. One way to accomplish this is by having a sampler registry. Since samplers are an extension point, users should be able to register custom types.

Other Solutions

Please discuss alternatives in the comments

Environment Variable Name

Name Description Notes Default
OTEL_SAMPLER The sampler to be used by the sdk E.g. probability, always_on, always_off, my_custom_sampler always_on

Note: We will need to define a canonical name for each sampler. Is lower snakecase a reasonable choice for built samplers, or should we choose something else?

Open Questions

How should we handle failure? What if an environment variable is set to an invalid name?

  • Should we fail to start?
  • Should we use the default AlwaysOn Sampler? If so, how should we indicate this to the user?

I'm open to any other proposals or suggestions. I just wanted to get the discussion going with this proposal.

@yurishkuro
Copy link
Member

If user specifies OTEL_SAMPLER=probabilistic, where would the rate come from?

In Jaeger SDKs we used two vars, mirroring the span tags set after sampling decision was made: sampler.type and sampler.param, where param is a number whose interpretation depends on the sampler, e.g. type=const param=1 means always-on.

@mwear
Copy link
Member Author

mwear commented Jun 30, 2020

Additional information can be specified using sampler specific environment variables. Currently #666 proposes OTEL_SAMPLING_PROBABILITY as the way to configure the probabililty sampler. While I like the idea of sampler.param I think there might be cases where a sampler does not have any additional configuration (like AlwaysOnSampler, or AlwaysOffSampler). Similarly, there might be samplers that have more complicated configuration that isn't easy to fit into a single parameter. Is it reasonable to have sampler specific environment variables?

@yurishkuro
Copy link
Member

yeah, sounds ok

@Oberon00 Oberon00 added area:sampling Related to trace sampling area:sdk Related to the SDK spec:trace Related to the specification/trace directory labels Jul 7, 2020
@carlosalberto carlosalberto added the release:required-for-ga Must be resolved before GA release, or nice to have before GA label Jul 10, 2020
@andrewhsu andrewhsu added the priority:p3 Lowest priority level label Jul 17, 2020
@tigrannajaryan
Copy link
Member

I think this is a nice-to-have, not a must-have for 1.0 GA. It can be added after GA in a non-breaking manner. I suggest to remove release:required-for-ga label.

@tigrannajaryan
Copy link
Member

Since I don't see any objections I am moving this to after GA.

@tigrannajaryan tigrannajaryan added release:after-ga Not required before GA release, and not going to work on before GA and removed release:required-for-ga Must be resolved before GA release, or nice to have before GA labels Sep 15, 2020
@andrewhsu andrewhsu removed the priority:p3 Lowest priority level label Sep 18, 2020
@iNikem
Copy link
Contributor

iNikem commented Aug 3, 2021

We already have OTEL_TRACES_SAMPLER in https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/sdk-environment-variables.md. I think this issue can be closed now, @mwear

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:sampling Related to trace sampling area:sdk Related to the SDK release:after-ga Not required before GA release, and not going to work on before GA spec:trace Related to the specification/trace directory
Projects
None yet
Development

No branches or pull requests

7 participants