From 4b08c015f500fd89774ff6e7e1fdf3231a8724ed Mon Sep 17 00:00:00 2001 From: James Moessis Date: Thu, 14 Nov 2024 13:36:11 +1100 Subject: [PATCH 01/20] Prototypes propagation with multiple values. Adds MultiTextMapCarrier, extending TextMapCarrier. Gives example extracting requests with multiple 'baggage' headers set. --- propagation/baggage.go | 32 +++++++++++++++++++++--- propagation/baggage_test.go | 49 +++++++++++++++++++++++++++++++++++++ propagation/propagation.go | 16 +++++++++++- 3 files changed, 93 insertions(+), 4 deletions(-) diff --git a/propagation/baggage.go b/propagation/baggage.go index 552263ba734..5b341071229 100644 --- a/propagation/baggage.go +++ b/propagation/baggage.go @@ -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) + } + 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) + 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) } diff --git a/propagation/baggage_test.go b/propagation/baggage_test.go index 4559a808fe2..a8a894af1b3 100644 --- a/propagation/baggage_test.go +++ b/propagation/baggage_test.go @@ -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 { diff --git a/propagation/propagation.go b/propagation/propagation.go index 8c8286aab4d..9d4a729533c 100644 --- a/propagation/propagation.go +++ b/propagation/propagation.go @@ -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. +type MultiTextMapCarrier interface { + TextMapCarrier + // GetAll returns all values associated with the passed key. + GetAll(key string) []string + // 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) From 4cffc3cdf58d35172db9aa64387d42092958600e Mon Sep 17 00:00:00 2001 From: James Moessis Date: Mon, 28 Apr 2025 14:59:49 +1000 Subject: [PATCH 02/20] implement composability suggestion with regards to interface design --- propagation/propagation.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/propagation/propagation.go b/propagation/propagation.go index 9d4a729533c..e2fd508c640 100644 --- a/propagation/propagation.go +++ b/propagation/propagation.go @@ -32,6 +32,12 @@ type TextMapCarrier interface { // MultiTextMapCarrier is a TextMapCarrier that can return multiple values for a single key. type MultiTextMapCarrier interface { TextMapCarrier + MultiGetter +} + +// MultiGetter can return multiple values for a single key, +// with contrast to TextMapCarrier.Get which returns a single value. +type MultiGetter interface { // GetAll returns all values associated with the passed key. GetAll(key string) []string // DO NOT CHANGE: any modification will not be backwards compatible and From 5970fd4ce93fd35568243f908ee18dc628d1f766 Mon Sep 17 00:00:00 2001 From: James Moessis Date: Mon, 28 Apr 2025 15:04:08 +1000 Subject: [PATCH 03/20] rename GetAll to Values as suggested --- propagation/baggage.go | 2 +- propagation/propagation.go | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/propagation/baggage.go b/propagation/baggage.go index 5b341071229..e10f90b5ac8 100644 --- a/propagation/baggage.go +++ b/propagation/baggage.go @@ -55,7 +55,7 @@ func extractSingleBaggage(parent context.Context, carrier TextMapCarrier) contex } func extractMultiBaggage(parent context.Context, carrier MultiTextMapCarrier) context.Context { - bVals := carrier.GetAll(baggageHeader) + bVals := carrier.Values(baggageHeader) members := make([]baggage.Member, 0) for _, bStr := range bVals { currBag, err := baggage.Parse(bStr) diff --git a/propagation/propagation.go b/propagation/propagation.go index e2fd508c640..11e0034f26a 100644 --- a/propagation/propagation.go +++ b/propagation/propagation.go @@ -38,8 +38,8 @@ type MultiTextMapCarrier interface { // MultiGetter can return multiple values for a single key, // with contrast to TextMapCarrier.Get which returns a single value. type MultiGetter interface { - // GetAll returns all values associated with the passed key. - GetAll(key string) []string + // Values returns all values associated with the passed key. + Values(key string) []string // DO NOT CHANGE: any modification will not be backwards compatible and // must never be done outside of a new major release. } @@ -78,8 +78,8 @@ 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 { +// Values returns all values associated with the passed key. +func (hc HeaderCarrier) Values(key string) []string { return http.Header(hc).Values(key) } From a63c63ab99ec15d32b3331db406f57286f021904 Mon Sep 17 00:00:00 2001 From: James Moessis Date: Mon, 28 Apr 2025 15:04:50 +1000 Subject: [PATCH 04/20] ensure backwards compatibility message is in all places it should be --- propagation/propagation.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/propagation/propagation.go b/propagation/propagation.go index 11e0034f26a..f958f134b5b 100644 --- a/propagation/propagation.go +++ b/propagation/propagation.go @@ -31,6 +31,9 @@ type TextMapCarrier interface { // MultiTextMapCarrier is a TextMapCarrier that can return multiple values for a single key. type MultiTextMapCarrier interface { + // DO NOT CHANGE: any modification will not be backwards compatible and + // must never be done outside of a new major release. + TextMapCarrier MultiGetter } @@ -38,6 +41,9 @@ type MultiTextMapCarrier interface { // MultiGetter can return multiple values for a single key, // with contrast to TextMapCarrier.Get which returns a single value. type MultiGetter interface { + // DO NOT CHANGE: any modification will not be backwards compatible and + // must never be done outside of a new major release. + // Values returns all values associated with the passed key. Values(key string) []string // DO NOT CHANGE: any modification will not be backwards compatible and From 9e4e3485923c7e8d609761c4c57625bdd6e0d3db Mon Sep 17 00:00:00 2001 From: James Moessis Date: Mon, 28 Apr 2025 15:15:06 +1000 Subject: [PATCH 05/20] optimise when bVals is empty as suggested -- also add test case for empty bVals --- propagation/baggage.go | 3 +++ propagation/baggage_test.go | 5 +++++ 2 files changed, 8 insertions(+) diff --git a/propagation/baggage.go b/propagation/baggage.go index e10f90b5ac8..84b187619ea 100644 --- a/propagation/baggage.go +++ b/propagation/baggage.go @@ -56,6 +56,9 @@ func extractSingleBaggage(parent context.Context, carrier TextMapCarrier) contex func extractMultiBaggage(parent context.Context, carrier MultiTextMapCarrier) context.Context { bVals := carrier.Values(baggageHeader) + if len(bVals) == 0 { + return parent + } members := make([]baggage.Member, 0) for _, bStr := range bVals { currBag, err := baggage.Parse(bStr) diff --git a/propagation/baggage_test.go b/propagation/baggage_test.go index a8a894af1b3..567798610a7 100644 --- a/propagation/baggage_test.go +++ b/propagation/baggage_test.go @@ -162,6 +162,11 @@ func TestExtractValidMultipleBaggageHeaders(t *testing.T) { headers: []string{"", ""}, want: members{}, }, + { + name: "none", + headers: []string{}, + want: members{}, + }, } for _, tt := range tests { From c72ade67fe920644b7e260947234d07f23239b61 Mon Sep 17 00:00:00 2001 From: James Moessis Date: Mon, 28 Apr 2025 15:22:19 +1000 Subject: [PATCH 06/20] add changelog entry --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 687e493e21c..74ac0ecc50a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm The package contains semantic conventions from the `v1.31.0` version of the OpenTelemetry Semantic Conventions. See the [migration documentation](./semconv/v1.31.0/MIGRATION.md) for information on how to upgrade from `go.opentelemetry.io/otel/semconv/v1.30.0`(#6479) - Add `Recording`, `Scope`, and `Record` types in `go.opentelemetry.io/otel/log/logtest`. (#6507) +- Allow extraction of multiple values in propagation. Implements for baggage propagator. (#5973) ### Removed From 9de8b16bda32f0bc8646e3e5312f4471c68e134b Mon Sep 17 00:00:00 2001 From: James Moessis Date: Mon, 28 Apr 2025 15:24:19 +1000 Subject: [PATCH 07/20] appease linter --- propagation/baggage_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/propagation/baggage_test.go b/propagation/baggage_test.go index 567798610a7..76ae71d8a4b 100644 --- a/propagation/baggage_test.go +++ b/propagation/baggage_test.go @@ -171,7 +171,7 @@ func TestExtractValidMultipleBaggageHeaders(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - req, _ := http.NewRequest("GET", "http://example.com", nil) + req, _ := http.NewRequest(http.MethodGet, "http://example.com", nil) req.Header["Baggage"] = tt.headers ctx := context.Background() From cf66b6820a9587ff7ec4b0895b8268aaecc10b82 Mon Sep 17 00:00:00 2001 From: James Moessis Date: Tue, 29 Apr 2025 15:51:32 +1000 Subject: [PATCH 08/20] Update propagation/baggage.go MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Robert Pająk --- propagation/baggage.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/propagation/baggage.go b/propagation/baggage.go index 84b187619ea..b5c07ef0fad 100644 --- a/propagation/baggage.go +++ b/propagation/baggage.go @@ -29,8 +29,7 @@ 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 { + if multiCarrier, ok := carrier.(MultiTextMapCarrier); ok { return extractMultiBaggage(parent, multiCarrier) } return extractSingleBaggage(parent, carrier) From 52f3e063e5b0fd4f062f9d3a939d15b7f4e65162 Mon Sep 17 00:00:00 2001 From: James Moessis Date: Tue, 29 Apr 2025 16:06:09 +1000 Subject: [PATCH 09/20] rename MultiGetter to ValuesGetter, implementing review suggestion --- propagation/propagation.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/propagation/propagation.go b/propagation/propagation.go index f958f134b5b..39d907cf5de 100644 --- a/propagation/propagation.go +++ b/propagation/propagation.go @@ -35,12 +35,12 @@ type MultiTextMapCarrier interface { // must never be done outside of a new major release. TextMapCarrier - MultiGetter + ValuesGetter } -// MultiGetter can return multiple values for a single key, +// ValuesGetter can return multiple values for a single key, // with contrast to TextMapCarrier.Get which returns a single value. -type MultiGetter interface { +type ValuesGetter interface { // DO NOT CHANGE: any modification will not be backwards compatible and // must never be done outside of a new major release. From 8bbb867a5e667e20a1710f4ff80dcfed8a72e00f Mon Sep 17 00:00:00 2001 From: James Moessis Date: Tue, 29 Apr 2025 16:06:42 +1000 Subject: [PATCH 10/20] add more information into the go doc of Extract, as per review --- propagation/baggage.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/propagation/baggage.go b/propagation/baggage.go index b5c07ef0fad..486b0681333 100644 --- a/propagation/baggage.go +++ b/propagation/baggage.go @@ -28,6 +28,8 @@ func (b Baggage) Inject(ctx context.Context, carrier TextMapCarrier) { } // Extract returns a copy of parent with the baggage from the carrier added. +// If carrier implements [MultiTextMapCarrier] (e.g. [HeaderCarrier]), Values is invoked +// for multiple values extraction. Otherwise, Get is called. func (b Baggage) Extract(parent context.Context, carrier TextMapCarrier) context.Context { if multiCarrier, ok := carrier.(MultiTextMapCarrier); ok { return extractMultiBaggage(parent, multiCarrier) From 1bceb793d63c918f90778b9c47500844cb70040b Mon Sep 17 00:00:00 2001 From: James Moessis Date: Tue, 29 Apr 2025 16:07:41 +1000 Subject: [PATCH 11/20] update HeaderCarrier go doc to explicitly state that it satisfies MultiTextMapCarrier, as per review --- propagation/propagation.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/propagation/propagation.go b/propagation/propagation.go index 39d907cf5de..8b57a8109ab 100644 --- a/propagation/propagation.go +++ b/propagation/propagation.go @@ -76,7 +76,7 @@ func (c MapCarrier) Keys() []string { return keys } -// HeaderCarrier adapts http.Header to satisfy the TextMapCarrier interface. +// HeaderCarrier adapts http.Header to satisfy the MultiTextMapCarrier interface. type HeaderCarrier http.Header // Get returns the first value associated with the passed key. From c7bf43253d95ee57daf8a8ff1c6a4c461e74410f Mon Sep 17 00:00:00 2001 From: James Moessis Date: Tue, 29 Apr 2025 16:11:37 +1000 Subject: [PATCH 12/20] update TextMapPropagator.Extract go doc to mention checking if carrier implements MultiTextMapCarrier --- propagation/propagation.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/propagation/propagation.go b/propagation/propagation.go index 8b57a8109ab..4bece22fac2 100644 --- a/propagation/propagation.go +++ b/propagation/propagation.go @@ -115,6 +115,8 @@ type TextMapPropagator interface { // must never be done outside of a new major release. // Extract reads cross-cutting concerns from the carrier into a Context. + // Implementations may check if the carrier implements MultiTextMapCarrier, + // to support usage of multiple values per key. Extract(ctx context.Context, carrier TextMapCarrier) context.Context // DO NOT CHANGE: any modification will not be backwards compatible and // must never be done outside of a new major release. From 5c239a5b557724726323f6132a42fa7f7ec248ba Mon Sep 17 00:00:00 2001 From: James Moessis Date: Thu, 8 May 2025 15:13:18 +1000 Subject: [PATCH 13/20] remove MultiTextMapCarrier and just reference ValuesGetter directly, as per robert's review --- propagation/baggage.go | 6 +++--- propagation/propagation.go | 16 ++++------------ 2 files changed, 7 insertions(+), 15 deletions(-) diff --git a/propagation/baggage.go b/propagation/baggage.go index 486b0681333..1ed0f12f479 100644 --- a/propagation/baggage.go +++ b/propagation/baggage.go @@ -28,10 +28,10 @@ func (b Baggage) Inject(ctx context.Context, carrier TextMapCarrier) { } // Extract returns a copy of parent with the baggage from the carrier added. -// If carrier implements [MultiTextMapCarrier] (e.g. [HeaderCarrier]), Values is invoked +// If carrier implements [ValuesGetter] (e.g. [HeaderCarrier]), Values is invoked // for multiple values extraction. Otherwise, Get is called. func (b Baggage) Extract(parent context.Context, carrier TextMapCarrier) context.Context { - if multiCarrier, ok := carrier.(MultiTextMapCarrier); ok { + if multiCarrier, ok := carrier.(ValuesGetter); ok { return extractMultiBaggage(parent, multiCarrier) } return extractSingleBaggage(parent, carrier) @@ -55,7 +55,7 @@ func extractSingleBaggage(parent context.Context, carrier TextMapCarrier) contex return baggage.ContextWithBaggage(parent, bag) } -func extractMultiBaggage(parent context.Context, carrier MultiTextMapCarrier) context.Context { +func extractMultiBaggage(parent context.Context, carrier ValuesGetter) context.Context { bVals := carrier.Values(baggageHeader) if len(bVals) == 0 { return parent diff --git a/propagation/propagation.go b/propagation/propagation.go index 4bece22fac2..23d61474357 100644 --- a/propagation/propagation.go +++ b/propagation/propagation.go @@ -9,6 +9,7 @@ import ( ) // TextMapCarrier is the storage medium used by a TextMapPropagator. +// See ValuesGetter for how a TextMapCarrier can get multiple values for a key. type TextMapCarrier interface { // DO NOT CHANGE: any modification will not be backwards compatible and // must never be done outside of a new major release. @@ -29,15 +30,6 @@ 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. -type MultiTextMapCarrier interface { - // DO NOT CHANGE: any modification will not be backwards compatible and - // must never be done outside of a new major release. - - TextMapCarrier - ValuesGetter -} - // ValuesGetter can return multiple values for a single key, // with contrast to TextMapCarrier.Get which returns a single value. type ValuesGetter interface { @@ -76,7 +68,7 @@ func (c MapCarrier) Keys() []string { return keys } -// HeaderCarrier adapts http.Header to satisfy the MultiTextMapCarrier interface. +// HeaderCarrier adapts http.Header to satisfy the TextMapCarrier and ValuesGetter interfaces. type HeaderCarrier http.Header // Get returns the first value associated with the passed key. @@ -115,8 +107,8 @@ type TextMapPropagator interface { // must never be done outside of a new major release. // Extract reads cross-cutting concerns from the carrier into a Context. - // Implementations may check if the carrier implements MultiTextMapCarrier, - // to support usage of multiple values per key. + // Implementations may check if the carrier implements ValuesGetter, + // to support extraction of multiple values per key. Extract(ctx context.Context, carrier TextMapCarrier) context.Context // DO NOT CHANGE: any modification will not be backwards compatible and // must never be done outside of a new major release. From 47204d07caa9b84fab2a08f8b8c1c323626f51f9 Mon Sep 17 00:00:00 2001 From: James Moessis Date: Fri, 9 May 2025 09:36:48 +1000 Subject: [PATCH 14/20] Update propagation/baggage.go MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Robert Pająk --- propagation/baggage.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/propagation/baggage.go b/propagation/baggage.go index 1ed0f12f479..ebda5026d6b 100644 --- a/propagation/baggage.go +++ b/propagation/baggage.go @@ -60,7 +60,7 @@ func extractMultiBaggage(parent context.Context, carrier ValuesGetter) context.C if len(bVals) == 0 { return parent } - members := make([]baggage.Member, 0) + var members []baggage.Member for _, bStr := range bVals { currBag, err := baggage.Parse(bStr) if err != nil { From a239650a1a19aaade30d34a1b42c0dd00536fbdd Mon Sep 17 00:00:00 2001 From: James Moessis Date: Fri, 9 May 2025 09:37:28 +1000 Subject: [PATCH 15/20] Update CHANGELOG.md MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Robert Pająk --- CHANGELOG.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1550208f92a..af1f2d09436 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,7 +16,9 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm See the [migration documentation](./semconv/v1.31.0/MIGRATION.md) for information on how to upgrade from `go.opentelemetry.io/otel/semconv/v1.30.0`(#6479) - Add `Recording`, `Scope`, and `Record` types in `go.opentelemetry.io/otel/log/logtest`. (#6507) - Add `WithHTTPClient` option to configure the `http.Client` used by `go.opentelemetry.io/otel/exporters/otlp/otlplog/otlploghttp`. (#6688) -- Allow extraction of multiple values in propagation. Implements for baggage propagator. (#5973) +- Add `ValuesGetter` in `go.opentelemetry.io/otel/propagation`, a `TextMapCarrier` that supports retrieving multiple values for a single key. (#5973) +- Add `Values` method to `HeaderCarrier` to implement the new `ValuesGetter` interface in `go.opentelemetry.io/otel/propagation`. (#5973) +- Update `Baggage` in `go.opentelemetry.io/otel/propagation` to retrieve multiple values for a key when the carrier implements `ValuesGetter`. (#5973) ### Removed From 41191a93ea0fb5672d0bff277fc561bb442021d1 Mon Sep 17 00:00:00 2001 From: James Moessis Date: Fri, 9 May 2025 09:43:11 +1000 Subject: [PATCH 16/20] add compile time checks for implementing interfaces --- propagation/propagation.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/propagation/propagation.go b/propagation/propagation.go index 23d61474357..5c8c26ea2eb 100644 --- a/propagation/propagation.go +++ b/propagation/propagation.go @@ -71,6 +71,12 @@ func (c MapCarrier) Keys() []string { // HeaderCarrier adapts http.Header to satisfy the TextMapCarrier and ValuesGetter interfaces. type HeaderCarrier http.Header +// Compile time check that HeaderCarrier implements ValuesGetter. +var _ TextMapCarrier = HeaderCarrier{} + +// Compile time check that HeaderCarrier implements TextMapCarrier. +var _ ValuesGetter = HeaderCarrier{} + // Get returns the first value associated with the passed key. func (hc HeaderCarrier) Get(key string) string { return http.Header(hc).Get(key) From 96985521b6060095daf1bebd59d9cca82b3b6516 Mon Sep 17 00:00:00 2001 From: James Moessis Date: Fri, 9 May 2025 09:58:44 +1000 Subject: [PATCH 17/20] update test to additionally test baggage with map carrier, as map carrier does not implement ValuesGetter so increases test coverage on single-valued path --- propagation/baggage_test.go | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-) diff --git a/propagation/baggage_test.go b/propagation/baggage_test.go index 76ae71d8a4b..11492ca3547 100644 --- a/propagation/baggage_test.go +++ b/propagation/baggage_test.go @@ -56,7 +56,7 @@ func (m members) Baggage(t *testing.T) baggage.Baggage { return bag } -func TestExtractValidBaggageFromHTTPReq(t *testing.T) { +func TestExtractValidBaggage(t *testing.T) { prop := propagation.TextMapPropagator(propagation.Baggage{}) tests := []struct { name string @@ -113,17 +113,29 @@ func TestExtractValidBaggageFromHTTPReq(t *testing.T) { {Key: "key1", Value: "val%2"}, }, }, + { + name: "empty header", + header: "", + want: members{}, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { + mapCarr := propagation.MapCarrier{} + mapCarr["baggage"] = tt.header req, _ := http.NewRequest(http.MethodGet, "http://example.com", nil) req.Header.Set("baggage", tt.header) - ctx := context.Background() - ctx = prop.Extract(ctx, propagation.HeaderCarrier(req.Header)) + // test with http header carrier (which implements ValuesGetter) + ctx := prop.Extract(context.Background(), propagation.HeaderCarrier(req.Header)) expected := tt.want.Baggage(t) assert.Equal(t, expected, baggage.FromContext(ctx)) + + // test with map carrier (which does not implement ValuesGetter) + ctx = prop.Extract(context.Background(), mapCarr) + expected = tt.want.Baggage(t) + assert.Equal(t, expected, baggage.FromContext(ctx)) }) } } From d1162293fe71848445e68958391b9e72fd1e4196 Mon Sep 17 00:00:00 2001 From: James Moessis Date: Wed, 14 May 2025 12:02:42 +1000 Subject: [PATCH 18/20] Update propagation/baggage_test.go MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Robert Pająk --- propagation/baggage_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/propagation/baggage_test.go b/propagation/baggage_test.go index 11492ca3547..678b3578897 100644 --- a/propagation/baggage_test.go +++ b/propagation/baggage_test.go @@ -130,7 +130,7 @@ func TestExtractValidBaggage(t *testing.T) { // test with http header carrier (which implements ValuesGetter) ctx := prop.Extract(context.Background(), propagation.HeaderCarrier(req.Header)) expected := tt.want.Baggage(t) - assert.Equal(t, expected, baggage.FromContext(ctx)) + assert.Equal(t, expected, baggage.FromContext(ctx), "should extract baggage for HeaderCarrier") // test with map carrier (which does not implement ValuesGetter) ctx = prop.Extract(context.Background(), mapCarr) From 0d124feb82cd3ad1e25348bc62c76310219c26b0 Mon Sep 17 00:00:00 2001 From: James Moessis Date: Wed, 14 May 2025 12:02:54 +1000 Subject: [PATCH 19/20] Update propagation/baggage_test.go MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Robert Pająk --- propagation/baggage_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/propagation/baggage_test.go b/propagation/baggage_test.go index 678b3578897..df5495f6516 100644 --- a/propagation/baggage_test.go +++ b/propagation/baggage_test.go @@ -133,7 +133,7 @@ func TestExtractValidBaggage(t *testing.T) { assert.Equal(t, expected, baggage.FromContext(ctx), "should extract baggage for HeaderCarrier") // test with map carrier (which does not implement ValuesGetter) - ctx = prop.Extract(context.Background(), mapCarr) + ctx = prop.Extract(context.Background(), mapCarr, "should extract baggage for MapCarrier") expected = tt.want.Baggage(t) assert.Equal(t, expected, baggage.FromContext(ctx)) }) From 91a4ffc40d9ac05b4307b461c0745dc0de1b0be5 Mon Sep 17 00:00:00 2001 From: James Moessis Date: Thu, 15 May 2025 11:15:12 +1000 Subject: [PATCH 20/20] fix applied suggestion --- propagation/baggage_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/propagation/baggage_test.go b/propagation/baggage_test.go index df5495f6516..8ce34bdec5c 100644 --- a/propagation/baggage_test.go +++ b/propagation/baggage_test.go @@ -133,9 +133,9 @@ func TestExtractValidBaggage(t *testing.T) { assert.Equal(t, expected, baggage.FromContext(ctx), "should extract baggage for HeaderCarrier") // test with map carrier (which does not implement ValuesGetter) - ctx = prop.Extract(context.Background(), mapCarr, "should extract baggage for MapCarrier") + ctx = prop.Extract(context.Background(), mapCarr) expected = tt.want.Baggage(t) - assert.Equal(t, expected, baggage.FromContext(ctx)) + assert.Equal(t, expected, baggage.FromContext(ctx), "should extract baggage for MapCarrier") }) } }