-
Notifications
You must be signed in to change notification settings - Fork 933
[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
Changes from 6 commits
c62c0f6
7e7ab7a
b656358
7d30864
05d0ac0
eff137d
4ba04d9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -19,6 +19,12 @@ | |
+ [Process Spawning](#process-spawning) | ||
+ [Security](#security) | ||
+ [Case Sensitivity](#case-sensitivity) | ||
- [Propagator API](#propagator-api) | ||
* [Environment Variable Propagator Decorator](#environment-variable-propagator-decorator) | ||
+ [Examples](#examples) | ||
- [Go](#go) | ||
- [Python](#python) | ||
- [Swift](#swift) | ||
|
||
<!-- tocstop --> | ||
|
||
|
@@ -149,3 +155,76 @@ Windows. | |
- For maximum compatibility, implementations MUST: | ||
- Use uppercase names consistently (`TRACEPARENT` not `TraceParent`). | ||
- Use the canonical case when setting environment variables. | ||
|
||
## Propagator API | ||
|
||
### 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 | ||
carlosalberto marked this conversation as resolved.
Show resolved
Hide resolved
|
||
restrictions and handle environment variable naming conventions as described in | ||
the [Environment Variable Names](#environment-variable-names) and [Format | ||
Restrictions](#format-restrictions) sections. | ||
Comment on lines
+161
to
+170
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. |
||
|
||
The `EnvVarPropagator` MAY define an `EnvVarCarrier` type that implements the | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
`TextMap` carrier interface when calling `Inject` and `Extract` operations. | ||
|
||
#### Examples | ||
|
||
##### Go | ||
|
||
```go | ||
type TextMapPropagator interface { | ||
// Includes Inject, Extract, and Fields | ||
... | ||
} | ||
|
||
type EnvVarPropagator func(TextMapPropagator) TextMapPropagator | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
... | ||
} | ||
Comment on lines
+187
to
+191
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
|
||
func (envp EnvVarPropagator) Extract(ctx context.Context, carrier TextMapCarrier) context.Context { | ||
// Extract context from environment variables | ||
... | ||
} | ||
... | ||
``` | ||
Comment on lines
+179
to
+198
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
|
||
##### Python | ||
|
||
```python | ||
class EnvVarPropagator(TextMapPropagator): | ||
def inject(self, context, carrier): | ||
env_dict = os.environ.copy() | ||
# Inject context into environment variables | ||
... | ||
|
||
def extract(self, carrier): | ||
# Extract context from environment variables | ||
... | ||
``` | ||
|
||
##### Swift | ||
|
||
```swift | ||
public struct EnvVarPropagator: TextMapPropagator { | ||
public func inject(...) { | ||
// Inject context into environment variables | ||
... | ||
} | ||
|
||
public func extract(...) -> SpanContext? { | ||
// Extract context from environment variables | ||
... | ||
} | ||
} | ||
``` | ||
|
||
[dec]: https://wikipedia.org/wiki/Decorator_pattern |
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.