Add table summarizing sampler properties and processing by cijothomas · Pull Request #871 · open-telemetry/opentelemetry-specification

cijothomas · 2020-08-25T05:44:49Z

Fixes #782
No change to existing behavior. Just clarifying when does SpanProcessor and SpanExporters get Span, based on IsRecording and Sampled flag, with a table to make it very obvious.

Please provide a brief description of the changes here. Update the
CHANGELOG.md for non-trivial changes.

Related issues #

Related oteps #

carlosalberto · 2020-08-25T12:45:51Z

LGTM - although, at least in Java, SpanProcessors can be configured to also export Spans without the Sampled flag set. Maybe worth a follow up to clarify this possibility?

yurishkuro · 2020-08-25T13:56:05Z

-  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 `true` the current `Span` records tracing


Suggested change

* `IsRecording` field of a `Span`. If `true` the current `Span` records tracing

* `IsRecording` field of a `Span`. If `true` the current `Span` records in memory tracing

What is "in memory data"? I think the current wording (== the old wording) is more clear here. What would be more clear IMHO would be:

Suggested change

* `IsRecording` field of a `Span`. If `true` the current `Span` records tracing

* `IsRecording` field of a `Span`. If `false` the current `Span` discards all tracing

and remove the "otherwise ...".

Co-authored-by: Yuri Shkuro <[email protected]>

Co-authored-by: Christian Neumüller <[email protected]>

Co-authored-by: Yuri Shkuro <[email protected]>

Co-authored-by: Christian Neumüller <[email protected]>

cijothomas · 2020-08-25T15:30:22Z

-  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


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

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

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

lzchen

