[API] Baggage - Trim white spaces and check for semi colon which isn't allowed octet#7009
[API] Baggage - Trim white spaces and check for semi colon which isn't allowed octet#7009nabutabu wants to merge 13 commits intoopen-telemetry:mainfrom
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7009 +/- ##
==========================================
+ Coverage 88.79% 88.83% +0.03%
==========================================
Files 271 271
Lines 12970 12976 +6
==========================================
+ Hits 11517 11527 +10
+ Misses 1453 1449 -4
Flags with carried forward coverage won't be shown. Click here to find out more.
|
|
Please add a CHANGELOG entry. |
|
This PR was marked stale due to lack of activity and will be closed in 7 days. Commenting or pushing will instruct the bot to automatically remove the label. This bot runs once per day. |
Benchmark Results SummaryCompleted benchmarks with changes
Raw DataBefore my changes: After my changes @martincostello One other thing, I was looking at how the benchmarking is working. Execution Summary
|
|
If the code path you're touching isn't benchmarked, then let's add another that deals with that path. |
Benchmark changesI changed how the baggageHeader is being built, keys and values need to be trimmed now and semicolon needs to be removed Raw DataAfter my changes Before my changes These two rows are standouts: Interestingly mean time after my changes has decreased, I thought this would be a slight deterioration. |
…s.cs Co-authored-by: Martin Costello <[email protected]>
There was a problem hiding this comment.
Pull request overview
This PR updates the W3C baggage header parsing in BaggagePropagator to be compatible with optional whitespace around = and to treat ;... as baggage properties rather than part of the value, aligning behavior with the W3C Baggage spec and addressing reported interoperability issues.
Changes:
- Trim optional whitespace (OWS) around
=when extracting baggage key/value pairs. - Strip
;propertiesfrom extracted baggage values. - Unskip/add unit tests covering OWS + semicolon-property extraction, plus extend benchmark scenarios.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| test/OpenTelemetry.Api.Tests/Context/Propagation/BaggagePropagatorTests.cs | Enables previously skipped spec-alignment tests and adds reinjection/regression coverage. |
| test/Benchmarks/Context/Propagation/BaggagePropagatorBenchmarks.cs | Adds a benchmark parameter to vary header formatting (OWS/properties) during extraction. |
| src/OpenTelemetry.Api/Context/Propagation/BaggagePropagator.cs | Implements OWS trimming and semicolon-property stripping during extraction. |
| src/OpenTelemetry.Api/CHANGELOG.md | Adds changelog entries describing the baggage parsing fixes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Kielek
left a comment
There was a problem hiding this comment.
Changed description to make Towards instead of Fixes, as there is still some test/requirements to be fixed.
CHANGELOG fixed.
In general changes LGTM. Could you please double check Copilot Comment related to Properites-benchamrk stuff?
Towards #6816, #5210
Changes
BaggagePropagatorto trim optional whitespace (OWS) around=separatorswhen parsing the
baggageheader, as required by theW3C Baggage specification. This caused
incompatibility with .NET 10's default W3C propagator, which emits headers with
OWS (e.g.
key = value), resulting in spaces being URL-encoded as+inoutbound headers received by downstream services.
BaggagePropagatorto strip baggage properties (;metadata) from valueswhen parsing the
baggageheader. Semicolon is not a validbaggage-octetperthe W3C Baggage specification and must be interpreted as the start of a property,
not part of the value.
(#5210)
Merge requirement checklist
CHANGELOG.mdfiles updated for non-trivial changesI don't think this is non-trivial but let me know if I need to do this