From 2557df6b9699435ce40d49ae29d42649098cf7c0 Mon Sep 17 00:00:00 2001 From: Andrew Wilkins Date: Thu, 28 Aug 2025 15:22:50 +0800 Subject: [PATCH 01/17] Fix concurrent mutation of attributes in options WithInstrumentationAttributes is creating a closure with a reference to a slice which is later passed to attribute.NewSet. attribute.NewSet may mutate the slice, so this will lead to a data race when the option is applied concurrently. We can fix this by moving the call to attribute.NewSet outside the closure. --- CHANGELOG.md | 1 + log/logger.go | 3 ++- log/logger_test.go | 22 ++++++++++++++++------ metric/config.go | 7 +++++-- metric/config_test.go | 24 +++++++++++++++++------- trace/config.go | 3 ++- trace/config_test.go | 22 ++++++++++++++++------ 7 files changed, 59 insertions(+), 23 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f0e4d0cbc88..970e5492b01 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -80,6 +80,7 @@ The next release will require at least [Go 1.24]. - `SetBody` method of `Record` in `go.opentelemetry.io/otel/sdk/log` now deduplicates key-value collections (`log.Value` of `log.KindMap` from `go.opentelemetry.io/otel/log`). (#7002) - Fix the `go.opentelemetry.io/otel/exporters/stdout/stdouttrace` self-observability component type and name. (#7195) - Fix partial export count metric in `go.opentelemetry.io/otel/exporters/stdout/stdouttrace`. (#7199) +- Fix a data race in WithInstrumentationAttributes options. (#7266) diff --git a/log/logger.go b/log/logger.go index 8441ca88408..08771770946 100644 --- a/log/logger.go +++ b/log/logger.go @@ -119,8 +119,9 @@ func WithInstrumentationVersion(version string) LoggerOption { // // The passed attributes will be de-duplicated. func WithInstrumentationAttributes(attr ...attribute.KeyValue) LoggerOption { + set := attribute.NewSet(attr...) return loggerOptionFunc(func(config LoggerConfig) LoggerConfig { - config.attrs = attribute.NewSet(attr...) + config.attrs = set return config }) } diff --git a/log/logger_test.go b/log/logger_test.go index 23f2ab64be0..44df4ab042c 100644 --- a/log/logger_test.go +++ b/log/logger_test.go @@ -4,6 +4,7 @@ package log_test import ( + "sync" "testing" "github.com/stretchr/testify/assert" @@ -19,14 +20,23 @@ func TestNewLoggerConfig(t *testing.T) { attribute.String("user", "alice"), attribute.Bool("admin", true), ) - - c := log.NewLoggerConfig( + options := []log.LoggerOption{ log.WithInstrumentationVersion(version), log.WithSchemaURL(schemaURL), log.WithInstrumentationAttributes(attr.ToSlice()...), - ) + } - assert.Equal(t, version, c.InstrumentationVersion(), "instrumentation version") - assert.Equal(t, schemaURL, c.SchemaURL(), "schema URL") - assert.Equal(t, attr, c.InstrumentationAttributes(), "instrumentation attributes") + // Ensure that options can be used concurrently. + var wg sync.WaitGroup + for i := 0; i < 10; i++ { + wg.Add(1) + go func() { + defer wg.Done() + c := log.NewLoggerConfig(options...) + assert.Equal(t, version, c.InstrumentationVersion(), "instrumentation version") + assert.Equal(t, schemaURL, c.SchemaURL(), "schema URL") + assert.Equal(t, attr, c.InstrumentationAttributes(), "instrumentation attributes") + }() + } + wg.Wait() } diff --git a/metric/config.go b/metric/config.go index d9e3b13e4d1..f779fc87b52 100644 --- a/metric/config.go +++ b/metric/config.go @@ -3,7 +3,9 @@ package metric // import "go.opentelemetry.io/otel/metric" -import "go.opentelemetry.io/otel/attribute" +import ( + "go.opentelemetry.io/otel/attribute" +) // MeterConfig contains options for Meters. type MeterConfig struct { @@ -66,8 +68,9 @@ func WithInstrumentationVersion(version string) MeterOption { // // The passed attributes will be de-duplicated. func WithInstrumentationAttributes(attr ...attribute.KeyValue) MeterOption { + set := attribute.NewSet(attr...) return meterOptionFunc(func(config MeterConfig) MeterConfig { - config.attrs = attribute.NewSet(attr...) + config.attrs = set return config }) } diff --git a/metric/config_test.go b/metric/config_test.go index f5e65150faa..a5afa807f4f 100644 --- a/metric/config_test.go +++ b/metric/config_test.go @@ -4,6 +4,7 @@ package metric_test import ( + "sync" "testing" "github.com/stretchr/testify/assert" @@ -12,21 +13,30 @@ import ( "go.opentelemetry.io/otel/metric" ) -func TestConfig(t *testing.T) { +func TestNewMeterConfig(t *testing.T) { version := "v1.1.1" schemaURL := "https://opentelemetry.io/schemas/1.0.0" attr := attribute.NewSet( attribute.String("user", "alice"), attribute.Bool("admin", true), ) - - c := metric.NewMeterConfig( + options := []metric.MeterOption{ metric.WithInstrumentationVersion(version), metric.WithSchemaURL(schemaURL), metric.WithInstrumentationAttributes(attr.ToSlice()...), - ) + } - assert.Equal(t, version, c.InstrumentationVersion(), "instrumentation version") - assert.Equal(t, schemaURL, c.SchemaURL(), "schema URL") - assert.Equal(t, attr, c.InstrumentationAttributes(), "instrumentation attributes") + // Ensure that options can be used concurrently. + var wg sync.WaitGroup + for i := 0; i < 10; i++ { + wg.Add(1) + go func() { + defer wg.Done() + c := metric.NewMeterConfig(options...) + assert.Equal(t, version, c.InstrumentationVersion(), "instrumentation version") + assert.Equal(t, schemaURL, c.SchemaURL(), "schema URL") + assert.Equal(t, attr, c.InstrumentationAttributes(), "instrumentation attributes") + }() + } + wg.Wait() } diff --git a/trace/config.go b/trace/config.go index aea11a2b52c..09fd04125c3 100644 --- a/trace/config.go +++ b/trace/config.go @@ -308,8 +308,9 @@ func WithInstrumentationVersion(version string) TracerOption { // // The passed attributes will be de-duplicated. func WithInstrumentationAttributes(attr ...attribute.KeyValue) TracerOption { + set := attribute.NewSet(attr...) return tracerOptionFunc(func(config TracerConfig) TracerConfig { - config.attrs = attribute.NewSet(attr...) + config.attrs = set return config }) } diff --git a/trace/config_test.go b/trace/config_test.go index 00e6507e80c..79951a806f0 100644 --- a/trace/config_test.go +++ b/trace/config_test.go @@ -4,6 +4,7 @@ package trace import ( + "sync" "testing" "time" @@ -217,18 +218,27 @@ func TestTracerConfig(t *testing.T) { attribute.String("user", "alice"), attribute.Bool("admin", true), ) - - c := NewTracerConfig( + options := []TracerOption{ // Multiple calls should overwrite. WithInstrumentationVersion(v1), WithInstrumentationVersion(v2), WithSchemaURL(schemaURL), WithInstrumentationAttributes(attrs.ToSlice()...), - ) + } - assert.Equal(t, v2, c.InstrumentationVersion(), "instrumentation version") - assert.Equal(t, schemaURL, c.SchemaURL(), "schema URL") - assert.Equal(t, attrs, c.InstrumentationAttributes(), "instrumentation attributes") + // Ensure that options can be used concurrently. + var wg sync.WaitGroup + for i := 0; i < 10; i++ { + wg.Add(1) + go func() { + defer wg.Done() + c := NewTracerConfig(options...) + assert.Equal(t, v2, c.InstrumentationVersion(), "instrumentation version") + assert.Equal(t, schemaURL, c.SchemaURL(), "schema URL") + assert.Equal(t, attrs, c.InstrumentationAttributes(), "instrumentation attributes") + }() + } + wg.Wait() } // Save benchmark results to a file level var to avoid the compiler optimizing From b0723b82940c87f0e39409c5038f5b401765f3db Mon Sep 17 00:00:00 2001 From: Andrew Wilkins Date: Fri, 29 Aug 2025 08:27:34 +0800 Subject: [PATCH 02/17] Apply suggestions from code review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Robert Pająk --- CHANGELOG.md | 4 +++- log/logger_test.go | 2 +- metric/config_test.go | 2 +- trace/config_test.go | 2 +- 4 files changed, 6 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 970e5492b01..40ac3f2ceec 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -80,7 +80,9 @@ The next release will require at least [Go 1.24]. - `SetBody` method of `Record` in `go.opentelemetry.io/otel/sdk/log` now deduplicates key-value collections (`log.Value` of `log.KindMap` from `go.opentelemetry.io/otel/log`). (#7002) - Fix the `go.opentelemetry.io/otel/exporters/stdout/stdouttrace` self-observability component type and name. (#7195) - Fix partial export count metric in `go.opentelemetry.io/otel/exporters/stdout/stdouttrace`. (#7199) -- Fix a data race in WithInstrumentationAttributes options. (#7266) +- Fix a data race in `WithInstrumentationAttributes` option in `go.opentelemetry.io/otel/trace`. (#7266) +- Fix a data race in `WithInstrumentationAttributes` option in `go.opentelemetry.io/otel/metric`. (#7266) +- Fix a data race in `WithInstrumentationAttributes` option in `go.opentelemetry.io/otel/log`. (#7266) diff --git a/log/logger_test.go b/log/logger_test.go index 44df4ab042c..926d1c874e5 100644 --- a/log/logger_test.go +++ b/log/logger_test.go @@ -28,7 +28,7 @@ func TestNewLoggerConfig(t *testing.T) { // Ensure that options can be used concurrently. var wg sync.WaitGroup - for i := 0; i < 10; i++ { + for range 10 { wg.Add(1) go func() { defer wg.Done() diff --git a/metric/config_test.go b/metric/config_test.go index a5afa807f4f..d13d2152ed6 100644 --- a/metric/config_test.go +++ b/metric/config_test.go @@ -28,7 +28,7 @@ func TestNewMeterConfig(t *testing.T) { // Ensure that options can be used concurrently. var wg sync.WaitGroup - for i := 0; i < 10; i++ { + for range 10 { wg.Add(1) go func() { defer wg.Done() diff --git a/trace/config_test.go b/trace/config_test.go index 79951a806f0..4ec40bde6b7 100644 --- a/trace/config_test.go +++ b/trace/config_test.go @@ -228,7 +228,7 @@ func TestTracerConfig(t *testing.T) { // Ensure that options can be used concurrently. var wg sync.WaitGroup - for i := 0; i < 10; i++ { + for range 10 { wg.Add(1) go func() { defer wg.Done() From f89cefcd725ee1f6bd4d3c5cbed315b7ee844e0e Mon Sep 17 00:00:00 2001 From: Andrew Wilkins Date: Fri, 29 Aug 2025 08:30:11 +0800 Subject: [PATCH 03/17] Revert import formatting change --- metric/config.go | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/metric/config.go b/metric/config.go index f779fc87b52..97181ef3db7 100644 --- a/metric/config.go +++ b/metric/config.go @@ -3,9 +3,7 @@ package metric // import "go.opentelemetry.io/otel/metric" -import ( - "go.opentelemetry.io/otel/attribute" -) +import "go.opentelemetry.io/otel/attribute" // MeterConfig contains options for Meters. type MeterConfig struct { From 431e6a2087ddcb9889008291feed3d0537ee0be5 Mon Sep 17 00:00:00 2001 From: Andrew Wilkins Date: Fri, 29 Aug 2025 09:27:41 +0800 Subject: [PATCH 04/17] Make changelog more specific --- CHANGELOG.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 40ac3f2ceec..2545a179e51 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -80,9 +80,9 @@ The next release will require at least [Go 1.24]. - `SetBody` method of `Record` in `go.opentelemetry.io/otel/sdk/log` now deduplicates key-value collections (`log.Value` of `log.KindMap` from `go.opentelemetry.io/otel/log`). (#7002) - Fix the `go.opentelemetry.io/otel/exporters/stdout/stdouttrace` self-observability component type and name. (#7195) - Fix partial export count metric in `go.opentelemetry.io/otel/exporters/stdout/stdouttrace`. (#7199) -- Fix a data race in `WithInstrumentationAttributes` option in `go.opentelemetry.io/otel/trace`. (#7266) -- Fix a data race in `WithInstrumentationAttributes` option in `go.opentelemetry.io/otel/metric`. (#7266) -- Fix a data race in `WithInstrumentationAttributes` option in `go.opentelemetry.io/otel/log`. (#7266) +- Fix a data race when reusing the `WithInstrumentationAttributes` option in concurrent calls to `go.opentelemetry.io/otel/trace.TraceProvider.Tracer`. (#7266) +- Fix a data race when reusing the `WithInstrumentationAttributes` option in concurrent calls to `go.opentelemetry.io/otel/metric.MeterProvider.Meter`. (#7266) +- Fix a data race when reusing the `WithInstrumentationAttributes` option in concurrent calls to `go.opentelemetry.io/otel/log.LogProvider.Logger`. (#7266) From 40022eb8963a98523df5eebbe3ab04eb6d865477 Mon Sep 17 00:00:00 2001 From: Andrew Wilkins Date: Fri, 29 Aug 2025 09:45:31 +0800 Subject: [PATCH 05/17] Mutating the attributes doesn't affect config --- log/logger_test.go | 12 ++++++++---- metric/config_test.go | 12 ++++++++---- trace/config_test.go | 12 ++++++++---- 3 files changed, 24 insertions(+), 12 deletions(-) diff --git a/log/logger_test.go b/log/logger_test.go index 926d1c874e5..274b887e8e2 100644 --- a/log/logger_test.go +++ b/log/logger_test.go @@ -16,16 +16,20 @@ import ( func TestNewLoggerConfig(t *testing.T) { version := "v1.1.1" schemaURL := "https://opentelemetry.io/schemas/1.0.0" - attr := attribute.NewSet( + attr := []attribute.KeyValue{ attribute.String("user", "alice"), attribute.Bool("admin", true), - ) + } + attrSet := attribute.NewSet(attr...) options := []log.LoggerOption{ log.WithInstrumentationVersion(version), log.WithSchemaURL(schemaURL), - log.WithInstrumentationAttributes(attr.ToSlice()...), + log.WithInstrumentationAttributes(attr...), } + // Modifications to attr should not affect the config. + attr[0] = attribute.String("user", "bob") + // Ensure that options can be used concurrently. var wg sync.WaitGroup for range 10 { @@ -35,7 +39,7 @@ func TestNewLoggerConfig(t *testing.T) { c := log.NewLoggerConfig(options...) assert.Equal(t, version, c.InstrumentationVersion(), "instrumentation version") assert.Equal(t, schemaURL, c.SchemaURL(), "schema URL") - assert.Equal(t, attr, c.InstrumentationAttributes(), "instrumentation attributes") + assert.Equal(t, attrSet, c.InstrumentationAttributes(), "instrumentation attributes") }() } wg.Wait() diff --git a/metric/config_test.go b/metric/config_test.go index d13d2152ed6..f35743c59da 100644 --- a/metric/config_test.go +++ b/metric/config_test.go @@ -16,16 +16,20 @@ import ( func TestNewMeterConfig(t *testing.T) { version := "v1.1.1" schemaURL := "https://opentelemetry.io/schemas/1.0.0" - attr := attribute.NewSet( + attr := []attribute.KeyValue{ attribute.String("user", "alice"), attribute.Bool("admin", true), - ) + } + attrSet := attribute.NewSet(attr...) options := []metric.MeterOption{ metric.WithInstrumentationVersion(version), metric.WithSchemaURL(schemaURL), - metric.WithInstrumentationAttributes(attr.ToSlice()...), + metric.WithInstrumentationAttributes(attr...), } + // Modifications to attr should not affect the config. + attr[0] = attribute.String("user", "bob") + // Ensure that options can be used concurrently. var wg sync.WaitGroup for range 10 { @@ -35,7 +39,7 @@ func TestNewMeterConfig(t *testing.T) { c := metric.NewMeterConfig(options...) assert.Equal(t, version, c.InstrumentationVersion(), "instrumentation version") assert.Equal(t, schemaURL, c.SchemaURL(), "schema URL") - assert.Equal(t, attr, c.InstrumentationAttributes(), "instrumentation attributes") + assert.Equal(t, attrSet, c.InstrumentationAttributes(), "instrumentation attributes") }() } wg.Wait() diff --git a/trace/config_test.go b/trace/config_test.go index 4ec40bde6b7..2cfee34ad63 100644 --- a/trace/config_test.go +++ b/trace/config_test.go @@ -214,18 +214,22 @@ func TestTracerConfig(t *testing.T) { v1 := "semver:0.0.1" v2 := "semver:1.0.0" schemaURL := "https://opentelemetry.io/schemas/1.21.0" - attrs := attribute.NewSet( + attrs := []attribute.KeyValue{ attribute.String("user", "alice"), attribute.Bool("admin", true), - ) + } + attrSet := attribute.NewSet(attrs...) options := []TracerOption{ // Multiple calls should overwrite. WithInstrumentationVersion(v1), WithInstrumentationVersion(v2), WithSchemaURL(schemaURL), - WithInstrumentationAttributes(attrs.ToSlice()...), + WithInstrumentationAttributes(attrs...), } + // Modifications to attr should not affect the config. + attrs[0] = attribute.String("user", "bob") + // Ensure that options can be used concurrently. var wg sync.WaitGroup for range 10 { @@ -235,7 +239,7 @@ func TestTracerConfig(t *testing.T) { c := NewTracerConfig(options...) assert.Equal(t, v2, c.InstrumentationVersion(), "instrumentation version") assert.Equal(t, schemaURL, c.SchemaURL(), "schema URL") - assert.Equal(t, attrs, c.InstrumentationAttributes(), "instrumentation attributes") + assert.Equal(t, attrSet, c.InstrumentationAttributes(), "instrumentation attributes") }() } wg.Wait() From 3af7ce30aa5324d0a59b25bc92d966a9768284f0 Mon Sep 17 00:00:00 2001 From: Robert Pajak Date: Fri, 29 Aug 2025 15:31:11 +0200 Subject: [PATCH 06/17] WithInstrumentationAttributes synchronously de-duplicates the passed attributes --- CHANGELOG.md | 3 +++ log/logger.go | 2 +- metric/config.go | 2 +- sdk/log/provider_test.go | 16 +++++++--------- sdk/metric/provider_test.go | 5 +++-- sdk/trace/provider_test.go | 20 ++++++++++++++++++++ trace/config.go | 2 +- 7 files changed, 36 insertions(+), 14 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2545a179e51..a456bd32f5b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -68,6 +68,9 @@ The next release will require at least [Go 1.24]. - Change `SDKProcessorLogQueueCapacity`, `SDKProcessorLogQueueSize`, `SDKProcessorSpanQueueSize`, and `SDKProcessorSpanQueueCapacity` in `go.opentelemetry.io/otel/semconv/v1.36.0/otelconv` to use a `Int64ObservableUpDownCounter`. (#7041) - Change `DefaultExemplarReservoirProviderSelector` in `go.opentelemetry.io/otel/sdk/metric` to use `runtime.GOMAXPROCS(0)` instead of `runtime.NumCPU()` for the `FixedSizeReservoirProvider` default size. (#7094) - Optimize `TraceIDFromHex` and `SpanIDFromHex` in `go.opentelemetry.io/otel/sdk/trace`. (#6791) +- `WithInstrumentationAttributes` in `go.opentelemetry.io/otel/trace` synchronously de-duplicates the passed attributes instead of delegating it to the returned `TracerOption`. (#7270) +- `WithInstrumentationAttributes` in `go.opentelemetry.io/otel/meter` synchronously de-duplicates the passed attributes instead of delegating it to the returned `MeterOption`. (#7270) +- `WithInstrumentationAttributes` in `go.opentelemetry.io/otel/log` synchronously de-duplicates the passed attributes instead of delegating it to the returned `LoggerOption`. (#7270) ### Deprecated diff --git a/log/logger.go b/log/logger.go index 08771770946..e77a0158638 100644 --- a/log/logger.go +++ b/log/logger.go @@ -117,7 +117,7 @@ func WithInstrumentationVersion(version string) LoggerOption { // WithInstrumentationAttributes returns a [LoggerOption] that sets the // instrumentation attributes of a [Logger]. // -// The passed attributes will be de-duplicated. +// Note that the passed attributes are de-duplicated by WithInstrumentationAttributes. func WithInstrumentationAttributes(attr ...attribute.KeyValue) LoggerOption { set := attribute.NewSet(attr...) return loggerOptionFunc(func(config LoggerConfig) LoggerConfig { diff --git a/metric/config.go b/metric/config.go index 97181ef3db7..42500d8fa89 100644 --- a/metric/config.go +++ b/metric/config.go @@ -64,7 +64,7 @@ func WithInstrumentationVersion(version string) MeterOption { // WithInstrumentationAttributes sets the instrumentation attributes. // -// The passed attributes will be de-duplicated. +// Note that the passed attributes are de-duplicated by WithInstrumentationAttributes. func WithInstrumentationAttributes(attr ...attribute.KeyValue) MeterOption { set := attribute.NewSet(attr...) return meterOptionFunc(func(config MeterConfig) MeterConfig { diff --git a/sdk/log/provider_test.go b/sdk/log/provider_test.go index 23a74630cea..0b6e33062a2 100644 --- a/sdk/log/provider_test.go +++ b/sdk/log/provider_test.go @@ -249,24 +249,22 @@ func TestWithResource(t *testing.T) { } func TestLoggerProviderConcurrentSafe(*testing.T) { - const goRoutineN = 10 - - var wg sync.WaitGroup - wg.Add(goRoutineN) - p := NewLoggerProvider(WithProcessor(newProcessor("0"))) - const name = "testLogger" + ctx := context.Background() - for range goRoutineN { + const name = "testLogger" + opt := log.WithInstrumentationAttributes(attribute.String("key", "value")) + var wg sync.WaitGroup + for range 10 { + wg.Add(1) go func() { defer wg.Done() - _ = p.Logger(name) + _ = p.Logger(name, opt) _ = p.Shutdown(ctx) _ = p.ForceFlush(ctx) }() } - wg.Wait() } diff --git a/sdk/metric/provider_test.go b/sdk/metric/provider_test.go index 262e4ac594d..e0daa0eef8b 100644 --- a/sdk/metric/provider_test.go +++ b/sdk/metric/provider_test.go @@ -24,14 +24,15 @@ import ( func TestMeterConcurrentSafe(*testing.T) { const name = "TestMeterConcurrentSafe meter" mp := NewMeterProvider() + opt := api.WithInstrumentationAttributes(attribute.String("key", "value")) done := make(chan struct{}) go func() { defer close(done) - _ = mp.Meter(name) + _ = mp.Meter(name, opt) }() - _ = mp.Meter(name) + _ = mp.Meter(name, opt) <-done } diff --git a/sdk/trace/provider_test.go b/sdk/trace/provider_test.go index 9fa1923a7ce..1e5d8c89072 100644 --- a/sdk/trace/provider_test.go +++ b/sdk/trace/provider_test.go @@ -8,6 +8,7 @@ import ( "errors" "fmt" "math/rand/v2" + "sync" "testing" "github.com/stretchr/testify/assert" @@ -401,6 +402,25 @@ func TestTracerProviderReturnsSameTracer(t *testing.T) { assert.Same(t, t2, t5) } +func TestTracerProviderConcurrentSafe(t *testing.T) { + p := NewTracerProvider(WithSyncer(NewTestExporter())) + + ctx := context.Background() + opt := trace.WithInstrumentationAttributes(attribute.String("key", "value")) + var wg sync.WaitGroup + for range 10 { + wg.Add(1) + go func() { + defer wg.Done() + + _ = p.Tracer(t.Name(), opt) + _ = p.Shutdown(ctx) + _ = p.ForceFlush(ctx) + }() + } + wg.Wait() +} + func TestTracerProviderSelfObservability(t *testing.T) { handler.Reset() p := NewTracerProvider() diff --git a/trace/config.go b/trace/config.go index 09fd04125c3..2237bd24fea 100644 --- a/trace/config.go +++ b/trace/config.go @@ -306,7 +306,7 @@ func WithInstrumentationVersion(version string) TracerOption { // WithInstrumentationAttributes sets the instrumentation attributes. // -// The passed attributes will be de-duplicated. +// Note that the passed attributes are de-duplicated by WithInstrumentationAttributes. func WithInstrumentationAttributes(attr ...attribute.KeyValue) TracerOption { set := attribute.NewSet(attr...) return tracerOptionFunc(func(config TracerConfig) TracerConfig { From 4ae95adb47086d79b2668ec08237844ffa6499b5 Mon Sep 17 00:00:00 2001 From: Andrew Wilkins Date: Tue, 2 Sep 2025 08:27:17 +0800 Subject: [PATCH 07/17] Update PR number --- CHANGELOG.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a456bd32f5b..2845d1d73bf 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -68,9 +68,9 @@ The next release will require at least [Go 1.24]. - Change `SDKProcessorLogQueueCapacity`, `SDKProcessorLogQueueSize`, `SDKProcessorSpanQueueSize`, and `SDKProcessorSpanQueueCapacity` in `go.opentelemetry.io/otel/semconv/v1.36.0/otelconv` to use a `Int64ObservableUpDownCounter`. (#7041) - Change `DefaultExemplarReservoirProviderSelector` in `go.opentelemetry.io/otel/sdk/metric` to use `runtime.GOMAXPROCS(0)` instead of `runtime.NumCPU()` for the `FixedSizeReservoirProvider` default size. (#7094) - Optimize `TraceIDFromHex` and `SpanIDFromHex` in `go.opentelemetry.io/otel/sdk/trace`. (#6791) -- `WithInstrumentationAttributes` in `go.opentelemetry.io/otel/trace` synchronously de-duplicates the passed attributes instead of delegating it to the returned `TracerOption`. (#7270) -- `WithInstrumentationAttributes` in `go.opentelemetry.io/otel/meter` synchronously de-duplicates the passed attributes instead of delegating it to the returned `MeterOption`. (#7270) -- `WithInstrumentationAttributes` in `go.opentelemetry.io/otel/log` synchronously de-duplicates the passed attributes instead of delegating it to the returned `LoggerOption`. (#7270) +- `WithInstrumentationAttributes` in `go.opentelemetry.io/otel/trace` synchronously de-duplicates the passed attributes instead of delegating it to the returned `TracerOption`. (#7266) +- `WithInstrumentationAttributes` in `go.opentelemetry.io/otel/meter` synchronously de-duplicates the passed attributes instead of delegating it to the returned `MeterOption`. (#7266) +- `WithInstrumentationAttributes` in `go.opentelemetry.io/otel/log` synchronously de-duplicates the passed attributes instead of delegating it to the returned `LoggerOption`. (#7266) ### Deprecated From 5879a58b1670d149209c9111c95f530f94492919 Mon Sep 17 00:00:00 2001 From: Andrew Wilkins Date: Tue, 2 Sep 2025 08:31:21 +0800 Subject: [PATCH 08/17] Fix changelog --- CHANGELOG.md | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 30de5a3824a..34897dfd92c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,12 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm ## [Unreleased] +### Changed + +- `WithInstrumentationAttributes` in `go.opentelemetry.io/otel/trace` synchronously de-duplicates the passed attributes instead of delegating it to the returned `TracerOption`. (#7266) +- `WithInstrumentationAttributes` in `go.opentelemetry.io/otel/meter` synchronously de-duplicates the passed attributes instead of delegating it to the returned `MeterOption`. (#7266) +- `WithInstrumentationAttributes` in `go.opentelemetry.io/otel/log` synchronously de-duplicates the passed attributes instead of delegating it to the returned `LoggerOption`. (#7266) + ### Removed - Drop support for [Go 1.23]. (#7274) @@ -76,9 +82,6 @@ The next release will require at least [Go 1.24]. - Optimize `TraceIDFromHex` and `SpanIDFromHex` in `go.opentelemetry.io/otel/sdk/trace`. (#6791) - Change `AssertEqual` in `go.opentelemetry.io/otel/log/logtest` to accept `TestingT` in order to support benchmarks and fuzz tests. (#6908) - Change `DefaultExemplarReservoirProviderSelector` in `go.opentelemetry.io/otel/sdk/metric` to use `runtime.GOMAXPROCS(0)` instead of `runtime.NumCPU()` for the `FixedSizeReservoirProvider` default size. (#7094) -- `WithInstrumentationAttributes` in `go.opentelemetry.io/otel/trace` synchronously de-duplicates the passed attributes instead of delegating it to the returned `TracerOption`. (#7266) -- `WithInstrumentationAttributes` in `go.opentelemetry.io/otel/meter` synchronously de-duplicates the passed attributes instead of delegating it to the returned `MeterOption`. (#7266) -- `WithInstrumentationAttributes` in `go.opentelemetry.io/otel/log` synchronously de-duplicates the passed attributes instead of delegating it to the returned `LoggerOption`. (#7266) ### Fixed @@ -86,9 +89,6 @@ The next release will require at least [Go 1.24]. - Fix `go.opentelemetry.io/otel/exporters/prometheus` to not append a suffix if it's already present in metric name. (#7088) - Fix the `go.opentelemetry.io/otel/exporters/stdout/stdouttrace` self-observability component type and name. (#7195) - Fix partial export count metric in `go.opentelemetry.io/otel/exporters/stdout/stdouttrace`. (#7199) -- Fix a data race when reusing the `WithInstrumentationAttributes` option in concurrent calls to `go.opentelemetry.io/otel/trace.TraceProvider.Tracer`. (#7266) -- Fix a data race when reusing the `WithInstrumentationAttributes` option in concurrent calls to `go.opentelemetry.io/otel/metric.MeterProvider.Meter`. (#7266) -- Fix a data race when reusing the `WithInstrumentationAttributes` option in concurrent calls to `go.opentelemetry.io/otel/log.LogProvider.Logger`. (#7266) ### Deprecated From 1602472e7126499ffa2c9c4bb22f2eb473ac6436 Mon Sep 17 00:00:00 2001 From: Andrew Wilkins Date: Tue, 9 Sep 2025 10:46:55 +0800 Subject: [PATCH 09/17] Delegate to WithInstrumentationAttributesSet --- log/logger.go | 23 ++++------------------- metric/config.go | 23 ++++------------------- trace/config.go | 23 ++++------------------- 3 files changed, 12 insertions(+), 57 deletions(-) diff --git a/log/logger.go b/log/logger.go index d63e7df5a51..a2cc3c70ef2 100644 --- a/log/logger.go +++ b/log/logger.go @@ -129,30 +129,15 @@ func mergeSets(a, b attribute.Set) attribute.Set { // WithInstrumentationAttributes returns a [LoggerOption] that sets the // instrumentation attributes of a [Logger]. // -// The passed attributes will be de-duplicated. -// -// Note that [WithInstrumentationAttributeSet] is recommended as -// it is more efficient and also allows safely reusing the passed argument. +// This is equivalent to calling WithInstrumentationAttributeSet with an +// attribute.Set created from the passed attributes. +// [WithInstrumentationAttributeSet] is recommended for more control. // // If multiple [WithInstrumentationAttributes] or [WithInstrumentationAttributeSet] // options are passed, the attributes will be merged together in the order // they are passed. Attributes with duplicate keys will use the last value passed. func WithInstrumentationAttributes(attr ...attribute.KeyValue) LoggerOption { - if len(attr) == 0 { - return loggerOptionFunc(func(config LoggerConfig) LoggerConfig { - return config - }) - } - - newAttrs := attribute.NewSet(attr...) - return loggerOptionFunc(func(config LoggerConfig) LoggerConfig { - if config.attrs.Len() == 0 { - config.attrs = newAttrs - } else { - config.attrs = mergeSets(config.attrs, newAttrs) - } - return config - }) + return WithInstrumentationAttributeSet(attribute.NewSet(attr...)) } // WithInstrumentationAttributeSet returns a [LoggerOption] that adds the diff --git a/metric/config.go b/metric/config.go index d532f314b88..07a39edc3ee 100644 --- a/metric/config.go +++ b/metric/config.go @@ -64,30 +64,15 @@ func WithInstrumentationVersion(version string) MeterOption { // WithInstrumentationAttributes adds the instrumentation attributes. // -// The passed attributes will be de-duplicated. -// -// Note that [WithInstrumentationAttributeSet] is recommended as -// it is more efficient and also allows safely reusing the passed argument. +// This is equivalent to calling WithInstrumentationAttributeSet with an +// attribute.Set created from the passed attributes. +// [WithInstrumentationAttributeSet] is recommended for more control. // // If multiple [WithInstrumentationAttributes] or [WithInstrumentationAttributeSet] // options are passed, the attributes will be merged together in the order // they are passed. Attributes with duplicate keys will use the last value passed. func WithInstrumentationAttributes(attr ...attribute.KeyValue) MeterOption { - if len(attr) == 0 { - return meterOptionFunc(func(config MeterConfig) MeterConfig { - return config - }) - } - - newAttrs := attribute.NewSet(attr...) - return meterOptionFunc(func(config MeterConfig) MeterConfig { - if config.attrs.Len() == 0 { - config.attrs = newAttrs - } else { - config.attrs = mergeSets(config.attrs, newAttrs) - } - return config - }) + return WithInstrumentationAttributeSet(attribute.NewSet(attr...)) } // WithInstrumentationAttributeSet adds the instrumentation attributes. diff --git a/trace/config.go b/trace/config.go index 1b2ead96730..738faed58e3 100644 --- a/trace/config.go +++ b/trace/config.go @@ -318,30 +318,15 @@ func mergeSets(a, b attribute.Set) attribute.Set { // WithInstrumentationAttributes adds the instrumentation attributes. // -// The passed attributes will be de-duplicated. -// -// Note that [WithInstrumentationAttributeSet] is recommended as -// it is more efficient and also allows safely reusing the passed argument. +// This is equivalent to calling WithInstrumentationAttributeSet with an +// attribute.Set created from the passed attributes. +// [WithInstrumentationAttributeSet] is recommended for more control. // // If multiple [WithInstrumentationAttributes] or [WithInstrumentationAttributeSet] // options are passed, the attributes will be merged together in the order // they are passed. Attributes with duplicate keys will use the last value passed. func WithInstrumentationAttributes(attr ...attribute.KeyValue) TracerOption { - if len(attr) == 0 { - return tracerOptionFunc(func(config TracerConfig) TracerConfig { - return config - }) - } - - newAttrs := attribute.NewSet(attr...) - return tracerOptionFunc(func(config TracerConfig) TracerConfig { - if config.attrs.Len() == 0 { - config.attrs = newAttrs - } else { - config.attrs = mergeSets(config.attrs, newAttrs) - } - return config - }) + return WithInstrumentationAttributeSet(attribute.NewSet(attr...)) } // WithInstrumentationAttributeSet adds the instrumentation attributes. From f9ea4b8d7657278cd21c4b7b148e26662eaaa3e7 Mon Sep 17 00:00:00 2001 From: Andrew Wilkins Date: Tue, 9 Sep 2025 10:58:18 +0800 Subject: [PATCH 10/17] Split out concurrency tests --- log/logger_test.go | 27 +++++++++++++++++++-------- metric/config_test.go | 29 ++++++++++++++++++++--------- trace/config_test.go | 27 +++++++++++++++++++-------- 3 files changed, 58 insertions(+), 25 deletions(-) diff --git a/log/logger_test.go b/log/logger_test.go index 6a44f463a80..ef19816ad93 100644 --- a/log/logger_test.go +++ b/log/logger_test.go @@ -16,16 +16,29 @@ import ( func TestNewLoggerConfig(t *testing.T) { version := "v1.1.1" schemaURL := "https://opentelemetry.io/schemas/1.37.0" - attr := []attribute.KeyValue{ + attr := attribute.NewSet( attribute.String("user", "alice"), attribute.Bool("admin", true), - } - attrSet := attribute.NewSet(attr...) - options := []log.LoggerOption{ + ) + + c := log.NewLoggerConfig( log.WithInstrumentationVersion(version), log.WithSchemaURL(schemaURL), - log.WithInstrumentationAttributes(attr...), + log.WithInstrumentationAttributes(attr.ToSlice()...), + ) + + assert.Equal(t, version, c.InstrumentationVersion(), "instrumentation version") + assert.Equal(t, schemaURL, c.SchemaURL(), "schema URL") + assert.Equal(t, attr, c.InstrumentationAttributes(), "instrumentation attributes") +} + +func TestWithInstrumentationAttributesConcurrentSafe(t *testing.T) { + attr := []attribute.KeyValue{ + attribute.String("user", "alice"), + attribute.Bool("admin", true), } + attrSet := attribute.NewSet(attr...) + option := log.WithInstrumentationAttributes(attr...) // Modifications to attr should not affect the config. attr[0] = attribute.String("user", "bob") @@ -36,9 +49,7 @@ func TestNewLoggerConfig(t *testing.T) { wg.Add(1) go func() { defer wg.Done() - c := log.NewLoggerConfig(options...) - assert.Equal(t, version, c.InstrumentationVersion(), "instrumentation version") - assert.Equal(t, schemaURL, c.SchemaURL(), "schema URL") + c := log.NewLoggerConfig(option) assert.Equal(t, attrSet, c.InstrumentationAttributes(), "instrumentation attributes") }() } diff --git a/metric/config_test.go b/metric/config_test.go index cc3c79c8ffa..4af6a6cc887 100644 --- a/metric/config_test.go +++ b/metric/config_test.go @@ -13,19 +13,32 @@ import ( "go.opentelemetry.io/otel/metric" ) -func TestNewMeterConfig(t *testing.T) { +func TestConfig(t *testing.T) { version := "v1.1.1" schemaURL := "https://opentelemetry.io/schemas/1.37.0" - attr := []attribute.KeyValue{ + attr := attribute.NewSet( attribute.String("user", "alice"), attribute.Bool("admin", true), - } - attrSet := attribute.NewSet(attr...) - options := []metric.MeterOption{ + ) + + c := metric.NewMeterConfig( metric.WithInstrumentationVersion(version), metric.WithSchemaURL(schemaURL), - metric.WithInstrumentationAttributes(attr...), + metric.WithInstrumentationAttributes(attr.ToSlice()...), + ) + + assert.Equal(t, version, c.InstrumentationVersion(), "instrumentation version") + assert.Equal(t, schemaURL, c.SchemaURL(), "schema URL") + assert.Equal(t, attr, c.InstrumentationAttributes(), "instrumentation attributes") +} + +func TestWithInstrumentationAttributesConcurrentSafe(t *testing.T) { + attr := []attribute.KeyValue{ + attribute.String("user", "alice"), + attribute.Bool("admin", true), } + attrSet := attribute.NewSet(attr...) + option := metric.WithInstrumentationAttributes(attr...) // Modifications to attr should not affect the config. attr[0] = attribute.String("user", "bob") @@ -36,9 +49,7 @@ func TestNewMeterConfig(t *testing.T) { wg.Add(1) go func() { defer wg.Done() - c := metric.NewMeterConfig(options...) - assert.Equal(t, version, c.InstrumentationVersion(), "instrumentation version") - assert.Equal(t, schemaURL, c.SchemaURL(), "schema URL") + c := metric.NewMeterConfig(option) assert.Equal(t, attrSet, c.InstrumentationAttributes(), "instrumentation attributes") }() } diff --git a/trace/config_test.go b/trace/config_test.go index bcdce115de9..038d26a5f92 100644 --- a/trace/config_test.go +++ b/trace/config_test.go @@ -214,18 +214,31 @@ func TestTracerConfig(t *testing.T) { v1 := "semver:0.0.1" v2 := "semver:1.0.0" schemaURL := "https://opentelemetry.io/schemas/1.21.0" - attrs := []attribute.KeyValue{ + attrs := attribute.NewSet( attribute.String("user", "alice"), attribute.Bool("admin", true), - } - attrSet := attribute.NewSet(attrs...) - options := []TracerOption{ + ) + + c := NewTracerConfig( // Multiple calls should overwrite. WithInstrumentationVersion(v1), WithInstrumentationVersion(v2), WithSchemaURL(schemaURL), - WithInstrumentationAttributes(attrs...), + WithInstrumentationAttributes(attrs.ToSlice()...), + ) + + assert.Equal(t, v2, c.InstrumentationVersion(), "instrumentation version") + assert.Equal(t, schemaURL, c.SchemaURL(), "schema URL") + assert.Equal(t, attrs, c.InstrumentationAttributes(), "instrumentation attributes") +} + +func TestWithInstrumentationAttributesConcurrentSafe(t *testing.T) { + attrs := []attribute.KeyValue{ + attribute.String("user", "alice"), + attribute.Bool("admin", true), } + attrSet := attribute.NewSet(attrs...) + option := WithInstrumentationAttributes(attrs...) // Modifications to attr should not affect the config. attrs[0] = attribute.String("user", "bob") @@ -236,9 +249,7 @@ func TestTracerConfig(t *testing.T) { wg.Add(1) go func() { defer wg.Done() - c := NewTracerConfig(options...) - assert.Equal(t, v2, c.InstrumentationVersion(), "instrumentation version") - assert.Equal(t, schemaURL, c.SchemaURL(), "schema URL") + c := NewTracerConfig(option) assert.Equal(t, attrSet, c.InstrumentationAttributes(), "instrumentation attributes") }() } From d645366f7330f1420f41ea0e145e7b32ee674882 Mon Sep 17 00:00:00 2001 From: Andrew Wilkins Date: Wed, 10 Sep 2025 10:13:16 +0800 Subject: [PATCH 11/17] Apply suggestions from code review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Robert Pająk Co-authored-by: Flc゛ --- log/logger.go | 2 +- metric/config.go | 2 +- trace/config.go | 4 ++-- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/log/logger.go b/log/logger.go index a2cc3c70ef2..1f107a6ab5a 100644 --- a/log/logger.go +++ b/log/logger.go @@ -130,7 +130,7 @@ func mergeSets(a, b attribute.Set) attribute.Set { // instrumentation attributes of a [Logger]. // // This is equivalent to calling WithInstrumentationAttributeSet with an -// attribute.Set created from the passed attributes. +// [attribute.Set] created from the passed attributes. // [WithInstrumentationAttributeSet] is recommended for more control. // // If multiple [WithInstrumentationAttributes] or [WithInstrumentationAttributeSet] diff --git a/metric/config.go b/metric/config.go index 07a39edc3ee..1e21d6cc79d 100644 --- a/metric/config.go +++ b/metric/config.go @@ -65,7 +65,7 @@ func WithInstrumentationVersion(version string) MeterOption { // WithInstrumentationAttributes adds the instrumentation attributes. // // This is equivalent to calling WithInstrumentationAttributeSet with an -// attribute.Set created from the passed attributes. +// [attribute.Set] created from the passed attributes. // [WithInstrumentationAttributeSet] is recommended for more control. // // If multiple [WithInstrumentationAttributes] or [WithInstrumentationAttributeSet] diff --git a/trace/config.go b/trace/config.go index 738faed58e3..b74758d7470 100644 --- a/trace/config.go +++ b/trace/config.go @@ -318,8 +318,8 @@ func mergeSets(a, b attribute.Set) attribute.Set { // WithInstrumentationAttributes adds the instrumentation attributes. // -// This is equivalent to calling WithInstrumentationAttributeSet with an -// attribute.Set created from the passed attributes. +// This is equivalent to calling [WithInstrumentationAttributeSet] with an +// [attribute.Set] created from the passed attributes. // [WithInstrumentationAttributeSet] is recommended for more control. // // If multiple [WithInstrumentationAttributes] or [WithInstrumentationAttributeSet] From c8178ceb0881ea5229c0475ec8a0bc5a319984d7 Mon Sep 17 00:00:00 2001 From: Andrew Wilkins Date: Wed, 10 Sep 2025 10:13:58 +0800 Subject: [PATCH 12/17] Update metric/config.go MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Flc゛ --- metric/config.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/metric/config.go b/metric/config.go index 1e21d6cc79d..d72268aa09b 100644 --- a/metric/config.go +++ b/metric/config.go @@ -64,7 +64,7 @@ func WithInstrumentationVersion(version string) MeterOption { // WithInstrumentationAttributes adds the instrumentation attributes. // -// This is equivalent to calling WithInstrumentationAttributeSet with an +// This is equivalent to calling [WithInstrumentationAttributeSet] with an // [attribute.Set] created from the passed attributes. // [WithInstrumentationAttributeSet] is recommended for more control. // From 20ab6a27efe930023b196e0def167b740681fb37 Mon Sep 17 00:00:00 2001 From: Andrew Wilkins Date: Wed, 10 Sep 2025 10:09:32 +0800 Subject: [PATCH 13/17] Remove tests --- log/logger_test.go | 25 ------------------------- metric/config_test.go | 25 ------------------------- trace/config_test.go | 25 ------------------------- 3 files changed, 75 deletions(-) diff --git a/log/logger_test.go b/log/logger_test.go index ef19816ad93..137a3284c47 100644 --- a/log/logger_test.go +++ b/log/logger_test.go @@ -4,7 +4,6 @@ package log_test import ( - "sync" "testing" "github.com/stretchr/testify/assert" @@ -32,30 +31,6 @@ func TestNewLoggerConfig(t *testing.T) { assert.Equal(t, attr, c.InstrumentationAttributes(), "instrumentation attributes") } -func TestWithInstrumentationAttributesConcurrentSafe(t *testing.T) { - attr := []attribute.KeyValue{ - attribute.String("user", "alice"), - attribute.Bool("admin", true), - } - attrSet := attribute.NewSet(attr...) - option := log.WithInstrumentationAttributes(attr...) - - // Modifications to attr should not affect the config. - attr[0] = attribute.String("user", "bob") - - // Ensure that options can be used concurrently. - var wg sync.WaitGroup - for range 10 { - wg.Add(1) - go func() { - defer wg.Done() - c := log.NewLoggerConfig(option) - assert.Equal(t, attrSet, c.InstrumentationAttributes(), "instrumentation attributes") - }() - } - wg.Wait() -} - func TestWithInstrumentationAttributeSet(t *testing.T) { attrs := attribute.NewSet( attribute.String("service", "test"), diff --git a/metric/config_test.go b/metric/config_test.go index 4af6a6cc887..07f0088a95e 100644 --- a/metric/config_test.go +++ b/metric/config_test.go @@ -4,7 +4,6 @@ package metric_test import ( - "sync" "testing" "github.com/stretchr/testify/assert" @@ -32,30 +31,6 @@ func TestConfig(t *testing.T) { assert.Equal(t, attr, c.InstrumentationAttributes(), "instrumentation attributes") } -func TestWithInstrumentationAttributesConcurrentSafe(t *testing.T) { - attr := []attribute.KeyValue{ - attribute.String("user", "alice"), - attribute.Bool("admin", true), - } - attrSet := attribute.NewSet(attr...) - option := metric.WithInstrumentationAttributes(attr...) - - // Modifications to attr should not affect the config. - attr[0] = attribute.String("user", "bob") - - // Ensure that options can be used concurrently. - var wg sync.WaitGroup - for range 10 { - wg.Add(1) - go func() { - defer wg.Done() - c := metric.NewMeterConfig(option) - assert.Equal(t, attrSet, c.InstrumentationAttributes(), "instrumentation attributes") - }() - } - wg.Wait() -} - func TestWithInstrumentationAttributeSet(t *testing.T) { attrs := attribute.NewSet( attribute.String("service", "test"), diff --git a/trace/config_test.go b/trace/config_test.go index 038d26a5f92..e884150256e 100644 --- a/trace/config_test.go +++ b/trace/config_test.go @@ -4,7 +4,6 @@ package trace import ( - "sync" "testing" "time" @@ -232,30 +231,6 @@ func TestTracerConfig(t *testing.T) { assert.Equal(t, attrs, c.InstrumentationAttributes(), "instrumentation attributes") } -func TestWithInstrumentationAttributesConcurrentSafe(t *testing.T) { - attrs := []attribute.KeyValue{ - attribute.String("user", "alice"), - attribute.Bool("admin", true), - } - attrSet := attribute.NewSet(attrs...) - option := WithInstrumentationAttributes(attrs...) - - // Modifications to attr should not affect the config. - attrs[0] = attribute.String("user", "bob") - - // Ensure that options can be used concurrently. - var wg sync.WaitGroup - for range 10 { - wg.Add(1) - go func() { - defer wg.Done() - c := NewTracerConfig(option) - assert.Equal(t, attrSet, c.InstrumentationAttributes(), "instrumentation attributes") - }() - } - wg.Wait() -} - func TestWithInstrumentationAttributeSet(t *testing.T) { attrs := attribute.NewSet( attribute.String("service", "test"), From dbb9d49d2c7fb88d98bc38d302cd73a234f17a46 Mon Sep 17 00:00:00 2001 From: Andrew Wilkins Date: Wed, 10 Sep 2025 10:40:33 +0800 Subject: [PATCH 14/17] Clone attributes before creating sets This prevents races while creating the set. --- log/logger.go | 4 +++- metric/config.go | 9 +++++++-- trace/config.go | 4 +++- 3 files changed, 13 insertions(+), 4 deletions(-) diff --git a/log/logger.go b/log/logger.go index 1f107a6ab5a..035f535540f 100644 --- a/log/logger.go +++ b/log/logger.go @@ -5,6 +5,7 @@ package log // import "go.opentelemetry.io/otel/log" import ( "context" + "slices" "go.opentelemetry.io/otel/attribute" "go.opentelemetry.io/otel/log/embedded" @@ -137,7 +138,8 @@ func mergeSets(a, b attribute.Set) attribute.Set { // options are passed, the attributes will be merged together in the order // they are passed. Attributes with duplicate keys will use the last value passed. func WithInstrumentationAttributes(attr ...attribute.KeyValue) LoggerOption { - return WithInstrumentationAttributeSet(attribute.NewSet(attr...)) + set := attribute.NewSet(slices.Clone(attr)...) + return WithInstrumentationAttributeSet(set) } // WithInstrumentationAttributeSet returns a [LoggerOption] that adds the diff --git a/metric/config.go b/metric/config.go index d72268aa09b..b068fc9d739 100644 --- a/metric/config.go +++ b/metric/config.go @@ -3,7 +3,11 @@ package metric // import "go.opentelemetry.io/otel/metric" -import "go.opentelemetry.io/otel/attribute" +import ( + "slices" + + "go.opentelemetry.io/otel/attribute" +) // MeterConfig contains options for Meters. type MeterConfig struct { @@ -72,7 +76,8 @@ func WithInstrumentationVersion(version string) MeterOption { // options are passed, the attributes will be merged together in the order // they are passed. Attributes with duplicate keys will use the last value passed. func WithInstrumentationAttributes(attr ...attribute.KeyValue) MeterOption { - return WithInstrumentationAttributeSet(attribute.NewSet(attr...)) + set := attribute.NewSet(slices.Clone(attr)...) + return WithInstrumentationAttributeSet(set) } // WithInstrumentationAttributeSet adds the instrumentation attributes. diff --git a/trace/config.go b/trace/config.go index b74758d7470..8d797e85723 100644 --- a/trace/config.go +++ b/trace/config.go @@ -4,6 +4,7 @@ package trace // import "go.opentelemetry.io/otel/trace" import ( + "slices" "time" "go.opentelemetry.io/otel/attribute" @@ -326,7 +327,8 @@ func mergeSets(a, b attribute.Set) attribute.Set { // options are passed, the attributes will be merged together in the order // they are passed. Attributes with duplicate keys will use the last value passed. func WithInstrumentationAttributes(attr ...attribute.KeyValue) TracerOption { - return WithInstrumentationAttributeSet(attribute.NewSet(attr...)) + set := attribute.NewSet(slices.Clone(attr)...) + return WithInstrumentationAttributeSet(set) } // WithInstrumentationAttributeSet adds the instrumentation attributes. From 808bef9bbe6053241d611016205e5bc369321185 Mon Sep 17 00:00:00 2001 From: Andrew Wilkins Date: Wed, 10 Sep 2025 10:56:26 +0800 Subject: [PATCH 15/17] Clarify comments --- log/logger.go | 2 +- metric/config.go | 2 +- trace/config.go | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/log/logger.go b/log/logger.go index 035f535540f..d9decebdef1 100644 --- a/log/logger.go +++ b/log/logger.go @@ -131,7 +131,7 @@ func mergeSets(a, b attribute.Set) attribute.Set { // instrumentation attributes of a [Logger]. // // This is equivalent to calling WithInstrumentationAttributeSet with an -// [attribute.Set] created from the passed attributes. +// [attribute.Set] created from a clone of the passed attributes. // [WithInstrumentationAttributeSet] is recommended for more control. // // If multiple [WithInstrumentationAttributes] or [WithInstrumentationAttributeSet] diff --git a/metric/config.go b/metric/config.go index b068fc9d739..e42dd6e70ab 100644 --- a/metric/config.go +++ b/metric/config.go @@ -69,7 +69,7 @@ func WithInstrumentationVersion(version string) MeterOption { // WithInstrumentationAttributes adds the instrumentation attributes. // // This is equivalent to calling [WithInstrumentationAttributeSet] with an -// [attribute.Set] created from the passed attributes. +// [attribute.Set] created from a clone of the passed attributes. // [WithInstrumentationAttributeSet] is recommended for more control. // // If multiple [WithInstrumentationAttributes] or [WithInstrumentationAttributeSet] diff --git a/trace/config.go b/trace/config.go index 8d797e85723..d9ecef1cad2 100644 --- a/trace/config.go +++ b/trace/config.go @@ -320,7 +320,7 @@ func mergeSets(a, b attribute.Set) attribute.Set { // WithInstrumentationAttributes adds the instrumentation attributes. // // This is equivalent to calling [WithInstrumentationAttributeSet] with an -// [attribute.Set] created from the passed attributes. +// [attribute.Set] created from a clone of the passed attributes. // [WithInstrumentationAttributeSet] is recommended for more control. // // If multiple [WithInstrumentationAttributes] or [WithInstrumentationAttributeSet] From 8843cdb4aa64720051409929a3fb799d12b1e999 Mon Sep 17 00:00:00 2001 From: Andrew Wilkins Date: Thu, 11 Sep 2025 08:41:31 +0800 Subject: [PATCH 16/17] Revert test changes --- sdk/log/provider_test.go | 16 +++++++++------- sdk/metric/provider_test.go | 5 ++--- sdk/trace/provider_test.go | 20 -------------------- 3 files changed, 11 insertions(+), 30 deletions(-) diff --git a/sdk/log/provider_test.go b/sdk/log/provider_test.go index 0b6e33062a2..23a74630cea 100644 --- a/sdk/log/provider_test.go +++ b/sdk/log/provider_test.go @@ -249,22 +249,24 @@ func TestWithResource(t *testing.T) { } func TestLoggerProviderConcurrentSafe(*testing.T) { - p := NewLoggerProvider(WithProcessor(newProcessor("0"))) + const goRoutineN = 10 - ctx := context.Background() - const name = "testLogger" - opt := log.WithInstrumentationAttributes(attribute.String("key", "value")) var wg sync.WaitGroup - for range 10 { - wg.Add(1) + wg.Add(goRoutineN) + + p := NewLoggerProvider(WithProcessor(newProcessor("0"))) + const name = "testLogger" + ctx := context.Background() + for range goRoutineN { go func() { defer wg.Done() - _ = p.Logger(name, opt) + _ = p.Logger(name) _ = p.Shutdown(ctx) _ = p.ForceFlush(ctx) }() } + wg.Wait() } diff --git a/sdk/metric/provider_test.go b/sdk/metric/provider_test.go index e0daa0eef8b..262e4ac594d 100644 --- a/sdk/metric/provider_test.go +++ b/sdk/metric/provider_test.go @@ -24,15 +24,14 @@ import ( func TestMeterConcurrentSafe(*testing.T) { const name = "TestMeterConcurrentSafe meter" mp := NewMeterProvider() - opt := api.WithInstrumentationAttributes(attribute.String("key", "value")) done := make(chan struct{}) go func() { defer close(done) - _ = mp.Meter(name, opt) + _ = mp.Meter(name) }() - _ = mp.Meter(name, opt) + _ = mp.Meter(name) <-done } diff --git a/sdk/trace/provider_test.go b/sdk/trace/provider_test.go index 1e5d8c89072..9fa1923a7ce 100644 --- a/sdk/trace/provider_test.go +++ b/sdk/trace/provider_test.go @@ -8,7 +8,6 @@ import ( "errors" "fmt" "math/rand/v2" - "sync" "testing" "github.com/stretchr/testify/assert" @@ -402,25 +401,6 @@ func TestTracerProviderReturnsSameTracer(t *testing.T) { assert.Same(t, t2, t5) } -func TestTracerProviderConcurrentSafe(t *testing.T) { - p := NewTracerProvider(WithSyncer(NewTestExporter())) - - ctx := context.Background() - opt := trace.WithInstrumentationAttributes(attribute.String("key", "value")) - var wg sync.WaitGroup - for range 10 { - wg.Add(1) - go func() { - defer wg.Done() - - _ = p.Tracer(t.Name(), opt) - _ = p.Shutdown(ctx) - _ = p.ForceFlush(ctx) - }() - } - wg.Wait() -} - func TestTracerProviderSelfObservability(t *testing.T) { handler.Reset() p := NewTracerProvider() From e9b3b8163f52f4f3b4de1bd1d0489f47f7573a57 Mon Sep 17 00:00:00 2001 From: Andrew Wilkins Date: Thu, 11 Sep 2025 08:46:39 +0800 Subject: [PATCH 17/17] New tests --- log/logger_test.go | 17 +++++++++++++++++ metric/config_test.go | 17 +++++++++++++++++ trace/config_test.go | 17 +++++++++++++++++ 3 files changed, 51 insertions(+) diff --git a/log/logger_test.go b/log/logger_test.go index 137a3284c47..e2b6e972c4a 100644 --- a/log/logger_test.go +++ b/log/logger_test.go @@ -31,6 +31,23 @@ func TestNewLoggerConfig(t *testing.T) { assert.Equal(t, attr, c.InstrumentationAttributes(), "instrumentation attributes") } +func TestWithInstrumentationAttributesNotLazy(t *testing.T) { + attrs := []attribute.KeyValue{ + attribute.String("service", "test"), + attribute.Int("three", 3), + } + want := attribute.NewSet(attrs...) + + // WithInstrumentationAttributes is expected to immediately + // create an immutable set from the attributes, so later changes + // to attrs should not affect the config. + opt := log.WithInstrumentationAttributes(attrs...) + attrs[0] = attribute.String("service", "changed") + + c := log.NewLoggerConfig(opt) + assert.Equal(t, want, c.InstrumentationAttributes(), "instrumentation attributes") +} + func TestWithInstrumentationAttributeSet(t *testing.T) { attrs := attribute.NewSet( attribute.String("service", "test"), diff --git a/metric/config_test.go b/metric/config_test.go index 07f0088a95e..a24ba1ca59d 100644 --- a/metric/config_test.go +++ b/metric/config_test.go @@ -31,6 +31,23 @@ func TestConfig(t *testing.T) { assert.Equal(t, attr, c.InstrumentationAttributes(), "instrumentation attributes") } +func TestWithInstrumentationAttributesNotLazy(t *testing.T) { + attrs := []attribute.KeyValue{ + attribute.String("service", "test"), + attribute.Int("three", 3), + } + want := attribute.NewSet(attrs...) + + // WithInstrumentationAttributes is expected to immediately + // create an immutable set from the attributes, so later changes + // to attrs should not affect the config. + opt := metric.WithInstrumentationAttributes(attrs...) + attrs[0] = attribute.String("service", "changed") + + c := metric.NewMeterConfig(opt) + assert.Equal(t, want, c.InstrumentationAttributes(), "instrumentation attributes") +} + func TestWithInstrumentationAttributeSet(t *testing.T) { attrs := attribute.NewSet( attribute.String("service", "test"), diff --git a/trace/config_test.go b/trace/config_test.go index e884150256e..40667fa01b3 100644 --- a/trace/config_test.go +++ b/trace/config_test.go @@ -231,6 +231,23 @@ func TestTracerConfig(t *testing.T) { assert.Equal(t, attrs, c.InstrumentationAttributes(), "instrumentation attributes") } +func TestWithInstrumentationAttributesNotLazy(t *testing.T) { + attrs := []attribute.KeyValue{ + attribute.String("service", "test"), + attribute.Int("three", 3), + } + want := attribute.NewSet(attrs...) + + // WithInstrumentationAttributes is expected to immediately + // create an immutable set from the attributes, so later changes + // to attrs should not affect the config. + opt := WithInstrumentationAttributes(attrs...) + attrs[0] = attribute.String("service", "changed") + + c := NewTracerConfig(opt) + assert.Equal(t, want, c.InstrumentationAttributes(), "instrumentation attributes") +} + func TestWithInstrumentationAttributeSet(t *testing.T) { attrs := attribute.NewSet( attribute.String("service", "test"),