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

Specify Span ID creation with sampling. #998

Closed
Closed
Show file tree
Hide file tree
Changes from 10 commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,8 @@ Updates:
- SDK: Specify known values, as well as basic error handling for OTEL_PROPAGATORS.
([#962](https://github.com/open-telemetry/opentelemetry-specification/pull/962))
([#995](https://github.com/open-telemetry/opentelemetry-specification/pull/995))
- SDK: Specify when to generate new IDs with sampling
([#998](https://github.com/open-telemetry/opentelemetry-specification/pull/998))
- Remove custom header name for Baggage, use official header
([#993](https://github.com/open-telemetry/opentelemetry-specification/pull/993))
- Trace API: Clarifications for `Span.End`, e.g. IsRecording becomes false after End
Expand Down
23 changes: 18 additions & 5 deletions specification/trace/sdk.md
Original file line number Diff line number Diff line change
Expand Up @@ -60,11 +60,24 @@ The following table summarizes the expected behavior for each combination of
The SDK defines the interface [`Sampler`](#sampler) as well as a set of
[built-in samplers](#built-in-samplers) and associates a `Sampler` with each [`TracerProvider`].

When asked to create a Span, the SDK MUST query the `Sampler`'s [`ShouldSample`](#shouldsample) method before actually creating the span, and act accordingly:
see description of [`ShouldSample`'s](#shouldsample) return value below for how to set `IsRecording` and `Sampled` on the Span,
and the [table above](#recording-sampled-reaction-table) on whether to pass the `Span` to `SpanProcessor`s.
A non-recording span MAY be implemented using the same mechanism as when a `Span` is created with no API-implementation installed
(sometimes called a `NoOpSpan` or `DefaultSpan`).
### SDK Span creation
Oberon00 marked this conversation as resolved.
Show resolved Hide resolved

When asked to create a Span, the SDK MUST act as if doing the following in order:

1. If there is a valid parent trace ID, use it. Otherwise generate a new one (
note: this must be done before calling `ShouldSample`, because it expects
a valid trace Id as input).
Oberon00 marked this conversation as resolved.
Show resolved Hide resolved
2. Query the `Sampler`'s [`ShouldSample`](#shouldsample) method.
yurishkuro marked this conversation as resolved.
Show resolved Hide resolved
3. If the decision is `DROP` and there is a valid parent span ID, use it.
Oberon00 marked this conversation as resolved.
Show resolved Hide resolved
Otherwise (if the decision is not `DROP` or there was no valid parent span ID)
a new span ID MUST be generated.
Copy link
Member

Choose a reason for hiding this comment

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

The current state of this PR sounds like the right thing to do to me, but I'm not entirely sure of the consequences.
Fellow @open-telemetry/technical-committee, please take a look (especially at previous comments #998 (comment)) as well. We might need to sort this out before GA.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes. This is tricky, but I like @Oberon00's proposal that DROP sampling decisions at the root of a trace should generate a new trace ID and new span ID without the sampled bit set. Am I understanding?

Copy link
Member Author

Choose a reason for hiding this comment

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

Am I understanding?

Yes, "no valid parent span ID" is equivalent to a root span.

4. Create a span depending on the decision returned by `ShouldSample`:
see description of [`ShouldSample`'s](#shouldsample) return value below
for how to set `IsRecording` and `Sampled` on the Span,
and the [table above](#recording-sampled-reaction-table) on whether
to pass the `Span` to `SpanProcessor`s.
A non-recording span MAY be implemented using the same mechanism as when a
`Span` is created with no API-implementation installed (i.e., a [Propagated Span](api.md#propagated-span-creation)).

### Sampler

Expand Down