[baggage] baggage item value encoding fix - custom encoder

[lachmatt]

Fixes #5260

Changes

Fixes encoding of ' ' char in baggage item values when injecting baggage. Adds custom encoder, based on WebUtility.Encode from runtime repository, with modifications related to space character encoding. Additionally, only characters required by the specification are percent-encoded, which results in minimal, compliant representation. This is important considering baggage limits. The need for the shortest, compliant representation expressed in https://github.com/open-telemetry/opentelemetry-dotnet/pull/2012#discussion_r629518477

Alternatives:

Use Uri.EscapeDataString which correctly encodes spaces. The downside of this approach is many characters not required to be percent-encoded by the specification are percent-encoded.

Merge requirement checklist

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

[codecov[bot]]

Codecov Report

Attention: 22 lines in your changes are missing coverage. Please review.

Comparison is base (6250307) 83.38% compared to head (6c646b4) 83.08%. Report is 71 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5292      +/-   ##
==========================================
- Coverage   83.38%   83.08%   -0.31%     
==========================================
  Files         297      273      -24     
  Lines       12531    12011     -520     
==========================================
- Hits        10449     9979     -470     
+ Misses       2082     2032      -50     

Flag	Coverage Δ
unittests	?
unittests-Solution-Experimental	83.06% <82.40%> (?)
unittests-Solution-Stable	82.98% <82.40%> (?)

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%> (ø)
....Api/Context/Propagation/TraceContextPropagator.cs	90.00% <100.00%> (+0.52%)	:arrow_up:
...etryProtocol/Implementation/ExperimentalOptions.cs	100.00% <ø> (ø)
...tation/OpenTelemetryProtocolExporterEventSource.cs	100.00% <100.00%> (ø)
...rotocol/Implementation/OtlpLogRecordTransformer.cs	93.45% <100.00%> (ø)
...tation.AspNetCore/Implementation/HttpInListener.cs	89.79% <100.00%> (+0.21%)	:arrow_up:
...AspNetCore/Implementation/HttpInMetricsListener.cs	89.74% <100.00%> (+0.26%)	:arrow_up:
...ntation.GrpcNetClient/GrpcClientInstrumentation.cs	100.00% <100.00%> (ø)
...NetClient/GrpcClientTraceInstrumentationOptions.cs	100.00% <ø> (ø)
...ent/Implementation/GrpcClientDiagnosticListener.cs	75.80% <100.00%> (-2.77%)	:arrow_down:
... and 12 more

... and 30 files with indirect coverage changes

[reyang]

I haven't got time to look into this PR. Here is my general feedback:

1. There are two specs - 1. the W3C Baggage Spec, which is "W3C Working Draft" rather than "W3C Recommendation" 2. the OpenTelemetry Baggage Spec, which is "Stable/Feature-freeze".
2. When we say "follow the spec", I think we should be very clear about which specification to follow. IMHO we are not following the "W3C Working Draft".
3. There are PRs clarifying Baggage in the OpenTelemetry Specification repository, considering OpenTelemetry .NET Baggage is stable, I guess we don't want to rush https://github.com/open-telemetry/opentelemetry-specification/pull/3801.

[lachmatt]

1. There are two specs - 1. the W3C Baggage Spec, which is "W3C Working Draft" rather than "W3C Recommendation" 2. the OpenTelemetry Baggage Spec, which is "Stable/Feature-freeze".

There is a section in OpenTelemetry Baggage Spec related to propagation that references W3C Baggage Specification. I assumed requirements from there apply to BaggagePropagator modified in this PR.

2. When we say "follow the spec", I think we should be very clear about which specification to follow. IMHO we are not following the "W3C Working Draft".

Code comment mentions editor's draft.

[github-actions[bot]]

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.

[github-actions[bot]]

Closed as inactive. Feel free to reopen if this PR is still being worked on.