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 OTEL_TRACE_SAMPLER_ARG env variable definition. #1202

Merged
merged 5 commits into from
Nov 9, 2020

Conversation

carlosalberto
Copy link
Contributor

@carlosalberto carlosalberto commented Nov 6, 2020

Fixes #1105
Closes #1190 (alternative solution)

Changes

It was discussed in the latest TC meeting trying out the approach taken by Jaeger, which uses two environment variables to configure samplers:

  1. JAEGER_SAMPLER_TYPE
  2. JAEGER_SAMPLER_PARAM: the Sampler-specific parameters (as defined by each specific type).

This way, each Sampler will use the same environment variable, and we could even extend the future expected input, i.e. accept json.

@carlosalberto carlosalberto added area:sdk Related to the SDK area:sampling Related to trace sampling spec:trace Related to the specification/trace directory release:allowed-for-ga Editorial changes that can still be added before GA since they don't require action by SIGs labels Nov 6, 2020
@carlosalberto carlosalberto requested review from a team as code owners November 6, 2020 02:41
Copy link
Member

@yurishkuro yurishkuro left a comment

Choose a reason for hiding this comment

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

lgtm

@carlosalberto
Copy link
Contributor Author

@open-telemetry/specs-approvers Please review this issue (will merge by EOD Monday).

Copy link
Member

@Oberon00 Oberon00 left a comment

Choose a reason for hiding this comment

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

Nice solution!

Co-authored-by: Christian Neumüller <christian+github@neumueller.me>
Copy link
Member

@tigrannajaryan tigrannajaryan left a comment

Choose a reason for hiding this comment

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

LGTM, just one minor comment left.

specification/sdk-environment-variables.md Outdated Show resolved Hide resolved
Co-authored-by: Tigran Najaryan <4194920+tigrannajaryan@users.noreply.github.com>
@bogdandrutu bogdandrutu merged commit f5519f2 into open-telemetry:master Nov 9, 2020
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:allowed-for-ga Editorial changes that can still be added before GA since they don't require action by SIGs spec:trace Related to the specification/trace directory
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add OTEL_SAMPLING_PROBABILITY (or similar named) to sdk environment variable spec
7 participants