From 6db714adc4a5925b45582e85890f1fbeb8f2477d Mon Sep 17 00:00:00 2001 From: Tyler Yahn Date: Tue, 3 May 2022 13:33:00 -0700 Subject: [PATCH 01/22] Add autoprop module --- propagators/autoprop/doc.go | 23 ++++++ propagators/autoprop/go.mod | 11 +++ propagators/autoprop/go.sum | 42 ++++++++++ propagators/autoprop/propagator.go | 121 +++++++++++++++++++++++++++++ propagators/autoprop/registry.go | 86 ++++++++++++++++++++ 5 files changed, 283 insertions(+) create mode 100644 propagators/autoprop/doc.go create mode 100644 propagators/autoprop/go.mod create mode 100644 propagators/autoprop/go.sum create mode 100644 propagators/autoprop/propagator.go create mode 100644 propagators/autoprop/registry.go diff --git a/propagators/autoprop/doc.go b/propagators/autoprop/doc.go new file mode 100644 index 00000000000..0f09e745747 --- /dev/null +++ b/propagators/autoprop/doc.go @@ -0,0 +1,23 @@ +// 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 autoprop provides an OpenTelemetry TextMapPropagator creation +// function. The OpenTelemetry specification states that the default +// TextMapPropagator needs to be a no-operation implementation. The +// opentelemetry-go project adheres to this requirement. However, for systems +// that perform propagation this default is not ideal. This package provides a +// TextMapPropagator with useful defaults (a combined TraceContext and Baggage +// TextMapPropagator), and supports environment overrides using the +// OTEL_PROPAGATORS environment variable. +package autoprop // import "go.opentelemetry.io/contrib/propagators/autoprop" diff --git a/propagators/autoprop/go.mod b/propagators/autoprop/go.mod new file mode 100644 index 00000000000..abfdc615345 --- /dev/null +++ b/propagators/autoprop/go.mod @@ -0,0 +1,11 @@ +module go.opentelemetry.io/contrib/propagators/autoprop + +go 1.16 + +require ( + go.opentelemetry.io/contrib/propagators/aws v1.7.0 + go.opentelemetry.io/contrib/propagators/b3 v1.7.0 + go.opentelemetry.io/contrib/propagators/jaeger v1.7.0 + go.opentelemetry.io/contrib/propagators/ot v1.7.0 + go.opentelemetry.io/otel v1.7.0 +) diff --git a/propagators/autoprop/go.sum b/propagators/autoprop/go.sum new file mode 100644 index 00000000000..5e301bccc24 --- /dev/null +++ b/propagators/autoprop/go.sum @@ -0,0 +1,42 @@ +github.com/davecgh/go-spew v1.1.0/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= +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.2.3 h1:2DntVwHkVopvECVRSlL5PSo9eG+cAkDCuckLubN+rq0= +github.com/go-logr/logr v1.2.3/go.mod h1:jdQByPbusPIv2/zmleS9BjJVeZ6kBagPoEUsqbVz/1A= +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.5.7 h1:81/ik6ipDQS2aGcBfIN5dHDB36BwrStyeAQquSYCV4o= +github.com/google/go-cmp v0.5.7/go.mod h1:n+brtR0CgQNWTVd5ZUFpTBC8YFBDLK/h/bpaJ8/DtOE= +github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZbAQM= +github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4= +github.com/stretchr/objx v0.1.0/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME= +github.com/stretchr/testify v1.3.0/go.mod h1:M5WIy9Dh21IEIfnGCwXGc5bZfKNJtfHm1UVUgZn+9EI= +github.com/stretchr/testify v1.7.0/go.mod h1:6Fq8oRcR53rry900zMqJjRRixrwX3KX962/h/Wwjteg= +github.com/stretchr/testify v1.7.1 h1:5TQK59W5E3v0r2duFAb7P95B6hEeOyEnHRa8MjYSMTY= +github.com/stretchr/testify v1.7.1/go.mod h1:6Fq8oRcR53rry900zMqJjRRixrwX3KX962/h/Wwjteg= +go.opentelemetry.io/contrib/propagators/aws v1.7.0 h1:hzLtX+K4YhsrBabA35uBYxCENb5rS/9Z9X8MToTlA3k= +go.opentelemetry.io/contrib/propagators/aws v1.7.0/go.mod h1:h/ql5T6e1XLRFplWNNdzLHp8eb0dkBu+xYOCYxerh0Q= +go.opentelemetry.io/contrib/propagators/b3 v1.7.0 h1:oRAenUhj+GFttfIp3gj7HYVzBhPOHgq/dWPDSmLCXSY= +go.opentelemetry.io/contrib/propagators/b3 v1.7.0/go.mod h1:gXx7AhL4xXCF42gpm9dQvdohoDa2qeyEx4eIIxqK+h4= +go.opentelemetry.io/contrib/propagators/jaeger v1.7.0 h1:x2mXKtONfOJFfNFSx4QXFx1fms6bKIPVvWvgdiaPdRI= +go.opentelemetry.io/contrib/propagators/jaeger v1.7.0/go.mod h1:kt2lNImfxV6dETRsDCENd6jU6G0mPRS+P0qlNuvtkTE= +go.opentelemetry.io/contrib/propagators/ot v1.7.0 h1:KPPToDRxyY/HI3qD4RqwWRbaQ65RIpF8uKWDqWkFHDA= +go.opentelemetry.io/contrib/propagators/ot v1.7.0/go.mod h1:5qxBZR730yb71uXc3bazxt2Si8o8LQK3iJTnSLca/BU= +go.opentelemetry.io/otel v1.7.0 h1:Z2lA3Tdch0iDcrhJXDIlC94XE+bxok1F9B+4Lz/lGsM= +go.opentelemetry.io/otel v1.7.0/go.mod h1:5BdUoMIz5WEs0vt0CUEMtSSaTSHBBVwrhnz7+nrD5xk= +go.opentelemetry.io/otel/sdk v1.7.0 h1:4OmStpcKVOfvDOgCt7UriAPtKolwIhxpnSNI/yK+1B0= +go.opentelemetry.io/otel/sdk v1.7.0/go.mod h1:uTEOTwaqIVuTGiJN7ii13Ibp75wJmYUDe374q6cZwUU= +go.opentelemetry.io/otel/trace v1.7.0 h1:O37Iogk1lEkMRXewVtZ1BBTVn5JEp8GrJvP92bJqC6o= +go.opentelemetry.io/otel/trace v1.7.0/go.mod h1:fzLSB9nqR2eXzxPXb2JW9IKE+ScyXA48yyE4TNvoHqU= +go.uber.org/atomic v1.7.0 h1:ADUqmZGgLDDfbSL9ZmPxKTybcoEYHgpYfELNoN+7hsw= +go.uber.org/atomic v1.7.0/go.mod h1:fEN4uk6kAWBTFdckzkM89CLk9XfWZrxpCo0nPH17wJc= +go.uber.org/multierr v1.8.0 h1:dg6GjLku4EH+249NNmoIciG9N/jURbDG+pFlTkhzIC8= +go.uber.org/multierr v1.8.0/go.mod h1:7EAYxJLBy9rStEaz58O2t4Uvip6FSURkq8/ppBp95ak= +golang.org/x/sys v0.0.0-20210423185535-09eb48e85fd7 h1:iGu644GcxtEcrInvDsQRCwJjtCIOlT2V7IRt6ah2Whw= +golang.org/x/sys v0.0.0-20210423185535-09eb48e85fd7/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs= +golang.org/x/xerrors v0.0.0-20191204190536-9bdfabe68543/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0= +gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0= +gopkg.in/yaml.v3 v3.0.0-20200313102051-9f266ea9e77c/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM= +gopkg.in/yaml.v3 v3.0.0-20210107192922-496545a6307b h1:h8qDotaEPuJATrMmW04NCwg7v22aHH28wwpauUhK9Oo= +gopkg.in/yaml.v3 v3.0.0-20210107192922-496545a6307b/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM= diff --git a/propagators/autoprop/propagator.go b/propagators/autoprop/propagator.go new file mode 100644 index 00000000000..c7074f258c6 --- /dev/null +++ b/propagators/autoprop/propagator.go @@ -0,0 +1,121 @@ +// 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 autoprop + +import ( + "errors" + "fmt" + "os" + "strings" + + "go.opentelemetry.io/otel" + "go.opentelemetry.io/otel/propagation" +) + +// otelPropagatorsEnvKey is the environment variable name identifying +// propagators to use. +const otelPropagatorsEnvKey = "OTEL_PROPAGATORS" + +// NewTextMapPropagator returns a new TextMapPropagator composited by props or +// one defined by the OTEL_PROPAGATORS environment variable. The +// TextMapPropagator defined by OTEL_PROPAGATORS, if set, will take precedence +// to the once composited by props. +// +// The propagators supported with the OTEL_PROPAGATORS environment variable by +// default are: tracecontext, baggage, b3, b3multi, jaeger, xray, ottrace, and +// none. Each of these values, and their combination, are supported in +// conformance with the OpenTelemetry specification. See +// https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/sdk-environment-variables.md#general-sdk-configuration +// for more information. +// +// The supported environment variable propagators can be extended to include +// custom 3rd-party TextMapPropagator. See the Register function for more +// information. +// +// If OTEL_PROPAGATORS is not defined and props is no provided, the returned +// TextMapPropagator will be a composite of the TraceContext and Baggage +// propagators. +func NewTextMapPropagator(props ...propagation.TextMapPropagator) propagation.TextMapPropagator { + // Environment variable defined propagator has precedence over arguments. + envProp, err := parseEnv() + if err != nil { + // Communicate to the user their supplied value will not be used. + otel.Handle(err) + } + if envProp != nil { + return envProp + } + + if len(props) != 0 { + return propagation.NewCompositeTextMapPropagator(props...) + } + + return propagation.NewCompositeTextMapPropagator(propagation.TraceContext{}, propagation.Baggage{}) +} + +// errUnknownPropagator is returned when an unknown propagator name is used in +// the OTEL_PROPAGATORS environment variable. +var errUnknownPropagator = errors.New("unknown propagator") + +// parseEnv returns the composite TextMapPropagators defined by the +// OTEL_PROPAGATORS environment variable. A nil TextMapPropagator is returned +// if no propagator is defined for the environment variable. A no-op +// TextMapPropagator will be returned if "none" is defined anywhere in the +// environment variable. +func parseEnv() (propagation.TextMapPropagator, error) { + propStrs, defined := os.LookupEnv(otelPropagatorsEnvKey) + if !defined { + return nil, nil + } + + const sep = "," + + var ( + props []propagation.TextMapPropagator + unknown []string + ) + + for _, pStr := range strings.Split(propStrs, sep) { + if pStr == none { + // If "none" is passed in combination with any other propagator, + // the result still needs to be a no-op propagator. Therefore, + // short-circuit here. + return propagation.NewCompositeTextMapPropagator(), nil + } + + p, ok := envRegistry.load(pStr) + if !ok { + unknown = append(unknown, pStr) + continue + } + props = append(props, p) + } + + var err error + if len(unknown) > 0 { + joined := strings.Join(unknown, sep) + err = fmt.Errorf("%w: %s", errUnknownPropagator, joined) + } + + switch len(props) { + case 0: + return nil, err + case 1: + // Do not return a composite of a single propagator. + return props[0], err + default: + return propagation.NewCompositeTextMapPropagator(props...), err + } +} diff --git a/propagators/autoprop/registry.go b/propagators/autoprop/registry.go new file mode 100644 index 00000000000..4c617c98d42 --- /dev/null +++ b/propagators/autoprop/registry.go @@ -0,0 +1,86 @@ +// 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 autoprop + +import ( + "sync" + + "go.opentelemetry.io/contrib/propagators/aws/xray" + "go.opentelemetry.io/contrib/propagators/b3" + "go.opentelemetry.io/contrib/propagators/jaeger" + "go.opentelemetry.io/contrib/propagators/ot" + "go.opentelemetry.io/otel/propagation" +) + +// none is the specical "propagator" name that means no propagator shall be +// configured. +const none = "none" + +// envRegistry is the index of all supported environment variable +// values and their mapping to a TextMapPropagator. +var envRegistry = ®istry{ + index: map[string]propagation.TextMapPropagator{ + // W3C Trace Context. + "tracecontext": propagation.TraceContext{}, + // W3C Baggage. + "baggage": propagation.Baggage{}, + // B3 single-header format. + "b3": b3.New(), + // B3 multi-header format. + "b3multi": b3.New(b3.WithInjectEncoding(b3.B3MultipleHeader)), + // Jaeger. + "jaeger": jaeger.Jaeger{}, + // AWS X-Ray. + "xray": xray.Propagator{}, + // OpenTracing Trace. + "ottrace": ot.OT{}, + + // No-op TextMapPropagator. + none: propagation.NewCompositeTextMapPropagator(), + }, +} + +// registry maintains an index of propagator names to TextMapPropagator +// implementations that is safe for concurrent use by multiple goroutines +// without additional locking or coordination. +type registry struct { + mu sync.Mutex + index map[string]propagation.TextMapPropagator +} + +// load returns the value stored in the registry index for a key, or nil if no +// value is present. The ok result indicates whether value was found in the +// index. +func (r *registry) load(key string) (p propagation.TextMapPropagator, ok bool) { + r.mu.Lock() + p, ok = r.index[key] + r.mu.Unlock() + return p, ok +} + +// store sets the value for a key. +func (r *registry) store(key string, value propagation.TextMapPropagator) { + r.mu.Lock() + r.index[key] = value + r.mu.Unlock() +} + +// RegisterTextMapPropagator sets the TextMapPropagator p to be used when the +// OTEL_PROPAGATORS environment variable contains the propagator name. This +// allows the default supported environment TextMapPropagators to be extended +// with 3rd-part implementations. +func RegisterTextMapPropagator(name string, p propagation.TextMapPropagator) { + envRegistry.store(name, p) +} From 6f771c364b6fa6d9c91e0b281b20b2cea72ce272 Mon Sep 17 00:00:00 2001 From: Tyler Yahn Date: Tue, 3 May 2022 13:36:01 -0700 Subject: [PATCH 02/22] Add example tests --- propagators/autoprop/example_test.go | 84 ++++++++++++++++++++++++++++ 1 file changed, 84 insertions(+) create mode 100644 propagators/autoprop/example_test.go diff --git a/propagators/autoprop/example_test.go b/propagators/autoprop/example_test.go new file mode 100644 index 00000000000..f43abff28c4 --- /dev/null +++ b/propagators/autoprop/example_test.go @@ -0,0 +1,84 @@ +// 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 autoprop_test + +import ( + "fmt" + "os" + "sort" + + "go.opentelemetry.io/contrib/propagators/autoprop" + "go.opentelemetry.io/contrib/propagators/b3" + "go.opentelemetry.io/otel" + "go.opentelemetry.io/otel/propagation" +) + +func ExampleNewTextMapPropagator() { + // NewTextMapPropagator returns a TraceContext and Baggage propagator by + // default. The response of this function can be directly registered with + // the go.opentelemetry.io/otel package. + otel.SetTextMapPropagator(autoprop.NewTextMapPropagator()) + + fields := otel.GetTextMapPropagator().Fields() + sort.Strings(fields) + fmt.Println(fields) + // Output: [baggage traceparent tracestate] +} + +func ExampleNewTextMapPropagator_arguments() { + // NewTextMapPropagator behaves the same as the + // NewCompositeTextMapPropagator function in the + // go.opentelemetry.io/otel/propagation package when TextMapPropagator are + // passed as arguments. + fields := autoprop.NewTextMapPropagator( + propagation.TraceContext{}, + propagation.Baggage{}, + b3.New(), + ).Fields() + sort.Strings(fields) + fmt.Println(fields) + // Output: [baggage traceparent tracestate x-b3-flags x-b3-sampled x-b3-spanid x-b3-traceid] +} + +func ExampleNewTextMapPropagator_environment() { + // Propagators set for the OTEL_PROPAGATORS environment variable take + // precedence and will override any arguments passed to + // NewTextMapPropagator. + os.Setenv("OTEL_PROPAGATORS", "b3,baggage") + + // Returns only a B3 and Baggage TextMapPropagator (i.e. does not include + // TraceContext). + fields := autoprop.NewTextMapPropagator(propagation.TraceContext{}).Fields() + sort.Strings(fields) + fmt.Println(fields) + // Output: [baggage x-b3-flags x-b3-sampled x-b3-spanid x-b3-traceid] +} + +type myTextMapPropagator struct{ propagation.TextMapPropagator } + +func (myTextMapPropagator) Fields() []string { + return []string{"my-header-val"} +} + +func ExampleRegisterTextMapPropagator() { + // To use your own or a 3rd-party exporter via the OTEL_PROPAGATORS + // environment variable, it needs to be registered prior to calling + // NewTextMapPropagator. + autoprop.RegisterTextMapPropagator("custom-prop", myTextMapPropagator{}) + + os.Setenv("OTEL_PROPAGATORS", "custom-prop") + fmt.Println(autoprop.NewTextMapPropagator().Fields()) + // Output: [my-header-val] +} From c334328cd48fc6a5212a1175f121e7634f8f8b6c Mon Sep 17 00:00:00 2001 From: Tyler Yahn Date: Tue, 3 May 2022 14:17:03 -0700 Subject: [PATCH 03/22] Add unit tests for registry.go --- propagators/autoprop/registry.go | 6 ++- propagators/autoprop/registry_test.go | 66 +++++++++++++++++++++++++++ 2 files changed, 71 insertions(+), 1 deletion(-) create mode 100644 propagators/autoprop/registry_test.go diff --git a/propagators/autoprop/registry.go b/propagators/autoprop/registry.go index 4c617c98d42..4235d1a1825 100644 --- a/propagators/autoprop/registry.go +++ b/propagators/autoprop/registry.go @@ -73,8 +73,12 @@ func (r *registry) load(key string) (p propagation.TextMapPropagator, ok bool) { // store sets the value for a key. func (r *registry) store(key string, value propagation.TextMapPropagator) { r.mu.Lock() + defer r.mu.Unlock() + if r.index == nil { + r.index = map[string]propagation.TextMapPropagator{key: value} + return + } r.index[key] = value - r.mu.Unlock() } // RegisterTextMapPropagator sets the TextMapPropagator p to be used when the diff --git a/propagators/autoprop/registry_test.go b/propagators/autoprop/registry_test.go new file mode 100644 index 00000000000..65ef197621f --- /dev/null +++ b/propagators/autoprop/registry_test.go @@ -0,0 +1,66 @@ +// 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 autoprop + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "go.opentelemetry.io/otel/propagation" +) + +var noop = propagation.NewCompositeTextMapPropagator() + +func TestRegistryEmptyStore(t *testing.T) { + r := registry{} + assert.NotPanics(t, func() { r.store("first", noop) }) +} + +func TestRegistryEmptyLoad(t *testing.T) { + r := registry{} + assert.NotPanics(t, func() { + v, ok := r.load("non-existent") + assert.False(t, ok, "empty registry should hold nothing") + assert.Nil(t, v, "non-nil propagator returned") + }) +} + +func TestRegistryConcurrentSafe(t *testing.T) { + const propName = "prop" + + r := registry{} + assert.NotPanics(t, func() { r.store(propName, noop) }) + + go func() { + assert.NotPanics(t, func() { r.store(propName, noop) }) + }() + + go func() { + assert.NotPanics(t, func() { + v, ok := r.load(propName) + assert.True(t, ok, "missing propagator in registry") + assert.Equal(t, noop, v, "wrong propagator retuned") + }) + }() +} + +func TestRegisterTextMapPropagator(t *testing.T) { + const propName = "custom" + RegisterTextMapPropagator(propName, noop) + + v, ok := envRegistry.load(propName) + assert.True(t, ok, "missing propagator in envRegistry") + assert.Equal(t, noop, v, "wrong propagator stored") +} From b86fa71806ab52c38004a675f45fa3117d59025f Mon Sep 17 00:00:00 2001 From: Tyler Yahn Date: Tue, 3 May 2022 14:30:38 -0700 Subject: [PATCH 04/22] Add unit test for NewTextMapPropagator --- propagators/autoprop/propagator.go | 13 ++++++-- propagators/autoprop/propagator_test.go | 40 +++++++++++++++++++++++++ 2 files changed, 51 insertions(+), 2 deletions(-) create mode 100644 propagators/autoprop/propagator_test.go diff --git a/propagators/autoprop/propagator.go b/propagators/autoprop/propagator.go index c7074f258c6..78c51161aee 100644 --- a/propagators/autoprop/propagator.go +++ b/propagators/autoprop/propagator.go @@ -58,11 +58,20 @@ func NewTextMapPropagator(props ...propagation.TextMapPropagator) propagation.Te return envProp } - if len(props) != 0 { + switch len(props) { + case 0: + // Default to TraceContext and Baggage. + return propagation.NewCompositeTextMapPropagator( + propagation.TraceContext{}, propagation.Baggage{}, + ) + case 1: + // Do not add overhead with a composite propagator wrapping a single + // propagator, return it directly. + return props[0] + default: return propagation.NewCompositeTextMapPropagator(props...) } - return propagation.NewCompositeTextMapPropagator(propagation.TraceContext{}, propagation.Baggage{}) } // errUnknownPropagator is returned when an unknown propagator name is used in diff --git a/propagators/autoprop/propagator_test.go b/propagators/autoprop/propagator_test.go new file mode 100644 index 00000000000..39f8aa9746c --- /dev/null +++ b/propagators/autoprop/propagator_test.go @@ -0,0 +1,40 @@ +// 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 autoprop + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "go.opentelemetry.io/otel" +) + +type handler struct { + err error +} + +var _ otel.ErrorHandler = (*handler)(nil) + +func (h *handler) Handle(err error) { h.err = err } + +func TestNewTextMapPropagatorInvalidEnvVal(t *testing.T) { + h := &handler{} + otel.SetErrorHandler(h) + + const name = "invalid-name" + t.Setenv(otelPropagatorsEnvKey, name) + _ = NewTextMapPropagator() + assert.ErrorIs(t, h.err, errUnknownPropagator) +} From db7767600c94293d6ea70f8b54b742ecd09e4a42 Mon Sep 17 00:00:00 2001 From: Tyler Yahn Date: Tue, 3 May 2022 14:40:08 -0700 Subject: [PATCH 05/22] Add NewTextMapPropagator unit tests --- propagators/autoprop/propagator_test.go | 30 +++++++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/propagators/autoprop/propagator_test.go b/propagators/autoprop/propagator_test.go index 39f8aa9746c..b5048f6c6ed 100644 --- a/propagators/autoprop/propagator_test.go +++ b/propagators/autoprop/propagator_test.go @@ -15,10 +15,12 @@ package autoprop import ( + "context" "testing" "github.com/stretchr/testify/assert" "go.opentelemetry.io/otel" + "go.opentelemetry.io/otel/propagation" ) type handler struct { @@ -38,3 +40,31 @@ func TestNewTextMapPropagatorInvalidEnvVal(t *testing.T) { _ = NewTextMapPropagator() assert.ErrorIs(t, h.err, errUnknownPropagator) } + +func TestNewTextMapPropagatorDefault(t *testing.T) { + expect := []string{"traceparent", "tracestate", "baggage"} + assert.ElementsMatch(t, expect, NewTextMapPropagator().Fields()) +} + +type ptrNoop struct{} + +var _ propagation.TextMapPropagator = (*ptrNoop)(nil) + +func (*ptrNoop) Inject(context.Context, propagation.TextMapCarrier) {} + +func (*ptrNoop) Extract(context.Context, propagation.TextMapCarrier) context.Context { + return context.Background() +} +func (*ptrNoop) Fields() []string { + return nil +} + +func TestNewTextMapPropagatorSingleNoOverhead(t *testing.T) { + p := &ptrNoop{} + assert.Same(t, p, NewTextMapPropagator(p)) +} + +func TestNewTextMapPropagatorMultiEnvNone(t *testing.T) { + t.Setenv(otelPropagatorsEnvKey, "b3,none,tracecontext") + assert.Equal(t, noop, NewTextMapPropagator()) +} From 67fb8ec4afdf733ef455269a945cd4e9036bfac3 Mon Sep 17 00:00:00 2001 From: Tyler Yahn Date: Tue, 3 May 2022 14:44:36 -0700 Subject: [PATCH 06/22] Fix RegisterTextMapPropagator func name --- propagators/autoprop/propagator.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/propagators/autoprop/propagator.go b/propagators/autoprop/propagator.go index 78c51161aee..920176a5d57 100644 --- a/propagators/autoprop/propagator.go +++ b/propagators/autoprop/propagator.go @@ -41,8 +41,8 @@ const otelPropagatorsEnvKey = "OTEL_PROPAGATORS" // for more information. // // The supported environment variable propagators can be extended to include -// custom 3rd-party TextMapPropagator. See the Register function for more -// information. +// custom 3rd-party TextMapPropagator. See the RegisterTextMapPropagator +// function for more information. // // If OTEL_PROPAGATORS is not defined and props is no provided, the returned // TextMapPropagator will be a composite of the TraceContext and Baggage From 3aab5aa25a2aea084c99a07b864e98102e76f579 Mon Sep 17 00:00:00 2001 From: Tyler Yahn Date: Tue, 3 May 2022 14:55:24 -0700 Subject: [PATCH 07/22] Fix lint --- propagators/autoprop/example_test.go | 4 ++-- propagators/autoprop/propagator.go | 1 - 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/propagators/autoprop/example_test.go b/propagators/autoprop/example_test.go index f43abff28c4..f98ca9c022f 100644 --- a/propagators/autoprop/example_test.go +++ b/propagators/autoprop/example_test.go @@ -56,7 +56,7 @@ func ExampleNewTextMapPropagator_environment() { // Propagators set for the OTEL_PROPAGATORS environment variable take // precedence and will override any arguments passed to // NewTextMapPropagator. - os.Setenv("OTEL_PROPAGATORS", "b3,baggage") + _ = os.Setenv("OTEL_PROPAGATORS", "b3,baggage") // Returns only a B3 and Baggage TextMapPropagator (i.e. does not include // TraceContext). @@ -78,7 +78,7 @@ func ExampleRegisterTextMapPropagator() { // NewTextMapPropagator. autoprop.RegisterTextMapPropagator("custom-prop", myTextMapPropagator{}) - os.Setenv("OTEL_PROPAGATORS", "custom-prop") + _ = os.Setenv("OTEL_PROPAGATORS", "custom-prop") fmt.Println(autoprop.NewTextMapPropagator().Fields()) // Output: [my-header-val] } diff --git a/propagators/autoprop/propagator.go b/propagators/autoprop/propagator.go index 920176a5d57..fed4311ddc3 100644 --- a/propagators/autoprop/propagator.go +++ b/propagators/autoprop/propagator.go @@ -71,7 +71,6 @@ func NewTextMapPropagator(props ...propagation.TextMapPropagator) propagation.Te default: return propagation.NewCompositeTextMapPropagator(props...) } - } // errUnknownPropagator is returned when an unknown propagator name is used in From 1055a5c94ec7d79d5639e7ba67ece74103318e82 Mon Sep 17 00:00:00 2001 From: Tyler Yahn Date: Tue, 3 May 2022 15:00:46 -0700 Subject: [PATCH 08/22] Go import lint fix --- propagators/autoprop/go.mod | 1 + propagators/autoprop/go.sum | 1 + propagators/autoprop/propagator.go | 2 +- propagators/autoprop/propagator_test.go | 1 + propagators/autoprop/registry.go | 2 +- propagators/autoprop/registry_test.go | 1 + 6 files changed, 6 insertions(+), 2 deletions(-) diff --git a/propagators/autoprop/go.mod b/propagators/autoprop/go.mod index abfdc615345..f2675c663c1 100644 --- a/propagators/autoprop/go.mod +++ b/propagators/autoprop/go.mod @@ -3,6 +3,7 @@ module go.opentelemetry.io/contrib/propagators/autoprop go 1.16 require ( + github.com/stretchr/testify v1.7.1 go.opentelemetry.io/contrib/propagators/aws v1.7.0 go.opentelemetry.io/contrib/propagators/b3 v1.7.0 go.opentelemetry.io/contrib/propagators/jaeger v1.7.0 diff --git a/propagators/autoprop/go.sum b/propagators/autoprop/go.sum index 5e301bccc24..e0001c2d25a 100644 --- a/propagators/autoprop/go.sum +++ b/propagators/autoprop/go.sum @@ -36,6 +36,7 @@ go.uber.org/multierr v1.8.0/go.mod h1:7EAYxJLBy9rStEaz58O2t4Uvip6FSURkq8/ppBp95a golang.org/x/sys v0.0.0-20210423185535-09eb48e85fd7 h1:iGu644GcxtEcrInvDsQRCwJjtCIOlT2V7IRt6ah2Whw= golang.org/x/sys v0.0.0-20210423185535-09eb48e85fd7/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs= golang.org/x/xerrors v0.0.0-20191204190536-9bdfabe68543/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0= +gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405 h1:yhCVgyC4o1eVCa2tZl7eS0r+SDo693bJlVdllGtEeKM= gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0= gopkg.in/yaml.v3 v3.0.0-20200313102051-9f266ea9e77c/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM= gopkg.in/yaml.v3 v3.0.0-20210107192922-496545a6307b h1:h8qDotaEPuJATrMmW04NCwg7v22aHH28wwpauUhK9Oo= diff --git a/propagators/autoprop/propagator.go b/propagators/autoprop/propagator.go index fed4311ddc3..1fe17781227 100644 --- a/propagators/autoprop/propagator.go +++ b/propagators/autoprop/propagator.go @@ -12,7 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. -package autoprop +package autoprop // import "go.opentelemetry.io/contrib/propagators/autoprop" import ( "errors" diff --git a/propagators/autoprop/propagator_test.go b/propagators/autoprop/propagator_test.go index b5048f6c6ed..0d64ae78ab3 100644 --- a/propagators/autoprop/propagator_test.go +++ b/propagators/autoprop/propagator_test.go @@ -19,6 +19,7 @@ import ( "testing" "github.com/stretchr/testify/assert" + "go.opentelemetry.io/otel" "go.opentelemetry.io/otel/propagation" ) diff --git a/propagators/autoprop/registry.go b/propagators/autoprop/registry.go index 4235d1a1825..b8017b80805 100644 --- a/propagators/autoprop/registry.go +++ b/propagators/autoprop/registry.go @@ -12,7 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. -package autoprop +package autoprop // import "go.opentelemetry.io/contrib/propagators/autoprop" import ( "sync" diff --git a/propagators/autoprop/registry_test.go b/propagators/autoprop/registry_test.go index 65ef197621f..df2b2947315 100644 --- a/propagators/autoprop/registry_test.go +++ b/propagators/autoprop/registry_test.go @@ -18,6 +18,7 @@ import ( "testing" "github.com/stretchr/testify/assert" + "go.opentelemetry.io/otel/propagation" ) From 492a7008a6fa6b39143c1191402c48e31b9540f6 Mon Sep 17 00:00:00 2001 From: Tyler Yahn Date: Tue, 3 May 2022 15:01:03 -0700 Subject: [PATCH 09/22] Add dependabot entry --- .github/dependabot.yml | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/.github/dependabot.yml b/.github/dependabot.yml index ccf705af38b..a0fb05c3366 100644 --- a/.github/dependabot.yml +++ b/.github/dependabot.yml @@ -532,6 +532,15 @@ updates: schedule: interval: weekly day: sunday + - package-ecosystem: gomod + directory: /propagators/autoprop + labels: + - dependencies + - go + - Skip Changelog + schedule: + interval: weekly + day: sunday - package-ecosystem: gomod directory: /propagators/aws labels: From 9201c4b3c76c8fe0c55670845717f16ec6dcf42f Mon Sep 17 00:00:00 2001 From: Tyler Yahn Date: Tue, 3 May 2022 15:02:13 -0700 Subject: [PATCH 10/22] Add change to the changelog --- CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 769e5330467..8e1663c7883 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,10 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm ## [Unreleased] +### Added + +- The `go.opentelemetry.io/contrib/propagators/autoprop` package to provide configuration of propagators with useful defaults and envar support. (#2258) + ## [1.7.0/0.32.0] - 2022-04-28 ### Added From 999aa9c929f1cf77a9ecd1e85b3a3bd94509211e Mon Sep 17 00:00:00 2001 From: Tyler Yahn Date: Tue, 3 May 2022 15:03:08 -0700 Subject: [PATCH 11/22] Add autoprop to versions --- versions.yaml | 1 + 1 file changed, 1 insertion(+) diff --git a/versions.yaml b/versions.yaml index 544be0d0721..382272f6ce6 100644 --- a/versions.yaml +++ b/versions.yaml @@ -30,6 +30,7 @@ module-sets: version: v0.32.0 modules: - go.opentelemetry.io/contrib/detectors/aws/lambda + - go.opentelemetry.io/contrib/propagators/autoprop - go.opentelemetry.io/contrib/propagators/opencensus - go.opentelemetry.io/contrib/propagators/opencensus/examples - go.opentelemetry.io/contrib/instrumentation/gopkg.in/macaron.v1/otelmacaron From a45b5de33d937c94a74a7b62015e546bc6a355f8 Mon Sep 17 00:00:00 2001 From: Tyler Yahn Date: Tue, 3 May 2022 15:11:58 -0700 Subject: [PATCH 12/22] Add setenv test func to support Go 1.16 --- propagators/autoprop/propagator_test.go | 21 +++++++++++++++++++-- 1 file changed, 19 insertions(+), 2 deletions(-) diff --git a/propagators/autoprop/propagator_test.go b/propagators/autoprop/propagator_test.go index 0d64ae78ab3..6ca96abe90e 100644 --- a/propagators/autoprop/propagator_test.go +++ b/propagators/autoprop/propagator_test.go @@ -16,6 +16,7 @@ package autoprop import ( "context" + "os" "testing" "github.com/stretchr/testify/assert" @@ -24,6 +25,22 @@ import ( "go.opentelemetry.io/otel/propagation" ) +// setenv sets and unsets an environment variable in the context of a test +// run. It is used instead of t.Setenv so as to support Go <1.17. It can be +// replaced with t.Setenv(key, value) when this support is dropped. +func setenv(t *testing.T, key, value string) { + prevValue, ok := os.LookupEnv(key) + if err := os.Setenv(key, value); err != nil { + t.Fatalf("cannot set environment variable: %v", err) + } + + if ok { + t.Cleanup(func() { os.Setenv(key, prevValue) }) + } else { + t.Cleanup(func() { os.Unsetenv(key) }) + } +} + type handler struct { err error } @@ -37,7 +54,7 @@ func TestNewTextMapPropagatorInvalidEnvVal(t *testing.T) { otel.SetErrorHandler(h) const name = "invalid-name" - t.Setenv(otelPropagatorsEnvKey, name) + setenv(t, otelPropagatorsEnvKey, name) _ = NewTextMapPropagator() assert.ErrorIs(t, h.err, errUnknownPropagator) } @@ -66,6 +83,6 @@ func TestNewTextMapPropagatorSingleNoOverhead(t *testing.T) { } func TestNewTextMapPropagatorMultiEnvNone(t *testing.T) { - t.Setenv(otelPropagatorsEnvKey, "b3,none,tracecontext") + setenv(t, otelPropagatorsEnvKey, "b3,none,tracecontext") assert.Equal(t, noop, NewTextMapPropagator()) } From 7521e6b52cb1f279b93fca2477ab168a18d5a19c Mon Sep 17 00:00:00 2001 From: Tyler Yahn Date: Tue, 3 May 2022 15:48:56 -0700 Subject: [PATCH 13/22] Fix spelling error in comment --- propagators/autoprop/registry.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/propagators/autoprop/registry.go b/propagators/autoprop/registry.go index b8017b80805..03753e9099e 100644 --- a/propagators/autoprop/registry.go +++ b/propagators/autoprop/registry.go @@ -24,7 +24,7 @@ import ( "go.opentelemetry.io/otel/propagation" ) -// none is the specical "propagator" name that means no propagator shall be +// none is the special "propagator" name that means no propagator shall be // configured. const none = "none" From 037664e8dd4dd3cfc1d69ffcb2070db16c865558 Mon Sep 17 00:00:00 2001 From: Tyler Yahn Date: Wed, 4 May 2022 13:28:24 -0700 Subject: [PATCH 14/22] Rename registry index field to names --- propagators/autoprop/registry.go | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/propagators/autoprop/registry.go b/propagators/autoprop/registry.go index 03753e9099e..435f7f5ec43 100644 --- a/propagators/autoprop/registry.go +++ b/propagators/autoprop/registry.go @@ -31,7 +31,7 @@ const none = "none" // envRegistry is the index of all supported environment variable // values and their mapping to a TextMapPropagator. var envRegistry = ®istry{ - index: map[string]propagation.TextMapPropagator{ + names: map[string]propagation.TextMapPropagator{ // W3C Trace Context. "tracecontext": propagation.TraceContext{}, // W3C Baggage. @@ -52,12 +52,12 @@ var envRegistry = ®istry{ }, } -// registry maintains an index of propagator names to TextMapPropagator +// registry maintains a map of propagator names to TextMapPropagator // implementations that is safe for concurrent use by multiple goroutines // without additional locking or coordination. type registry struct { mu sync.Mutex - index map[string]propagation.TextMapPropagator + names map[string]propagation.TextMapPropagator } // load returns the value stored in the registry index for a key, or nil if no @@ -65,7 +65,7 @@ type registry struct { // index. func (r *registry) load(key string) (p propagation.TextMapPropagator, ok bool) { r.mu.Lock() - p, ok = r.index[key] + p, ok = r.names[key] r.mu.Unlock() return p, ok } @@ -74,11 +74,11 @@ func (r *registry) load(key string) (p propagation.TextMapPropagator, ok bool) { func (r *registry) store(key string, value propagation.TextMapPropagator) { r.mu.Lock() defer r.mu.Unlock() - if r.index == nil { - r.index = map[string]propagation.TextMapPropagator{key: value} + if r.names == nil { + r.names = map[string]propagation.TextMapPropagator{key: value} return } - r.index[key] = value + r.names[key] = value } // RegisterTextMapPropagator sets the TextMapPropagator p to be used when the From a92287d1560e037350785a53dc9e23207a909eee Mon Sep 17 00:00:00 2001 From: Tyler Yahn Date: Wed, 4 May 2022 13:28:52 -0700 Subject: [PATCH 15/22] Remove interface assertion --- propagators/autoprop/propagator_test.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/propagators/autoprop/propagator_test.go b/propagators/autoprop/propagator_test.go index 6ca96abe90e..4f90f39e0b1 100644 --- a/propagators/autoprop/propagator_test.go +++ b/propagators/autoprop/propagator_test.go @@ -66,8 +66,6 @@ func TestNewTextMapPropagatorDefault(t *testing.T) { type ptrNoop struct{} -var _ propagation.TextMapPropagator = (*ptrNoop)(nil) - func (*ptrNoop) Inject(context.Context, propagation.TextMapCarrier) {} func (*ptrNoop) Extract(context.Context, propagation.TextMapCarrier) context.Context { From 79ada0bbbaf51d94f9b5cd1140135eaa91d72644 Mon Sep 17 00:00:00 2001 From: Tyler Yahn Date: Tue, 24 May 2022 08:31:41 -0700 Subject: [PATCH 16/22] Fix lint --- propagators/autoprop/propagator_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/propagators/autoprop/propagator_test.go b/propagators/autoprop/propagator_test.go index 4f90f39e0b1..8962b43fd36 100644 --- a/propagators/autoprop/propagator_test.go +++ b/propagators/autoprop/propagator_test.go @@ -35,9 +35,9 @@ func setenv(t *testing.T, key, value string) { } if ok { - t.Cleanup(func() { os.Setenv(key, prevValue) }) + t.Cleanup(func() { _ = os.Setenv(key, prevValue) }) } else { - t.Cleanup(func() { os.Unsetenv(key) }) + t.Cleanup(func() { _ = os.Unsetenv(key) }) } } From f1c9e3de0ab4a8cf6f37241360fed6bee207d96b Mon Sep 17 00:00:00 2001 From: Tyler Yahn Date: Wed, 25 May 2022 07:53:31 -0700 Subject: [PATCH 17/22] Update RegisterTextMapPropagator docs --- propagators/autoprop/registry.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/propagators/autoprop/registry.go b/propagators/autoprop/registry.go index 435f7f5ec43..112581f039b 100644 --- a/propagators/autoprop/registry.go +++ b/propagators/autoprop/registry.go @@ -83,8 +83,8 @@ func (r *registry) store(key string, value propagation.TextMapPropagator) { // RegisterTextMapPropagator sets the TextMapPropagator p to be used when the // OTEL_PROPAGATORS environment variable contains the propagator name. This -// allows the default supported environment TextMapPropagators to be extended -// with 3rd-part implementations. +// allows adding additional environment TextMapPropagators, or replacing the +// default TextMapPropagators with 3rd-party implementations. func RegisterTextMapPropagator(name string, p propagation.TextMapPropagator) { envRegistry.store(name, p) } From dd600fed046af01bfb1bf5245875a4026867344e Mon Sep 17 00:00:00 2001 From: Tyler Yahn Date: Wed, 25 May 2022 07:54:30 -0700 Subject: [PATCH 18/22] Remove compile time assertion and test of impl --- propagators/autoprop/propagator_test.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/propagators/autoprop/propagator_test.go b/propagators/autoprop/propagator_test.go index 8962b43fd36..9753e6ad1bc 100644 --- a/propagators/autoprop/propagator_test.go +++ b/propagators/autoprop/propagator_test.go @@ -45,8 +45,6 @@ type handler struct { err error } -var _ otel.ErrorHandler = (*handler)(nil) - func (h *handler) Handle(err error) { h.err = err } func TestNewTextMapPropagatorInvalidEnvVal(t *testing.T) { From 12a49522bfecc155e1948588c7f30cff8798cf04 Mon Sep 17 00:00:00 2001 From: Tyler Yahn Date: Thu, 26 May 2022 07:59:15 -0700 Subject: [PATCH 19/22] Upgrade to Go 1.17 as min supported version --- propagators/autoprop/go.mod | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/propagators/autoprop/go.mod b/propagators/autoprop/go.mod index f2675c663c1..d5ba5638cef 100644 --- a/propagators/autoprop/go.mod +++ b/propagators/autoprop/go.mod @@ -1,6 +1,6 @@ module go.opentelemetry.io/contrib/propagators/autoprop -go 1.16 +go 1.17 require ( github.com/stretchr/testify v1.7.1 @@ -10,3 +10,16 @@ require ( go.opentelemetry.io/contrib/propagators/ot v1.7.0 go.opentelemetry.io/otel v1.7.0 ) + +require ( + github.com/davecgh/go-spew v1.1.1 // indirect + github.com/go-logr/logr v1.2.3 // indirect + github.com/go-logr/stdr v1.2.2 // indirect + github.com/pmezard/go-difflib v1.0.0 // indirect + go.opentelemetry.io/otel/sdk v1.7.0 // indirect + go.opentelemetry.io/otel/trace v1.7.0 // indirect + go.uber.org/atomic v1.7.0 // indirect + go.uber.org/multierr v1.8.0 // indirect + golang.org/x/sys v0.0.0-20210423185535-09eb48e85fd7 // indirect + gopkg.in/yaml.v3 v3.0.0-20210107192922-496545a6307b // indirect +) From 0b37e5bfaa528deff2ffe6080e755016cb4e60e4 Mon Sep 17 00:00:00 2001 From: Tyler Yahn Date: Thu, 26 May 2022 08:01:11 -0700 Subject: [PATCH 20/22] Use T.Setenv instead of own setenv func --- propagators/autoprop/propagator_test.go | 21 ++------------------- 1 file changed, 2 insertions(+), 19 deletions(-) diff --git a/propagators/autoprop/propagator_test.go b/propagators/autoprop/propagator_test.go index 9753e6ad1bc..9eebc1a2f67 100644 --- a/propagators/autoprop/propagator_test.go +++ b/propagators/autoprop/propagator_test.go @@ -16,7 +16,6 @@ package autoprop import ( "context" - "os" "testing" "github.com/stretchr/testify/assert" @@ -25,22 +24,6 @@ import ( "go.opentelemetry.io/otel/propagation" ) -// setenv sets and unsets an environment variable in the context of a test -// run. It is used instead of t.Setenv so as to support Go <1.17. It can be -// replaced with t.Setenv(key, value) when this support is dropped. -func setenv(t *testing.T, key, value string) { - prevValue, ok := os.LookupEnv(key) - if err := os.Setenv(key, value); err != nil { - t.Fatalf("cannot set environment variable: %v", err) - } - - if ok { - t.Cleanup(func() { _ = os.Setenv(key, prevValue) }) - } else { - t.Cleanup(func() { _ = os.Unsetenv(key) }) - } -} - type handler struct { err error } @@ -52,7 +35,7 @@ func TestNewTextMapPropagatorInvalidEnvVal(t *testing.T) { otel.SetErrorHandler(h) const name = "invalid-name" - setenv(t, otelPropagatorsEnvKey, name) + t.Setenv(otelPropagatorsEnvKey, name) _ = NewTextMapPropagator() assert.ErrorIs(t, h.err, errUnknownPropagator) } @@ -79,6 +62,6 @@ func TestNewTextMapPropagatorSingleNoOverhead(t *testing.T) { } func TestNewTextMapPropagatorMultiEnvNone(t *testing.T) { - setenv(t, otelPropagatorsEnvKey, "b3,none,tracecontext") + t.Setenv(otelPropagatorsEnvKey, "b3,none,tracecontext") assert.Equal(t, noop, NewTextMapPropagator()) } From 1f54df0cf0647592a19d1159ecd898000b6082b1 Mon Sep 17 00:00:00 2001 From: Tyler Yahn Date: Thu, 26 May 2022 12:41:31 -0700 Subject: [PATCH 21/22] Panic on duplicate prop register --- propagators/autoprop/registry.go | 39 ++++++++++++++++++++++----- propagators/autoprop/registry_test.go | 13 +++++++++ 2 files changed, 46 insertions(+), 6 deletions(-) diff --git a/propagators/autoprop/registry.go b/propagators/autoprop/registry.go index 112581f039b..499a7da4b75 100644 --- a/propagators/autoprop/registry.go +++ b/propagators/autoprop/registry.go @@ -15,6 +15,8 @@ package autoprop // import "go.opentelemetry.io/contrib/propagators/autoprop" import ( + "errors" + "fmt" "sync" "go.opentelemetry.io/contrib/propagators/aws/xray" @@ -70,21 +72,46 @@ func (r *registry) load(key string) (p propagation.TextMapPropagator, ok bool) { return p, ok } -// store sets the value for a key. -func (r *registry) store(key string, value propagation.TextMapPropagator) { +var errDupReg = errors.New("duplicate registration") + +// store sets the value for a key if is not already in the registry. errDupReg +// is returned if the registry already contains key. +func (r *registry) store(key string, value propagation.TextMapPropagator) error { r.mu.Lock() defer r.mu.Unlock() if r.names == nil { r.names = map[string]propagation.TextMapPropagator{key: value} - return + return nil + } + if _, ok := r.names[key]; ok { + return fmt.Errorf("%w: %q", errDupReg, key) } r.names[key] = value + return nil +} + +// drop removes key from the registry if it exists, otherwise nothing. +func (r *registry) drop(key string) { + r.mu.Lock() + delete(r.names, key) + r.mu.Unlock() } // RegisterTextMapPropagator sets the TextMapPropagator p to be used when the // OTEL_PROPAGATORS environment variable contains the propagator name. This -// allows adding additional environment TextMapPropagators, or replacing the -// default TextMapPropagators with 3rd-party implementations. +// will panic if name has already been registered or is a default +// (tracecontext, baggage, b3, b3multi, jaeger, xray, or ottrace). func RegisterTextMapPropagator(name string, p propagation.TextMapPropagator) { - envRegistry.store(name, p) + if err := envRegistry.store(name, p); err != nil { + // envRegistry.store will return errDupReg if name is already + // registered. Panic here so the user is made aware of the duplicate + // registration, which could be done by malicious code trying to + // intercept cross-cutting concerns. + // + // Panic for all other errors as well. At this point there should not + // be any other errors returned from the store operation. If there + // are, alert the developer that adding them as soon as possible that + // they need to be handled here. + panic(err) + } } diff --git a/propagators/autoprop/registry_test.go b/propagators/autoprop/registry_test.go index df2b2947315..3182a86bd0a 100644 --- a/propagators/autoprop/registry_test.go +++ b/propagators/autoprop/registry_test.go @@ -15,6 +15,7 @@ package autoprop import ( + "fmt" "testing" "github.com/stretchr/testify/assert" @@ -60,8 +61,20 @@ func TestRegistryConcurrentSafe(t *testing.T) { func TestRegisterTextMapPropagator(t *testing.T) { const propName = "custom" RegisterTextMapPropagator(propName, noop) + t.Cleanup(func() { envRegistry.drop(propName) }) v, ok := envRegistry.load(propName) assert.True(t, ok, "missing propagator in envRegistry") assert.Equal(t, noop, v, "wrong propagator stored") } + +func TestDuplicateRegisterTextMapPropagatorPanics(t *testing.T) { + const propName = "custom" + RegisterTextMapPropagator(propName, noop) + t.Cleanup(func() { envRegistry.drop(propName) }) + + errString := fmt.Sprintf("%s: %q", errDupReg, propName) + assert.PanicsWithError(t, errString, func() { + RegisterTextMapPropagator(propName, noop) + }) +} From 0e495e2040c2fa74f9dbbb3e23e17f2538ed2ba4 Mon Sep 17 00:00:00 2001 From: Tyler Yahn Date: Tue, 31 May 2022 08:26:43 -0700 Subject: [PATCH 22/22] Check errs in registry_test --- propagators/autoprop/registry_test.go | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/propagators/autoprop/registry_test.go b/propagators/autoprop/registry_test.go index 3182a86bd0a..7f284b6a27a 100644 --- a/propagators/autoprop/registry_test.go +++ b/propagators/autoprop/registry_test.go @@ -19,6 +19,7 @@ import ( "testing" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" "go.opentelemetry.io/otel/propagation" ) @@ -27,7 +28,9 @@ var noop = propagation.NewCompositeTextMapPropagator() func TestRegistryEmptyStore(t *testing.T) { r := registry{} - assert.NotPanics(t, func() { r.store("first", noop) }) + assert.NotPanics(t, func() { + require.NoError(t, r.store("first", noop)) + }) } func TestRegistryEmptyLoad(t *testing.T) { @@ -43,10 +46,14 @@ func TestRegistryConcurrentSafe(t *testing.T) { const propName = "prop" r := registry{} - assert.NotPanics(t, func() { r.store(propName, noop) }) + assert.NotPanics(t, func() { + require.NoError(t, r.store(propName, noop)) + }) go func() { - assert.NotPanics(t, func() { r.store(propName, noop) }) + assert.NotPanics(t, func() { + require.ErrorIs(t, r.store(propName, noop), errDupReg) + }) }() go func() {