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

Should SpanProcessor.OnStart/OnEnd be called for non-recording Spans? #782

Closed
Oberon00 opened this issue Aug 11, 2020 · 1 comment · Fixed by #871
Closed

Should SpanProcessor.OnStart/OnEnd be called for non-recording Spans? #782

Oberon00 opened this issue Aug 11, 2020 · 1 comment · Fixed by #871
Assignees
Labels
area:sampling Related to trace sampling area:sdk Related to the SDK 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 11, 2020

As discussed in the SIG meeting, this issue is split from #634.

In https://github.com/open-telemetry/opentelemetry-specification/blob/bc272ab52869df2145065d55300d721ff008e34b/specification/trace/sdk.md#shouldsample, the spec describes that when the sampler returns NOT_RECORD, then

IsRecording() == false, span will not be recorded and all events and attributes will be dropped.

Should the SpanProcessor be notified in this case? If so, should it only be notified for the start event, or also for the end event (OnStart can be invoked immediately without much hassle; OnEnd would preclude implementation of an unsampled span with a default span, because it would need a reference to its SpanProcessor).

I assume we agree that the SpanProcessor must be invoked for not sampled but recorded spans, but this could also be made explicit.

Assigned @alolita as per #634 (comment)

@Oberon00 Oberon00 added 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 labels Aug 11, 2020
@andrewhsu andrewhsu added the priority:p3 Lowest priority level label Aug 18, 2020
@cijothomas
Copy link
Member

@alolita I had opened a PR to address this, and hopefully we can close this once its merged.

@cijothomas cijothomas self-assigned this Aug 25, 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 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.

4 participants