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

Helper methods for accessing Span directly from Context in Span API #950

Closed
jsuereth opened this issue Aug 15, 2021 · 3 comments · Fixed by #967
Closed

Helper methods for accessing Span directly from Context in Span API #950

jsuereth opened this issue Aug 15, 2021 · 3 comments · Fixed by #967

Comments

@jsuereth
Copy link
Contributor

jsuereth commented Aug 15, 2021

The 1.0.0 specification states:

This section defines all operations within the Tracing API that interact with the Context.

The API MUST provide the following functionality to interact with a Context instance:

- Extract the Span from a Context instance
- Insert the Span to a Context instance
The functionality listed above is necessary because API users SHOULD NOT have access to the Context Key used by the Tracing API implementation.

If the language has support for implicitly propagated Context (see here), the API SHOULD also provide the following functionality:

- Get the currently active span from the implicit context. This is equivalent to getting the implicit context, then extracting the Span from the context.
- Set the currently active span to the implicit context. This is equivalent to getting the implicit context, then inserting the Span to the context.

I was looking for documentation or example (e.g. context propagation) where this is done. In some languages, they provide helper methods directly on Span which will extract/insert into context (both explicitly or implicitly).

Specifically, I think this may need to be public: https:/open-telemetry/opentelemetry-cpp/blob/main/api/include/opentelemetry/trace/propagation/detail/context.h#L14

It looks like these methods may be on Tracer: https:/open-telemetry/opentelemetry-cpp/blob/main/api/include/opentelemetry/trace/tracer.h#L146. This is inconsistent with other APIs.

Ideally the propogation:detail + tracer methods would get reconciled into a standard header with Span <-> Context interaction.

@lalitb
Copy link
Member

lalitb commented Aug 28, 2021

@jsuereth -
To ensure that I understand the ask here:

  • Below functions are to be moved from trace::propagation to trace namespace ( and make them part of public API ), either keep them as inline function, or make them static function in Span class:
    - trace::propagation::GetSpan()
    - trace::propagation::SetSpan()
  • No changes are required for below static methods on implicitly propagated context:
    - trace::Tracer:: GetCurrentSpan()
    - trace::Tracer:: WithActiveSpan()

@lalitb
Copy link
Member

lalitb commented Aug 31, 2021

@jsuereth - would you please help clarifying the questions above. Thanks :)

@jsuereth
Copy link
Contributor Author

jsuereth commented Sep 2, 2021

Sorry for huge delay in response. Just getting back to OSS things.

Below functions are to be moved from trace::propagation to trace namespace ( and make them part of public API ), either keep them as inline function, or make them static function in Span class

Yeah, the ask is move this out of the propagation package and have them "near" the span class. You can make them inline or static on Span, but they should be near span. The PR (#967) you sent LGTM

No changes are required for below static methods on implicitly propagated context:

I'm suggesting, for consistency with other SDKs/APIs, that these methods live near the other context + span related methods. The Specification does NOT place these on Tracer, so it's a major inconsistency.

You can leave the static methods on tracer, but I'd suggest you also provide them near the GetSpan/SetSpan methods and just have Tracer call into it. I'd also deprecate the trace::Tracer::WithActiveSpan version of the method. I think it's leftover from earlier specification.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants