Merge branch 'main' into otel-host-header

CHANGELOG.md

@@ -29,12 +29,14 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
 - Improve `go.opentelemetry.io/otel/trace.TraceState`'s performance. (#4722)
 - Improve `go.opentelemetry.io/otel/propagation.TraceContext`'s performance. (#4721)
 - Improve `go.opentelemetry.io/otel/baggage` performance. (#4743)
+- `Member.String` in `go.opentelemetry.io/otel/baggage` percent-encodes only when necessary. (#4775)
 
 ### Fixed
 
 - Fix `Parse` in `go.opentelemetry.io/otel/baggage` to validate member value before percent-decoding. (#4755)
 - Fix whitespace encoding of `Member.String` in `go.opentelemetry.io/otel/baggage`. (#4756)
 - Fix processing `host` header in `WithHeaders` (#4780)
+- Fix baggage item key so that it is not canonicalized in `go.opentelemetry.io/otel/bridge/opentracing`. (#4776)
 
 ## [1.21.0/0.44.0] 2023-11-16
 

baggage/baggage.go

@@ -323,7 +323,7 @@ func (m Member) String() string {
 	// A key is just an ASCII string. A value is restricted to be
 	// US-ASCII characters excluding CTLs, whitespace,
 	// DQUOTE, comma, semicolon, and backslash.
-	s := fmt.Sprintf("%s%s%s", m.key, keyValueDelimiter, url.PathEscape(m.value))
+	s := fmt.Sprintf("%s%s%s", m.key, keyValueDelimiter, valueEscape(m.value))
 	if len(m.properties) > 0 {
 		s = fmt.Sprintf("%s%s%s", s, propertyDelimiter, m.properties.String())
 	}
@@ -652,9 +652,65 @@ func validateValue(s string) bool {
 }
 
 func validateValueChar(c int32) bool {
-	return (c >= 0x23 && c <= 0x2b) ||
+	return c == 0x21 ||
+		(c >= 0x23 && c <= 0x2b) ||
 		(c >= 0x2d && c <= 0x3a) ||
 		(c >= 0x3c && c <= 0x5b) ||
-		(c >= 0x5d && c <= 0x7e) ||
-		c == 0x21
+		(c >= 0x5d && c <= 0x7e)
+}
+
+// valueEscape escapes the string so it can be safely placed inside a baggage value,
+// replacing special characters with %XX sequences as needed.
+//
+// The implementation is based on:
+// https://github.com/golang/go/blob/f6509cf5cdbb5787061b784973782933c47f1782/src/net/url/url.go#L285.
+func valueEscape(s string) string {
+	hexCount := 0
+	for i := 0; i < len(s); i++ {
+		c := s[i]
+		if shouldEscape(c) {
+			hexCount++
+		}
+	}
+
+	if hexCount == 0 {
+		return s
+	}
+
+	var buf [64]byte
+	var t []byte
+
+	required := len(s) + 2*hexCount
+	if required <= len(buf) {
+		t = buf[:required]
+	} else {
+		t = make([]byte, required)
+	}
+
+	j := 0
+	for i := 0; i < len(s); i++ {
+		c := s[i]
+		if shouldEscape(s[i]) {
+			const upperhex = "0123456789ABCDEF"
+			t[j] = '%'
+			t[j+1] = upperhex[c>>4]
+			t[j+2] = upperhex[c&15]
+			j += 3
+		} else {
+			t[j] = c
+			j++
+		}
+	}
+
+	return string(t)
+}
+
+// shouldEscape returns true if the specified byte should be escaped when
+// appearing in a baggage value string.
+func shouldEscape(c byte) bool {
+	if c == '%' {
+		// The percent character must be encoded so that percent-encoding can work.
+		return true
+	}
+	return !validateValueChar(int32(c))
 }

baggage/baggage_test.go

@@ -22,6 +22,7 @@ import (
 	"testing"
 
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 
 	"go.opentelemetry.io/otel/internal/baggage"
 )
@@ -390,12 +391,19 @@ func TestBaggageParse(t *testing.T) {
 			},
 		},
 		{
-			name: "url encoded value",
+			name: "encoded ASCII string",
 			in:   "key1=val%252%2C",
 			want: baggage.List{
 				"key1": {Value: "val%2,"},
 			},
 		},
+		{
+			name: "encoded UTF-8 string",
+			in:   "foo=%C4%85%C5%9B%C4%87",
+			want: baggage.List{
+				"foo": {Value: "ąść"},
+			},
+		},
 		{
 			name: "invalid member: empty",
 			in:   "foo=,,bar=",
@@ -501,13 +509,7 @@ func TestBaggageString(t *testing.T) {
 			// Meaning, US-ASCII characters excluding CTLs, whitespace,
 			// DQUOTE, comma, semicolon, and backslash. All excluded
 			// characters need to be percent encoded.
-			//
-			// Ideally, the want result is:
-			// out: "foo=%00%01%02%03%04%05%06%07%08%09%0A%0B%0C%0D%0E%0F%10%11%12%13%14%15%16%17%18%19%1A%1B%1C%1D%1E%1F%20!%22#$%25&'()*+%2C-./0123456789:%3B<=>?@ABCDEFGHIJKLMNOPQRSTUVWXYZ[%5C]^_%60abcdefghijklmnopqrstuvwxyz{|}~%7F",
-			// However, the following characters are escaped:
-			// !#'()*/<>?[]^{|}
-			// It is not necessary, but still provides a correct result.
-			out: "foo=%00%01%02%03%04%05%06%07%08%09%0A%0B%0C%0D%0E%0F%10%11%12%13%14%15%16%17%18%19%1A%1B%1C%1D%1E%1F%20%21%22%23$%25&%27%28%29%2A+%2C-.%2F0123456789:%3B%3C=%3E%3F@ABCDEFGHIJKLMNOPQRSTUVWXYZ%5B%5C%5D%5E_%60abcdefghijklmnopqrstuvwxyz%7B%7C%7D~%7F",
+			out: "foo=%00%01%02%03%04%05%06%07%08%09%0A%0B%0C%0D%0E%0F%10%11%12%13%14%15%16%17%18%19%1A%1B%1C%1D%1E%1F%20!%22#$%25&'()*+%2C-./0123456789:%3B<=>?@ABCDEFGHIJKLMNOPQRSTUVWXYZ[%5C]^_`abcdefghijklmnopqrstuvwxyz{|}~%7F",
 			baggage: baggage.List{
 				"foo": {Value: func() string {
 					// All US-ASCII characters.
@@ -519,6 +521,13 @@ func TestBaggageString(t *testing.T) {
 				}()},
 			},
 		},
+		{
+			name: "non-ASCII UTF-8 string",
+			out:  "foo=%C4%85%C5%9B%C4%87",
+			baggage: baggage.List{
+				"foo": {Value: "ąść"},
+			},
+		},
 		{
 			name: "plus",
 			out:  "foo=1+1",
@@ -937,3 +946,48 @@ func BenchmarkParse(b *testing.B) {
 		benchBaggage, _ = Parse(`userId=alice,serverNode = DF28 , isProduction = false,hasProp=stuff;propKey;propWValue=value`)
 	}
 }
+
+func BenchmarkString(b *testing.B) {
+	var members []Member
+	addMember := func(k, v string) {
+		m, err := NewMember(k, valueEscape(v))
+		require.NoError(b, err)
+		members = append(members, m)
+	}
+
+	addMember("key1", "val1")
+	addMember("key2", " ;,%")
+	addMember("key3", "Witaj świecie!")
+	addMember("key4", strings.Repeat("Hello world!", 10))
+
+	bg, err := New(members...)
+	require.NoError(b, err)
+
+	b.ReportAllocs()
+	b.ResetTimer()
+
+	for i := 0; i < b.N; i++ {
+		_ = bg.String()
+	}
+}
+
+func BenchmarkValueEscape(b *testing.B) {
+	testCases := []struct {
+		name string
+		in   string
+	}{
+		{name: "nothing to escape", in: "value"},
+		{name: "requires escaping", in: " ;,%"},
+		{name: "long value", in: strings.Repeat("Hello world!", 20)},
+	}
+
+	for _, tc := range testCases {
+		b.Run(tc.name, func(b *testing.B) {
+			b.ReportAllocs()
+
+			for i := 0; i < b.N; i++ {
+				_ = valueEscape(tc.in)
+			}
+		})
+	}
+}

bridge/opencensus/go.mod

@@ -13,7 +13,7 @@ require (
 
 require (
 	github.com/davecgh/go-spew v1.1.1 // indirect
-	github.com/go-logr/logr v1.3.0 // indirect
+	github.com/go-logr/logr v1.4.1 // indirect
 	github.com/go-logr/stdr v1.2.2 // indirect
 	github.com/golang/groupcache v0.0.0-20200121045136-8c9f03a8e57e // indirect
 	github.com/kr/pretty v0.1.0 // indirect

bridge/opencensus/go.sum

@@ -11,8 +11,8 @@ github.com/envoyproxy/go-control-plane v0.9.1-0.20191026205805-5f8ba28d4473/go.m
 github.com/envoyproxy/go-control-plane v0.9.4/go.mod h1:6rpuAdCZL397s3pYoYcLgu1mIlRU8Am5FuJP05cCM98=
 github.com/envoyproxy/protoc-gen-validate v0.1.0/go.mod h1:iSmxcyjqTsJpI2R4NaDN7+kN2VEUnK/pcBlmesArF7c=
 github.com/go-logr/logr v1.2.2/go.mod h1:jdQByPbusPIv2/zmleS9BjJVeZ6kBagPoEUsqbVz/1A=
-github.com/go-logr/logr v1.3.0 h1:2y3SDp0ZXuc6/cjLSZ+Q3ir+QB9T/iG5yYRXqsagWSY=
-github.com/go-logr/logr v1.3.0/go.mod h1:9T104GzyrTigFIr8wt5mBrctHMim0Nb2HLGrmQ40KvY=
+github.com/go-logr/logr v1.4.1 h1:pKouT5E8xu9zeFC39JXRDukb6JFQPXM5p5I91188VAQ=
+github.com/go-logr/logr v1.4.1/go.mod h1:9T104GzyrTigFIr8wt5mBrctHMim0Nb2HLGrmQ40KvY=
 github.com/go-logr/stdr v1.2.2 h1:hSWxHoqTgW2S2qGc0LTAI563KZ5YKYRhT3MFKZMbjag=
 github.com/go-logr/stdr v1.2.2/go.mod h1:mMo/vtBO5dYbehREoey6XUKy/eSumjCCveDpRre4VKE=
 github.com/golang/glog v0.0.0-20160126235308-23def4e6c14b/go.mod h1:SBH7ygxi8pfUlaOkMMuAQtPIUF8ecWP5IEl/CR7VP2Q=

bridge/opencensus/test/go.mod

@@ -11,7 +11,7 @@ require (
 )
 
 require (
-	github.com/go-logr/logr v1.3.0 // indirect
+	github.com/go-logr/logr v1.4.1 // indirect
 	github.com/go-logr/stdr v1.2.2 // indirect
 	github.com/golang/groupcache v0.0.0-20200121045136-8c9f03a8e57e // indirect
 	go.opentelemetry.io/otel/metric v1.21.0 // indirect

bridge/opencensus/test/go.sum

@@ -11,8 +11,8 @@ github.com/envoyproxy/go-control-plane v0.9.1-0.20191026205805-5f8ba28d4473/go.m
 github.com/envoyproxy/go-control-plane v0.9.4/go.mod h1:6rpuAdCZL397s3pYoYcLgu1mIlRU8Am5FuJP05cCM98=
 github.com/envoyproxy/protoc-gen-validate v0.1.0/go.mod h1:iSmxcyjqTsJpI2R4NaDN7+kN2VEUnK/pcBlmesArF7c=
 github.com/go-logr/logr v1.2.2/go.mod h1:jdQByPbusPIv2/zmleS9BjJVeZ6kBagPoEUsqbVz/1A=
-github.com/go-logr/logr v1.3.0 h1:2y3SDp0ZXuc6/cjLSZ+Q3ir+QB9T/iG5yYRXqsagWSY=
-github.com/go-logr/logr v1.3.0/go.mod h1:9T104GzyrTigFIr8wt5mBrctHMim0Nb2HLGrmQ40KvY=
+github.com/go-logr/logr v1.4.1 h1:pKouT5E8xu9zeFC39JXRDukb6JFQPXM5p5I91188VAQ=
+github.com/go-logr/logr v1.4.1/go.mod h1:9T104GzyrTigFIr8wt5mBrctHMim0Nb2HLGrmQ40KvY=
 github.com/go-logr/stdr v1.2.2 h1:hSWxHoqTgW2S2qGc0LTAI563KZ5YKYRhT3MFKZMbjag=
 github.com/go-logr/stdr v1.2.2/go.mod h1:mMo/vtBO5dYbehREoey6XUKy/eSumjCCveDpRre4VKE=
 github.com/golang/glog v0.0.0-20160126235308-23def4e6c14b/go.mod h1:SBH7ygxi8pfUlaOkMMuAQtPIUF8ecWP5IEl/CR7VP2Q=

bridge/opentracing/bridge.go

@@ -17,7 +17,6 @@ package opentracing // import "go.opentelemetry.io/otel/bridge/opentracing"
 import (
 	"context"
 	"fmt"
-	"net/http"
 	"strings"
 	"sync"
 
@@ -74,17 +73,15 @@ func (c *bridgeSpanContext) ForeachBaggageItem(handler func(k, v string) bool) {
 }
 
 func (c *bridgeSpanContext) setBaggageItem(restrictedKey, value string) {
-	crk := http.CanonicalHeaderKey(restrictedKey)
-	m, err := baggage.NewMember(crk, value)
+	m, err := baggage.NewMember(restrictedKey, value)
 	if err != nil {
 		return
 	}
 	c.bag, _ = c.bag.SetMember(m)
 }
 
 func (c *bridgeSpanContext) baggageItem(restrictedKey string) baggage.Member {
-	crk := http.CanonicalHeaderKey(restrictedKey)
-	return c.bag.Member(crk)
+	return c.bag.Member(restrictedKey)
 }
 
 type bridgeSpan struct {

bridge/opentracing/bridge_test.go

@@ -574,3 +574,87 @@ func TestBridgeSpanContextPromotedMethods(t *testing.T) {
 		assert.True(t, spanContext.(spanContextProvider).HasTraceID())
 	})
 }
+
+func TestBridgeCarrierBaggagePropagation(t *testing.T) {
+	carriers := []struct {
+		name    string
+		carrier interface{}
+		format  ot.BuiltinFormat
+	}{
+		{
+			name:    "TextMapCarrier",
+			carrier: ot.TextMapCarrier{},
+			format:  ot.TextMap,
+		},
+		{
+			name:    "HTTPHeadersCarrier",
+			carrier: ot.HTTPHeadersCarrier{},
+			format:  ot.HTTPHeaders,
+		},
+	}
+
+	testCases := []struct {
+		name         string
+		baggageItems []bipBaggage
+	}{
+		{
+			name: "single baggage item",
+			baggageItems: []bipBaggage{
+				{
+					key:   "foo",
+					value: "bar",
+				},
+			},
+		},
+		{
+			name: "multiple baggage items",
+			baggageItems: []bipBaggage{
+				{
+					key:   "foo",
+					value: "bar",
+				},
+				{
+					key:   "foo2",
+					value: "bar2",
+				},
+			},
+		},
+	}
+
+	for _, c := range carriers {
+		for _, tc := range testCases {
+			t.Run(fmt.Sprintf("%s %s", c.name, tc.name), func(t *testing.T) {
+				mockOtelTracer := internal.NewMockTracer()
+				b, _ := NewTracerPair(mockOtelTracer)
+				b.SetTextMapPropagator(propagation.NewCompositeTextMapPropagator(
+					propagation.TraceContext{},
+					propagation.Baggage{}), // Required for baggage propagation.
+				)
+
+				// Set baggage items.
+				span := b.StartSpan("test")
+				for _, bi := range tc.baggageItems {
+					span.SetBaggageItem(bi.key, bi.value)
+				}
+				defer span.Finish()
+
+				err := b.Inject(span.Context(), c.format, c.carrier)
+				assert.NoError(t, err)
+
+				spanContext, err := b.Extract(c.format, c.carrier)
+				assert.NoError(t, err)
+
+				// Check baggage items.
+				bsc, ok := spanContext.(*bridgeSpanContext)
+				assert.True(t, ok)
+
+				var got []bipBaggage
+				for _, m := range bsc.bag.Members() {
+					got = append(got, bipBaggage{m.Key(), m.Value()})
+				}
+
+				assert.ElementsMatch(t, tc.baggageItems, got)
+			})
+		}
+	}
+}

bridge/opentracing/go.mod

@@ -15,7 +15,7 @@ require (
 
 require (
 	github.com/davecgh/go-spew v1.1.1 // indirect
-	github.com/go-logr/logr v1.3.0 // indirect
+	github.com/go-logr/logr v1.4.1 // indirect
 	github.com/go-logr/stdr v1.2.2 // indirect
 	github.com/pmezard/go-difflib v1.0.0 // indirect
 	go.opentelemetry.io/otel/metric v1.21.0 // indirect

bridge/opentracing/go.sum

@@ -2,8 +2,8 @@ github.com/davecgh/go-spew v1.1.0/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSs
 github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c=
 github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38=
 github.com/go-logr/logr v1.2.2/go.mod h1:jdQByPbusPIv2/zmleS9BjJVeZ6kBagPoEUsqbVz/1A=
-github.com/go-logr/logr v1.3.0 h1:2y3SDp0ZXuc6/cjLSZ+Q3ir+QB9T/iG5yYRXqsagWSY=
-github.com/go-logr/logr v1.3.0/go.mod h1:9T104GzyrTigFIr8wt5mBrctHMim0Nb2HLGrmQ40KvY=
+github.com/go-logr/logr v1.4.1 h1:pKouT5E8xu9zeFC39JXRDukb6JFQPXM5p5I91188VAQ=
+github.com/go-logr/logr v1.4.1/go.mod h1:9T104GzyrTigFIr8wt5mBrctHMim0Nb2HLGrmQ40KvY=
 github.com/go-logr/stdr v1.2.2 h1:hSWxHoqTgW2S2qGc0LTAI563KZ5YKYRhT3MFKZMbjag=
 github.com/go-logr/stdr v1.2.2/go.mod h1:mMo/vtBO5dYbehREoey6XUKy/eSumjCCveDpRre4VKE=
 github.com/google/go-cmp v0.6.0 h1:ofyhxvXcZhMsU5ulbFiLKl/XBFqE1GSq7atu8tAmTRI=

bridge/opentracing/mix_test.go

@@ -334,7 +334,7 @@ func newBaggageItemsPreservationTest() *baggageItemsPreservationTest {
 				value: "two",
 			},
 			{
-				key:   "Third",
+				key:   "third",
 				value: "three",
 			},
 		},
@@ -427,7 +427,7 @@ func newBaggageInteroperationTest() *baggageInteroperationTest {
 				value: "two",
 			},
 			{
-				key:   "Third",
+				key:   "third",
 				value: "three",
 			},
 		},