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

Merged
merged 22 commits into from
Aug 26, 2020
Merged

Add table summarizing sampler properties and processing #871

Changes from 14 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
ddcc2d7
Add table summarizing sampler properties and processing
cijothomas Aug 25, 2020
03d6d70
markdownlink
cijothomas Aug 25, 2020
29a4a91
Merge branch 'master' into cijothomas/sdksamper1
cijothomas Aug 25, 2020
66d1620
event -> data to avoid confusion with Event
cijothomas Aug 25, 2020
62aed50
events -> data
cijothomas Aug 25, 2020
3de6f46
plural
cijothomas Aug 25, 2020
0b625c8
use MUST
cijothomas Aug 25, 2020
7c7f265
Update specification/trace/sdk.md
cijothomas Aug 25, 2020
36b83c5
Update specification/trace/sdk.md
cijothomas Aug 25, 2020
686969e
reword
cijothomas Aug 25, 2020
9452678
spacing
cijothomas Aug 25, 2020
bbad7aa
more links fix
cijothomas Aug 25, 2020
f77ffdd
markdownl
cijothomas Aug 25, 2020
fbfe97d
Merge branch 'master' into cijothomas/sdksamper1
cijothomas Aug 25, 2020
5d92a46
Update specification/trace/sdk.md
cijothomas Aug 25, 2020
6ce78c8
Merge branch 'master' into cijothomas/sdksamper1
cijothomas Aug 25, 2020
8dbcc78
Merge branch 'master' into cijothomas/sdksamper1
cijothomas Aug 25, 2020
94e9bb4
Merge branch 'master' into cijothomas/sdksamper1
yurishkuro Aug 26, 2020
455b600
Merge branch 'master' into cijothomas/sdksamper1
cijothomas Aug 26, 2020
29fab81
Merge branch 'master' into cijothomas/sdksamper1
carlosalberto Aug 26, 2020
e9ca916
Merge branch 'master' into cijothomas/sdksamper1
cijothomas Aug 26, 2020
c241b1d
Merge branch 'master' into cijothomas/sdksamper1
yurishkuro Aug 26, 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
30 changes: 20 additions & 10 deletions specification/trace/sdk.md
View file
Open in desktop
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
cijothomas marked this conversation as resolved.
Show resolved Hide resolved
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