-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -29,6 +29,19 @@ func (b Baggage) Inject(ctx context.Context, carrier TextMapCarrier) { | |
|
||
// Extract returns a copy of parent with the baggage from the carrier added. | ||
func (b Baggage) Extract(parent context.Context, carrier TextMapCarrier) context.Context { | ||
multiCarrier, isMultiCarrier := carrier.(MultiTextMapCarrier) | ||
if isMultiCarrier { | ||
return extractMultiBaggage(parent, multiCarrier) | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not as gross as I expected 👍 |
||
return extractSingleBaggage(parent, carrier) | ||
} | ||
|
||
// Fields returns the keys who's values are set with Inject. | ||
func (b Baggage) Fields() []string { | ||
return []string{baggageHeader} | ||
} | ||
|
||
func extractSingleBaggage(parent context.Context, carrier TextMapCarrier) context.Context { | ||
bStr := carrier.Get(baggageHeader) | ||
if bStr == "" { | ||
return parent | ||
|
@@ -41,7 +54,20 @@ func (b Baggage) Extract(parent context.Context, carrier TextMapCarrier) context | |
return baggage.ContextWithBaggage(parent, bag) | ||
} | ||
|
||
// Fields returns the keys who's values are set with Inject. | ||
func (b Baggage) Fields() []string { | ||
return []string{baggageHeader} | ||
func extractMultiBaggage(parent context.Context, carrier MultiTextMapCarrier) context.Context { | ||
bVals := carrier.GetAll(baggageHeader) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We could return early here if |
||
members := make([]baggage.Member, 0) | ||
for _, bStr := range bVals { | ||
currBag, err := baggage.Parse(bStr) | ||
if err != nil { | ||
continue | ||
} | ||
members = append(members, currBag.Members()...) | ||
} | ||
|
||
b, err := baggage.New(members...) | ||
if err != nil || b.Len() == 0 { | ||
return parent | ||
} | ||
return baggage.ContextWithBaggage(parent, b) | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -29,6 +29,15 @@ type TextMapCarrier interface { | |
// must never be done outside of a new major release. | ||
} | ||
|
||
// MultiTextMapCarrier is a TextMapCarrier that can return multiple values for a single key. | ||
pellared marked this conversation as resolved.
Show resolved
Hide resolved
|
||
type MultiTextMapCarrier interface { | ||
TextMapCarrier | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does this need to be embedded. It limits the composability. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was also thinking about two interfaces; one defining the method, second embedding two interfaces. Inspired that in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, I definitely like this suggestion of improving the composability. Would something along these lines be what you are thinking? type MultiGetter interface {
GetAll(key string) []string
}
type MultiTextMapCarrier interface {
TextMapCarrier
MultiGetter
} |
||
// GetAll returns all values associated with the passed key. | ||
GetAll(key string) []string | ||
pellared marked this conversation as resolved.
Show resolved
Hide resolved
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we name this There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Spec is defining There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It matches expected naming by Go users There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also, please see the other comment about interface design. Not embedding will allow greater usability, especially with the stdlib Header. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I suppose we are talking about a tradeoff between matching the otel spec (other language implementations of same spec), and matching the |
||
// DO NOT CHANGE: any modification will not be backwards compatible and | ||
// must never be done outside of a new major release. | ||
} | ||
|
||
// MapCarrier is a TextMapCarrier that uses a map held in memory as a storage | ||
// medium for propagated key-value pairs. | ||
type MapCarrier map[string]string | ||
|
@@ -58,11 +67,16 @@ func (c MapCarrier) Keys() []string { | |
// HeaderCarrier adapts http.Header to satisfy the TextMapCarrier interface. | ||
type HeaderCarrier http.Header | ||
|
||
// Get returns the value associated with the passed key. | ||
// Get returns the first value associated with the passed key. | ||
func (hc HeaderCarrier) Get(key string) string { | ||
return http.Header(hc).Get(key) | ||
} | ||
|
||
// GetAll returns all values associated with the passed key. | ||
func (hc HeaderCarrier) GetAll(key string) []string { | ||
return http.Header(hc).Values(key) | ||
} | ||
|
||
// Set stores the key-value pair. | ||
func (hc HeaderCarrier) Set(key string, value string) { | ||
http.Header(hc).Set(key, value) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AFAIK type assertions can introduce an additional heap allocation. The extraction is on the hot path.
Can you add some benchmark for this method and add
benchstat
results to the PR description so that we can see what is the performance overhead?Can you also implement the
MultiTextMapCarrier
forTraceContext
so that we can also use the existingBenchmarkExtract
to validate the performance overhead forTraceContext
(which is probably more popular than Baggage)?PS. I do not expect anyone would say that an additional heap allocation would be a blocker. Maybe at some point Go runtime would not make a heap allocation when doing a type assertion. Still, we should be aware of the performance consequences of a new functionality.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made a quick checkout of the PR and the
gopls
withgc_details
enabled has not shown that there is any additional heap allocation.