From 4bf6150fa94e18bdf01c96ed78ee6d1c76f8e308 Mon Sep 17 00:00:00 2001 From: Tyler Yahn Date: Tue, 8 Jun 2021 15:06:37 +0000 Subject: [PATCH] Add baggage implementation based on the W3C and OpenTelemetry specification (#1967) * Rename baggage context file * Initial baggage implementation * Initial tests * More tests * Update baggage context functionality * Add New method to baggage pkg * Update namedtracer example * URL encode baggage values * Refactor and use internal baggage pkg * Update OpenTracing bridge * Update baggage propagator * Fix lint and test errors * Add changes to changelog * Apply suggestions from code review * Rename testcase field per suggestion * Update test to verify last-one-wins semantics * Explicitly seed random numbers with static seed in tests * Parse Member key/value with string split * Add test for member parse with equal signs in value * Trim whitespaces for member key/value --- CHANGELOG.md | 5 + baggage/baggage.go | 534 +++++++++++++++++++-- baggage/baggage_test.go | 700 ++++++++++++++++++++++++++-- baggage/context.go | 39 ++ baggage/context_test.go | 36 ++ bridge/opentracing/bridge.go | 77 +-- bridge/opentracing/internal/mock.go | 55 ++- bridge/opentracing/mix_test.go | 22 +- example/namedtracer/main.go | 6 +- internal/baggage/baggage.go | 339 +------------- internal/baggage/baggage_test.go | 285 ----------- internal/baggage/context.go | 95 ++++ internal/baggage/context_test.go | 96 ++++ propagation/baggage.go | 73 +-- propagation/baggage_test.go | 225 ++++----- 15 files changed, 1657 insertions(+), 930 deletions(-) create mode 100644 baggage/context.go create mode 100644 baggage/context_test.go delete mode 100644 internal/baggage/baggage_test.go create mode 100644 internal/baggage/context.go create mode 100644 internal/baggage/context_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 597f780f08e..ab004c7bcfe 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -33,6 +33,9 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm This method returns the number of list-members the `TraceState` holds. (#1937) - Creates package `go.opentelemetry.io/otel/exporters/otlp/otlptrace` that defines a trace exporter that uses a `otlptrace.Client` to send data. Creates package `go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracegrpc` implementing a gRPC `otlptrace.Client` and offers convenience functions, `NewExportPipeline` and `InstallNewPipeline`, to setup and install a `otlptrace.Exporter` in tracing .(#1922) +- The `Baggage`, `Member`, and `Property` types are added to the `go.opentelemetry.io/otel/baggage` package along with their related functions. (#1967) +- The new `ContextWithBaggage`, `ContextWithoutBaggage`, and `FromContext` functions were added to the `go.opentelemetry.io/otel/baggage` package. + These functions replace the `Set`, `Value`, `ContextWithValue`, `ContextWithoutValue`, and `ContextWithEmpty` functions from that package and directly work with the new `Baggage` type. (#1967) - The `OTEL_SERVICE_NAME` environment variable is the preferred source for `service.name`, used by the environment resource detector if a service name is present both there and in `OTEL_RESOURCE_ATTRIBUTES`. (#1969) - Creates package `go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracehttp` implementing a HTTP `otlptrace.Client` and offers convenience functions, `NewExportPipeline` and `InstallNewPipeline`, to setup and install a `otlptrace.Exporter` in tracing. (#1963) @@ -89,6 +92,8 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm The `"go.opentelemetry.io/otel".Tracer` function or a `TracerProvider` should be used to acquire a library specific `Tracer` instead. (#1900) - The `http.url` attribute generated by `HTTPClientAttributesFromHTTPRequest` will no longer include username or password information. (#1919) - The `IsEmpty` method of the `TraceState` type in the `go.opentelemetry.io/otel/trace` package is removed in favor of using the added `TraceState.Len` method. (#1931) +- The `Set`, `Value`, `ContextWithValue`, `ContextWithoutValue`, and `ContextWithEmpty` functions in the `go.opentelemetry.io/otel/baggage` package are removed. + Handling of baggage is now done using the added `Baggage` type and related context functions (`ContextWithBaggage`, `ContextWithoutBaggage`, and `FromContext`) in that package. (TBD) ### Fixed diff --git a/baggage/baggage.go b/baggage/baggage.go index 365388c654e..8de2bc9df12 100644 --- a/baggage/baggage.go +++ b/baggage/baggage.go @@ -12,56 +12,498 @@ // See the License for the specific language governing permissions and // limitations under the License. -package baggage // import "go.opentelemetry.io/otel/baggage" +package baggage import ( - "context" + "errors" + "fmt" + "net/url" + "regexp" + "strings" - "go.opentelemetry.io/otel/attribute" "go.opentelemetry.io/otel/internal/baggage" ) -// Set returns a copy of the set of baggage key-values in ctx. -func Set(ctx context.Context) attribute.Set { - // TODO (MrAlias, #1222): The underlying storage, the Map, shares many of - // the functional elements of the attribute.Set. These should be unified so - // this conversion is unnecessary and there is no performance hit calling - // this. - m := baggage.MapFromContext(ctx) - values := make([]attribute.KeyValue, 0, m.Len()) - m.Foreach(func(kv attribute.KeyValue) bool { - values = append(values, kv) - return true - }) - return attribute.NewSet(values...) -} - -// Value returns the value related to key in the baggage of ctx. If no -// value is set, the returned attribute.Value will be an uninitialized zero-value -// with type INVALID. -func Value(ctx context.Context, key attribute.Key) attribute.Value { - v, _ := baggage.MapFromContext(ctx).Value(key) - return v -} - -// ContextWithValues returns a copy of parent with pairs updated in the baggage. -func ContextWithValues(parent context.Context, pairs ...attribute.KeyValue) context.Context { - m := baggage.MapFromContext(parent).Apply(baggage.MapUpdate{ - MultiKV: pairs, - }) - return baggage.ContextWithMap(parent, m) -} - -// ContextWithoutValues returns a copy of parent in which the values related -// to keys have been removed from the baggage. -func ContextWithoutValues(parent context.Context, keys ...attribute.Key) context.Context { - m := baggage.MapFromContext(parent).Apply(baggage.MapUpdate{ - DropMultiK: keys, - }) - return baggage.ContextWithMap(parent, m) -} - -// ContextWithEmpty returns a copy of parent without baggage. -func ContextWithEmpty(parent context.Context) context.Context { - return baggage.ContextWithNoCorrelationData(parent) +const ( + maxMembers = 180 + maxBytesPerMembers = 4096 + maxBytesPerBaggageString = 8192 + + listDelimiter = "," + keyValueDelimiter = "=" + propertyDelimiter = ";" + + keyDef = `([\x21\x23-\x27\x2A\x2B\x2D\x2E\x30-\x39\x41-\x5a\x5e-\x7a\x7c\x7e]+)` + valueDef = `([\x21\x23-\x2b\x2d-\x3a\x3c-\x5B\x5D-\x7e]*)` + keyValueDef = `\s*` + keyDef + `\s*` + keyValueDelimiter + `\s*` + valueDef + `\s*` +) + +var ( + keyRe = regexp.MustCompile(`^` + keyDef + `$`) + valueRe = regexp.MustCompile(`^` + valueDef + `$`) + propertyRe = regexp.MustCompile(`^(?:\s*` + keyDef + `\s*|` + keyValueDef + `)$`) +) + +var ( + errInvalidKey = errors.New("invalid key") + errInvalidValue = errors.New("invalid value") + errInvalidProperty = errors.New("invalid baggage list-member property") + errInvalidMember = errors.New("invalid baggage list-member") + errMemberNumber = errors.New("too many list-members in baggage-string") + errMemberBytes = errors.New("list-member too large") + errBaggageBytes = errors.New("baggage-string too large") +) + +// Property is an additional metadata entry for a baggage list-member. +type Property struct { + key, value string + + // hasValue indicates if a zero-value value means the property does not + // have a value or if it was the zero-value. + hasValue bool +} + +func NewKeyProperty(key string) (Property, error) { + p := Property{} + if !keyRe.MatchString(key) { + return p, fmt.Errorf("%w: %q", errInvalidKey, key) + } + p.key = key + return p, nil +} + +func NewKeyValueProperty(key, value string) (Property, error) { + p := Property{} + if !keyRe.MatchString(key) { + return p, fmt.Errorf("%w: %q", errInvalidKey, key) + } + if !valueRe.MatchString(value) { + return p, fmt.Errorf("%w: %q", errInvalidValue, value) + } + p.key = key + p.value = value + p.hasValue = true + return p, nil +} + +// parseProperty attempts to decode a Property from the passed string. It +// returns an error if the input is invalid according to the W3C Baggage +// specification. +func parseProperty(property string) (Property, error) { + p := Property{} + if property == "" { + return p, nil + } + + match := propertyRe.FindStringSubmatch(property) + if len(match) != 4 { + return p, fmt.Errorf("%w: %q", errInvalidProperty, property) + } + + if match[1] != "" { + p.key = match[1] + } else { + p.key = match[2] + p.value = match[3] + p.hasValue = true + } + return p, nil +} + +// validate ensures p conforms to the W3C Baggage specification, returning an +// error otherwise. +func (p Property) validate() error { + errFunc := func(err error) error { + return fmt.Errorf("invalid property: %w", err) + } + + if !keyRe.MatchString(p.key) { + return errFunc(fmt.Errorf("%w: %q", errInvalidKey, p.key)) + } + if p.hasValue && !valueRe.MatchString(p.value) { + return errFunc(fmt.Errorf("%w: %q", errInvalidValue, p.value)) + } + if !p.hasValue && p.value != "" { + return errFunc(errors.New("inconsistent value")) + } + return nil +} + +// Key returns the Property key. +func (p Property) Key() string { + return p.key +} + +// Value returns the Property value. Additionally a boolean value is returned +// indicating if the returned value is the empty if the Property has a value +// that is empty or if the value is not set. +func (p Property) Value() (string, bool) { + return p.value, p.hasValue +} + +// String encodes Property into a string compliant with the W3C Baggage +// specification. +func (p Property) String() string { + if p.hasValue { + return fmt.Sprintf("%s%s%v", p.key, keyValueDelimiter, p.value) + } + return p.key +} + +type properties []Property + +func fromInternalProperties(iProps []baggage.Property) properties { + if len(iProps) == 0 { + return nil + } + + props := make(properties, len(iProps)) + for i, p := range iProps { + props[i] = Property{ + key: p.Key, + value: p.Value, + hasValue: p.HasValue, + } + } + return props +} + +func (p properties) asInternal() []baggage.Property { + if len(p) == 0 { + return nil + } + + iProps := make([]baggage.Property, len(p)) + for i, prop := range p { + iProps[i] = baggage.Property{ + Key: prop.key, + Value: prop.value, + HasValue: prop.hasValue, + } + } + return iProps +} + +func (p properties) Copy() properties { + if len(p) == 0 { + return nil + } + + props := make(properties, len(p)) + copy(props, p) + return props +} + +// validate ensures each Property in p conforms to the W3C Baggage +// specification, returning an error otherwise. +func (p properties) validate() error { + for _, prop := range p { + if err := prop.validate(); err != nil { + return err + } + } + return nil +} + +// String encodes properties into a string compliant with the W3C Baggage +// specification. +func (p properties) String() string { + props := make([]string, len(p)) + for i, prop := range p { + props[i] = prop.String() + } + return strings.Join(props, propertyDelimiter) +} + +// Member is a list-member of a baggage-string as defined by the W3C Baggage +// specification. +type Member struct { + key, value string + properties properties +} + +// NewMember returns a new Member from the passed arguments. An error is +// returned if the created Member would be invalid according to the W3C +// Baggage specification. +func NewMember(key, value string, props ...Property) (Member, error) { + m := Member{key: key, value: value, properties: properties(props).Copy()} + if err := m.validate(); err != nil { + return Member{}, err + } + + return m, nil +} + +// parseMember attempts to decode a Member from the passed string. It returns +// an error if the input is invalid according to the W3C Baggage +// specification. +func parseMember(member string) (Member, error) { + if n := len(member); n > maxBytesPerMembers { + return Member{}, fmt.Errorf("%w: %d", errMemberBytes, n) + } + + var ( + key, value string + props properties + ) + + parts := strings.SplitN(member, propertyDelimiter, 2) + switch len(parts) { + case 2: + // Parse the member properties. + for _, pStr := range strings.Split(parts[1], propertyDelimiter) { + p, err := parseProperty(pStr) + if err != nil { + return Member{}, err + } + props = append(props, p) + } + fallthrough + case 1: + // Parse the member key/value pair. + + // Take into account a value can contain equal signs (=). + kv := strings.SplitN(parts[0], keyValueDelimiter, 2) + if len(kv) != 2 { + return Member{}, fmt.Errorf("%w: %q", errInvalidMember, member) + } + // "Leading and trailing whitespaces are allowed but MUST be trimmed + // when converting the header into a data structure." + key, value = strings.TrimSpace(kv[0]), strings.TrimSpace(kv[1]) + if !keyRe.MatchString(key) { + return Member{}, fmt.Errorf("%w: %q", errInvalidKey, key) + } + if !valueRe.MatchString(value) { + return Member{}, fmt.Errorf("%w: %q", errInvalidValue, value) + } + default: + // This should never happen unless a developer has changed the string + // splitting somehow. Panic instead of failing silently and allowing + // the bug to slip past the CI checks. + panic("failed to parse baggage member") + } + + return Member{key: key, value: value, properties: props}, nil +} + +// validate ensures m conforms to the W3C Baggage specification, returning an +// error otherwise. +func (m Member) validate() error { + if !keyRe.MatchString(m.key) { + return fmt.Errorf("%w: %q", errInvalidKey, m.key) + } + if !valueRe.MatchString(m.value) { + return fmt.Errorf("%w: %q", errInvalidValue, m.value) + } + return m.properties.validate() +} + +// Key returns the Member key. +func (m Member) Key() string { return m.key } + +// Value returns the Member value. +func (m Member) Value() string { return m.value } + +// Properties returns a copy of the Member properties. +func (m Member) Properties() []Property { return m.properties.Copy() } + +// String encodes Member into a string compliant with the W3C Baggage +// specification. +func (m Member) String() string { + // A key is just an ASCII string, but a value is URL encoded UTF-8. + s := fmt.Sprintf("%s%s%s", m.key, keyValueDelimiter, url.QueryEscape(m.value)) + if len(m.properties) > 0 { + s = fmt.Sprintf("%s%s%s", s, propertyDelimiter, m.properties.String()) + } + return s +} + +// Baggage is a list of baggage members representing the baggage-string as +// defined by the W3C Baggage specification. +type Baggage struct { //nolint:golint + list baggage.List +} + +// New returns a new valid Baggage. It returns an error if the passed members +// are invalid according to the W3C Baggage specification or if it results in +// a Baggage exceeding limits set in that specification. +func New(members ...Member) (Baggage, error) { + if len(members) == 0 { + return Baggage{}, nil + } + + b := make(baggage.List) + for _, m := range members { + if err := m.validate(); err != nil { + return Baggage{}, err + } + // OpenTelemetry resolves duplicates by last-one-wins. + b[m.key] = baggage.Item{ + Value: m.value, + Properties: m.properties.asInternal(), + } + } + + // Check member numbers after deduplicating. + if len(b) > maxMembers { + return Baggage{}, errMemberNumber + } + + bag := Baggage{b} + if n := len(bag.String()); n > maxBytesPerBaggageString { + return Baggage{}, fmt.Errorf("%w: %d", errBaggageBytes, n) + } + + return bag, nil +} + +// Parse attempts to decode a baggage-string from the passed string. It +// returns an error if the input is invalid according to the W3C Baggage +// specification. +// +// If there are duplicate list-members contained in baggage, the last one +// defined (reading left-to-right) will be the only one kept. This diverges +// from the W3C Baggage specification which allows duplicate list-members, but +// conforms to the OpenTelemetry Baggage specification. +func Parse(bStr string) (Baggage, error) { + if bStr == "" { + return Baggage{}, nil + } + + if n := len(bStr); n > maxBytesPerBaggageString { + return Baggage{}, fmt.Errorf("%w: %d", errBaggageBytes, n) + } + + b := make(baggage.List) + for _, memberStr := range strings.Split(bStr, listDelimiter) { + m, err := parseMember(memberStr) + if err != nil { + return Baggage{}, err + } + // OpenTelemetry resolves duplicates by last-one-wins. + b[m.key] = baggage.Item{ + Value: m.value, + Properties: m.properties.asInternal(), + } + } + + // OpenTelemetry does not allow for duplicate list-members, but the W3C + // specification does. Now that we have deduplicated, ensure the baggage + // does not exceed list-member limits. + if len(b) > maxMembers { + return Baggage{}, errMemberNumber + } + + return Baggage{b}, nil +} + +// Member returns the baggage list-member identified by key. +// +// If there is no list-member matching the passed key the returned Member will +// be a zero-value Member. +func (b Baggage) Member(key string) Member { + v, ok := b.list[key] + if !ok { + // We do not need to worry about distiguising between the situation + // where a zero-valued Member is included in the Baggage because a + // zero-valued Member is invalid according to the W3C Baggage + // specification (it has an empty key). + return Member{} + } + + return Member{ + key: key, + value: v.Value, + properties: fromInternalProperties(v.Properties), + } +} + +// Members returns all the baggage list-members. +// The order of the returned list-members does not have significance. +func (b Baggage) Members() []Member { + if len(b.list) == 0 { + return nil + } + + members := make([]Member, 0, len(b.list)) + for k, v := range b.list { + members = append(members, Member{ + key: k, + value: v.Value, + properties: fromInternalProperties(v.Properties), + }) + } + return members +} + +// SetMember returns a copy the Baggage with the member included. If the +// baggage contains a Member with the same key the existing Member is +// replaced. +// +// If member is invalid according to the W3C Baggage specification, an error +// is returned with the original Baggage. +func (b Baggage) SetMember(member Member) (Baggage, error) { + if err := member.validate(); err != nil { + return b, fmt.Errorf("%w: %s", errInvalidMember, err) + } + + n := len(b.list) + if _, ok := b.list[member.key]; !ok { + n++ + } + list := make(baggage.List, n) + + for k, v := range b.list { + // Do not copy if we are just going to overwrite. + if k == member.key { + continue + } + list[k] = v + } + + list[member.key] = baggage.Item{ + Value: member.value, + Properties: member.properties.asInternal(), + } + + return Baggage{list: list}, nil +} + +// DeleteMember returns a copy of the Baggage with the list-member identified +// by key removed. +func (b Baggage) DeleteMember(key string) Baggage { + n := len(b.list) + if _, ok := b.list[key]; ok { + n-- + } + list := make(baggage.List, n) + + for k, v := range b.list { + if k == key { + continue + } + list[k] = v + } + + return Baggage{list: list} +} + +// Len returns the number of list-members in the Baggage. +func (b Baggage) Len() int { + return len(b.list) +} + +// String encodes Baggage into a string compliant with the W3C Baggage +// specification. The returned string will be invalid if the Baggage contains +// any invalid list-members. +func (b Baggage) String() string { + members := make([]string, 0, len(b.list)) + for k, v := range b.list { + members = append(members, Member{ + key: k, + value: v.Value, + properties: fromInternalProperties(v.Properties), + }.String()) + } + return strings.Join(members, listDelimiter) } diff --git a/baggage/baggage_test.go b/baggage/baggage_test.go index 53f57b1bcd4..0635c823e9e 100644 --- a/baggage/baggage_test.go +++ b/baggage/baggage_test.go @@ -15,72 +15,682 @@ package baggage import ( - "context" + "fmt" + "math/rand" + "sort" + "strings" "testing" - "go.opentelemetry.io/otel/attribute" + "github.com/stretchr/testify/assert" + "go.opentelemetry.io/otel/internal/baggage" ) -func TestBaggage(t *testing.T) { - ctx := context.Background() - ctx = baggage.ContextWithMap(ctx, baggage.NewEmptyMap()) +var rng *rand.Rand + +func init() { + // Seed with a static value to ensure deterministic results. + rng = rand.New(rand.NewSource(1)) +} - b := Set(ctx) - if b.Len() != 0 { - t.Fatalf("empty baggage returned a set with %d elements", b.Len()) +func TestKeyRegExp(t *testing.T) { + // ASCII only + invalidKeyRune := []rune{ + '\x00', '\x01', '\x02', '\x03', '\x04', '\x05', '\x06', '\x07', + '\x08', '\x09', '\x0A', '\x0B', '\x0C', '\x0D', '\x0E', '\x0F', + '\x10', '\x11', '\x12', '\x13', '\x14', '\x15', '\x16', '\x17', + '\x18', '\x19', '\x1A', '\x1B', '\x1C', '\x1D', '\x1E', '\x1F', ' ', + '(', ')', '<', '>', '@', ',', ';', ':', '\\', '"', '/', '[', ']', '?', + '=', '{', '}', '\x7F', } - first, second, third := attribute.Key("first"), attribute.Key("second"), attribute.Key("third") - ctx = ContextWithValues(ctx, first.Bool(true), second.String("2")) - m := baggage.MapFromContext(ctx) - v, ok := m.Value(first) - if !ok { - t.Fatal("WithValues failed to set first value") + for _, ch := range invalidKeyRune { + assert.NotRegexp(t, keyDef, fmt.Sprintf("%c", ch)) } - if !v.AsBool() { - t.Fatal("WithValues failed to set first correct value") +} + +func TestValueRegExp(t *testing.T) { + // ASCII only + invalidValueRune := []rune{ + '\x00', '\x01', '\x02', '\x03', '\x04', '\x05', '\x06', '\x07', + '\x08', '\x09', '\x0A', '\x0B', '\x0C', '\x0D', '\x0E', '\x0F', + '\x10', '\x11', '\x12', '\x13', '\x14', '\x15', '\x16', '\x17', + '\x18', '\x19', '\x1A', '\x1B', '\x1C', '\x1D', '\x1E', '\x1F', ' ', + '"', ',', ';', '\\', '\x7F', } - v, ok = m.Value(second) - if !ok { - t.Fatal("WithValues failed to set second value") + + for _, ch := range invalidValueRune { + assert.NotRegexp(t, `^`+valueDef+`$`, fmt.Sprintf("invalid-%c-value", ch)) } - if v.AsString() != "2" { - t.Fatal("WithValues failed to set second correct value") +} + +func TestParseProperty(t *testing.T) { + p := Property{key: "key", value: "value", hasValue: true} + + testcases := []struct { + in string + expected Property + }{ + { + in: "", + expected: Property{}, + }, + { + in: "key", + expected: Property{ + key: "key", + }, + }, + { + in: "key=", + expected: Property{ + key: "key", + hasValue: true, + }, + }, + { + in: "key=value", + expected: p, + }, + { + in: " key=value ", + expected: p, + }, + { + in: "key = value", + expected: p, + }, + { + in: " key = value ", + expected: p, + }, + { + in: "\tkey=value", + expected: p, + }, + } + + for _, tc := range testcases { + actual, err := parseProperty(tc.in) + + if !assert.NoError(t, err) { + continue + } + + assert.Equal(t, tc.expected.Key(), actual.Key(), tc.in) + + actualV, actualOk := actual.Value() + expectedV, expectedOk := tc.expected.Value() + assert.Equal(t, expectedOk, actualOk, tc.in) + assert.Equal(t, expectedV, actualV, tc.in) } - _, ok = m.Value(third) - if ok { - t.Fatal("WithValues set an unexpected third value") +} + +func TestParsePropertyError(t *testing.T) { + _, err := parseProperty(",;,") + assert.ErrorIs(t, err, errInvalidProperty) +} + +func TestNewKeyProperty(t *testing.T) { + p, err := NewKeyProperty(" ") + assert.ErrorIs(t, err, errInvalidKey) + assert.Equal(t, Property{}, p) + + p, err = NewKeyProperty("key") + assert.NoError(t, err) + assert.Equal(t, Property{key: "key"}, p) +} + +func TestNewKeyValueProperty(t *testing.T) { + p, err := NewKeyValueProperty(" ", "") + assert.ErrorIs(t, err, errInvalidKey) + assert.Equal(t, Property{}, p) + + p, err = NewKeyValueProperty("key", ";") + assert.ErrorIs(t, err, errInvalidValue) + assert.Equal(t, Property{}, p) + + p, err = NewKeyValueProperty("key", "value") + assert.NoError(t, err) + assert.Equal(t, Property{key: "key", value: "value", hasValue: true}, p) +} + +func TestPropertyValidate(t *testing.T) { + p := Property{} + assert.ErrorIs(t, p.validate(), errInvalidKey) + + p.key = "k" + assert.NoError(t, p.validate()) + + p.value = ";" + assert.EqualError(t, p.validate(), "invalid property: inconsistent value") + + p.hasValue = true + assert.ErrorIs(t, p.validate(), errInvalidValue) + + p.value = "v" + assert.NoError(t, p.validate()) +} + +func TestNewEmptyBaggage(t *testing.T) { + b, err := New() + assert.NoError(t, err) + assert.Equal(t, Baggage{}, b) +} + +func TestNewBaggage(t *testing.T) { + b, err := New(Member{key: "k"}) + assert.NoError(t, err) + assert.Equal(t, Baggage{list: baggage.List{"k": {}}}, b) +} + +func TestNewBaggageWithDuplicates(t *testing.T) { + // Having this many members would normally cause this to error, but since + // these are duplicates of the same key they will be collapsed into a + // single entry. + m := make([]Member, maxMembers+1) + for i := range m { + // Duplicates are collapsed. + m[i] = Member{ + key: "a", + value: fmt.Sprintf("%d", i), + } + } + b, err := New(m...) + assert.NoError(t, err) + + // Ensure that the last-one-wins by verifying the value. + v := fmt.Sprintf("%d", maxMembers) + want := Baggage{list: baggage.List{"a": {Value: v}}} + assert.Equal(t, want, b) +} + +func TestNewBaggageErrorInvalidMember(t *testing.T) { + _, err := New(Member{key: ""}) + assert.ErrorIs(t, err, errInvalidKey) +} + +func key(n int) string { + r := []rune("0123456789abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ") + + b := make([]rune, n) + for i := range b { + b[i] = r[rng.Intn(len(r))] + } + return string(b) +} + +func TestNewBaggageErrorTooManyBytes(t *testing.T) { + m := make([]Member, (maxBytesPerBaggageString/maxBytesPerMembers)+1) + for i := range m { + m[i] = Member{key: key(maxBytesPerMembers)} + } + _, err := New(m...) + assert.ErrorIs(t, err, errBaggageBytes) +} + +func TestNewBaggageErrorTooManyMembers(t *testing.T) { + m := make([]Member, maxMembers+1) + for i := range m { + m[i] = Member{key: fmt.Sprintf("%d", i)} + } + _, err := New(m...) + assert.ErrorIs(t, err, errMemberNumber) +} + +func TestBaggageParse(t *testing.T) { + tooLarge := key(maxBytesPerBaggageString + 1) + + tooLargeMember := key(maxBytesPerMembers + 1) + + m := make([]string, maxMembers+1) + for i := range m { + m[i] = fmt.Sprintf("a%d=", i) } + tooManyMembers := strings.Join(m, listDelimiter) - b = Set(ctx) - if b.Len() != 2 { - t.Fatalf("Baggage returned a set with %d elements, want 2", b.Len()) + testcases := []struct { + name string + in string + want baggage.List + err error + }{ + { + name: "empty value", + in: "", + want: baggage.List(nil), + }, + { + name: "single member empty value no properties", + in: "foo=", + want: baggage.List{ + "foo": {Value: ""}, + }, + }, + { + name: "single member no properties", + in: "foo=1", + want: baggage.List{ + "foo": {Value: "1"}, + }, + }, + { + name: "single member with spaces", + in: " foo \t= 1\t\t ", + want: baggage.List{ + "foo": {Value: "1"}, + }, + }, + { + name: "single member empty value with properties", + in: "foo=;state=on;red", + want: baggage.List{ + "foo": { + Value: "", + Properties: []baggage.Property{ + {Key: "state", Value: "on", HasValue: true}, + {Key: "red"}, + }, + }, + }, + }, + { + name: "single member with properties", + in: "foo=1;state=on;red", + want: baggage.List{ + "foo": { + Value: "1", + Properties: []baggage.Property{ + {Key: "state", Value: "on", HasValue: true}, + {Key: "red"}, + }, + }, + }, + }, + { + name: "single member with value containing equal signs", + in: "foo=0=0=0", + want: baggage.List{ + "foo": {Value: "0=0=0"}, + }, + }, + { + name: "two members with properties", + in: "foo=1;state=on;red,bar=2;yellow", + want: baggage.List{ + "foo": { + Value: "1", + Properties: []baggage.Property{ + {Key: "state", Value: "on", HasValue: true}, + {Key: "red"}, + }, + }, + "bar": { + Value: "2", + Properties: []baggage.Property{{Key: "yellow"}}, + }, + }, + }, + { + // According to the OTel spec, last value wins. + name: "duplicate key", + in: "foo=1;state=on;red,foo=2", + want: baggage.List{ + "foo": {Value: "2"}, + }, + }, + { + name: "invalid member: empty", + in: "foo=,,bar=", + err: errInvalidMember, + }, + { + name: "invalid member: no key", + in: "=foo", + err: errInvalidKey, + }, + { + name: "invalid member: no value", + in: "foo", + err: errInvalidMember, + }, + { + name: "invalid member: invalid key", + in: "\\=value", + err: errInvalidKey, + }, + { + name: "invalid member: invalid value", + in: "foo=\\", + err: errInvalidValue, + }, + { + name: "invalid property: invalid key", + in: "foo=1;=v", + err: errInvalidProperty, + }, + { + name: "invalid property: invalid value", + in: "foo=1;key=\\", + err: errInvalidProperty, + }, + { + name: "invalid baggage string: too large", + in: tooLarge, + err: errBaggageBytes, + }, + { + name: "invalid baggage string: member too large", + in: tooLargeMember, + err: errMemberBytes, + }, + { + name: "invalid baggage string: too many members", + in: tooManyMembers, + err: errMemberNumber, + }, } - v = Value(ctx, first) - if v.Type() != attribute.BOOL || !v.AsBool() { - t.Fatal("Value failed to get correct first value") + for _, tc := range testcases { + t.Run(tc.name, func(t *testing.T) { + actual, err := Parse(tc.in) + assert.ErrorIs(t, err, tc.err) + assert.Equal(t, Baggage{list: tc.want}, actual) + }) } - v = Value(ctx, second) - if v.Type() != attribute.STRING || v.AsString() != "2" { - t.Fatal("Value failed to get correct second value") +} + +func TestBaggageString(t *testing.T) { + testcases := []struct { + name string + out string + baggage baggage.List + }{ + { + name: "empty value", + out: "", + baggage: baggage.List(nil), + }, + { + name: "single member empty value no properties", + out: "foo=", + baggage: baggage.List{ + "foo": {Value: ""}, + }, + }, + { + name: "single member no properties", + out: "foo=1", + baggage: baggage.List{ + "foo": {Value: "1"}, + }, + }, + { + name: "URL encoded value", + out: "foo=1%3D1", + baggage: baggage.List{ + "foo": {Value: "1=1"}, + }, + }, + { + name: "single member empty value with properties", + out: "foo=;red;state=on", + baggage: baggage.List{ + "foo": { + Value: "", + Properties: []baggage.Property{ + {Key: "state", Value: "on", HasValue: true}, + {Key: "red"}, + }, + }, + }, + }, + { + name: "single member with properties", + // Properties are "opaque values" meaning they are sent as they + // are set and no encoding is performed. + out: "foo=1;red;state=on;z=z=z", + baggage: baggage.List{ + "foo": { + Value: "1", + Properties: []baggage.Property{ + {Key: "state", Value: "on", HasValue: true}, + {Key: "red"}, + {Key: "z", Value: "z=z", HasValue: true}, + }, + }, + }, + }, + { + name: "two members with properties", + out: "bar=2;yellow,foo=1;red;state=on", + baggage: baggage.List{ + "foo": { + Value: "1", + Properties: []baggage.Property{ + {Key: "state", Value: "on", HasValue: true}, + {Key: "red"}, + }, + }, + "bar": { + Value: "2", + Properties: []baggage.Property{{Key: "yellow"}}, + }, + }, + }, } - ctx = ContextWithoutValues(ctx, first) - m = baggage.MapFromContext(ctx) - _, ok = m.Value(first) - if ok { - t.Fatal("WithoutValues failed to remove a baggage value") + orderer := func(s string) string { + members := strings.Split(s, listDelimiter) + for i, m := range members { + parts := strings.Split(m, propertyDelimiter) + if len(parts) > 1 { + sort.Strings(parts[1:]) + members[i] = strings.Join(parts, propertyDelimiter) + } + } + sort.Strings(members) + return strings.Join(members, listDelimiter) } - _, ok = m.Value(second) - if !ok { - t.Fatal("WithoutValues removed incorrect value") + + for _, tc := range testcases { + b := Baggage{tc.baggage} + assert.Equal(t, tc.out, orderer(b.String())) } +} + +func TestBaggageLen(t *testing.T) { + b := Baggage{} + assert.Equal(t, 0, b.Len()) + + b.list = make(baggage.List, 1) + assert.Equal(t, 0, b.Len()) + + b.list["k"] = baggage.Item{} + assert.Equal(t, 1, b.Len()) +} + +func TestBaggageDeleteMember(t *testing.T) { + key := "k" + + b0 := Baggage{} + b1 := b0.DeleteMember(key) + assert.NotContains(t, b1.list, key) + + b0 = Baggage{list: baggage.List{ + key: {}, + "other": {}, + }} + b1 = b0.DeleteMember(key) + assert.Contains(t, b0.list, key) + assert.NotContains(t, b1.list, key) +} + +func TestBaggageSetMemberError(t *testing.T) { + _, err := Baggage{}.SetMember(Member{}) + assert.ErrorIs(t, err, errInvalidMember) +} + +func TestBaggageSetMember(t *testing.T) { + b0 := Baggage{} + + key := "k" + m := Member{key: key} + b1, err := b0.SetMember(m) + assert.NoError(t, err) + assert.NotContains(t, b0.list, key) + assert.Equal(t, baggage.Item{}, b1.list[key]) + assert.Equal(t, 0, len(b0.list)) + assert.Equal(t, 1, len(b1.list)) + + m.value = "v" + b2, err := b1.SetMember(m) + assert.NoError(t, err) + assert.Equal(t, baggage.Item{}, b1.list[key]) + assert.Equal(t, baggage.Item{Value: "v"}, b2.list[key]) + assert.Equal(t, 1, len(b1.list)) + assert.Equal(t, 1, len(b2.list)) + + p := properties{{key: "p"}} + m.properties = p + b3, err := b2.SetMember(m) + assert.NoError(t, err) + assert.Equal(t, baggage.Item{Value: "v"}, b2.list[key]) + assert.Equal(t, baggage.Item{Value: "v", Properties: []baggage.Property{{Key: "p"}}}, b3.list[key]) + assert.Equal(t, 1, len(b2.list)) + assert.Equal(t, 1, len(b3.list)) - ctx = ContextWithEmpty(ctx) - m = baggage.MapFromContext(ctx) - if m.Len() != 0 { - t.Fatal("WithoutBaggage failed to clear baggage") + // The returned baggage needs to be immutable and should use a copy of the + // properties slice. + p[0] = Property{key: "different"} + assert.Equal(t, baggage.Item{Value: "v", Properties: []baggage.Property{{Key: "p"}}}, b3.list[key]) + // Reset for below. + p[0] = Property{key: "p"} + + m = Member{key: "another"} + b4, err := b3.SetMember(m) + assert.NoError(t, err) + assert.Equal(t, baggage.Item{Value: "v", Properties: []baggage.Property{{Key: "p"}}}, b3.list[key]) + assert.NotContains(t, b3.list, m.key) + assert.Equal(t, baggage.Item{Value: "v", Properties: []baggage.Property{{Key: "p"}}}, b4.list[key]) + assert.Equal(t, baggage.Item{}, b4.list[m.key]) + assert.Equal(t, 1, len(b3.list)) + assert.Equal(t, 2, len(b4.list)) +} + +func TestNilBaggageMembers(t *testing.T) { + assert.Nil(t, Baggage{}.Members()) +} + +func TestBaggageMembers(t *testing.T) { + members := []Member{ + { + key: "foo", + value: "1", + properties: properties{ + {key: "state", value: "on", hasValue: true}, + {key: "red"}, + }, + }, + { + key: "bar", + value: "2", + properties: properties{ + {key: "yellow"}, + }, + }, } + + baggage := Baggage{list: baggage.List{ + "foo": { + Value: "1", + Properties: []baggage.Property{ + {Key: "state", Value: "on", HasValue: true}, + {Key: "red"}, + }, + }, + "bar": { + Value: "2", + Properties: []baggage.Property{{Key: "yellow"}}, + }, + }} + + assert.ElementsMatch(t, members, baggage.Members()) +} + +func TestBaggageMember(t *testing.T) { + baggage := Baggage{list: baggage.List{"foo": {Value: "1"}}} + assert.Equal(t, Member{key: "foo", value: "1"}, baggage.Member("foo")) + assert.Equal(t, Member{}, baggage.Member("bar")) +} + +func TestMemberKey(t *testing.T) { + m := Member{} + assert.Equal(t, "", m.Key(), "even invalid values should be returned") + + key := "k" + m.key = key + assert.Equal(t, key, m.Key()) +} + +func TestMemberValue(t *testing.T) { + m := Member{key: "k", value: "\\"} + assert.Equal(t, "\\", m.Value(), "even invalid values should be returned") + + value := "v" + m.value = value + assert.Equal(t, value, m.Value()) +} + +func TestMemberProperties(t *testing.T) { + m := Member{key: "k", value: "v"} + assert.Nil(t, m.Properties()) + + p := []Property{{key: "foo"}} + m.properties = properties(p) + got := m.Properties() + assert.Equal(t, p, got) + + // Returned slice needs to be a copy so the original is immutable. + got[0] = Property{key: "bar"} + assert.NotEqual(t, m.properties, got) +} + +func TestMemberValidation(t *testing.T) { + m := Member{} + assert.ErrorIs(t, m.validate(), errInvalidKey) + + m.key, m.value = "k", "\\" + assert.ErrorIs(t, m.validate(), errInvalidValue) + + m.value = "v" + assert.NoError(t, m.validate()) +} + +func TestNewMember(t *testing.T) { + m, err := NewMember("", "") + assert.ErrorIs(t, err, errInvalidKey) + assert.Equal(t, Member{}, m) + + key, val := "k", "v" + p := Property{key: "foo"} + m, err = NewMember(key, val, p) + assert.NoError(t, err) + expected := Member{key: key, value: val, properties: properties{{key: "foo"}}} + assert.Equal(t, expected, m) + + // Ensure new member is immutable. + p.key = "bar" + assert.Equal(t, expected, m) +} + +func TestPropertiesValidate(t *testing.T) { + p := properties{{}} + assert.ErrorIs(t, p.validate(), errInvalidKey) + + p[0].key = "foo" + assert.NoError(t, p.validate()) + + p = append(p, Property{key: "bar"}) + assert.NoError(t, p.validate()) } diff --git a/baggage/context.go b/baggage/context.go new file mode 100644 index 00000000000..0ec4e39736a --- /dev/null +++ b/baggage/context.go @@ -0,0 +1,39 @@ +// Copyright The OpenTelemetry Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package baggage // import "go.opentelemetry.io/otel/baggage" + +import ( + "context" + + "go.opentelemetry.io/otel/internal/baggage" +) + +// ContextWithBaggage returns a copy of parent with baggage. +func ContextWithBaggage(parent context.Context, b Baggage) context.Context { + // Delegate so any hooks for the OpenTracing bridge are handled. + return baggage.ContextWithList(parent, b.list) +} + +// ContextWithBaggage returns a copy of parent with no baggage. +func ContextWithoutBaggage(parent context.Context) context.Context { + // Delegate so any hooks for the OpenTracing bridge are handled. + return baggage.ContextWithList(parent, nil) +} + +// FromContext returns the baggage contained in ctx. +func FromContext(ctx context.Context) Baggage { + // Delegate so any hooks for the OpenTracing bridge are handled. + return Baggage{list: baggage.ListFromContext(ctx)} +} diff --git a/baggage/context_test.go b/baggage/context_test.go new file mode 100644 index 00000000000..1b32acdb54d --- /dev/null +++ b/baggage/context_test.go @@ -0,0 +1,36 @@ +// Copyright The OpenTelemetry Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package baggage + +import ( + "context" + "testing" + + "github.com/stretchr/testify/assert" + + "go.opentelemetry.io/otel/internal/baggage" +) + +func TestContext(t *testing.T) { + ctx := context.Background() + assert.Equal(t, Baggage{}, FromContext(ctx)) + + b := Baggage{list: baggage.List{"key": baggage.Item{Value: "val"}}} + ctx = ContextWithBaggage(ctx, b) + assert.Equal(t, b, FromContext(ctx)) + + ctx = ContextWithoutBaggage(ctx) + assert.Equal(t, Baggage{}, FromContext(ctx)) +} diff --git a/bridge/opentracing/bridge.go b/bridge/opentracing/bridge.go index 316ddd8764e..3e931b00e29 100644 --- a/bridge/opentracing/bridge.go +++ b/bridge/opentracing/bridge.go @@ -27,16 +27,17 @@ import ( "go.opentelemetry.io/otel" "go.opentelemetry.io/otel/attribute" + "go.opentelemetry.io/otel/baggage" "go.opentelemetry.io/otel/bridge/opentracing/migration" "go.opentelemetry.io/otel/codes" - "go.opentelemetry.io/otel/internal/baggage" + iBaggage "go.opentelemetry.io/otel/internal/baggage" "go.opentelemetry.io/otel/internal/trace/noop" "go.opentelemetry.io/otel/propagation" "go.opentelemetry.io/otel/trace" ) type bridgeSpanContext struct { - baggageItems baggage.Map + bag baggage.Baggage otelSpanContext trace.SpanContext } @@ -44,7 +45,7 @@ var _ ot.SpanContext = &bridgeSpanContext{} func newBridgeSpanContext(otelSpanContext trace.SpanContext, parentOtSpanContext ot.SpanContext) *bridgeSpanContext { bCtx := &bridgeSpanContext{ - baggageItems: baggage.NewEmptyMap(), + bag: baggage.Baggage{}, otelSpanContext: otelSpanContext, } if parentOtSpanContext != nil { @@ -57,20 +58,25 @@ func newBridgeSpanContext(otelSpanContext trace.SpanContext, parentOtSpanContext } func (c *bridgeSpanContext) ForeachBaggageItem(handler func(k, v string) bool) { - c.baggageItems.Foreach(func(kv attribute.KeyValue) bool { - return handler(string(kv.Key), kv.Value.Emit()) - }) + for _, m := range c.bag.Members() { + if !handler(m.Key(), m.Value()) { + return + } + } } func (c *bridgeSpanContext) setBaggageItem(restrictedKey, value string) { crk := http.CanonicalHeaderKey(restrictedKey) - c.baggageItems = c.baggageItems.Apply(baggage.MapUpdate{SingleKV: attribute.String(crk, value)}) + m, err := baggage.NewMember(crk, value) + if err != nil { + return + } + c.bag, _ = c.bag.SetMember(m) } -func (c *bridgeSpanContext) baggageItem(restrictedKey string) string { +func (c *bridgeSpanContext) baggageItem(restrictedKey string) baggage.Member { crk := http.CanonicalHeaderKey(restrictedKey) - val, _ := c.baggageItems.Value(attribute.Key(crk)) - return val.Emit() + return c.bag.Member(crk) } type bridgeSpan struct { @@ -247,7 +253,7 @@ func (s *bridgeSpan) updateOTelContext(restrictedKey, value string) { } func (s *bridgeSpan) BaggageItem(restrictedKey string) string { - return s.ctx.baggageItem(restrictedKey) + return s.ctx.baggageItem(restrictedKey).Value() } func (s *bridgeSpan) Tracer() ot.Tracer { @@ -341,12 +347,12 @@ func (t *BridgeTracer) SetTextMapPropagator(propagator propagation.TextMapPropag } func (t *BridgeTracer) NewHookedContext(ctx context.Context) context.Context { - ctx = baggage.ContextWithSetHook(ctx, t.baggageSetHook) - ctx = baggage.ContextWithGetHook(ctx, t.baggageGetHook) + ctx = iBaggage.ContextWithSetHook(ctx, t.baggageSetHook) + ctx = iBaggage.ContextWithGetHook(ctx, t.baggageGetHook) return ctx } -func (t *BridgeTracer) baggageSetHook(ctx context.Context) context.Context { +func (t *BridgeTracer) baggageSetHook(ctx context.Context, list iBaggage.List) context.Context { span := ot.SpanFromContext(ctx) if span == nil { t.warningHandler("No active OpenTracing span, can not propagate the baggage items from OpenTelemetry context\n") @@ -357,38 +363,43 @@ func (t *BridgeTracer) baggageSetHook(ctx context.Context) context.Context { t.warningHandler("Encountered a foreign OpenTracing span, will not propagate the baggage items from OpenTelemetry context\n") return ctx } - // we clear the context only to avoid calling a get hook - // during MapFromContext, but otherwise we don't change the - // context, so we don't care about the old hooks. - clearCtx, _, _ := baggage.ContextWithNoHooks(ctx) - m := baggage.MapFromContext(clearCtx) - m.Foreach(func(kv attribute.KeyValue) bool { - bSpan.setBaggageItemOnly(string(kv.Key), kv.Value.Emit()) - return true - }) + for k, v := range list { + bSpan.setBaggageItemOnly(k, v.Value) + } return ctx } -func (t *BridgeTracer) baggageGetHook(ctx context.Context, m baggage.Map) baggage.Map { +func (t *BridgeTracer) baggageGetHook(ctx context.Context, list iBaggage.List) iBaggage.List { span := ot.SpanFromContext(ctx) if span == nil { t.warningHandler("No active OpenTracing span, can not propagate the baggage items from OpenTracing span context\n") - return m + return list } bSpan, ok := span.(*bridgeSpan) if !ok { t.warningHandler("Encountered a foreign OpenTracing span, will not propagate the baggage items from OpenTracing span context\n") - return m + return list } items := bSpan.extraBaggageItems if len(items) == 0 { - return m + return list } - kv := make([]attribute.KeyValue, 0, len(items)) + + // Privilege of using the internal representation of Baggage here comes + // with the responsibility to make sure we maintain its immutability. We + // need to return a copy to ensure this. + + merged := make(iBaggage.List, len(list)) + for k, v := range list { + merged[k] = v + } + for k, v := range items { - kv = append(kv, attribute.String(k, v)) + // Overwrite according to OpenTelemetry specification. + merged[k] = iBaggage.Item{Value: v} } - return m.Apply(baggage.MapUpdate{MultiKV: kv}) + + return merged } // StartSpan is a part of the implementation of the OpenTracing Tracer @@ -634,7 +645,7 @@ func (t *BridgeTracer) Inject(sm ot.SpanContext, format interface{}, carrier int sc: bridgeSC.otelSpanContext, } ctx := trace.ContextWithSpan(context.Background(), fs) - ctx = baggage.ContextWithMap(ctx, bridgeSC.baggageItems) + ctx = baggage.ContextWithBaggage(ctx, bridgeSC.bag) t.getPropagator().Inject(ctx, propagation.HeaderCarrier(header)) return nil } @@ -653,9 +664,9 @@ func (t *BridgeTracer) Extract(format interface{}, carrier interface{}) (ot.Span } header := http.Header(hhcarrier) ctx := t.getPropagator().Extract(context.Background(), propagation.HeaderCarrier(header)) - baggage := baggage.MapFromContext(ctx) + baggage := baggage.FromContext(ctx) bridgeSC := &bridgeSpanContext{ - baggageItems: baggage, + bag: baggage, otelSpanContext: trace.SpanContextFromContext(ctx), } if !bridgeSC.otelSpanContext.IsValid() { diff --git a/bridge/opentracing/internal/mock.go b/bridge/opentracing/internal/mock.go index 11198805063..0d933686cd5 100644 --- a/bridge/opentracing/internal/mock.go +++ b/bridge/opentracing/internal/mock.go @@ -23,7 +23,6 @@ import ( "go.opentelemetry.io/otel/attribute" "go.opentelemetry.io/otel/codes" - "go.opentelemetry.io/otel/internal/baggage" "go.opentelemetry.io/otel/semconv" "go.opentelemetry.io/otel/trace" @@ -45,7 +44,6 @@ type MockContextKeyValue struct { } type MockTracer struct { - Resources baggage.Map FinishedSpans []*MockSpan SpareTraceIDs []trace.TraceID SpareSpanIDs []trace.SpanID @@ -60,7 +58,6 @@ var _ migration.DeferredContextSetupTracerExtension = &MockTracer{} func NewMockTracer() *MockTracer { return &MockTracer{ - Resources: baggage.NewEmptyMap(), FinishedSpans: nil, SpareTraceIDs: nil, SpareSpanIDs: nil, @@ -85,14 +82,12 @@ func (t *MockTracer) Start(ctx context.Context, name string, opts ...trace.SpanS mockTracer: t, officialTracer: t, spanContext: spanContext, - Attributes: baggage.NewMap(baggage.MapUpdate{ - MultiKV: config.Attributes(), - }), - StartTime: startTime, - EndTime: time.Time{}, - ParentSpanID: t.getParentSpanID(ctx, config), - Events: nil, - SpanKind: trace.ValidateSpanKind(config.SpanKind()), + Attributes: config.Attributes(), + StartTime: startTime, + EndTime: time.Time{}, + ParentSpanID: t.getParentSpanID(ctx, config), + Events: nil, + SpanKind: trace.ValidateSpanKind(config.SpanKind()), } if !migration.SkipContextSetup(ctx) { ctx = trace.ContextWithSpan(ctx, span) @@ -182,7 +177,7 @@ func (t *MockTracer) DeferredContextSetupHook(ctx context.Context, span trace.Sp type MockEvent struct { Timestamp time.Time Name string - Attributes baggage.Map + Attributes []attribute.KeyValue } type MockSpan struct { @@ -192,7 +187,7 @@ type MockSpan struct { SpanKind trace.SpanKind recording bool - Attributes baggage.Map + Attributes []attribute.KeyValue StartTime time.Time EndTime time.Time ParentSpanID trace.SpanID @@ -223,13 +218,29 @@ func (s *MockSpan) SetError(v bool) { } func (s *MockSpan) SetAttributes(attributes ...attribute.KeyValue) { - s.applyUpdate(baggage.MapUpdate{ - MultiKV: attributes, - }) + s.applyUpdate(attributes) } -func (s *MockSpan) applyUpdate(update baggage.MapUpdate) { - s.Attributes = s.Attributes.Apply(update) +func (s *MockSpan) applyUpdate(update []attribute.KeyValue) { + updateM := make(map[attribute.Key]attribute.Value, len(update)) + for _, kv := range update { + updateM[kv.Key] = kv.Value + } + + seen := make(map[attribute.Key]struct{}) + for i, kv := range s.Attributes { + if v, ok := updateM[kv.Key]; ok { + s.Attributes[i].Value = v + seen[kv.Key] = struct{}{} + } + } + + for k, v := range updateM { + if _, ok := seen[k]; ok { + continue + } + s.Attributes = append(s.Attributes, attribute.KeyValue{Key: k, Value: v}) + } } func (s *MockSpan) End(options ...trace.SpanEndOption) { @@ -269,11 +280,9 @@ func (s *MockSpan) Tracer() trace.Tracer { func (s *MockSpan) AddEvent(name string, o ...trace.EventOption) { c := trace.NewEventConfig(o...) s.Events = append(s.Events, MockEvent{ - Timestamp: c.Timestamp(), - Name: name, - Attributes: baggage.NewMap(baggage.MapUpdate{ - MultiKV: c.Attributes(), - }), + Timestamp: c.Timestamp(), + Name: name, + Attributes: c.Attributes(), }) } diff --git a/bridge/opentracing/mix_test.go b/bridge/opentracing/mix_test.go index 8dc9445b688..5324274bb5a 100644 --- a/bridge/opentracing/mix_test.go +++ b/bridge/opentracing/mix_test.go @@ -23,7 +23,7 @@ import ( "go.opentelemetry.io/otel" "go.opentelemetry.io/otel/attribute" - otelbaggage "go.opentelemetry.io/otel/internal/baggage" + "go.opentelemetry.io/otel/baggage" "go.opentelemetry.io/otel/trace" "go.opentelemetry.io/otel/bridge/opentracing/internal" @@ -515,7 +515,18 @@ func (bio *baggageInteroperationTest) addAndRecordBaggage(t *testing.T, ctx cont value := bio.baggageItems[idx].value otSpan.SetBaggageItem(otKey, value) - ctx = otelbaggage.NewContext(ctx, attribute.String(otelKey, value)) + + m, err := baggage.NewMember(otelKey, value) + if err != nil { + t.Error(err) + return ctx + } + b, err := baggage.FromContext(ctx).SetMember(m) + if err != nil { + t.Error(err) + return ctx + } + ctx = baggage.ContextWithBaggage(ctx, b) otRecording := make(map[string]string) otSpan.Context().ForeachBaggageItem(func(key, value string) bool { @@ -523,10 +534,9 @@ func (bio *baggageInteroperationTest) addAndRecordBaggage(t *testing.T, ctx cont return true }) otelRecording := make(map[string]string) - otelbaggage.MapFromContext(ctx).Foreach(func(kv attribute.KeyValue) bool { - otelRecording[string(kv.Key)] = kv.Value.Emit() - return true - }) + for _, m := range baggage.FromContext(ctx).Members() { + otelRecording[m.Key()] = m.Value() + } bio.recordedOTBaggage = append(bio.recordedOTBaggage, otRecording) bio.recordedOtelBaggage = append(bio.recordedOtelBaggage, otelRecording) return ctx diff --git a/example/namedtracer/main.go b/example/namedtracer/main.go index 0ea4669a846..37063a4e550 100644 --- a/example/namedtracer/main.go +++ b/example/namedtracer/main.go @@ -59,7 +59,11 @@ func main() { tracer := tp.Tracer("example/namedtracer/main") ctx := context.Background() defer func() { _ = tp.Shutdown(ctx) }() - ctx = baggage.ContextWithValues(ctx, fooKey.String("foo1"), barKey.String("bar1")) + + m0, _ := baggage.NewMember(string(fooKey), "foo1") + m1, _ := baggage.NewMember(string(barKey), "bar1") + b, _ := baggage.New(m0, m1) + ctx = baggage.ContextWithBaggage(ctx, b) var span trace.Span ctx, span = tracer.Start(ctx, "operation") diff --git a/internal/baggage/baggage.go b/internal/baggage/baggage.go index b1f7daf8d86..87d0af4054c 100644 --- a/internal/baggage/baggage.go +++ b/internal/baggage/baggage.go @@ -12,327 +12,32 @@ // See the License for the specific language governing permissions and // limitations under the License. -// Package baggage provides types and functions to manage W3C Baggage. +/* +Package baggage provides base types and functionality to store and retrieve +baggage in Go context. This package exists because the OpenTracing bridge to +OpenTelemetry needs to synchronize state whenever baggage for a context is +modified and that context contains an OpenTracing span. If it were not for +this need this package would not need to exist and the +`go.opentelemetry.io/otel/baggage` package would be the singular place where +W3C baggage is handled. +*/ package baggage -import ( - "context" +// List is the collection of baggage members. The W3C allows for duplicates, +// but OpenTelemetry does not, therefore, this is represented as a map. +type List map[string]Item - "go.opentelemetry.io/otel/attribute" -) - -type rawMap map[attribute.Key]attribute.Value -type keySet map[attribute.Key]struct{} - -// Map is an immutable storage for correlations. -type Map struct { - m rawMap -} - -// MapUpdate contains information about correlation changes to be -// made. -type MapUpdate struct { - // DropSingleK contains a single key to be dropped from - // correlations. Use this to avoid an overhead of a slice - // allocation if there is only one key to drop. - DropSingleK attribute.Key - // DropMultiK contains all the keys to be dropped from - // correlations. - DropMultiK []attribute.Key - - // SingleKV contains a single key-value pair to be added to - // correlations. Use this to avoid an overhead of a slice - // allocation if there is only one key-value pair to add. - SingleKV attribute.KeyValue - // MultiKV contains all the key-value pairs to be added to - // correlations. - MultiKV []attribute.KeyValue -} - -func newMap(raw rawMap) Map { - return Map{ - m: raw, - } -} - -// NewEmptyMap creates an empty correlations map. -func NewEmptyMap() Map { - return newMap(nil) -} - -// NewMap creates a map with the contents of the update applied. In -// this function, having an update with DropSingleK or DropMultiK -// makes no sense - those fields are effectively ignored. -func NewMap(update MapUpdate) Map { - return NewEmptyMap().Apply(update) -} - -// Apply creates a copy of the map with the contents of the update -// applied. Apply will first drop the keys from DropSingleK and -// DropMultiK, then add key-value pairs from SingleKV and MultiKV. -func (m Map) Apply(update MapUpdate) Map { - delSet, addSet := getModificationSets(update) - mapSize := getNewMapSize(m.m, delSet, addSet) - - r := make(rawMap, mapSize) - for k, v := range m.m { - // do not copy items we want to drop - if _, ok := delSet[k]; ok { - continue - } - // do not copy items we would overwrite - if _, ok := addSet[k]; ok { - continue - } - r[k] = v - } - if update.SingleKV.Key.Defined() { - r[update.SingleKV.Key] = update.SingleKV.Value - } - for _, kv := range update.MultiKV { - r[kv.Key] = kv.Value - } - if len(r) == 0 { - r = nil - } - return newMap(r) -} - -func getModificationSets(update MapUpdate) (delSet, addSet keySet) { - deletionsCount := len(update.DropMultiK) - if update.DropSingleK.Defined() { - deletionsCount++ - } - if deletionsCount > 0 { - delSet = make(map[attribute.Key]struct{}, deletionsCount) - for _, k := range update.DropMultiK { - delSet[k] = struct{}{} - } - if update.DropSingleK.Defined() { - delSet[update.DropSingleK] = struct{}{} - } - } - - additionsCount := len(update.MultiKV) - if update.SingleKV.Key.Defined() { - additionsCount++ - } - if additionsCount > 0 { - addSet = make(map[attribute.Key]struct{}, additionsCount) - for _, k := range update.MultiKV { - addSet[k.Key] = struct{}{} - } - if update.SingleKV.Key.Defined() { - addSet[update.SingleKV.Key] = struct{}{} - } - } - - return -} - -func getNewMapSize(m rawMap, delSet, addSet keySet) int { - mapSizeDiff := 0 - for k := range addSet { - if _, ok := m[k]; !ok { - mapSizeDiff++ - } - } - for k := range delSet { - if _, ok := m[k]; ok { - if _, inAddSet := addSet[k]; !inAddSet { - mapSizeDiff-- - } - } - } - return len(m) + mapSizeDiff -} - -// Value gets a value from correlations map and returns a boolean -// value indicating whether the key exist in the map. -func (m Map) Value(k attribute.Key) (attribute.Value, bool) { - value, ok := m.m[k] - return value, ok -} - -// HasValue returns a boolean value indicating whether the key exist -// in the map. -func (m Map) HasValue(k attribute.Key) bool { - _, has := m.Value(k) - return has -} - -// Len returns a length of the map. -func (m Map) Len() int { - return len(m.m) -} - -// Foreach calls a passed callback once on each key-value pair until -// all the key-value pairs of the map were iterated or the callback -// returns false, whichever happens first. -func (m Map) Foreach(f func(attribute.KeyValue) bool) { - for k, v := range m.m { - if !f(attribute.KeyValue{ - Key: k, - Value: v, - }) { - return - } - } -} - -type correlationsType struct{} - -// SetHookFunc describes a type of a callback that is called when -// storing baggage in the context. -type SetHookFunc func(context.Context) context.Context - -// GetHookFunc describes a type of a callback that is called when -// getting baggage from the context. -type GetHookFunc func(context.Context, Map) Map - -// value under this key is either of type Map or correlationsData -var correlationsKey = &correlationsType{} - -type correlationsData struct { - m Map - setHook SetHookFunc - getHook GetHookFunc -} - -func (d correlationsData) isHookless() bool { - return d.setHook == nil && d.getHook == nil -} - -type hookKind int - -const ( - hookKindSet hookKind = iota - hookKindGet -) - -func (d *correlationsData) overrideHook(kind hookKind, setHook SetHookFunc, getHook GetHookFunc) { - switch kind { - case hookKindSet: - d.setHook = setHook - case hookKindGet: - d.getHook = getHook - } -} - -// ContextWithSetHook installs a hook function that will be invoked -// every time ContextWithMap is called. To avoid unnecessary callback -// invocations (recursive or not), the callback can temporarily clear -// the hooks from the context with the ContextWithNoHooks function. -// -// Note that NewContext also calls ContextWithMap, so the hook will be -// invoked. -// -// Passing nil SetHookFunc creates a context with no set hook to call. -// -// This function should not be used by applications or libraries. It -// is mostly for interoperation with other observability APIs. -func ContextWithSetHook(ctx context.Context, hook SetHookFunc) context.Context { - return contextWithHook(ctx, hookKindSet, hook, nil) +// Item is the value and metadata properties part of a list-member. +type Item struct { + Value string + Properties []Property } -// ContextWithGetHook installs a hook function that will be invoked -// every time MapFromContext is called. To avoid unnecessary callback -// invocations (recursive or not), the callback can temporarily clear -// the hooks from the context with the ContextWithNoHooks function. -// -// Note that NewContext also calls MapFromContext, so the hook will be -// invoked. -// -// Passing nil GetHookFunc creates a context with no get hook to call. -// -// This function should not be used by applications or libraries. It -// is mostly for interoperation with other observability APIs. -func ContextWithGetHook(ctx context.Context, hook GetHookFunc) context.Context { - return contextWithHook(ctx, hookKindGet, nil, hook) -} - -func contextWithHook(ctx context.Context, kind hookKind, setHook SetHookFunc, getHook GetHookFunc) context.Context { - switch v := ctx.Value(correlationsKey).(type) { - case correlationsData: - v.overrideHook(kind, setHook, getHook) - if v.isHookless() { - return context.WithValue(ctx, correlationsKey, v.m) - } - return context.WithValue(ctx, correlationsKey, v) - case Map: - return contextWithOneHookAndMap(ctx, kind, setHook, getHook, v) - default: - m := NewEmptyMap() - return contextWithOneHookAndMap(ctx, kind, setHook, getHook, m) - } -} - -func contextWithOneHookAndMap(ctx context.Context, kind hookKind, setHook SetHookFunc, getHook GetHookFunc, m Map) context.Context { - d := correlationsData{m: m} - d.overrideHook(kind, setHook, getHook) - if d.isHookless() { - return ctx - } - return context.WithValue(ctx, correlationsKey, d) -} - -// ContextWithNoHooks creates a context with all the hooks -// disabled. Also returns old set and get hooks. This function can be -// used to temporarily clear the context from hooks and then reinstate -// them by calling ContextWithSetHook and ContextWithGetHook functions -// passing the hooks returned by this function. -// -// This function should not be used by applications or libraries. It -// is mostly for interoperation with other observability APIs. -func ContextWithNoHooks(ctx context.Context) (context.Context, SetHookFunc, GetHookFunc) { - switch v := ctx.Value(correlationsKey).(type) { - case correlationsData: - return context.WithValue(ctx, correlationsKey, v.m), v.setHook, v.getHook - default: - return ctx, nil, nil - } -} - -// ContextWithMap returns a context with the Map entered into it. -func ContextWithMap(ctx context.Context, m Map) context.Context { - switch v := ctx.Value(correlationsKey).(type) { - case correlationsData: - v.m = m - ctx = context.WithValue(ctx, correlationsKey, v) - if v.setHook != nil { - ctx = v.setHook(ctx) - } - return ctx - default: - return context.WithValue(ctx, correlationsKey, m) - } -} - -// ContextWithNoCorrelationData returns a context stripped of correlation -// data. -func ContextWithNoCorrelationData(ctx context.Context) context.Context { - return context.WithValue(ctx, correlationsKey, nil) -} - -// NewContext returns a context with the map from passed context -// updated with the passed key-value pairs. -func NewContext(ctx context.Context, keyvalues ...attribute.KeyValue) context.Context { - return ContextWithMap(ctx, MapFromContext(ctx).Apply(MapUpdate{ - MultiKV: keyvalues, - })) -} +// Property is a metadata entry for a list-member. +type Property struct { + Key, Value string -// MapFromContext gets the current Map from a Context. -func MapFromContext(ctx context.Context) Map { - switch v := ctx.Value(correlationsKey).(type) { - case correlationsData: - if v.getHook != nil { - return v.getHook(ctx, v.m) - } - return v.m - case Map: - return v - default: - return NewEmptyMap() - } + // HasValue indicates if a zero-value value means the property does not + // have a value or if it was the zero-value. + HasValue bool } diff --git a/internal/baggage/baggage_test.go b/internal/baggage/baggage_test.go deleted file mode 100644 index 8c164a5c5cf..00000000000 --- a/internal/baggage/baggage_test.go +++ /dev/null @@ -1,285 +0,0 @@ -// Copyright The OpenTelemetry Authors -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -package baggage - -import ( - "fmt" - "testing" - - "go.opentelemetry.io/otel/attribute" -) - -type testCase struct { - name string - value MapUpdate - init []int - wantKVs []attribute.KeyValue -} - -func TestMap(t *testing.T) { - for _, testcase := range getTestCases() { - t.Logf("Running test case %s", testcase.name) - var got Map - if len(testcase.init) > 0 { - got = makeTestMap(testcase.init).Apply(testcase.value) - } else { - got = NewMap(testcase.value) - } - for _, s := range testcase.wantKVs { - if ok := got.HasValue(s.Key); !ok { - t.Errorf("Expected Key %s to have Value", s.Key) - } - if g, ok := got.Value(s.Key); !ok || g != s.Value { - t.Errorf("+got: %v, -want: %v", g, s.Value) - } - } - // test Foreach() - got.Foreach(func(kv attribute.KeyValue) bool { - for _, want := range testcase.wantKVs { - if kv == want { - return false - } - } - t.Errorf("Expected label %v, but not found", kv) - return true - }) - if l, exp := got.Len(), len(testcase.wantKVs); l != exp { - t.Errorf("+got: %d, -want: %d", l, exp) - } - } -} - -func TestSizeComputation(t *testing.T) { - for _, testcase := range getTestCases() { - t.Logf("Running test case %s", testcase.name) - var initMap Map - if len(testcase.init) > 0 { - initMap = makeTestMap(testcase.init) - } else { - initMap = NewEmptyMap() - } - gotMap := initMap.Apply(testcase.value) - - delSet, addSet := getModificationSets(testcase.value) - mapSize := getNewMapSize(initMap.m, delSet, addSet) - - if gotMap.Len() != mapSize { - t.Errorf("Expected computed size to be %d, got %d", gotMap.Len(), mapSize) - } - } -} - -func getTestCases() []testCase { - return []testCase{ - { - name: "map with MultiKV", - value: MapUpdate{MultiKV: []attribute.KeyValue{ - attribute.Int64("key1", 1), - attribute.String("key2", "val2")}, - }, - init: []int{}, - wantKVs: []attribute.KeyValue{ - attribute.Int64("key1", 1), - attribute.String("key2", "val2"), - }, - }, - { - name: "map with SingleKV", - value: MapUpdate{SingleKV: attribute.String("key1", "val1")}, - init: []int{}, - wantKVs: []attribute.KeyValue{ - attribute.String("key1", "val1"), - }, - }, - { - name: "map with both add fields", - value: MapUpdate{SingleKV: attribute.Int64("key1", 3), - MultiKV: []attribute.KeyValue{ - attribute.String("key1", ""), - attribute.String("key2", "val2")}, - }, - init: []int{}, - wantKVs: []attribute.KeyValue{ - attribute.String("key1", ""), - attribute.String("key2", "val2"), - }, - }, - { - name: "map with empty MapUpdate", - value: MapUpdate{}, - init: []int{}, - wantKVs: []attribute.KeyValue{}, - }, - { - name: "map with DropSingleK", - value: MapUpdate{DropSingleK: attribute.Key("key1")}, - init: []int{}, - wantKVs: []attribute.KeyValue{}, - }, - { - name: "map with DropMultiK", - value: MapUpdate{DropMultiK: []attribute.Key{ - attribute.Key("key1"), attribute.Key("key2"), - }}, - init: []int{}, - wantKVs: []attribute.KeyValue{}, - }, - { - name: "map with both drop fields", - value: MapUpdate{ - DropSingleK: attribute.Key("key1"), - DropMultiK: []attribute.Key{ - attribute.Key("key1"), - attribute.Key("key2"), - }, - }, - init: []int{}, - wantKVs: []attribute.KeyValue{}, - }, - { - name: "map with all fields", - value: MapUpdate{ - DropSingleK: attribute.Key("key1"), - DropMultiK: []attribute.Key{ - attribute.Key("key1"), - attribute.Key("key2"), - }, - SingleKV: attribute.String("key4", "val4"), - MultiKV: []attribute.KeyValue{ - attribute.String("key1", ""), - attribute.String("key2", "val2"), - attribute.String("key3", "val3"), - }, - }, - init: []int{}, - wantKVs: []attribute.KeyValue{ - attribute.String("key1", ""), - attribute.String("key2", "val2"), - attribute.String("key3", "val3"), - attribute.String("key4", "val4"), - }, - }, - { - name: "Existing map with MultiKV", - value: MapUpdate{MultiKV: []attribute.KeyValue{ - attribute.Int64("key1", 1), - attribute.String("key2", "val2")}, - }, - init: []int{5}, - wantKVs: []attribute.KeyValue{ - attribute.Int64("key1", 1), - attribute.String("key2", "val2"), - attribute.Int("key5", 5), - }, - }, - { - name: "Existing map with SingleKV", - value: MapUpdate{SingleKV: attribute.String("key1", "val1")}, - init: []int{5}, - wantKVs: []attribute.KeyValue{ - attribute.String("key1", "val1"), - attribute.Int("key5", 5), - }, - }, - { - name: "Existing map with both add fields", - value: MapUpdate{SingleKV: attribute.Int64("key1", 3), - MultiKV: []attribute.KeyValue{ - attribute.String("key1", ""), - attribute.String("key2", "val2")}, - }, - init: []int{5}, - wantKVs: []attribute.KeyValue{ - attribute.String("key1", ""), - attribute.String("key2", "val2"), - attribute.Int("key5", 5), - }, - }, - { - name: "Existing map with empty MapUpdate", - value: MapUpdate{}, - init: []int{5}, - wantKVs: []attribute.KeyValue{ - attribute.Int("key5", 5), - }, - }, - { - name: "Existing map with DropSingleK", - value: MapUpdate{DropSingleK: attribute.Key("key1")}, - init: []int{1, 5}, - wantKVs: []attribute.KeyValue{ - attribute.Int("key5", 5), - }, - }, - { - name: "Existing map with DropMultiK", - value: MapUpdate{DropMultiK: []attribute.Key{ - attribute.Key("key1"), attribute.Key("key2"), - }}, - init: []int{1, 5}, - wantKVs: []attribute.KeyValue{ - attribute.Int("key5", 5), - }, - }, - { - name: "Existing map with both drop fields", - value: MapUpdate{ - DropSingleK: attribute.Key("key1"), - DropMultiK: []attribute.Key{ - attribute.Key("key1"), - attribute.Key("key2"), - }, - }, - init: []int{1, 2, 5}, - wantKVs: []attribute.KeyValue{ - attribute.Int("key5", 5), - }, - }, - { - name: "Existing map with all the fields", - value: MapUpdate{ - DropSingleK: attribute.Key("key1"), - DropMultiK: []attribute.Key{ - attribute.Key("key1"), - attribute.Key("key2"), - attribute.Key("key5"), - attribute.Key("key6"), - }, - SingleKV: attribute.String("key4", "val4"), - MultiKV: []attribute.KeyValue{ - attribute.String("key1", ""), - attribute.String("key2", "val2"), - attribute.String("key3", "val3"), - }, - }, - init: []int{5, 6, 7}, - wantKVs: []attribute.KeyValue{ - attribute.String("key1", ""), - attribute.String("key2", "val2"), - attribute.String("key3", "val3"), - attribute.String("key4", "val4"), - attribute.Int("key7", 7), - }, - }, - } -} - -func makeTestMap(ints []int) Map { - r := make(rawMap, len(ints)) - for _, v := range ints { - r[attribute.Key(fmt.Sprintf("key%d", v))] = attribute.IntValue(v) - } - return newMap(r) -} diff --git a/internal/baggage/context.go b/internal/baggage/context.go new file mode 100644 index 00000000000..7d8694e8c68 --- /dev/null +++ b/internal/baggage/context.go @@ -0,0 +1,95 @@ +// Copyright The OpenTelemetry Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package baggage + +import "context" + +type baggageContextKeyType int + +const baggageKey baggageContextKeyType = iota + +// SetHookFunc is a callback called when storing baggage in the context. +type SetHookFunc func(context.Context, List) context.Context + +// GetHookFunc is a callback called when getting baggage from the context. +type GetHookFunc func(context.Context, List) List + +type baggageState struct { + list List + + setHook SetHookFunc + getHook GetHookFunc +} + +// ContextWithSetHook returns a copy of parent with hook configured to be +// invoked every time ContextWithBaggage is called. +// +// Passing nil SetHookFunc creates a context with no set hook to call. +func ContextWithSetHook(parent context.Context, hook SetHookFunc) context.Context { + var s baggageState + switch v := parent.Value(baggageKey).(type) { + case baggageState: + s = v + } + + s.setHook = hook + return context.WithValue(parent, baggageKey, s) +} + +// ContextWithGetHook returns a copy of parent with hook configured to be +// invoked every time FromContext is called. +// +// Passing nil GetHookFunc creates a context with no get hook to call. +func ContextWithGetHook(parent context.Context, hook GetHookFunc) context.Context { + var s baggageState + switch v := parent.Value(baggageKey).(type) { + case baggageState: + s = v + } + + s.getHook = hook + return context.WithValue(parent, baggageKey, s) +} + +// ContextWithList returns a copy of parent with baggage. Passing nil list +// returns a context without any baggage. +func ContextWithList(parent context.Context, list List) context.Context { + var s baggageState + switch v := parent.Value(baggageKey).(type) { + case baggageState: + s = v + } + + s.list = list + ctx := context.WithValue(parent, baggageKey, s) + if s.setHook != nil { + ctx = s.setHook(ctx, list) + } + + return ctx +} + +// ListFromContext returns the baggage contained in ctx. +func ListFromContext(ctx context.Context) List { + switch v := ctx.Value(baggageKey).(type) { + case baggageState: + if v.getHook != nil { + return v.getHook(ctx, v.list) + } + return v.list + default: + return nil + } +} diff --git a/internal/baggage/context_test.go b/internal/baggage/context_test.go new file mode 100644 index 00000000000..ba98211232c --- /dev/null +++ b/internal/baggage/context_test.go @@ -0,0 +1,96 @@ +// Copyright The OpenTelemetry Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package baggage + +import ( + "context" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestContextWithList(t *testing.T) { + ctx := context.Background() + l := List{"foo": {Value: "1"}} + + nCtx := ContextWithList(ctx, l) + assert.Equal(t, baggageState{list: l}, nCtx.Value(baggageKey)) + assert.Nil(t, ctx.Value(baggageKey)) +} + +func TestClearContextOfList(t *testing.T) { + l := List{"foo": {Value: "1"}} + + ctx := context.Background() + ctx = context.WithValue(ctx, baggageKey, l) + + nCtx := ContextWithList(ctx, nil) + nL, ok := nCtx.Value(baggageKey).(baggageState) + require.True(t, ok, "wrong type stored in context") + assert.Nil(t, nL.list) + assert.Equal(t, l, ctx.Value(baggageKey)) +} + +func TestListFromContext(t *testing.T) { + ctx := context.Background() + assert.Nil(t, ListFromContext(ctx)) + + l := List{"foo": {Value: "1"}} + ctx = context.WithValue(ctx, baggageKey, baggageState{list: l}) + assert.Equal(t, l, ListFromContext(ctx)) +} + +func TestContextWithSetHook(t *testing.T) { + var called bool + f := func(ctx context.Context, list List) context.Context { + called = true + return ctx + } + + ctx := context.Background() + ctx = ContextWithSetHook(ctx, f) + assert.False(t, called, "SetHookFunc called when setting hook") + ctx = ContextWithList(ctx, nil) + assert.True(t, called, "SetHookFunc not called when setting List") + + // Ensure resetting the hook works. + called = false + ctx = ContextWithSetHook(ctx, f) + assert.False(t, called, "SetHookFunc called when re-setting hook") + ContextWithList(ctx, nil) + assert.True(t, called, "SetHookFunc not called when re-setting List") +} + +func TestContextWithGetHook(t *testing.T) { + var called bool + f := func(ctx context.Context, list List) List { + called = true + return list + } + + ctx := context.Background() + ctx = ContextWithGetHook(ctx, f) + assert.False(t, called, "GetHookFunc called when setting hook") + _ = ListFromContext(ctx) + assert.True(t, called, "GetHookFunc not called when getting List") + + // Ensure resetting the hook works. + called = false + ctx = ContextWithGetHook(ctx, f) + assert.False(t, called, "GetHookFunc called when re-setting hook") + _ = ListFromContext(ctx) + assert.True(t, called, "GetHookFunc not called when re-getting List") +} diff --git a/propagation/baggage.go b/propagation/baggage.go index bc76191892e..16afed5319b 100644 --- a/propagation/baggage.go +++ b/propagation/baggage.go @@ -16,11 +16,8 @@ package propagation // import "go.opentelemetry.io/otel/propagation" import ( "context" - "net/url" - "strings" - "go.opentelemetry.io/otel/attribute" - "go.opentelemetry.io/otel/internal/baggage" + "go.opentelemetry.io/otel/baggage" ) const baggageHeader = "baggage" @@ -35,74 +32,24 @@ var _ TextMapPropagator = Baggage{} // Inject sets baggage key-values from ctx into the carrier. func (b Baggage) Inject(ctx context.Context, carrier TextMapCarrier) { - baggageMap := baggage.MapFromContext(ctx) - firstIter := true - var headerValueBuilder strings.Builder - baggageMap.Foreach(func(kv attribute.KeyValue) bool { - if !firstIter { - headerValueBuilder.WriteRune(',') - } - firstIter = false - headerValueBuilder.WriteString(url.QueryEscape(strings.TrimSpace((string)(kv.Key)))) - headerValueBuilder.WriteRune('=') - headerValueBuilder.WriteString(url.QueryEscape(strings.TrimSpace(kv.Value.Emit()))) - return true - }) - if headerValueBuilder.Len() > 0 { - headerString := headerValueBuilder.String() - carrier.Set(baggageHeader, headerString) + bStr := baggage.FromContext(ctx).String() + if bStr != "" { + carrier.Set(baggageHeader, bStr) } } // Extract returns a copy of parent with the baggage from the carrier added. func (b Baggage) Extract(parent context.Context, carrier TextMapCarrier) context.Context { - bVal := carrier.Get(baggageHeader) - if bVal == "" { + bStr := carrier.Get(baggageHeader) + if bStr == "" { return parent } - baggageValues := strings.Split(bVal, ",") - keyValues := make([]attribute.KeyValue, 0, len(baggageValues)) - for _, baggageValue := range baggageValues { - valueAndProps := strings.Split(baggageValue, ";") - if len(valueAndProps) < 1 { - continue - } - nameValue := strings.Split(valueAndProps[0], "=") - if len(nameValue) < 2 { - continue - } - name, err := url.QueryUnescape(nameValue[0]) - if err != nil { - continue - } - trimmedName := strings.TrimSpace(name) - value, err := url.QueryUnescape(nameValue[1]) - if err != nil { - continue - } - trimmedValue := strings.TrimSpace(value) - - // TODO (skaris): properties defiend https://w3c.github.io/correlation-context/, are currently - // just put as part of the value. - var trimmedValueWithProps strings.Builder - trimmedValueWithProps.WriteString(trimmedValue) - for _, prop := range valueAndProps[1:] { - trimmedValueWithProps.WriteRune(';') - trimmedValueWithProps.WriteString(prop) - } - - keyValues = append(keyValues, attribute.String(trimmedName, trimmedValueWithProps.String())) - } - - if len(keyValues) > 0 { - // Only update the context if valid values were found - return baggage.ContextWithMap(parent, baggage.NewMap(baggage.MapUpdate{ - MultiKV: keyValues, - })) + bag, err := baggage.Parse(bStr) + if err != nil { + return parent } - - return parent + return baggage.ContextWithBaggage(parent, bag) } // Fields returns the keys who's values are set with Inject. diff --git a/propagation/baggage_test.go b/propagation/baggage_test.go index f9749d620df..299e009e3cf 100644 --- a/propagation/baggage_test.go +++ b/propagation/baggage_test.go @@ -21,65 +21,101 @@ import ( "testing" "github.com/google/go-cmp/cmp" + "github.com/stretchr/testify/assert" - "go.opentelemetry.io/otel/attribute" - "go.opentelemetry.io/otel/internal/baggage" + "go.opentelemetry.io/otel/baggage" "go.opentelemetry.io/otel/propagation" ) +type property struct { + Key, Value string +} + +type member struct { + Key, Value string + + Properties []property +} + +func (m member) Member(t *testing.T) baggage.Member { + props := make([]baggage.Property, 0, len(m.Properties)) + for _, p := range m.Properties { + p, err := baggage.NewKeyValueProperty(p.Key, p.Value) + if err != nil { + t.Fatal(err) + } + props = append(props, p) + } + + bMember, err := baggage.NewMember(m.Key, m.Value, props...) + if err != nil { + t.Fatal(err) + } + return bMember +} + +type members []member + +func (m members) Baggage(t *testing.T) baggage.Baggage { + bMembers := make([]baggage.Member, 0, len(m)) + for _, mem := range m { + bMembers = append(bMembers, mem.Member(t)) + } + bag, err := baggage.New(bMembers...) + if err != nil { + t.Fatal(err) + } + return bag +} + func TestExtractValidBaggageFromHTTPReq(t *testing.T) { prop := propagation.TextMapPropagator(propagation.Baggage{}) tests := []struct { - name string - header string - wantKVs []attribute.KeyValue + name string + header string + want members }{ { name: "valid w3cHeader", header: "key1=val1,key2=val2", - wantKVs: []attribute.KeyValue{ - attribute.String("key1", "val1"), - attribute.String("key2", "val2"), + want: members{ + {Key: "key1", Value: "val1"}, + {Key: "key2", Value: "val2"}, }, }, { name: "valid w3cHeader with spaces", header: "key1 = val1, key2 =val2 ", - wantKVs: []attribute.KeyValue{ - attribute.String("key1", "val1"), - attribute.String("key2", "val2"), + want: members{ + {Key: "key1", Value: "val1"}, + {Key: "key2", Value: "val2"}, }, }, { name: "valid w3cHeader with properties", header: "key1=val1,key2=val2;prop=1", - wantKVs: []attribute.KeyValue{ - attribute.String("key1", "val1"), - attribute.String("key2", "val2;prop=1"), - }, - }, - { - name: "valid header with url-escaped comma", - header: "key1=val1,key2=val2%2Cval3", - wantKVs: []attribute.KeyValue{ - attribute.String("key1", "val1"), - attribute.String("key2", "val2,val3"), + want: members{ + {Key: "key1", Value: "val1"}, + { + Key: "key2", + Value: "val2", + Properties: []property{ + {Key: "prop", Value: "1"}, + }, + }, }, }, { name: "valid header with an invalid header", header: "key1=val1,key2=val2,a,val3", - wantKVs: []attribute.KeyValue{ - attribute.String("key1", "val1"), - attribute.String("key2", "val2"), - }, + want: members{}, }, { name: "valid header with no value", header: "key1=,key2=val2", - wantKVs: []attribute.KeyValue{ - attribute.String("key1", ""), - attribute.String("key2", "val2"), + want: members{ + {Key: "key1", Value: ""}, + {Key: "key2", Value: "val2"}, }, }, } @@ -91,27 +127,8 @@ func TestExtractValidBaggageFromHTTPReq(t *testing.T) { ctx := context.Background() ctx = prop.Extract(ctx, propagation.HeaderCarrier(req.Header)) - gotBaggage := baggage.MapFromContext(ctx) - wantBaggage := baggage.NewMap(baggage.MapUpdate{MultiKV: tt.wantKVs}) - if gotBaggage.Len() != wantBaggage.Len() { - t.Errorf( - "Got and Want Baggage are not the same size %d != %d", - gotBaggage.Len(), - wantBaggage.Len(), - ) - } - totalDiff := "" - wantBaggage.Foreach(func(keyValue attribute.KeyValue) bool { - val, _ := gotBaggage.Value(keyValue.Key) - diff := cmp.Diff(keyValue, attribute.KeyValue{Key: keyValue.Key, Value: val}, cmp.AllowUnexported(attribute.Value{})) - if diff != "" { - totalDiff += diff + "\n" - } - return true - }) - if totalDiff != "" { - t.Errorf("Extract Tracecontext: %s: -got +want %s", tt.name, totalDiff) - } + expected := tt.want.Baggage(t) + assert.Equal(t, expected, baggage.FromContext(ctx)) }) } } @@ -121,7 +138,7 @@ func TestExtractInvalidDistributedContextFromHTTPReq(t *testing.T) { tests := []struct { name string header string - hasKVs []attribute.KeyValue + has members }{ { name: "no key values", @@ -130,17 +147,31 @@ func TestExtractInvalidDistributedContextFromHTTPReq(t *testing.T) { { name: "invalid header with existing context", header: "header2", - hasKVs: []attribute.KeyValue{ - attribute.String("key1", "val1"), - attribute.String("key2", "val2"), + has: members{ + {Key: "key1", Value: "val1"}, + {Key: "key2", Value: "val2"}, }, }, { name: "empty header value", header: "", - hasKVs: []attribute.KeyValue{ - attribute.String("key1", "val1"), - attribute.String("key2", "val2"), + has: members{ + {Key: "key1", Value: "val1"}, + {Key: "key2", Value: "val2"}, + }, + }, + { + name: "with properties", + header: "key1=val1,key2=val2;prop=1", + has: members{ + {Key: "key1", Value: "val1"}, + { + Key: "key2", + Value: "val2", + Properties: []property{ + {Key: "prop", Value: "1"}, + }, + }, }, }, } @@ -150,26 +181,10 @@ func TestExtractInvalidDistributedContextFromHTTPReq(t *testing.T) { req, _ := http.NewRequest("GET", "http://example.com", nil) req.Header.Set("baggage", tt.header) - ctx := baggage.NewContext(context.Background(), tt.hasKVs...) - wantBaggage := baggage.MapFromContext(ctx) + expected := tt.has.Baggage(t) + ctx := baggage.ContextWithBaggage(context.Background(), expected) ctx = prop.Extract(ctx, propagation.HeaderCarrier(req.Header)) - gotBaggage := baggage.MapFromContext(ctx) - if gotBaggage.Len() != wantBaggage.Len() { - t.Errorf( - "Got and Want Baggage are not the same size %d != %d", - gotBaggage.Len(), - wantBaggage.Len(), - ) - } - totalDiff := "" - wantBaggage.Foreach(func(keyValue attribute.KeyValue) bool { - val, _ := gotBaggage.Value(keyValue.Key) - diff := cmp.Diff(keyValue, attribute.KeyValue{Key: keyValue.Key, Value: val}, cmp.AllowUnexported(attribute.Value{})) - if diff != "" { - totalDiff += diff + "\n" - } - return true - }) + assert.Equal(t, expected, baggage.FromContext(ctx)) }) } } @@ -178,62 +193,50 @@ func TestInjectBaggageToHTTPReq(t *testing.T) { propagator := propagation.Baggage{} tests := []struct { name string - kvs []attribute.KeyValue + mems members wantInHeader []string - wantedLen int }{ { name: "two simple values", - kvs: []attribute.KeyValue{ - attribute.String("key1", "val1"), - attribute.String("key2", "val2"), + mems: members{ + {Key: "key1", Value: "val1"}, + {Key: "key2", Value: "val2"}, }, wantInHeader: []string{"key1=val1", "key2=val2"}, }, { - name: "two values with escaped chars", - kvs: []attribute.KeyValue{ - attribute.String("key1", "val1,val2"), - attribute.String("key2", "val3=4"), + name: "values with escaped chars", + mems: members{ + {Key: "key2", Value: "val3=4"}, }, - wantInHeader: []string{"key1=val1%2Cval2", "key2=val3%3D4"}, + wantInHeader: []string{"key2=val3%3D4"}, }, { - name: "values of non-string types", - kvs: []attribute.KeyValue{ - attribute.Bool("key1", true), - attribute.Int("key2", 123), - attribute.Int64("key3", 123), - attribute.Float64("key4", 123.567), + name: "with properties", + mems: members{ + {Key: "key1", Value: "val1"}, + { + Key: "key2", + Value: "val2", + Properties: []property{ + {Key: "prop", Value: "1"}, + }, + }, }, wantInHeader: []string{ - "key1=true", - "key2=123", - "key3=123", - "key4=123.567", + "key1=val1", + "key2=val2;prop=1", }, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { req, _ := http.NewRequest("GET", "http://example.com", nil) - ctx := baggage.ContextWithMap(context.Background(), baggage.NewMap(baggage.MapUpdate{MultiKV: tt.kvs})) + ctx := baggage.ContextWithBaggage(context.Background(), tt.mems.Baggage(t)) propagator.Inject(ctx, propagation.HeaderCarrier(req.Header)) - gotHeader := req.Header.Get("baggage") - wantedLen := len(strings.Join(tt.wantInHeader, ",")) - if wantedLen != len(gotHeader) { - t.Errorf( - "%s: Inject baggage incorrect length %d != %d.", tt.name, tt.wantedLen, len(gotHeader), - ) - } - for _, inHeader := range tt.wantInHeader { - if !strings.Contains(gotHeader, inHeader) { - t.Errorf( - "%s: Inject baggage missing part of header: %s in %s", tt.name, inHeader, gotHeader, - ) - } - } + got := strings.Split(req.Header.Get("baggage"), ",") + assert.ElementsMatch(t, tt.wantInHeader, got) }) } }