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.

Sign up for GitHub

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_SAMPLING_PROBABILITY env variable definition #1117

Closed
wants to merge 2 commits into from
Closed

Add OTEL_SAMPLING_PROBABILITY env variable definition #1117

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3ecad98
Add OTEL_SAMPLING_PROBABILITY env variable definition
Oct 20, 2020
30769ff
PR fixes
Oct 20, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@ New:

- Enforce that the Baggage API must be fully functional, even without an installed SDK.
([#1103](/~https://github.com/open-telemetry/opentelemetry-specification/pull/1103))
- Add OTEL_TRACE_SAMPLING_PROBABILITY env variable definition
([#1117](/~https://github.com/open-telemetry/opentelemetry-specification/pull/1117))
- Rename "Canonical status code" to "Status code"
([#1081](/~https://github.com/open-telemetry/opentelemetry-specification/pull/1081))
- Add Metadata for Baggage entries, and clarify W3C Baggage Propagator implementation
Expand Down
1 change: 1 addition & 0 deletions spec-compliance-matrix.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,7 @@ status of the feature is not known.
|OTEL_SPAN_EVENT_COUNT_LIMIT | | | | | | | | | | |
|OTEL_SPAN_LINK_COUNT_LIMIT | | | | | | | | | | |
|OTEL_TRACE_SAMPLER | | | | | | | | | | |
|OTEL_TRACE_SAMPLING_PROBABILITY | | | | | | | | | | |

## Exporters

Expand Down
3 changes: 2 additions & 1 deletion specification/sdk-environment-variables.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,8 @@ The goal of this specification is to unify the environment variable names betwee
| OTEL_RESOURCE_ATTRIBUTES | Key-value pairs to be used as resource attributes | | See [Resource SDK](./resource/sdk.md#specifying-resource-information-via-an-environment-variable) for more details. |
| OTEL_LOG_LEVEL | Log level used by the SDK logger | "info" | |
| OTEL_PROPAGATORS | Propagators to be used as a comma separated list | "tracecontext,baggage" | Values MUST be deduplicated in order to register a `Propagator` only once. Unrecognized values MUST generate a warning and be gracefully ignored. |
| OTEL_TRACE_SAMPLER | Sampler to be used for traces | "parentbased_always_on" | See [Sampling](./trace/sdk.md#sampling) |
| OTEL_TRACE_SAMPLER | Sampler to be used for traces | "parentbased_always_on" | See [Sampling](./trace/sdk.md#sampling) |
| OTEL_TRACE_SAMPLING_PROBABILITY| Sampling probability, a number in the [0..1] range, e.g. 0.25| 1 | |
Copy link
Member

@Oberon00 Oberon00 Oct 30, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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).

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

Or parentbased_traceidratio.

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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).

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member

@Oberon00 Oberon00 Oct 30, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

+1


Known values for OTEL_PROPAGATORS are: "tracecontext", "baggage", "b3", "jaeger".
Additional values can be specified in the respective SDK's documentation, in case third party `Propagator`s are supported, such as "xray" or "ottracer".
Expand Down