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

HTTPTextFormat propagator formats should support chaining propagators #210

Closed
toumorokoshi opened this issue Aug 10, 2019 · 8 comments
Closed
Labels
spec:context Related to the specification/context directory
Milestone

Comments

@toumorokoshi
Copy link
Member

I think an important capability for HTTPTextFormat propagators is the ability to attach more than one. I.E. Tracer.GetHTTPTextFormat could return a proxy object that executes extract / inject on multiple HTTPTextFormats.

This would be invaluable when migrating propagation standards, such as from b3 to tracestate.

@carlosalberto
Copy link
Contributor

This was mentioned in the Java repo some months ago, through a "multiformat" text adapter (to be added in the contrib section), precisely to support multiple formats: open-telemetry/opentelemetry-java#119

@toumorokoshi
Copy link
Member Author

Cool! It's not clear to me why we wouldn't just want to make this a feature exposed by a contrib package rather than just in the API itself. I think it's relatively straightforward to make them chainable by allowing for a proxy object, and making the propagators have an api like:

inject(context: SpanContext, getter: Getter, object: object) -> None

@iredelmeier iredelmeier added the spec:context Related to the specification/context directory label Aug 13, 2019
@carlosalberto
Copy link
Contributor

There's always the worry of making the API too cumbersome to use, as well as increasing the API surface overall.

Probably having an actual testcase/example could get things moving ;)

@toumorokoshi
Copy link
Member Author

Sounds good, am working on an example in opentelemetry-python.

@a-feld
Copy link
Member

a-feld commented Aug 21, 2019

@toumorokoshi I wonder if it would be helpful to have the context tracked in the propagator instance itself (either by having the context in the constructor or via calls to the context API within the propagator).

If the propagator is context-aware, an API called ExtractIntoCurrentContext or similar could be added where the propagator itself references the current context.

If the propagator understands how to set the context, the propagator extract becomes easily chainable by simply calling each propagator's extract 1 at a time, especially in case where both SpanContext and DistributedContext should be extracted.

This could also simplify the inject API by providing the current context through an attribute on the propagator rather than through the interface.

@tedsuo
Copy link
Contributor

tedsuo commented Dec 4, 2019

FWIW we have an example of this behavior here, in the LightStep OT client:
https://github.com/lightstep/lightstep-tracer-java-common/blob/master/common/src/main/java/com/lightstep/tracer/shared/PropagatorStack.java

Extract: try each header type in a list, until one is found.
Inject: Only inject the configured header type(s).

@toumorokoshi
Copy link
Member Author

My concern here will be addressed somewhat by the ability to enable multiple propagators here: open-telemetry/oteps#66. I think I'll potentially pick this issue up again around implementation details once that's merged in and there's a spec to build on.

@carlosalberto
Copy link
Contributor

@toumorokoshi Closing it after OTEP 66 was merged. Feel free to re-open it if you think we still need to track this ;)

TuckTuckFloof pushed a commit to TuckTuckFloof/opentelemetry-specification that referenced this issue Oct 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
spec:context Related to the specification/context directory
Projects
None yet
Development

No branches or pull requests

6 participants