Just curious, is IsRecording something that can be explicitly set by the user/instrumentations? I don't see anywhere in the API that actually gives the ability to change it. It doesn't seem to be a "must have" in span [creation](https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/trace/api.md#span-creation either.

Oberon00 · 2020-08-25T15:35:41Z

IsRecording is set according to the sampling decision, see the spec of ShouldSample's return value.

cijothomas · 2020-08-25T15:41:42Z

Just curious, is IsRecording something that can be explicitly set by the user/instrumentations? I don't see anywhere in the API that actually gives the ability to change it. It doesn't seem to be a "must have" in span [creation](https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/trace/api.md#span-creation either.

The only place which can set the IsRecording is Sampler as @Oberon00 pointed out. After that point, its a "read-only" property. Users can use IsRecording to know what was the relevant part of the sampling decision, but not change it.

lzchen · 2020-08-25T15:42:48Z

@Oberon00
I see, so the Decision returned is up to the discretion of the sampler? Reading the specs I was confused because I thought that the combination of isRecording and sampledFlag was the thing that DETERMINED the Decision, not the other way around.

Oberon00 · 2020-08-25T16:06:27Z

It goes both ways a bit. The sampler gets the parent SpanContext, which includes the parent Sampled flag, as input and can use that to influence the Decision it returns (see ParentBased sampler). But that decision, as you said, then determines the sampled+recorded flags of the child span.

Oberon00 · 2020-08-25T17:22:31Z

+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? |


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

Oberon00 · 2020-08-25T17:23:38Z

-  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


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

Co-authored-by: Christian Neumüller <[email protected]>

cijothomas · 2020-08-25T23:21:48Z

@yurishkuro I made more refactoring after initial approval- So re-requested review from you. (Also saw you as the assigned owner)

cijothomas · 2020-08-26T19:27:10Z

@yurishkuro Please merge.

…ry#871) * Add table summarizing sampler properties and processing * markdownlink * event -> data to avoid confusion with Event Co-authored-by: Yuri Shkuro <[email protected]> * events -> data Co-authored-by: Yuri Shkuro <[email protected]> * plural Co-authored-by: Yuri Shkuro <[email protected]> * use MUST Co-authored-by: Christian Neumüller <[email protected]> * Update specification/trace/sdk.md Co-authored-by: Yuri Shkuro <[email protected]> * Update specification/trace/sdk.md Co-authored-by: Christian Neumüller <[email protected]> * reword * spacing * more links fix * markdownl * Update specification/trace/sdk.md Co-authored-by: Christian Neumüller <[email protected]> Co-authored-by: Yuri Shkuro <[email protected]> Co-authored-by: Christian Neumüller <[email protected]> Co-authored-by: Carlos Alberto Cortez <[email protected]>

Co-authored-by: Armin Ruech <[email protected]>

Add table summarizing sampler properties and processing

ddcc2d7

cijothomas requested review from a team August 25, 2020 05:44

github-actions Bot assigned yurishkuro Aug 25, 2020

markdownlink

03d6d70

arminru approved these changes Aug 25, 2020

View reviewed changes

carlosalberto approved these changes Aug 25, 2020

View reviewed changes

Merge branch 'master' into cijothomas/sdksamper1

29a4a91

Oberon00 reviewed Aug 25, 2020

View reviewed changes

Comment thread specification/trace/sdk.md Outdated

Comment thread specification/trace/sdk.md Outdated

Comment thread specification/trace/sdk.md Outdated

Comment thread specification/trace/sdk.md Outdated

yurishkuro approved these changes Aug 25, 2020

View reviewed changes

cijothomas and others added 6 commits August 25, 2020 07:05

event -> data to avoid confusion with Event

66d1620

Co-authored-by: Yuri Shkuro <[email protected]>

events -> data

62aed50

Co-authored-by: Yuri Shkuro <[email protected]>

plural

3de6f46

Co-authored-by: Yuri Shkuro <[email protected]>

use MUST

0b625c8

Co-authored-by: Christian Neumüller <[email protected]>

Update specification/trace/sdk.md

7c7f265

Co-authored-by: Yuri Shkuro <[email protected]>

Update specification/trace/sdk.md

36b83c5

Co-authored-by: Christian Neumüller <[email protected]>

Oberon00 approved these changes Aug 25, 2020

View reviewed changes

Oberon00 reviewed Aug 25, 2020

View reviewed changes

Comment thread specification/trace/sdk.md Outdated

cijothomas added 4 commits August 25, 2020 07:47

reword

686969e

spacing

9452678

more links fix

bbad7aa

markdownl

f77ffdd

cijothomas commented Aug 25, 2020

View reviewed changes

lzchen approved these changes Aug 25, 2020

View reviewed changes

Merge branch 'master' into cijothomas/sdksamper1

fbfe97d

Oberon00 approved these changes Aug 25, 2020

View reviewed changes

Oberon00 added area:sampling Related to trace sampling area:sdk Related to the SDK spec:trace Related to the specification/trace directory labels Aug 25, 2020

Oberon00 reviewed Aug 25, 2020

View reviewed changes

Comment thread specification/trace/sdk.md Outdated

Oberon00 added priority:p3 Lowest priority level release:required-for-ga Must be resolved before GA release, or nice to have before GA labels Aug 25, 2020

cijothomas and others added 3 commits August 25, 2020 10:57

Update specification/trace/sdk.md

5d92a46

Co-authored-by: Christian Neumüller <[email protected]>

Merge branch 'master' into cijothomas/sdksamper1

6ce78c8

Merge branch 'master' into cijothomas/sdksamper1

8dbcc78

cijothomas requested a review from yurishkuro August 25, 2020 23:21

yurishkuro approved these changes Aug 26, 2020

View reviewed changes

Merge branch 'master' into cijothomas/sdksamper1

94e9bb4

anuraaga reviewed Aug 26, 2020

View reviewed changes

Comment thread specification/trace/sdk.md

cijothomas and others added 3 commits August 26, 2020 06:51

Merge branch 'master' into cijothomas/sdksamper1

455b600

Merge branch 'master' into cijothomas/sdksamper1

29fab81

Merge branch 'master' into cijothomas/sdksamper1

e9ca916

Merge branch 'master' into cijothomas/sdksamper1

c241b1d

yurishkuro merged commit 9750495 into open-telemetry:master Aug 26, 2020

cijothomas deleted the cijothomas/sdksamper1 branch August 26, 2020 21:14

Oberon00 mentioned this pull request Sep 11, 2020

Inconsistent Sampling Result Names #938

Merged

HaloFour mentioned this pull request Nov 30, 2023

Clarification on how processors handle sampling decisions. #3779

Open

schmikei pushed a commit to schmikei/opentelemetry-specification that referenced this pull request Apr 17, 2025

[chore] Move heroku attributes to the registry (open-telemetry#871)

48892b8

Co-authored-by: Armin Ruech <[email protected]>

	* `IsRecording` field of a `Span`. If `true` the current `Span` records tracing
	* `IsRecording` field of a `Span`. If `true` the current `Span` records in memory tracing

	* `IsRecording` field of a `Span`. If `true` the current `Span` records tracing
	* `IsRecording` field of a `Span`. If `false` the current `Span` discards all tracing

Conversation

cijothomas commented Aug 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

carlosalberto commented Aug 25, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

yurishkuro Aug 25, 2020

Choose a reason for hiding this comment

Uh oh!

Oberon00 Aug 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cijothomas Aug 25, 2020

Choose a reason for hiding this comment

Uh oh!

Oberon00 Aug 25, 2020

Choose a reason for hiding this comment

Uh oh!

cijothomas Aug 25, 2020

Choose a reason for hiding this comment

Uh oh!

lzchen left a comment

Choose a reason for hiding this comment

Uh oh!

Oberon00 commented Aug 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cijothomas commented Aug 25, 2020

Uh oh!

lzchen commented Aug 25, 2020

Uh oh!

Oberon00 commented Aug 25, 2020

Uh oh!

Oberon00 Aug 25, 2020

Choose a reason for hiding this comment

Uh oh!

Oberon00 Aug 25, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cijothomas commented Aug 25, 2020

Uh oh!

Uh oh!

cijothomas commented Aug 26, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

cijothomas commented Aug 25, 2020 •

edited

Loading

Oberon00 Aug 25, 2020 •

edited

Loading

Oberon00 commented Aug 25, 2020 •

edited

Loading