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

Set tracecontext, baggage as global propagators by default #2611

Closed
pellared opened this issue Feb 15, 2022 · 7 comments
Closed

Set tracecontext, baggage as global propagators by default #2611

pellared opened this issue Feb 15, 2022 · 7 comments
Labels
area:propagators Part of OpenTelemetry context propagation pkg:SDK Related to an SDK package
Milestone

Comments

@pellared
Copy link
Member

pellared commented Feb 15, 2022

What

Set tracecontext, baggage as global propagators by default.

Related issue: #2304

Why

This is how I understand the OTel specification: https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/sdk-environment-variables.md#general-sdk-configuration

I am aware that this would be a "behavioral breaking change". However, I think such a default value is very useful. It would even lead to simplifying the examples.

I am aware that OTEL_PROPAGATORS is not supported. However, if it would be then still this default should be applied. I think that the simplest way is just to default to it.

Originally posted by @pellared in open-telemetry/opentelemetry-go-contrib#1843 (review)

@pellared pellared added area:propagators Part of OpenTelemetry context propagation pkg:SDK Related to an SDK package labels Feb 15, 2022
@Aneurysm9
Copy link
Member

IMO the OTel spec says that the SDK should default to these propagators and the user should not need to specify them by hand.

This is in direct conflict with the API specification:

The OpenTelemetry API MUST use no-op propagators unless explicitly configured otherwise.

The language regarding the OTEL_PROPAGATORS environment variable is very old and predates the change from the API also having those default propagators to no propagators by default. I wonder whether this was an oversight in the change away from having default propagators in open-telemetry/opentelemetry-specification#721.

I would be very uncomfortable making a change like this without a major version increase.

@1046102779
Copy link
Contributor

1046102779 commented Feb 16, 2022

https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/sdk-environment-variables.md#general-sdk-configuration

OTEL_PROPAGATORS belongs to SDK

https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/context/api-propagators.md#global-propagators

The OpenTelemetry API MUST use no-op propagators unless explicitly configured otherwise.

it belongs to API.

The Opentelmetry API and SDK are different parts. I think it hasn't conflict.

@Aneurysm9
Copy link
Member

Global propagators belong to the API and have defined behavior, even in the absence of an SDK. The guiding principle of the OTel API/SDK split is that it requires explicit action on the part of the application developer and/or end user to change the default no-op behavior of the API.

@1046102779
Copy link
Contributor

Global propagators belong to the API and have defined behavior, even in the absence of an SDK. The guiding principle of the OTel API/SDK split is that it requires explicit action on the part of the application developer and/or end user to change the default no-op behavior of the API.

Does that mean that all Opentelemetry behaviors require end user or instrumentation calls explicitly to specify specific behaviors?

@Aneurysm9
Copy link
Member

My understanding of that is that is an extension that the application developer or end user must opt-in to using by including an additional artifact, beyond both the API and SDK artifacts, in their build process. Again, the API spec is clear that the global propagator MUST be a no-op "unless explicitly configured otherwise."

@pellared
Copy link
Member Author

pellared commented Feb 16, 2022

Quick analysis for other SDKs.

So at least others also see that API != SDK.

I am closing this issue as I think it will be safer (also for sake of non-breaking change) to explicitly "enable" for such functionality and this could be done as part of #2304

@Aneurysm9
Copy link
Member

Python actually sets those defaults in their API. This is wrong, but I don't have any control over what the python SIG does. v0v

As for doing this as part of #2304, I will object to setting defaults there as well. We can add support for configuring propagators through the environment variable, but to the extent that we would attempt to change the default behavior in the absence of explicit user configuration, I will strongly object.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:propagators Part of OpenTelemetry context propagation pkg:SDK Related to an SDK package
Projects
None yet
Development

No branches or pull requests

3 participants