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

Propagators API #1013

Closed
hdost opened this issue Mar 30, 2023 · 4 comments · Fixed by #1373
Closed

Propagators API #1013

hdost opened this issue Mar 30, 2023 · 4 comments · Fixed by #1373
Assignees
Labels
A-common Area:common issues that not related to specific pillar A-trace Area: issues related to tracing release:required-for-stable Must be resolved before GA release, or nice to have before GA.

Comments

@hdost
Copy link
Contributor

hdost commented Mar 30, 2023

According to the spec the propagators https://github.com/open-telemetry/opentelemetry-rust/blob/main/opentelemetry-sdk/src/propagation/composite.rs should be in the API instead of the SDK

Context Conversation: https://cloud-native.slack.com/archives/C03GDP0H023/p1679426866872239

@hdost hdost added A-trace Area: issues related to tracing A-common Area:common issues that not related to specific pillar labels Mar 30, 2023
@hdost hdost added this to the Tracing API And SDK Stable milestone Mar 30, 2023
@hdost hdost added the release:required-for-stable Must be resolved before GA release, or nice to have before GA. label Mar 30, 2023
@wperron
Copy link
Contributor

wperron commented Apr 5, 2023

As far as I understand it the current structure does respect the spec; the TextMapPropagator is defined in the API, and specific implementations are found in the SDK. Digging around, seems like this conversation already occurred in #266. @jtescher might know more.

@hdost
Copy link
Contributor Author

hdost commented Apr 6, 2023

If that were the case then these should also be in the SDK.
Java
C#

As well the specific propagators such as Jaeger and B3(a.k.a Zipkin) should be in different extension crates and the W3C ones can (probably should by included in the API).
https://sourcegraph.com/github.com/open-telemetry/opentelemetry-specification/-/blob/specification/context/api-propagators.md#propagators-distribution
Though

@cijothomas
Copy link
Member

https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/context/api-propagators.md#propagators-distribution From this spec, the W3C propagators may be distributed as part of API packages.

@hdost hdost self-assigned this Apr 25, 2023
@cijothomas cijothomas assigned TommyCpp and unassigned hdost Oct 24, 2023
@TommyCpp
Copy link
Contributor

TommyCpp commented Nov 12, 2023

Distribution of propagators in other language:

Propagator Language Location
baggage propagator Go API / propagation
baggage propagator Java API / baggage
baggage propagator .NET API / propagation
trace context propagator Go API / propagation
trace context propagator Java API / propagation
trace context propagator .NET API / propagation
composite propagator Go API / propagation
composite propagator Java API / propagation
composite propagator .NET API / propagation

For composite propagator I think we should move it to API because:

  • I see it more of a generic helper instead of an actual implementation so it's consistent with the definition of API
  • Most language implement is as part of the API along with the interface. Jave even implemented a composite method in the interface

For baggage propagator and trace context propagator I think we can leave them in the SDK for now. Although it's different from other languages I think it's aligned with our principle to keep the interfaces and implementations separate

hdost pushed a commit that referenced this issue Nov 21, 2023
Move `TextMapCompositePropagator` from `opentelemetry-sdk` to `opentelemetry` crate. 

Fixes #1013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-common Area:common issues that not related to specific pillar A-trace Area: issues related to tracing release:required-for-stable Must be resolved before GA release, or nice to have before GA.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants