[api-baggage] fix encoding of space chars in baggage item value

[lachmatt]

Fixes #5260

Changes

Fixes encoding of ' ' char in baggage item values when injecting baggage.
Spaces should be percent-encoded, instead of being converted to '+' character.

Note

Some characters not required to be percent-encoded by the W3C baggage specification are percent-encoded, which results in longer representation.
This might be important considering baggage limits.

Alternative:

Custom encoder, which escapes only characters required by the W3C baggage specification.
Draft in #5292

Merge requirement checklist

• CONTRIBUTING guidelines followed (license requirements, nullable enabled, static analysis, etc.)
• Unit tests added/updated
• Appropriate CHANGELOG.md files updated for non-trivial changes
• Changes in public API reviewed (if applicable)

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 85.58%. Comparing base (6250307) to head (8046eaa).
Report is 167 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5303      +/-   ##
==========================================
+ Coverage   83.38%   85.58%   +2.19%     
==========================================
  Files         297      289       -8     
  Lines       12531    12588      +57     
==========================================
+ Hits        10449    10773     +324     
+ Misses       2082     1815     -267     

Flag	Coverage Δ
unittests	?
unittests-Solution-Experimental	85.56% <100.00%> (?)
unittests-Solution-Stable	85.49% <100.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
...metry.Api/Context/Propagation/BaggagePropagator.cs	85.48% <100.00%> (ø)

... and 63 files with indirect coverage changes

[vishweshbankwar]

LGTM - requires changelog. @alanwest Could you take a look as well?

[Kielek]

I would go with #5292, but this is also improvement.

[vishweshbankwar]

@lachmatt I missed something earlier - I think this would cause an issue due to difference in versions. https://learn.microsoft.com/en-us/dotnet/api/system.uri.unescapedatastring?view=net-8.0#remarks

Consider a service using old sdk injecting baggage and receiving side service(incoming request) updates to this change. it would cause issues with space decoding.

need more clarity #5303 (comment)

[github-actions]

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.

[lachmatt]

@lachmatt I missed something earlier - I think this would cause an issue due to difference in versions. https://learn.microsoft.com/en-us/dotnet/api/system.uri.unescapedatastring?view=net-8.0#remarks

Consider a service using old sdk injecting baggage and receiving side service(incoming request) updates to this change. it would cause issues with space decoding.

@vishweshbankwar That's a valid concern, but I don't see a good solution for it.
On the one hand, you might have service using old sdk injecting baggage, where + actually encodes space, on the other there are other, spec-compliant implementations (e.g from other languages), where + encodes +.

[pellared]

@lachmatt I missed something earlier - I think this would cause an issue due to difference in versions. https://learn.microsoft.com/en-us/dotnet/api/system.uri.unescapedatastring?view=net-8.0#remarks

Consider a service using old sdk injecting baggage and receiving side service(incoming request) updates to this change. it would cause issues with space decoding.

True. This is just another reason that this should be fixed. Other (compliant with the specification e.g. from other languages) implementations will encode + as +. It is not possible to have decoding that would support both the buggy encoding and spec-complaint encoding.

[github-actions]

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.

[pellared]

@open-telemetry/dotnet-maintainers This bug makes the baggage propagation not working properly with other SDKs.

[vishweshbankwar]

@open-telemetry/dotnet-maintainers This bug makes the baggage propagation not working properly with other SDKs.

@lachmatt \ @pellared - Sorry for the delay on this. I do have some questions on the spec. I will reach out to you offline to get the clarification. Thanks for your patience.

[github-actions]

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.

[pellared]

@open-telemetry/dotnet-approvers PTAL. We would like to have it fixed at least for 1.8.0.

[vishweshbankwar]

LGTM

@pellared - Thanks for raising this w3c/baggage#134.

I have opened #5500 on our side to further expand the tests.

[vishweshbankwar]

@lachmatt - please add the changelog as well.

[CodeBlanch]

@lachmatt - please add the changelog as well.

Yes I think we should mention this in CHANGELOG and note that is it potentially a breaking change.

[pellared]

I suggest writing explicitly that space was encoded to + and now it is encoded to %20.

[lachmatt]

changed in 50ed920

[CodeBlanch]

LGTM