Add table summarizing sampler properties and processing #871

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?

specification/trace/sdk.md

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

specification/trace/sdk.md

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

specification/trace/sdk.md

Co-authored-by: Yuri Shkuro <yurishkuro@users.noreply.github.com>

Co-authored-by: Christian Neumüller <christian+github@neumueller.me>

Co-authored-by: Yuri Shkuro <yurishkuro@users.noreply.github.com>

Co-authored-by: Christian Neumüller <christian+github@neumueller.me>

specification/trace/sdk.md

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

specification/trace/sdk.md

-  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

specification/trace/sdk.md

+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

specification/trace/sdk.md

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

specification/trace/sdk.md

Co-authored-by: Christian Neumüller <christian+github@neumueller.me>

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)

specification/trace/sdk.md

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 <yurishkuro@users.noreply.github.com> * events -> data Co-authored-by: Yuri Shkuro <yurishkuro@users.noreply.github.com> * plural Co-authored-by: Yuri Shkuro <yurishkuro@users.noreply.github.com> * use MUST Co-authored-by: Christian Neumüller <christian+github@neumueller.me> * Update specification/trace/sdk.md Co-authored-by: Yuri Shkuro <yurishkuro@users.noreply.github.com> * Update specification/trace/sdk.md Co-authored-by: Christian Neumüller <christian+github@neumueller.me> * reword * spacing * more links fix * markdownl * Update specification/trace/sdk.md Co-authored-by: Christian Neumüller <christian+github@neumueller.me> Co-authored-by: Yuri Shkuro <yurishkuro@users.noreply.github.com> Co-authored-by: Christian Neumüller <christian+github@neumueller.me> Co-authored-by: Carlos Alberto Cortez <calberto.cortez@gmail.com>

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

specification/trace/sdk.md Outdated Show resolved Hide resolved

specification/trace/sdk.md Outdated Show resolved Hide resolved

specification/trace/sdk.md Outdated Show resolved Hide resolved

specification/trace/sdk.md Outdated Show resolved Hide resolved

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 <yurishkuro@users.noreply.github.com>

events -> data

62aed50

Co-authored-by: Yuri Shkuro <yurishkuro@users.noreply.github.com>

plural

3de6f46

Co-authored-by: Yuri Shkuro <yurishkuro@users.noreply.github.com>

use MUST

0b625c8

Co-authored-by: Christian Neumüller <christian+github@neumueller.me>

Update specification/trace/sdk.md

7c7f265

Co-authored-by: Yuri Shkuro <yurishkuro@users.noreply.github.com>

Update specification/trace/sdk.md

36b83c5

Co-authored-by: Christian Neumüller <christian+github@neumueller.me>

Oberon00 approved these changes Aug 25, 2020

View reviewed changes

Oberon00 reviewed Aug 25, 2020

View reviewed changes

specification/trace/sdk.md Outdated Show resolved Hide resolved

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

specification/trace/sdk.md Outdated Show resolved Hide resolved

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 <christian+github@neumueller.me>

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

specification/trace/sdk.md Show resolved Hide resolved

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add table summarizing sampler properties and processing #871

Add table summarizing sampler properties and processing #871

cijothomas commented Aug 25, 2020 •

edited

Loading

carlosalberto commented Aug 25, 2020

yurishkuro Aug 25, 2020

Oberon00 Aug 25, 2020 •

edited

Loading

cijothomas Aug 25, 2020

Oberon00 Aug 25, 2020

cijothomas Aug 25, 2020

lzchen left a comment

Oberon00 commented Aug 25, 2020 •

edited

Loading

cijothomas commented Aug 25, 2020

lzchen commented Aug 25, 2020

Oberon00 commented Aug 25, 2020

Oberon00 Aug 25, 2020

Oberon00 Aug 25, 2020

cijothomas commented Aug 25, 2020

cijothomas commented Aug 26, 2020

	* `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

Add table summarizing sampler properties and processing #871

Add table summarizing sampler properties and processing #871

Conversation

cijothomas commented Aug 25, 2020 • edited Loading

carlosalberto commented Aug 25, 2020

yurishkuro Aug 25, 2020

Choose a reason for hiding this comment

Oberon00 Aug 25, 2020 • edited Loading

Choose a reason for hiding this comment

cijothomas Aug 25, 2020

Choose a reason for hiding this comment

Oberon00 Aug 25, 2020

Choose a reason for hiding this comment

cijothomas Aug 25, 2020

Choose a reason for hiding this comment

lzchen left a comment

Choose a reason for hiding this comment

Oberon00 commented Aug 25, 2020 • edited Loading

cijothomas commented Aug 25, 2020

lzchen commented Aug 25, 2020

Oberon00 commented Aug 25, 2020

Oberon00 Aug 25, 2020

Choose a reason for hiding this comment

Oberon00 Aug 25, 2020

Choose a reason for hiding this comment

cijothomas commented Aug 25, 2020

cijothomas commented Aug 26, 2020

cijothomas commented Aug 25, 2020 •

edited

Loading

Oberon00 Aug 25, 2020 •

edited

Loading

Oberon00 commented Aug 25, 2020 •

edited

Loading