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

Sampling SDK spec is outdated (and wrong by now) #634

Closed
Oberon00 opened this issue Jun 4, 2020 · 7 comments
Closed

Sampling SDK spec is outdated (and wrong by now) #634

Oberon00 opened this issue Jun 4, 2020 · 7 comments
Assignees
Labels
area:sampling Related to trace sampling area:sdk Related to the SDK priority:p1 Highest priority level release:required-for-ga Must be resolved before GA release, or nice to have before GA spec:trace Related to the specification/trace directory

Comments

@Oberon00
Copy link
Member

Oberon00 commented Jun 4, 2020

See https://github.com/open-telemetry/opentelemetry-specification/blob/1729bc4337bdc1df83ff7eab53eb7405d6e75665/specification/trace/sdk.md#sampling

Sampling may be implemented on different stages of a trace collection. OpenTelemetry API defines a Sampler interface that can be used at instrumentation points by libraries to check the SamplingResult early and optimize the amount of telemetry that needs to be collected.

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.

@Oberon00 Oberon00 changed the title Sampling SDK spec is outdated Sampling SDK spec is outdated (and wrong by now) Jun 4, 2020
@cijothomas
Copy link
Member

The Instrumentations/libraries can still use span.IsRecording() which is an API concept? (though IsRecording is set by the SDK based on sampling, but this instrumentation doesn't care how it was set)

@Oberon00
Copy link
Member Author

Oberon00 commented Jun 5, 2020

They sure can use IsRecording. But I would expect the SDK spec to describe precisely that. Currently the connection between Sampler and Tracer(Provider) 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, .

@cijothomas
Copy link
Member

Got it.
+1 to having spec describing the sampling in more detail.

@bogdandrutu bogdandrutu added the spec:trace Related to the specification/trace directory label Jun 12, 2020
@carlosalberto carlosalberto added the area:sdk Related to the SDK label Jun 26, 2020
@carlosalberto carlosalberto added the release:required-for-ga Must be resolved before GA release, or nice to have before GA label Jul 8, 2020
@Oberon00 Oberon00 mentioned this issue Jul 14, 2020
@andrewhsu andrewhsu added the priority:p1 Highest priority level label Jul 17, 2020
@jmacd
Copy link
Contributor

jmacd commented Jul 21, 2020

Related: #625

@alolita
Copy link
Member

alolita commented Jul 24, 2020

Ack. @tedsuo will be providing an update in the Sampler SIG.

@alolita
Copy link
Member

alolita commented Aug 11, 2020

Based on the spec meeting discussion today, @Oberon00 please tag/assign me once you've created new issues that are more specific and closed this issue.

@Oberon00
Copy link
Member Author

Oberon00 commented Aug 11, 2020

Closing this in favor of #781 and #782.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:sampling Related to trace sampling area:sdk Related to the SDK priority:p1 Highest priority level release:required-for-ga Must be resolved before GA release, or nice to have before GA spec:trace Related to the specification/trace directory
Projects
None yet
Development

No branches or pull requests

7 participants