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 table summarizing sampler properties and processing #871

Merged
merged 22 commits into from
Aug 26, 2020
Merged
Changes from all commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
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
30 changes: 20 additions & 10 deletions specification/trace/sdk.md
Original file line number Diff line number Diff line change
Expand Up @@ -24,18 +24,18 @@ latest sampling could happen on the Collector which is out of process.

The OpenTelemetry API has two properties responsible for the data collection:

* `IsRecording` field of a `Span`. If `true` the current `Span` records
tracing events (attributes, events, status, etc.), otherwise all tracing
events are dropped. Users can use this property to determine if expensive
trace events can be avoided. [Span Processors](#span-processor) will receive
all spans with this flag set. However, [Span Exporter](#span-exporter) will
not receive them unless the `Sampled` flag was set.
* `IsRecording` field of a `Span`. If `false` the current `Span` discards all
tracing data (attributes, events, status, etc.). Users can use this property
to determine if collecting expensive trace data can be avoided. [Span
Processor](#span-processor) MUST receive only those spans which have this
field set to `true`. However, [Span Exporter](#span-exporter) SHOULD NOT
receive them unless the `Sampled` flag was also set.
* `Sampled` flag in `TraceFlags` on `SpanContext`. This flag is propagated via
the `SpanContext` to child Spans. For more details see the [W3C Trace Context
specification][trace-flags]. This flag indicates that the `Span` has been
`sampled` and will be exported. [Span Processor](#span-processor) and [Span
Exporter](#span-exporter) will receive spans with the `Sampled` flag set for
processing.
specification](https://www.w3.org/TR/trace-context/#sampled-flag). This flag indicates that the `Span` has been
`sampled` and will be exported. [Span Exporters](#span-exporter) MUST
receive those spans which have `Sampled` flag set to true and they SHOULD NOT receive the ones
Copy link
Member Author

Choose a reason for hiding this comment

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

Since the text here is "SHOULD NOT" - it means the recommendation is to not call Exporters for Spans without Sampled flag set. Processors could still do it, and spec is not prohibiting it.
As we used "SHOULD NOT" here, do we still require additional explicit clarification here, or in the table that "Depending on the Processor, Exporter may still get span without Sampled flag set? @carlosalberto @Oberon00

Copy link
Member

Choose a reason for hiding this comment

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

I don't think it's necessary but it would be nice. If you want, you can add a sentence to the built in span processors section that they MUST NOT call exporters for unsampled spans (unless explicitly requested).

Copy link
Member Author

Choose a reason for hiding this comment

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

Let me address that as a follow up PR. I fear adding it might delay this PRs progress.

(Meanwhile I'll to figure out under what scenarios a user want to sent unsampled spans to exporter).

that do not.

The flag combination `SampledFlag == false` and `IsRecording == true`
means that the current `Span` does record information, but most likely the child
Expand All @@ -45,6 +45,16 @@ The flag combination `SampledFlag == true` and `IsRecording == false`
could cause gaps in the distributed trace, and because of this OpenTelemetry API
MUST NOT allow this combination.

The following table summarizes the expected behavior for each combination of
`IsRecording` and `SampledFlag`.

| `IsRecording` | `Sampled` Flag | Span Processor receives Span? | Span Exporter receives Span? |
Copy link
Member

Choose a reason for hiding this comment

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

Actually the third column is a change / additional specification, but I think it's good.

@cijothomas Can you add "Fixes #782" to the PR description?

CC @alolita (you have #782 assigned currently).

| ------------- | -------------- | ----------------------------- | ---------------------------- |
| true | true | true | true |
| true | false | true | false |
| false | true | Not allowed | Not allowed |
cijothomas marked this conversation as resolved.
Show resolved Hide resolved
| false | false | false | false |

The SDK defines the interface [`Sampler`](#sampler) as well as a set of
[built-in samplers](#built-in-samplers).

Expand Down