NH-2313 Bugfixes: propagator gets tracestate from carrier; from_header usage fix

[tammy-baylis-swi]

Bugfixes:

(1)
OTel Python TraceState.from_header accepts a list of header strings, not a single header string. Using the latter results in quiet dropping of input values! 😦

(2)
SW Propagator should get and update tracestate header from the carrier before (re-)injection, not the span context. This is because the current span context's tracestate doesn't change during the loop through of all propagators' inject as part of CompositePropagator.inject. Currently, tracestate header injected by a first propagator (e.g. tracecontext) is being completely overwritten by a later propagator (e.g. solarwinds_propagator). To capture tracestate header changes made by a first propagator in the composite, later propagators should update that and re-inject.

In another PR (https://github.com/appoptics/opentelemetry-python-testbed/pull/22), we are restricting OTEL_PROPAGATORS so that tracecontext must always precede solarwinds_propagator. tracecontext maps to TraceContextTextMapPropagator which injects tracestate into the carrier: https://github.com/open-telemetry/opentelemetry-python/blob/main/opentelemetry-api/src/opentelemetry/trace/propagation/tracecontext.py#L89-L109 Therefore, tracestate should always be available in the carrier for SW Propagator to use.

Manual test results on the upper half of the table on this doc: https://swicloud.atlassian.net/wiki/spaces/NIT/pages/2909569037/2022-04-27+to+28+propagators+combinations+testing

Customer documentation draft updated here: https://swicloud.atlassian.net/wiki/spaces/NIT/pages/2867726621/NH+Python+Troubleshooting#Customizing-OTel-Propagators

[cheempz]

LGTM, thanks for the writeup and detailed test results @tammy-baylis-swi! I left a couple minor questions on the test results page, the main curiosity is whether the "last touched" tracestate value should be on the leftmost side. If it's something the OTel Python code isn't actually doing correctly we can just note that down and move on, no need to try addressing an upstream issue at this point.

[tammy-baylis-swi]

LGTM, thanks for the writeup and detailed test results @tammy-baylis-swi! I left a couple minor questions on the test results page, the main curiosity is whether the "last touched" tracestate value should be on the leftmost side. If it's something the OTel Python code isn't actually doing correctly we can just note that down and move on, no need to try addressing an upstream issue at this point.

Thanks Lin! I reply-commented on Confluence. tl;dr I think it's because #15 is separate from this one and I'll do another test after merging both.

[tammy-baylis-swi]

because #15 is separate from this one and I'll do another test after merging both.

Actually I'd test again after merging #14. Too many branchessss 😅

[tammy-baylis-swi]

Also mentioned on Confluence. After merging I think tracestate k=v ordering is correct now: https://swicloud.atlassian.net/wiki/spaces/NIT/pages/3039462335/2022-05-03+tracestate+order+testing

[jiwen624]

SW Propagator should get and update tracestate header from the carrier before (re-)injection, not the span context.

Here are my two cents re. the problem #2 @tammy-baylis-swi @cheempz I'm writing it based on the Java OpenTelemetry agent/API so thing may be different for Python.

1. The TexMapPropagator interface's inject method does not offer the getter of the carrier, which means we (i.e Java agent) cannot get the tracestate from the carrier. The span context is the only place we can access and get tracestate. That why in Java we used an additional attribute to pass the data we want.
2. It's true that in this case the downstream propagator cannot see the upstream propagators' update to a particular key. The value may be lost if the propagators writes/updates the same key to the carrier. I'm not sure if it's a design limit (as I see the OpenTelemetry's own propagators also fetch data from span context. They're doing the same thing as us. (I'll post this problem to the Otel repo)
3. However, for now (in Java) the three propagators configured in the NH Java agent don't conflict with each other (which means they don't write the same header to the carrier), so we are still fine. No changes are needed.

[jiwen624]

Asked a question in the OpenTelemetry API repo: open-telemetry/opentelemetry-java#4463

[jiwen624]

Currently, tracestate header injected by a first propagator (e.g. tracecontext) is being completely overwritten by a later propagator

BTW in Java the tracecontext propagator just fetches the tracestate from the span context (the same as us) and encode them into a string. As we (the Solarwinds propagator) encodes the tracestate in the same way (per the spec) so it's still fine in this case.

[jiwen624]

BTW in the spec it mentions that the value(s) to be injected should be retrieved from the span context, which is exactly what we are doing: https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/context/api-propagators.md#inject

[tammy-baylis-swi]

Thank you for those answers and for posting the question to OTel @jiwen624 !