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

Sampler.ShouldSample has wrong parameter descriptions. #883

Closed
Oberon00 opened this issue Aug 26, 2020 · 0 comments · Fixed by #884
Closed

Sampler.ShouldSample has wrong parameter descriptions. #883

Oberon00 opened this issue Aug 26, 2020 · 0 comments · Fixed by #884
Labels
area:sampling Related to trace sampling area:sdk Related to the SDK bug Something isn't working priority:p3 Lowest 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 Aug 26, 2020

Late follow-up from #634.

In https://github.com/open-telemetry/opentelemetry-specification/blob/14b5b6a/specification/trace/sdk.md#shouldsample, the spec has some confusing or even wrong wording when it describes the parameters, namely:

  • SpanContext of a parent Span.:

  • TraceId of the Span to be created. It can be different from the TraceId in the SpanContext. Typically in situations when the Span to be created starts a new Trace.

    I don't think this can be different. If there is any valid parent trace ID, then it must match the newly created Span. If the Span where to start a new Trace, the parent context would be empty (or null with the current wording).

Marking this as P3 since I think the text in question is not really normative. But it's still wrong, so I think it would be desirable to fix this before GA.

CC @alolita

@Oberon00 Oberon00 added bug Something isn't working area:sdk Related to the SDK area:sampling Related to trace sampling spec:trace Related to the specification/trace directory release:required-for-ga Must be resolved before GA release, or nice to have before GA priority:p3 Lowest priority level labels Aug 26, 2020
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 bug Something isn't working priority:p3 Lowest 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

Successfully merging a pull request may close this issue.

1 participant