[carriers] add env carrier #4609
Conversation
| KEY_MAPPING = { | ||
| "TRACEPARENT": "traceparent", | ||
| "TRACESTATE": "tracestate", | ||
| "BAGGAGE": "baggage" | ||
| } |
There was a problem hiding this comment.
Why do we have a special handling only for these 3 keys?
Couldn't 't it be a generic rule how the env carrier works?
There was a problem hiding this comment.
I've removed them and pushed the change. They were there from my working prototype enforcing on those were set. However, in the specification we do outline that those are the three well-known values that SHOULD be used. Ultimately, it doesn't matter. Any keys in the environment, when lowered, will only be considered valid context in the text map propagator when they match one of the well-known specs supported in the SDK. (b3/w3c/etc)
| """This class decorates Setter to enable setting context and baggage | ||
| to environment variables. |
There was a problem hiding this comment.
Isn't the purpose to set env var for some external process that is going to be spawned?
From https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/context/env-carriers.md#environment-variable-immutability and https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/context/env-carriers.md#process-spawning:
Applications SHOULD NOT modify context-related environment variables of the environment in which the parent process exists.
Parent processes SHOULD copy the current environment variables (if applicable), modify, and inject context when spawning child processes.
There was a problem hiding this comment.
yes! and i'm not sure how best to handle that implementation detail in practice at the carrier level, which is why this is the current set.
In my head, the only real path forward I see with that portion is to set a dictionary with that new context, and let the implementer use that returned dictionary when they do a popen run like the below psuedo code:
# somehow the user sets traceparent, tracestate, and baggage and setter returns an env dictionary instead of what it does now
# the user then takes that and sets it in popen
popen.run(["something"], env=env_dict_from_setter)
Description
Adds a prototype for the environment variable carrier, specifically decorating Getter and Setter instead of creating a dedicated propagator. The comes from the conversations within the environment variable context propagation specification pull request.
There's a corresponding thread in Slack around this topic.
The example use of the Getter portion of this prototype can be found in this gist.
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
The Getter was tested by using the prototype in a real environment. Unit tests need to be added after conversation around this prototype.
Does This PR Require a Contrib Repo Change?
Checklist: