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

Jump to bottom

Specify Span ID creation with sampling. #998

Closed
wants to merge 21 commits into from
Closed

Specify Span ID creation with sampling. #998

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
77bbb73
Specify Span ID creation with sampling.
Oberon00 Sep 24, 2020
48b76e4
Add CHANGELOG.
Oberon00 Sep 24, 2020
db30eaf
Merge branch 'master' into sdk-spanid-sampling
Oberon00 Sep 24, 2020
57a4e25
Mismerge/lint.
Oberon00 Sep 24, 2020
c59c207
Period.
Oberon00 Sep 24, 2020
e31741a
Merge branch 'master' into sdk-spanid-sampling
Oberon00 Sep 25, 2020
b6afd59
Merge branch 'master' into sdk-spanid-sampling
Oberon00 Oct 6, 2020
d0fbf4e
Update specification/trace/sdk.md
Oberon00 Oct 7, 2020
eceae84
Merge remote-tracking branch 'upstream/master' into sdk-spanid-sampling
Oberon00 Oct 12, 2020
4006816
Ensure that even DROPed spans always have a valid ID.
Oberon00 Oct 12, 2020
08b15ed
Update specification/trace/sdk.md
Oberon00 Oct 22, 2020
bfd652f
Add compliance matrix entry.
Oberon00 Oct 22, 2020
83e0410
Merge remote-tracking branch 'upstream/master' into sdk-spanid-sampling
Oberon00 Oct 22, 2020
6f04106
Fix compliance matrix.
Oberon00 Oct 22, 2020
34263a3
Apply suggestions from code review
Oberon00 Oct 28, 2020
9933136
Fix dead internal link.
Oberon00 Oct 28, 2020
bc9ef43
Merge branch 'master' into sdk-spanid-sampling
Oberon00 Nov 3, 2020
eb989b7
Add forward-ref to ParentBased sampler.
Oberon00 Nov 3, 2020
17a58fb
Merge branch 'master' into sdk-spanid-sampling
Oberon00 Nov 3, 2020
9c9d652
Typo
Oberon00 Nov 3, 2020
24b6032
Merge branch 'master' into sdk-spanid-sampling
Oberon00 Nov 11, 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 @@ -101,6 +101,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))

Expand Down
23 changes: 18 additions & 5 deletions specification/trace/sdk.md
View file
Open in desktop
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`, use the parent span ID or all-zero if there is none.
Copy link
Member Author

Choose a reason for hiding this comment

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

That means: If there is no parent and the decision is drop, we will have a span with the new trace ID but an invalid span ID. The SpanContext as a whole is thus invalid. We could also specify that the generated trace ID should be discarded in that case.

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually this may be worth re-considering: Generating trace+span ID for an unsampled root has implications if the span is injected. If we generate an invalid spancontext (with zero span ID), we will inject nothing as we have an invalid traceparent header as per W3C (cf. also #753). This causes downstream components to potentially create traces / run sampling logic that they would not if they got an incoming unsampled traceparent header.

Copy link
Contributor

Choose a reason for hiding this comment

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

We could also specify that the generated trace ID should be discarded in that case.

I'd be fine with this.

we will inject nothing as we have an invalid traceparent header as per W3C (cf. also #753). This causes downstream components to potentially create traces

Could you elaborate?

Copy link
Member Author

@Oberon00 Oberon00 Oct 12, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

@carlosalberto We never inject an invalid SpanContext, at least using W3C. If we would generate a new ID instead of using zero, we would inject a valid spancontext with unset sampled flag. This will likely cause the receiver to suppress the trace. On the other hand, if we use zero, the downstream component will not receive any tracestate and will likely start a new trace.

Now that I write, this, I realize that this even applies to local child spans: Normally if the root span is not sampled, local child spans would be unsampled too, but with this they would become root spans.

This is bad. I think we will have to create Span and Trace ID even for "dropped" root spans after all. It seems there is a dependency of #864 "Option to allow "default" IDs for unsampled traces" on #753 "Current "Invalid" SpanContext definition precludes TraceState-only SpanContext". We would have to have some sentinel SpanContext that is "valid, unsampled, but without any IDs". That's out of scope here of course.

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated in 4006816. Maybe this demonstrates how easy it is to mess up here and that it is thus important to have this specified clearly? 😃

Otherwise (if the decision is not `DROP`) a new span ID MUST be generated.
Copy link
Member Author

Choose a reason for hiding this comment

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

That means: RECORD_ONLY spans get a new span ID, always. I don't know what the purpose of RECORD_ONLY is (should we remove it?), but having different spans with the same trace+span ID seems worse than breaking the trace in case any children of the RECORD_ONLY span become RECORD_AND_SAMPLE .

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
(sometimes called a `NoOpSpan` or `DefaultSpan`).
Oberon00 marked this conversation as resolved.
Show resolved Hide resolved

### Sampler

Expand Down