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
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 29 additions & 3 deletions propagation/baggage.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Member

@pellared pellared Nov 18, 2024

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 for TraceContext so that we can also use the existing BenchmarkExtract to validate the performance overhead for TraceContext (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.

Copy link
Member

@pellared pellared Nov 26, 2024

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 with gc_details enabled has not shown that there is any additional heap allocation.

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 👍

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
Expand All @@ -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)
Copy link
Member

Choose a reason for hiding this comment

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

We could return early here if bVals is empty.
That's a nit, but maybe that shows we would need more benchmarks, especially with @pellared's comment above.

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)
}
49 changes: 49 additions & 0 deletions propagation/baggage_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,55 @@ func TestExtractValidBaggageFromHTTPReq(t *testing.T) {
}
}

func TestExtractValidMultipleBaggageHeaders(t *testing.T) {
prop := propagation.TextMapPropagator(propagation.Baggage{})
tests := []struct {
name string
headers []string
want members
}{
{
name: "non conflicting headers",
headers: []string{"key1=val1", "key2=val2"},
want: members{
{Key: "key1", Value: "val1"},
{Key: "key2", Value: "val2"},
},
},
{
name: "conflicting keys, uses last val",
headers: []string{"key1=val1", "key1=val2"},
want: members{
{Key: "key1", Value: "val2"},
},
},
{
name: "single empty",
headers: []string{"", "key1=val1"},
want: members{
{Key: "key1", Value: "val1"},
},
},
{
name: "all empty",
headers: []string{"", ""},
want: members{},
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
req, _ := http.NewRequest("GET", "http://example.com", nil)
req.Header["Baggage"] = tt.headers

ctx := context.Background()
ctx = prop.Extract(ctx, propagation.HeaderCarrier(req.Header))
expected := tt.want.Baggage(t)
assert.Equal(t, expected, baggage.FromContext(ctx))
})
}
}

func TestExtractInvalidDistributedContextFromHTTPReq(t *testing.T) {
prop := propagation.TextMapPropagator(propagation.Baggage{})
tests := []struct {
Expand Down
16 changes: 15 additions & 1 deletion propagation/propagation.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this need to be embedded. It limits the composability.

Copy link
Member

Choose a reason for hiding this comment

The 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 io package there is e.g. https://pkg.go.dev/io#ReadCloser.

Copy link
Author

@jamesmoessis jamesmoessis Nov 27, 2024

Choose a reason for hiding this comment

The 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
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we name this Values to match the http.Header?

https://pkg.go.dev/net/http#Header.Values

Copy link
Author

@jamesmoessis jamesmoessis Nov 26, 2024

Choose a reason for hiding this comment

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

Spec is defining GetAll not Values. Is there any benefit to matching http.header? As mentioned elsewhere in this PR, http.header does not implement TextMapCarrier in the first place.

Copy link
Contributor

Choose a reason for hiding this comment

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

It matches expected naming by Go users

Copy link
Contributor

@MrAlias MrAlias Nov 27, 2024

Choose a reason for hiding this comment

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

Copy link
Author

Choose a reason for hiding this comment

The 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 http.Header API. I don't really mind - what matters most to me is having correctness of propagation, which we currently don't have. So, if it's going to move the PR along then I'm happy to change the name to deviate from the name proposed in the spec.

// 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
Expand Down Expand Up @@ -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)
Expand Down
Loading