Skip to content
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

[baggage] baggage item value encoding fix - custom encoder #5292

Closed
wants to merge 1 commit into from

Conversation

lachmatt
Copy link
Contributor

@lachmatt lachmatt commented Jan 31, 2024

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 #2012 (comment)

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

  • 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)

@@ -94,7 +94,7 @@ public override void Inject<T>(PropagationContext context, T carrier, Action<T,
continue;
}

baggage.Append(WebUtility.UrlEncode(item.Key)).Append('=').Append(WebUtility.UrlEncode(item.Value)).Append(',');
baggage.Append(W3CBaggageValueEncoder.Encode(item.Key)).Append('=').Append(W3CBaggageValueEncoder.Encode(item.Value)).Append(',');
Copy link
Contributor Author

@lachmatt lachmatt Jan 31, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Baggage item keys should meet token requirement from the specification, and not be encoded.
I can address that in a separate PR.

Copy link

codecov bot commented Jan 31, 2024

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

Impacted file tree graph

@@            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%) ⬆️
...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%) ⬆️
...AspNetCore/Implementation/HttpInMetricsListener.cs 89.74% <100.00%> (+0.26%) ⬆️
...ntation.GrpcNetClient/GrpcClientInstrumentation.cs 100.00% <100.00%> (ø)
...NetClient/GrpcClientTraceInstrumentationOptions.cs 100.00% <ø> (ø)
...ent/Implementation/GrpcClientDiagnosticListener.cs 75.80% <100.00%> (-2.77%) ⬇️
... and 12 more

... and 30 files with indirect coverage changes

@reyang
Copy link
Member

reyang commented Jan 31, 2024

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 Specify allowed characters for Baggage keys and values opentelemetry-specification#3801.

@lachmatt lachmatt changed the title [baggage] baggage item value encoding fix [baggage] baggage item value encoding fix - custom encoder Feb 1, 2024
@lachmatt
Copy link
Contributor Author

lachmatt commented Feb 1, 2024

  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.

  1. 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.

Copy link
Contributor

github-actions bot commented Feb 9, 2024

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 github-actions bot added the Stale Issues and pull requests which have been flagged for closing due to inactivity label Feb 9, 2024
Copy link
Contributor

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

@github-actions github-actions bot closed this Feb 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Stale Issues and pull requests which have been flagged for closing due to inactivity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[baggage] space encoding in baggage item value when injecting baggage
2 participants