Decouple the responsibilities of the environment variable propagation carrier

[pellared]

Follows #4944

Per #4944 (comment)

Per #4961 (comment)

Changes

Decouple the responsibilities of the environment variable propagation carrier from concerns that are:

1. specific to individual propagators used with it,
2. already handled by the underlying environment variable APIs (error handling and limits when setting values).

[pellared]

CC @open-telemetry/semconv-cicd-approvers

[marcalff]

This is consistent with how C++ implemented it:

open-telemetry/opentelemetry-cpp#3834 (comment)

The Carrier is supposed to be ignorant of which propagator is used, only propagators will know the keys.

Conceptually, the environment variable carrier is supposed to work with any propagator, including B3, so it should not hard code any known keys.

[pellared]

This is consistent with how C++ implemented it:

open-telemetry/opentelemetry-cpp#3834 (comment)

The Carrier is supposed to be ignorant of which propagator is used, only propagators will know the keys.
Conceptually, the environment variable carrier is supposed to work with any propagator, including B3, so it should not hard code any known keys.

Exactly! The same applies for:

• Go: https://github.com/open-telemetry/opentelemetry-go-contrib/blob/main/propagators/envcar/carrier.go
• Python: https://github.com/open-telemetry/opentelemetry-python/blob/main/opentelemetry-api/src/opentelemetry/propagators/_envcarrier.py

I added the C++ implementation for reference.

In my opinion, the current Swift implementation is not good. My intuition tells me that this should be done by implementing Setter and Getter (disclaimer: I never wrote a single line of code in Swift). I am considering removing this implementation from this document as I think it is a misleading reference. However, I do not want to do this in this PR.
CC @open-telemetry/swift-core-approvers

[adrielp]

This is consistent with how C++ implemented it:
open-telemetry/opentelemetry-cpp#3834 (comment)

The Carrier is supposed to be ignorant of which propagator is used, only propagators will know the keys.
Conceptually, the environment variable carrier is supposed to work with any propagator, including B3, so it should not hard code any known keys.

Exactly! The same applies for:

• Go: https://github.com/open-telemetry/opentelemetry-go-contrib/blob/main/propagators/envcar/carrier.go
• Python: https://github.com/open-telemetry/opentelemetry-python/blob/main/opentelemetry-api/src/opentelemetry/propagators/_envcarrier.py

I added the C++ implementation for reference.

In my opinion, the current Swift implementation is not good. My intuition tells me that this should be done by implementing Setter and Getter (disclaimer: I never wrote a single line of code in Swift). I am considering removing this implementation from this document as I think it is a misleading reference. However, I do not want to do this in this PR. CC @open-telemetry/swift-core-approvers

It's a long standing, prior art implementation.

As a user of the OTel libraries, I've preferred experiences with propagator wrappers because I know what I'm getting. Whereas, with the carrier, it's less obvious (as in I have to dig) to figure out what is available.

Does this guidance now mean that the B3 propagator, as an example, will have to have normalization logic added in order to convert X_B3_TRACEID to X-B3-TraceId? Or do they already have logic that handles that?

[pellared]

@adrielp,

As a user of the OTel libraries, I've preferred experiences with propagator wrappers because I know what I'm getting. Whereas, with the carrier, it's less obvious (as in I have to dig) to figure out what is available.

Can you please be more specific and provide code examples that demonstrate how using carrier is less obvious (and what does it mean). It rather seems to opposite as e.g. it would requiring creating a new propagator each time you want to spawn a new process. This seems also against the idea of defining/setting propagator once and using it with different carriers. How would one use the same propagator for HTTP instrumentation where HTTP headers are used as propagation carrier and something that spawns a new CLI command needs to propagate the context via env vars? How would making a propagation wrapper would make this example better: https://go.dev/play/p/LV7REU1UXmJ ?

Does this guidance now mean that the B3 propagator, as an example, will have to have normalization logic added in order to convert X_B3_TRACEID to X-B3-TraceId? Or do they already have logic that handles that?

If it does then I made some mistakes in this PR. Is there any part that says that? The B3 propagator should have nothing to do with converting X_B3_TRACEID to X-B3-TraceId. It is the concern of the env var propagation carrier (or propagator wrapper if we find a use case where it is better than implementing via propagation carrier which we do not seem to currently have). Or maybe are there parts if this PR better described?

[adrielp]

@adrielp,

As a user of the OTel libraries, I've preferred experiences with propagator wrappers because I know what I'm getting. Whereas, with the carrier, it's less obvious (as in I have to dig) to figure out what is available.

Can you please be more specific and provide code examples that demonstrate how using carrier is less obvious (and what does it mean). It rather seems to opposite as e.g. it would requiring creating a new propagator each time you want to spawn a new process. This seems also against the idea of defining/setting propagator once and using it with different carriers. How would one use the same propagator for HTTP instrumentation where HTTP headers are used as propagation carrier and something that spawns a new CLI command needs to propagate the context via env vars? How would making a propagation wrapper would make this example better: https://go.dev/play/p/LV7REU1UXmJ ?

Does this guidance now mean that the B3 propagator, as an example, will have to have normalization logic added in order to convert X_B3_TRACEID to X-B3-TraceId? Or do they already have logic that handles that?

If it does then I made some mistakes in this PR. Is there any part that says that? The B3 propagator should have nothing to do with converting X_B3_TRACEID to X-B3-TraceId. It is the concern of the env var propagation carrier (or propagator wrapper if we find a use case where it is better than implementing via propagation carrier which we do not seem to currently have). Or maybe are there parts if this PR better described?

In one of the early working prototypes I had, the propagator had the mapping directly defined similar to how some of the other propagators did it. Because it was clearly defined in the code, it was quickly readable.

With carrier, you have to dive deeper to figure out what, when normalized, will be respected by the w/e propagator is being used.

AFAICT, the behavior being described here seems to require that the propagators have to do some type of remapping once the values are carried inside the code. TRACEPARENT works because there is no _ or - and so when normalized it matches the header key defined in the spec. X_B3_TRACEID would not match X-B3-TraceId unless 1) it's renormalized to replace _ with - in the b3 propagator and 2) the propagator doesn't care about case sensitivity or already normalizes the string characters to lower/upper.

Sure, everything will get carried, but as far as knowing what gets carried vs what gets propagated is harder in my mind to understand. Maybe I'm missing something, and maybe this shouldn't be specifically addressed in this pull request.

[pellared]

AFAICT, the behavior being described here seems to require that the propagators have to do some type of remapping once the values are carried inside the code.

The propagators are already responsible for encoding the values that are encoded in the propagation header values. These are already handled by the propagator implementation. For instance here is an implementation of Inject for B3 propagator in Go: https://github.com/open-telemetry/opentelemetry-go/blob/3ff9ad7291e201d583145061afa10ec6824cdeaf/propagation/baggage.go#L29-L35. If the propagator is used used with HTTP headers as carrier then it sets the encoded value in baggage HTTP header. If it is used with env vars as carrier then it sets the encoded value in BAGGAGE env var. My guess is that you are worried of the remapping that is already implemented in the propagators.

The env var carrier normalizes the X-B3-TraceId key to X-B3-TRACEID so that it would interoperate during both injection and extraction.

You may also be interested in this thread: #4944 (comment)

EDIT: I created a PR for Swift to follow the same pattern as in other languages open-telemetry/opentelemetry-swift-core#47

[adrielp]

#4944 (comment)

yes, that's exactly what I was worried about. thank you for the clarification.