-
Notifications
You must be signed in to change notification settings - Fork 926
[spec] add EnvVarPropagator
decorator for env variable context
#4484
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
Conversation
Again, I tried to keep it very simple. I would love some feedback around the description of the decorator, and feedback from experts in the Python, Go, and Swift SDK's for what the simple examples provide. |
Co-authored-by: Reiley Yang <[email protected]>
... | ||
} | ||
|
||
type EnvVarPropagator func(TextMapPropagator) TextMapPropagator |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a little confused by the type selection in this example. What do you think about using embedding here like so?
type EnvVarPropagator struct {
TextMapPropagator
}
func (envp EnvVarPropagator) Inject(ctx context.Context, carrier TextMapCarrier) { | ||
env := os.Environ() | ||
// Inject context into environment variable copy | ||
... | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not too familiar with the Propagators API, but I noticed the Inject
method in the TextMapPropagator
interface doesn't return anything. So I'm curious what future implementors should do with the scoped copy of environment variables? Set them to the mutable carrier?
I will do my best to review next week. |
```go | ||
type TextMapPropagator interface { | ||
// Includes Inject, Extract, and Fields | ||
... | ||
} | ||
|
||
type EnvVarPropagator func(TextMapPropagator) TextMapPropagator | ||
|
||
func (envp EnvVarPropagator) Inject(ctx context.Context, carrier TextMapCarrier) { | ||
env := os.Environ() | ||
// Inject context into environment variable copy | ||
... | ||
} | ||
|
||
func (envp EnvVarPropagator) Extract(ctx context.Context, carrier TextMapCarrier) context.Context { | ||
// Extract context from environment variables | ||
... | ||
} | ||
... | ||
``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't currently demonstrate the intended concept. Could you create a prototype (e.g., a draft PR) in https://github.com/open-telemetry/opentelemetry-go instead?
Also, the current design isn't quite convincing. Shouldn't we be implementing a TextMapCarrier
that works with environment variables? After all, the document is titled "Environment Variables as Context Propagation Carriers", which strongly suggests that direction.
### Environment Variable Propagator Decorator | ||
|
||
The `EnvVarPropagator` is a [decorator][dec] that wraps a `TextMapPropagator`, | ||
handling the injection and extraction of context and baggage into and from | ||
environment variables. | ||
|
||
The `EnvVarPropagator` SHOULD be configurable to match platform-specific | ||
restrictions and handle environment variable naming conventions as described in | ||
the [Environment Variable Names](#environment-variable-names) and [Format | ||
Restrictions](#format-restrictions) sections. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not understand why we need a propagator decorator. Isn't this something that is needed for Python propagator design?
Don't we simply need a new carrier implementation where environmental variables are the medium?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the [Environment Variable Names](#environment-variable-names) and [Format | ||
Restrictions](#format-restrictions) sections. | ||
|
||
The `EnvVarPropagator` MAY define an `EnvVarCarrier` type that implements the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to see some implementations of this across languages.
My suspicion is that there is not a great abstraction around ENV
+ fork()
you can use here, so instead I think non-normative recommendations for how to use TextMapPropagator
and then language-specific implementations may be better.
Ping @adrielp |
I haven't forgotten about this, just been thinking about the comments and been out-of-band. I recently wrote a working version in Python that I'd like to discuss (and can open a draft PR). I've also looked through the Go source code for how context propagation works. I'd like to discuss this next week (May 20, 2025) during the SIG call in depth. Due to language implementation details, I think I'm concerned about saying the environment is the carrier, without a dedicated wrapper. Based on the spec, there are things to be aware of that just does not seem to be best implemented at the carrier layer, but instead within a propagators natural inject/extra methods. One example was in the original Python prototype. The I'll try to get a simple prototype if I can find some time in Go before the spec call. Will respond to the other comments individually, but most is encompassed here. I will add the talking item to the next calls. Thanks for bearing with my low bandwidth here! |
I think it would be good to open it to get feedback from Python maintainers before the SIG meeting.
I created a prototype in Go which implements the feature by creating a new carrier: open-telemetry/opentelemetry-go#6778.
As you said, this is an implementation detail. From the specification perspective, we just need an carrier. If some languages need a new propagator for each carrier then it was SIG's decision to model it that way. For instance, I have totally no idea why the carrier in Go has a |
@pellared - thanks for the context on prior art within the Go SIG. Super looking forward to reading your prototype, appreciate you taking the initiative to put it together!! |
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
The `EnvVarPropagator` is a [decorator][dec] that wraps a `TextMapPropagator`, | ||
handling the injection and extraction of context and baggage into and from | ||
environment variables. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This document doesn't really address things like interactions with span linking and configuration of the global propagators. There are some difficult details that we had to deal with for the X-Ray Lambda Propagator.
based on the above and the conversations in slack, I added a related PR in the python SDK for env carrier. open-telemetry/opentelemetry-python#4609 |
The scope of documented information and propagation has diverged from the original intent of this pull request. I will be closing this pull request so that it's original intent is still available for historical context, then I will open a new pull request that provides supplementary guidelines for language implementations. There are now two prototype draft pull requests in Python & Go that will be updated to reflect the new guidelines. |
I still think having an env var propagation spec is useful, but there are some weird API edge cases, specifically with how it may interact with span linking and the global propagator. One way to mitigate those problems is by creating span links from all propagators in the chain that differ from the last one, but that also requires a spec change. |
## Changes Adds supplementary guidance around the behaviors the SDKs should look to implement. * [x] Related issues [#4470](#4470) & #4484 * [x] Related [OTEP 0258](https://github.com/open-telemetry/opentelemetry-specification/blob/main/oteps/0258-env-context-baggage-carriers.md) * [x] Links to the prototypes (when adding or changing features) - [Go](open-telemetry/opentelemetry-go#6778) - [Python](open-telemetry/opentelemetry-python#4609) * [x] [`CHANGELOG.md`](https://github.com/open-telemetry/opentelemetry-specification/blob/main/CHANGELOG.md) file updated for non-trivial changes --------- Co-authored-by: Robert Pająk <[email protected]> Co-authored-by: Liudmila Molkova <[email protected]> Co-authored-by: Reiley Yang <[email protected]>
propagation
Fixes #4470
Changes
Changelog has been updated to reflect that addition of the
EnvVarPropagator
decorator for theTextMapPropagator
CHANGELOG.md
file updated for non-trivial changes