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

It is unclear what the behavior of the fields() method/function should be on a composite TextMapPropagator #1809

Open
jkwatson opened this issue Jul 8, 2021 · 8 comments
Assignees
Labels
spec:context Related to the specification/context directory

Comments

@jkwatson
Copy link
Contributor

jkwatson commented Jul 8, 2021

When a composite TextMapPropagator doesn't have the same injectors as extractors, what should the behavior of the fields be? Should it only be the fields that are being injected, or only those that are extracted, or the union of both?

Context:
open-telemetry/opentelemetry-java#3366 in java to create a composite that doesn't have the same injectors as extractors.

@carlosalberto
Copy link
Contributor

I'd go with the union of both. cc @bogdandrutu

@ghost
Copy link

ghost commented Jul 9, 2021

Seems that it has to be union - we already have some code (eg io.opentelemetry.instrumentation.awslambda.v1_0.ApiGatewayProxyRequest) where fields are checked to verify if http context propagation is needed. Without union, in some cases, that would not work properly.

@slinkydeveloper
Copy link

From: open-telemetry/opentelemetry-java#3366 (comment)

Let's take a concrete case I have in mind: I have 2 extractors, 1 injector, assuming fields returns the union of the header names used by extractors and injectors, if you use fields to preallocate the headers when writing back to the wire, you're gonna waste allocations because you're gonna allocate for those extractors as well.

Perhaps you need 2 methods for getting the fields, one for injectors and one for extractors?

@ghost
Copy link

ghost commented Jul 14, 2021

I believe that 2 methods would be the purest idea, clearly separating extractor / injector fields. However, to maintain backwards compatibility we'd need to extend the TextMapPropagator with 2 new methods while leaving the old one (fields) returning the union. What do you think?

@ghost
Copy link

ghost commented Jul 20, 2021

Hey team, any thoughts on this? IT's blocking open-telemetry/opentelemetry-java#3366 that would otherwise got merged already.
/CC @bogdandrutu @anuraaga @iNikem

@carlosalberto
Copy link
Contributor

In general extractor/injector will work exactly on the same header (W3C's baggage and trace propagators). The difference only happens for 'legacy' propagation formats (B3, Jaeger), which IMHO doesn't justifies enough adding more interfaces, unless the current design creates a BIG problem.

@yurishkuro
Copy link
Member

I was not previously aware of fields() method, it seems poorly defined in the spec. Concretely, the only two examples the spec provides for the use of this method are to clear the keys on reused carrier or to pre-allocate space for injection. If that is the only purpose, then the answer to this ticket seems to be "it's only a union of fields() from injectors", and the spec should be clarified to narrow down the scope of the method.

@pierDipi
Copy link

pierDipi commented Sep 6, 2021

Hi, any update on this issue?
We're interested in open-telemetry/opentelemetry-java#3366.

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

5 participants