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

Default global propagators #428

Closed
carlosalberto opened this issue Jan 28, 2020 · 15 comments · Fixed by #930
Closed

Default global propagators #428

carlosalberto opened this issue Jan 28, 2020 · 15 comments · Fixed by #930
Assignees
Labels
area:api Cross language API specification issue priority:p1 Highest priority level release:required-for-ga Must be resolved before GA release, or nice to have before GA spec:context Related to the specification/context directory
Milestone

Comments

@carlosalberto
Copy link
Contributor

Now that OTEP 66 was merged, we need to decide what the default global propagators need to be, either no-op or something meaningful (like TraceContext/CorrelationContext).

See: #208

@carlosalberto carlosalberto added spec:context Related to the specification/context directory api vs sdk bounds labels Jan 28, 2020
@carlosalberto carlosalberto added this to the Alpha v0.4 milestone Jan 28, 2020
@Oberon00
Copy link
Member

How has the situation changed here with OTEP 66? Some default HttpTextFormat or equivalent had to exist even before.

@carlosalberto
Copy link
Contributor Author

I know a few languages already had TraceContext (W3C's) in the API package, and were exposed in their DefaultTracer already ;) (I know in Java we have this, but only us).

@dyladan
Copy link
Member

dyladan commented Mar 18, 2020

In JS the TraceContext propagator is in the SDK package not the API package. As far as I'm aware, this is the case for most of the SIGs. Additionally, in JS context management is no-op by default, which means even if the propagator extracted a value, it would be added to a no-op context and dropped. At least in JS, we cannot have a default context manager that works in both web and nodejs. We could have platform-specific defaults if required, but this is not the way it currently works.

@carlosalberto carlosalberto modified the milestones: v0.5, v0.6 Jun 9, 2020
@bogdandrutu bogdandrutu added the area:api Cross language API specification issue label Jun 26, 2020
@carlosalberto carlosalberto added the release:required-for-ga Must be resolved before GA release, or nice to have before GA label Jul 2, 2020
@jkwatson
Copy link
Contributor

jkwatson commented Jul 8, 2020

Sooo. What's the state of this issue? Someone needs to own it.

@carlosalberto carlosalberto self-assigned this Jul 8, 2020
@carlosalberto
Copy link
Contributor Author

I will own it. Will dig what's the state of this for each SIG and will re-start the discussion.

@Oberon00
Copy link
Member

Oberon00 commented Jul 8, 2020

As per #208 (comment) I would suggest we stay with no-op.

@carlosalberto
Copy link
Contributor Author

(I'm starting to feel having no propagators registered by default is the way to go - agents and similar layers can be the ones setting them as needed)

@Oberon00
Copy link
Member

Oberon00 commented Jul 8, 2020

I think it should be easy (single function call or right package in classpath etc) to configure OpenTelemetry to act as transparent proxy, but it should not be the default.

@dyladan
Copy link
Member

dyladan commented Jul 14, 2020

I will own it. Will dig what's the state of this for each SIG and will re-start the discussion.

A lot of the work to determine the state of each SIG has already been done on this issue.

#520

@iNikem
Copy link
Contributor

iNikem commented Jul 14, 2020

My 2c: OTel should have sensible defaults so that it would do sensible things out of the box. Without requiring any configuration. Like using OTLP exporter to localhost:$defaultPort. And using W3C TraceContext propagator. Etc. Not forcing users to make a lot movements to get something working...

@Oberon00
Copy link
Member

I think we have to discuss separately:

  1. What should be the propagators if we only have the context layer? Probably no-op, since SpanContext, etc does not exist in this package.
  2. What should be the propagators if we add the API artifact? I think if we have the API but not the SDK that indicates that some library with built-in OpenTelemetry support is used but the application does not configure or even know about OpenTelemetry. I think we should use no-op here to avoid surprises. Otherwise adding built-in OpenTelemetry support to any library will be a breaking, behavior changing change.
  3. What should be the propagators if we add the SDK artifact? I think that having at least a pass-through propagator here would make sense.

@iNikem
Copy link
Contributor

iNikem commented Jul 14, 2020

Without SDK and without auto-instrumentation - no-op, agreed. As soon as we have SDK or auto-instrumentation we should propagate context even if we don't do anything ourselves in this process. We just should decide which wire format to use. My vote goes to W3C TraceContext.

@jkwatson
Copy link
Contributor

@carlosalberto can we get a status update on this?

@carlosalberto
Copy link
Contributor Author

@jkwatson hey, thanks for asking. I have a draft proposal locally that I will send as a PR tomorrow (and FYI, in the proposal we go propagatorless by default).

@jkwatson
Copy link
Contributor

@jkwatson hey, thanks for asking. I have a draft proposal locally that I will send as a PR tomorrow (and FYI, in the proposal we go propagatorless by default).

Great. I'll start work on a PR for #689 assuming that will be the case. Thanks for the update!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:api Cross language API specification issue priority:p1 Highest priority level release:required-for-ga Must be resolved before GA release, or nice to have before GA spec:context Related to the specification/context directory
Projects
None yet
7 participants