-
Notifications
You must be signed in to change notification settings - Fork 893
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
Propagation - Extract handles multiple values on carrier using same key #4295
Propagation - Extract handles multiple values on carrier using same key #4295
Conversation
Co-authored-by: Trask Stalnaker <[email protected]>
Co-authored-by: Trask Stalnaker <[email protected]>
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.
Couple of minor comments
Co-authored-by: jack-berg <[email protected]>
Co-authored-by: Trask Stalnaker <[email protected]>
The Go prototype does not have benchmarks that would showcase that there is no considerable performance degradation in hot-path. Without the benchmarks the OTel Go maintainers are not able to guarantee that we OTel Go is going to support |
Only propagators that have the possibility of values being contained in multiple headers would need to call this new method, and be concerned about the perf overhead. The only propagator I know that fits this is the w3c baggage propagator. And the perf hit (if any) seems to be essential complexity required by the w3c specification. |
W3C TraceContext as well (sic!):
I mean a performance hit even in a scenario that everything is in one header. The performance hit we are worried about it related to Go how it handles type assertions. People may start to complain that in most common scenario (when 1 header) the performance is worse. See open-telemetry/opentelemetry-go#5973 (comment). |
Co-authored-by: Robert Pająk <[email protected]>
Co-authored-by: Robert Pająk <[email protected]>
Co-authored-by: Robert Pająk <[email protected]>
Co-authored-by: Robert Pająk <[email protected]>
Co-authored-by: Robert Pająk <[email protected]>
Co-authored-by: Robert Pająk <[email protected]>
Co-authored-by: Robert Pająk <[email protected]>
@pellared do you still have performance concern after you found no extra allocations? From your comment: open-telemetry/opentelemetry-go#5973 (comment) I also think there's something to be said about the value of correctness outweighing potential minor performance concerns in this case. |
No. Approving. Thanks for your contribution 🏅 |
Build is failing because of unrelated broken link, fixed here: #4314 |
Fixes #433
Discussed in 11/5/24 Spec SIG, and prototyped in Java and Go.
Changes
Adds
GetAll
method to Getter, allowing for the retrieval of multiple keys for the same value. For example, an HTTP request may have multiplebaggage
headers.As written in the changes, the implementation of
GetAll
is strictly optional. This is for two reasons:Links to Prototypes
This includes implementations of
GetAll()
in Java and Go, as well as using the method in the W3C Baggage Propagators (multiple baggage headers are allowed, spec reference).CHANGELOG.md
file updated for non-trivial changes