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

no-op StartSpan: always return the parent #2121

Merged
merged 5 commits into from
Dec 2, 2021

Conversation

tsloughter
Copy link
Member

Changes

This change makes it so no span id generation is required in the API's no-op tracer. My understanding from the discussion back when this part was added to the spec the implementation is supposed to create a new span id in the no-op tracer if the parent is a recording span -- while saying this is almost never the case it still would require it to be possible.

This is what we had implemented in the Erlang API:

    %% Spec: Behavior of the API in the absence of an installed SDK
    case otel_tracer:current_span_ctx(Ctx) of
        Parent=#span_ctx{trace_id=TraceId,
                         is_recording=false} when TraceId =/= 0 ->
            %% if the parent is a non-recording Span it MAY return the parent Span
            Parent;
        Parent=#span_ctx{trace_id=TraceId} when TraceId =/= 0 ->
            %% API MUST create a non-recording Span with the SpanContext in the parent Context
            SpanCtx = Parent#span_ctx{span_id=opentelemetry:generate_span_id(),
                                      is_recording=false},
            SpanCtx;
        _ ->
            %% If the parent Context contains no valid Span,
            %% an empty non-recording Span MUST be returned
            ?NOOP_SPAN_CTX
    end.

After implementing configurable IdGenerators which required moving the id generators to the SDK, https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/sdk.md#id-generators, I realized the above code could no longer be possible and would not make sense even if it were.

Go and Java both always return the parent or the empty non-recording span, and maybe others. If this is the case for those two and others then this change is an attempt to bring the spec inline with existing implementations. So I think that makes it a non-breaking change :)

@tsloughter tsloughter requested review from a team as code owners November 13, 2021 00:08
@arminru arminru added area:api Cross language API specification issue spec:trace Related to the specification/trace directory labels Nov 16, 2021
@github-actions
Copy link

This PR was marked stale due to lack of activity. It will be closed in 7 days.

@github-actions github-actions bot added the Stale label Nov 27, 2021
specification/trace/api.md Outdated Show resolved Hide resolved
specification/trace/api.md Show resolved Hide resolved
specification/trace/api.md Outdated Show resolved Hide resolved
specification/trace/api.md Outdated Show resolved Hide resolved
specification/trace/api.md Outdated Show resolved Hide resolved
tsloughter and others added 2 commits December 1, 2021 12:11
Co-authored-by: Christian Neumüller <christian+github@neumueller.me>
@jmacd jmacd merged commit a6cfbc9 into open-telemetry:main Dec 2, 2021
@carlosalberto
Copy link
Contributor

Hey, we forgot to add a CHANGELOG entry! @tsloughter think you could cook one?

@tsloughter
Copy link
Member Author

@carlosalberto woops, yup, will open a PR in a minute.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:api Cross language API specification issue spec:trace Related to the specification/trace directory Stale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants