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

Encode values for the Jaeger propagator #2032

Open
carlosalberto opened this issue Nov 6, 2020 · 9 comments
Open

Encode values for the Jaeger propagator #2032

carlosalberto opened this issue Nov 6, 2020 · 9 comments
Labels
blocked:spec blocked on open or unresolved spec release:required-for-ga Required for 1.0 GA release

Comments

@carlosalberto
Copy link
Contributor

carlosalberto commented Nov 6, 2020

As a follow-up of #1549 we may need to support encoded Baggage values too.

As Jaeger has support for both URL-encoded and non-encoded values, how should this be implemented? Should we have two Jaeger propagators, i.e. JaegerPropagator.getEncodedInstance() and JaegerPropagator.getInstance()?

(Also, we may need to add a value for OTEL_PROPAGATORS to support this dual case)

@yurishkuro would you mind advising how to proceed here?

@carlosalberto carlosalberto added the release:required-for-ga Required for 1.0 GA release label Nov 6, 2020
@yurishkuro
Copy link
Member

What is OTEL's plan for propagators that can be used with transports that do not have HTTP restrictions on the headers?

If the plan is to continue using HTTP propagator (at least for now), then you only need one Jaeger propagator that applies url-encoding to header values. But there will be danger that the serialized format will be incompatible with native Jaeger SDKs for non-HTTP cases.

@jkwatson
Copy link
Contributor

jkwatson commented Dec 2, 2020

@carlosalberto is there a spec for this?

@yurishkuro
Copy link
Member

@jkwatson no spec, just reference implementations in Jaeger SDK. TL;DR; - "encoded" means the values are url-encoded/decoded, which is what is used for OpenTracing.HTTP_HEADERS propagator, and unencoded for OpenTracing.TEXT_MAP propagator.

@jkwatson
Copy link
Contributor

jkwatson commented Dec 2, 2020

@jkwatson no spec, just reference implementations in Jaeger SDK. TL;DR; - "encoded" means the values are url-encoded/decoded, which is what is used for OpenTracing.HTTP_HEADERS propagator, and unencoded for OpenTracing.TEXT_MAP propagator.

I was more referring to what SDKs are required to implement wrt. to this functionality, rather than the exact details of the functionality. IIRC, what SDKs are supposed to do for Jaeger exporters is woefully underspecified currently.

@yurishkuro
Copy link
Member

That sounds like a different issue from this ticket, which is specifically about url-encoding of in-band trace context

@jkwatson
Copy link
Contributor

jkwatson commented Dec 2, 2020

That sounds like a different issue from this ticket, which is specifically about url-encoding of in-band trace context

sorry...mis-typed there. I meant the jaeger propagator specification, which is completely missing from the otel specs.

@yurishkuro
Copy link
Member

@jkwatson
Copy link
Contributor

jkwatson commented Dec 2, 2020

I think that simply linking to another document is not an acceptable "specification". For example, the Jaeger spec has things in it like "We’re considering a new feature that allows downstream services to upsample if they find their tracing level is too low". What should OTel do with that? How will OTel implementors know when/if that needs to be added?

@carlosalberto
Copy link
Contributor Author

I think we need to add some of features that we expect to support in Jaeger (handy if we don't plan to support all). We did this for B3 Propagation, and it's worth spending a few cycles for this, adding also an Encoded values in the Spec Matrix under Jaeger Propagation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked:spec blocked on open or unresolved spec release:required-for-ga Required for 1.0 GA release
Projects
None yet
Development

No branches or pull requests

3 participants