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

Prototypes proposed spec changes to allow extraction of multiple header values #5973

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jamesmoessis
Copy link

  • Adds MultiTextMapCarrier, extending TextMapCarrier - adds GetAll(string) []string.
  • Implements MultiTextMapCarrier for HeaderCarrier
  • Change propagator.Baggage to use the GetAll() method if it's implemented.
  • Gives example tests extracting requests with multiple 'baggage' headers set.

Does not introduce any breaking changes or alter any existing tests.

Spec issue: open-telemetry/opentelemetry-specification#433
Corresponding Java prototype: open-telemetry/opentelemetry-java#6852

…, extending TextMapCarrier.

Gives example extracting requests with multiple 'baggage' headers set.
@jamesmoessis jamesmoessis changed the title Prototypes proposed spec changes to allow multiple header values Prototypes proposed spec changes to allow extraction of multiple header values Nov 14, 2024
Copy link

codecov bot commented Nov 14, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.6%. Comparing base (d428313) to head (530b431).

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##            main   #5973     +/-   ##
=======================================
- Coverage   84.6%   84.6%   -0.1%     
=======================================
  Files        272     272             
  Lines      22897   22918     +21     
=======================================
+ Hits       19391   19401     +10     
- Misses      3162    3173     +11     
  Partials     344     344             

see 2 files with indirect coverage changes

propagation/propagation.go Show resolved Hide resolved
propagation/propagation.go Show resolved Hide resolved
multiCarrier, isMultiCarrier := carrier.(MultiTextMapCarrier)
if isMultiCarrier {
return extractMultiBaggage(parent, multiCarrier)
}
Copy link
Member

Choose a reason for hiding this comment

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

Not as gross as I expected 👍

Copy link
Member

@pellared pellared left a comment

Choose a reason for hiding this comment

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

I think your prototype is good. For an actual PR we would need to improve the docs and call out the MultiTextMapCarrier in more places.

PS. Not feeling well today so treat my review with low confidence (like an pre-approval). I will do my best to look at it again once I recover and have a clearer mind.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants