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

Fix typo #702

Merged
merged 2 commits into from
Jul 15, 2020
Merged

Fix typo #702

merged 2 commits into from
Jul 15, 2020

Conversation

reyang
Copy link
Member

@reyang reyang commented Jul 14, 2020

Changes

Sampler is defined by the SDK, not API. This seems to be a leftover when we moved the sampler from API to SDK.

@reyang reyang requested review from a team July 14, 2020 18:13
@reyang reyang added area:sampling Related to trace sampling bug Something isn't working spec:trace Related to the specification/trace directory labels Jul 14, 2020
Oberon00
Oberon00 approved these changes Jul 14, 2020
View reviewed changes
Copy link
Member

@Oberon00 Oberon00 left a comment

Choose a reason for hiding this comment

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

Related #634

specification/trace/sdk.md Outdated Show resolved Hide resolved
Oberon00
Oberon00 previously requested changes Jul 14, 2020
View reviewed changes
Copy link
Member

@Oberon00 Oberon00 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Actually, no wait.

This is actively telling instrumentations to use the SDK. I think it is better to have this obviously wrong than subtly wrong and actively misleading. Of course, if you want to tackle #634 and make a larger overhaul of this, feel free!

@bogdandrutu bogdandrutu requested a review from Oberon00 July 14, 2020 20:23
@bogdandrutu
Copy link
Member

@Oberon00 what is your suggestion or recommendation?

@Oberon00
Copy link
Member

@bogdandrutu I documented that on #634, pasted here:

This is wrong, since samplers are an SDK concept and thus cannot (or at least should not) be used by instrumentations. Also, there is no mention of the TracerProvider having a sampler asssociated with it, or the tracer automatically checking the sampler when starting a span.

[...]

They [instrumentations] sure can use IsRecording. But I would expect the SDK spec to describe precisely that. Currently the connection between Sampler and Tracer(Provider) [and IsRecording] is not described at all. For example, if we want to describe what Java implements, we could add something like this to the spec:

When starting a span, the Tracer MUST query the Sampler that is assigned to the TracerProvider it was obtained from. If the samplers decision is NOT_RECORD, a Span with a valid trace and span ID generated/inherited as usual and IsRecording being false MUST be created and the SpanProcessors MUST NOT be notified for this Span.

In short, it is simply wrong that instrumentations interact with samplers directly. Removing the whole paragraph would be an improvement over the current state (and also over the state after this PR) IMHO.

cijothomas
cijothomas reviewed Jul 15, 2020
View reviewed changes
specification/trace/sdk.md Outdated
@@ -18,7 +18,7 @@ OpenTelemetry by reducing the number of samples of traces collected and sent to
the backend.

Sampling may be implemented on different stages of a trace collection.
OpenTelemetry API defines a `Sampler` interface that can be used at
The OpenTelemetry SDK defines a `Sampler` interface that can be used at
instrumentation points by libraries to check the `SamplingResult` early and
Copy link
Member

Choose a reason for hiding this comment

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

This is still misleading I believe. Libraries should check IsRecording and not SamplingResult..

Oberon00
Oberon00 reviewed Jul 15, 2020
View reviewed changes
specification/trace/sdk.md
even out of process in Agent or Collector.
Sampling may be implemented on different stages of a trace collection. The
earliest sampling could happen before the trace is actually created, and the
latest sampling could happen on the Collector which is out of process.

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

Choose a reason for hiding this comment

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

It looks like these paragraphs should be in the API spec.

specification/trace/sdk.md

All other sampling algorithms may be implemented on SDK layer in exporters, or
even out of process in Agent or Collector.
Sampling may be implemented on different stages of a trace collection. The
Copy link
Member

@Oberon00 Oberon00 Jul 15, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

This is rather generic, and not really SDK related. The SDK allows sampling only in StartSpan (before OnStart is / would be called).

Copy link
Member Author

Choose a reason for hiding this comment

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

This is rather generic, and not really SDK related. The SDK allows sampling only in StartSpan (before OnStart is / would be called).

I agree that this is very generic. We also have "Sampling is a mechanism to control the noise and overhead introduced by
OpenTelemetry by reducing the number of samples of traces collected and sent to
the backend." in the SDK spec which is also generic.

My suggestion would be keep them for now until we have a better place. I don't think API is a better place as these are very generic context not specific to API/SDK.

@Oberon00 Oberon00 dismissed their stale review July 15, 2020 07:28

Misleading text was removed.

@bogdandrutu bogdandrutu merged commit b610b07 into open-telemetry:master Jul 15, 2020
@reyang reyang deleted the reyang/fixtypo branch July 18, 2020 05:47
codeboten pushed a commit to codeboten/opentelemetry-specification that referenced this pull request Jul 20, 2020
@reyang
Fix typo (open-telemetry#702)
d51eac7 
* fix typo

* rephrase
carlosalberto pushed a commit to carlosalberto/opentelemetry-specification that referenced this pull request Oct 31, 2024
@reyang
Fix typo (open-telemetry#702)
f4b2bdc 
* fix typo

* rephrase
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Oberon00 Oberon00 Oberon00 left review comments

@cijothomas cijothomas cijothomas approved these changes

@bogdandrutu bogdandrutu bogdandrutu approved these changes

@carlosalberto carlosalberto carlosalberto approved these changes

Assignees
No one assigned
Labels
area:sampling Related to trace sampling bug Something isn't working spec:trace Related to the specification/trace directory
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@reyang @bogdandrutu @Oberon00 @carlosalberto @cijothomas