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

Define the list of built-in propagation formats #8

Closed
bogdandrutu opened this issue May 13, 2019 · 17 comments
Closed

Define the list of built-in propagation formats #8

bogdandrutu opened this issue May 13, 2019 · 17 comments
Labels
area:sdk Related to the SDK spec:context Related to the specification/context directory
Milestone

Comments

@bogdandrutu
Copy link
Member

  1. Define the list of propagation formats that the OpenTelemetry will officially support, proposal:
  2. Where does the implementation for each format live?
    • In API?
    • In SDK?
    • In contrib?
@austinlparker
Copy link
Member

Related to the first point, what about x-ot headers? Maybe have the API support a list of propagators with a default implementation of the W3C spec in SDK, then support B3/Jaeger/etc. through contrib?

@bogdandrutu
Copy link
Member Author

Where is x-ot header defined?

@austinlparker
Copy link
Member

austinlparker commented May 13, 2019

I don't think it's actually defined anywhere (and whoops, it's actually ot-tracer-*), but there's quite a few usages of it: https://github.com/search?q=ot-tracer-traceid&type=Code (although a lot of these seem like they're just gists...)

I'll see if I can find a canonical reference for it, might just be something that got decided in issues when talking about mock/noop tracer implementations.

@bogdandrutu
Copy link
Member Author

So if there is a specification of these protocol, we can definitely support it (without a specification we cannot support it).

Do you know if anyone uses this in real production or is just define for no-op testing Tracers?

@austinlparker
Copy link
Member

Yeah, I know of some LightStep customers that use it. Presumably others do as well via Envoy/Veneur/Istio/etc. I'll see if we can get it codified into a spec somehow.

Alternately, I think it'd be fine if this was only supported by the opentracing bridge as a legacy protocol. That might be the most straightforward way to handle it.

@austinlparker
Copy link
Member

I talked this over internally and our consensus is that ot-tracer headers shouldn't be supported in the core API/SDK, so no need to consider them for this. I think, from a maintainability perspective, it makes the most sense to have a single blessed W3C implementation in core and then address B3 and others in contrib.

@SergeyKanzhelev SergeyKanzhelev added this to the SDK complete milestone Jun 3, 2019
@iredelmeier
Copy link
Member

re: where exporters should live - my inclination would be to put W3C support as close to core as makes sense, e.g., a standalone library that is also exposed as the default by SDKs. Anything "non-standard" - B3, Jaeger, legacy protocols, etc. - should not be part of the core SDK, so that versioning (especially deprecating!) is decoupled.

@rochdev
Copy link
Member

rochdev commented Jun 5, 2019

I'm not sure I understand what's the difference between W3C format and other formats, other than it being standard. As a user, why would I want to always pull W3C if I'm not going to use it for example?

@SergeyKanzhelev SergeyKanzhelev added area:sdk Related to the SDK and removed area:sdk Related to the SDK labels Jun 21, 2019
@SergeyKanzhelev SergeyKanzhelev changed the title Define the list of standard propagation formats Define the list of built-in propagation formats Jun 21, 2019
@iredelmeier iredelmeier added the spec:context Related to the specification/context directory label Jul 30, 2019
@SergeyKanzhelev SergeyKanzhelev modified the milestones: SDK proposal complete, Alpha v0.3 Sep 27, 2019
@jmacd
Copy link
Contributor

jmacd commented Dec 4, 2019

Note that the Go repo recently moved propagators into the API. This may make it difficult to remove support for these formats in the future: open-telemetry/opentelemetry-go#362

@tedsuo
Copy link
Contributor

tedsuo commented Dec 5, 2019

API vs SDK

I like the original list, as far as SDK support. I also don't see an issue with expanding it to include more as people request them - but only in the SDK. It does not seem like these need to be in the API? I'd like to see examples of why that would be necessary. If we are putting constructors for some of these in the API, it should be a very short list - I would only suggest B3 and W3C. Otherwise, every implementation will have to implement an old Jaeger format until the end of time.

Some addition headers to consider supporting

There are headers defined in Envoy, which I'm not a fan of. But we need to either get envoy to retire them, or we need to support them: https://www.envoyproxy.io/docs/envoy/latest/configuration/http/http_conn_man/headers#config-http-conn-man-headers-x-ot-span-context

@austinlparker FWIW the "ot-tracer-*" headers have their origin in the OpenTracing "basic tracers." LightStep also uses them. https://github.com/opentracing/basictracer-go/blob/master/propagation_ot.go#L22

@austinlparker
Copy link
Member

Should the API just define an interface for propagators (inject and extract), and maybe the propagator stack, and then all implementations live in the SDK?

@tedsuo
Copy link
Contributor

tedsuo commented Dec 5, 2019

@austinlparker yes that's how I imagined it would work. Whether something is B3 vs TraceContext only arises in the constructor - the interfaces propagator interfaces are all in the API.

@jmacd
Copy link
Contributor

jmacd commented Dec 5, 2019

Somehow I feel we should put the Names of these standard propagation formats into the API and the Implementations in the SDK.

@carlosalberto
Copy link
Contributor

carlosalberto commented Dec 5, 2019

Btw, if we happen to provide out-of-the-box propagation with the default no-op Tracer (and Meter?), we might need to have at least W3C TraceContext in the API.

@jmacd
Copy link
Contributor

jmacd commented Dec 5, 2019

I would be happy to state that I think a NoOp implementation should be a true no-op. If the user wants to disable tracing and metrics but continue propagating context in a pass-through way, they could install a propagate-only SDK with support for all the propagators they like.

@austinlparker
Copy link
Member

Somehow I feel we should put the Names of these standard propagation formats into the API and the Implementations in the SDK.

This feels like it'd be great except for when people want to implement their own - for instance, what if someone writes a propagator for X-Amzn-*? We'd need to update the API, or provide a way to pass arbitrary names to the SDK.

@jmacd
Copy link
Contributor

jmacd commented Dec 6, 2019

I have proposed we think of "named" propagators, yes:
open-telemetry/oteps#66 (comment)

@carlosalberto carlosalberto modified the milestones: v0.5, v0.6 Jun 9, 2020
TuckTuckFloof pushed a commit to TuckTuckFloof/opentelemetry-specification that referenced this issue Oct 15, 2020
rockb1017 pushed a commit to rockb1017/opentelemetry-specification that referenced this issue Nov 18, 2021
- Ensure all config vars are in table format
- Prefer SPLUNK prefix
- Fix deployment environment typo

Addresses open-telemetry#7 open-telemetry#8
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:sdk Related to the SDK spec:context Related to the specification/context directory
Projects
None yet
Development

No branches or pull requests

8 participants