From 07a91dd2b07f8ba752b78e212b9404f26f43c973 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20Paj=C4=85k?= Date: Mon, 8 Sep 2025 17:21:37 +0200 Subject: [PATCH] trace,metric,log: add WithInstrumentationAttributeSet option (#7287) Per https://github.com/open-telemetry/opentelemetry-go/pull/7266#issuecomment-3237027300 Related to https://github.com/open-telemetry/opentelemetry-go/issues/7217 ## What This PR adds `WithInstrumentationAttributeSet` option functions to the `log`, `metric`, and `trace` packages as suggested in https://github.com/open-telemetry/opentelemetry-go/pull/7266#issuecomment-3237027300. These new functions provide a more concurrent-safe alternative to the existing `WithInstrumentationAttributes` functions by accepting a pre-constructed `attribute.Set` instead of variadic `attribute.KeyValue` parameters. ## Why As discussed in #7266, the existing `WithInstrumentationAttributes` functions can lead to data races when used concurrently because `attribute.NewSet()` may mutate the passed slice in-place. While the issue was partially addressed by moving the `attribute.NewSet()` call outside the closure, the best long-term solution is to provide an alternative that accepts an immutable `attribute.Set`. **Benefits:** 1. **Concurrent Safety**: Since `attribute.Set` is immutable, these functions are inherently safe for concurrent use 2. **Performance**: Avoids repeated calls to `attribute.NewSet()` when the same attributes are used multiple times 3. **Consistency**: Matches the existing pattern used in `metric.WithAttributeSet()` 4. **Flexibility**: Allows users to pre-compute attribute sets and reuse them Deprecating `WithInstrumentationAttributes` is out of scope. See https://github.com/open-telemetry/opentelemetry-go/pull/7287#issuecomment-3245820459. ## Benchmarks ``` goos: linux goarch: amd64 pkg: go.opentelemetry.io/otel/trace cpu: 13th Gen Intel(R) Core(TM) i7-13800H BenchmarkNewTracerConfig/with_no_options-20 280298306 4.268 ns/op 0 B/op 0 allocs/op BenchmarkNewTracerConfig/with_an_instrumentation_version-20 33389427 30.84 ns/op 0 B/op 0 allocs/op BenchmarkNewTracerConfig/with_a_schema_url-20 35441077 30.46 ns/op 0 B/op 0 allocs/op BenchmarkNewTracerConfig/with_instrumentation_attribute-20 17607649 88.23 ns/op 64 B/op 1 allocs/op BenchmarkNewTracerConfig/with_instrumentation_attribute_set-20 38336211 31.30 ns/op 0 B/op 0 allocs/op ``` ``` goos: linux goarch: amd64 pkg: go.opentelemetry.io/otel/metric cpu: 13th Gen Intel(R) Core(TM) i7-13800H BenchmarkNewMeterConfig/with_no_options-20 262998199 4.525 ns/op 0 B/op 0 allocs/op BenchmarkNewMeterConfig/with_an_instrumentation_version-20 40483780 29.31 ns/op 0 B/op 0 allocs/op BenchmarkNewMeterConfig/with_a_schema_url-20 39162420 30.58 ns/op 0 B/op 0 allocs/op BenchmarkNewMeterConfig/with_instrumentation_attribute-20 19900275 77.50 ns/op 64 B/op 1 allocs/op BenchmarkNewMeterConfig/with_instrumentation_attribute_set-20 37519020 31.93 ns/op 0 B/op 0 allocs/op ``` ``` goos: linux goarch: amd64 pkg: go.opentelemetry.io/otel/log cpu: 13th Gen Intel(R) Core(TM) i7-13800H BenchmarkNewLoggerConfig/with_no_options-20 271100760 4.322 ns/op 0 B/op 0 allocs/op BenchmarkNewLoggerConfig/with_an_instrumentation_version-20 38392390 30.77 ns/op 0 B/op 0 allocs/op BenchmarkNewLoggerConfig/with_a_schema_url-20 39615074 30.25 ns/op 0 B/op 0 allocs/op BenchmarkNewLoggerConfig/with_instrumentation_attribute-20 17108463 82.51 ns/op 64 B/op 1 allocs/op BenchmarkNewLoggerConfig/with_instrumentation_attribute_set-20 37746534 31.70 ns/op 0 B/op 0 allocs/op ``` --- CHANGELOG.md | 5 +++ log/logger.go | 32 +++++++++++-- log/logger_test.go | 102 ++++++++++++++++++++++++++++++++++++++++++ metric/config.go | 33 ++++++++++++-- metric/config_test.go | 102 ++++++++++++++++++++++++++++++++++++++++++ trace/config.go | 33 ++++++++++++-- trace/config_test.go | 79 +++++++++++++++++++++++++++++--- 7 files changed, 370 insertions(+), 16 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index dee7836d0..b3abe4588 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,11 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm ## [Unreleased] +### Added + +- Add `WithInstrumentationAttributeSet` option to `go.opentelemetry.io/otel/log`, `go.opentelemetry.io/otel/metric`, and `go.opentelemetry.io/otel/trace` packages. + This provides a concurrent-safe and performant alternative to `WithInstrumentationAttributes` by accepting a pre-constructed `attribute.Set`. (#7287) + ### Fixed - Fix `WithInstrumentationAttributes` options in `go.opentelemetry.io/otel/trace`, `go.opentelemetry.io/otel/metric`, and `go.opentelemetry.io/otel/log` to properly merge attributes when passed multiple times instead of replacing them. Attributes with duplicate keys will use the last value passed. (#7300) diff --git a/log/logger.go b/log/logger.go index 3cd60ed55..c2fa322b1 100644 --- a/log/logger.go +++ b/log/logger.go @@ -131,9 +131,12 @@ func mergeSets(a, b attribute.Set) attribute.Set { // // The passed attributes will be de-duplicated. // -// If multiple WithInstrumentationAttributes 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. +// Note that [WithInstrumentationAttributeSet] is recommended as +// it is more efficient and also allows safely reusing the passed argument. +// +// 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 { @@ -152,6 +155,29 @@ func WithInstrumentationAttributes(attr ...attribute.KeyValue) LoggerOption { }) } +// WithInstrumentationAttributeSet returns a [LoggerOption] that adds the +// instrumentation attributes of a [Logger]. +// +// 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 WithInstrumentationAttributeSet(set attribute.Set) LoggerOption { + if set.Len() == 0 { + return loggerOptionFunc(func(config LoggerConfig) LoggerConfig { + return config + }) + } + + return loggerOptionFunc(func(config LoggerConfig) LoggerConfig { + if config.attrs.Len() == 0 { + config.attrs = set + } else { + config.attrs = mergeSets(config.attrs, set) + } + return config + }) +} + // WithSchemaURL returns a [LoggerOption] that sets the schema URL for a // [Logger]. func WithSchemaURL(schemaURL string) LoggerOption { diff --git a/log/logger_test.go b/log/logger_test.go index 3da344f5c..137a3284c 100644 --- a/log/logger_test.go +++ b/log/logger_test.go @@ -31,6 +31,19 @@ func TestNewLoggerConfig(t *testing.T) { assert.Equal(t, attr, c.InstrumentationAttributes(), "instrumentation attributes") } +func TestWithInstrumentationAttributeSet(t *testing.T) { + attrs := attribute.NewSet( + attribute.String("service", "test"), + attribute.Int("three", 3), + ) + + c := log.NewLoggerConfig( + log.WithInstrumentationAttributeSet(attrs), + ) + + assert.Equal(t, attrs, c.InstrumentationAttributes(), "instrumentation attributes") +} + func TestWithInstrumentationAttributesMerge(t *testing.T) { aliceAttr := attribute.String("user", "Alice") bobAttr := attribute.String("user", "Bob") @@ -76,4 +89,93 @@ func TestWithInstrumentationAttributesMerge(t *testing.T) { assert.Equal(t, alice, c.InstrumentationAttributes(), "Empty attributes should not affect existing ones.") }) + + t.Run("SameKeyWithSet", func(t *testing.T) { + c := log.NewLoggerConfig( + log.WithInstrumentationAttributeSet(alice), + log.WithInstrumentationAttributeSet(bob), + ) + assert.Equal(t, bob, c.InstrumentationAttributes(), + "Later values for the same key should overwrite earlier ones.") + }) + + t.Run("DifferentKeysWithSet", func(t *testing.T) { + c := log.NewLoggerConfig( + log.WithInstrumentationAttributeSet(alice), + log.WithInstrumentationAttributeSet(attribute.NewSet(adminAttr)), + ) + assert.Equal(t, aliceAdmin, c.InstrumentationAttributes(), + "Different keys should be merged.") + }) + + t.Run("MixedWithSet", func(t *testing.T) { + c := log.NewLoggerConfig( + log.WithInstrumentationAttributeSet(aliceAdmin), + log.WithInstrumentationAttributeSet(bob), + ) + assert.Equal(t, bobAdmin, c.InstrumentationAttributes(), + "Combination of same and different keys should be merged.") + }) + + t.Run("MergedEmptyWithSet", func(t *testing.T) { + c := log.NewLoggerConfig( + log.WithInstrumentationAttributeSet(alice), + log.WithInstrumentationAttributeSet(attribute.NewSet()), + ) + assert.Equal(t, alice, c.InstrumentationAttributes(), + "Empty attribute set should not affect existing ones.") + }) + + t.Run("MixedAttributesAndSet", func(t *testing.T) { + c := log.NewLoggerConfig( + log.WithInstrumentationAttributes(aliceAttr), + log.WithInstrumentationAttributeSet(attribute.NewSet(bobAttr, adminAttr)), + ) + assert.Equal(t, bobAdmin, c.InstrumentationAttributes(), + "Attributes and attribute sets should be merged together.") + }) +} + +func BenchmarkNewLoggerConfig(b *testing.B) { + for _, bb := range []struct { + name string + options []log.LoggerOption + }{ + { + name: "with no options", + }, + { + name: "with an instrumentation version", + options: []log.LoggerOption{ + log.WithInstrumentationVersion("testing version"), + }, + }, + { + name: "with a schema url", + options: []log.LoggerOption{ + log.WithSchemaURL("testing URL"), + }, + }, + { + name: "with instrumentation attribute", + options: []log.LoggerOption{ + log.WithInstrumentationAttributes(attribute.String("foo", "value")), + }, + }, + { + name: "with instrumentation attribute set", + options: []log.LoggerOption{ + log.WithInstrumentationAttributeSet(attribute.NewSet(attribute.String("bar", "value"))), + }, + }, + } { + b.Run(bb.name, func(b *testing.B) { + b.ReportAllocs() + b.ResetTimer() + + for b.Loop() { + log.NewLoggerConfig(bb.options...) + } + }) + } } diff --git a/metric/config.go b/metric/config.go index 8c1c3fa2b..adcdae5d9 100644 --- a/metric/config.go +++ b/metric/config.go @@ -62,13 +62,16 @@ func WithInstrumentationVersion(version string) MeterOption { }) } -// WithInstrumentationAttributes sets the instrumentation attributes. +// WithInstrumentationAttributes adds the instrumentation attributes. // // The passed attributes will be de-duplicated. // -// If multiple WithInstrumentationAttributes 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. +// Note that [WithInstrumentationAttributeSet] is recommended as +// it is more efficient and also allows safely reusing the passed argument. +// +// 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 { @@ -87,6 +90,28 @@ func WithInstrumentationAttributes(attr ...attribute.KeyValue) MeterOption { }) } +// WithInstrumentationAttributeSet adds the instrumentation attributes. +// +// 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 WithInstrumentationAttributeSet(set attribute.Set) MeterOption { + if set.Len() == 0 { + return meterOptionFunc(func(config MeterConfig) MeterConfig { + return config + }) + } + + return meterOptionFunc(func(config MeterConfig) MeterConfig { + if config.attrs.Len() == 0 { + config.attrs = set + } else { + config.attrs = mergeSets(config.attrs, set) + } + return config + }) +} + // WithSchemaURL sets the schema URL. func WithSchemaURL(schemaURL string) MeterOption { return meterOptionFunc(func(config MeterConfig) MeterConfig { diff --git a/metric/config_test.go b/metric/config_test.go index bd4305701..07f0088a9 100644 --- a/metric/config_test.go +++ b/metric/config_test.go @@ -31,6 +31,19 @@ func TestConfig(t *testing.T) { assert.Equal(t, attr, c.InstrumentationAttributes(), "instrumentation attributes") } +func TestWithInstrumentationAttributeSet(t *testing.T) { + attrs := attribute.NewSet( + attribute.String("service", "test"), + attribute.Int("three", 3), + ) + + c := metric.NewMeterConfig( + metric.WithInstrumentationAttributeSet(attrs), + ) + + assert.Equal(t, attrs, c.InstrumentationAttributes(), "instrumentation attributes") +} + func TestWithInstrumentationAttributesMerge(t *testing.T) { aliceAttr := attribute.String("user", "Alice") bobAttr := attribute.String("user", "Bob") @@ -77,4 +90,93 @@ func TestWithInstrumentationAttributesMerge(t *testing.T) { assert.Equal(t, alice, c.InstrumentationAttributes(), "Empty attributes should not affect existing ones.") }) + + t.Run("SameKeyWithSet", func(t *testing.T) { + c := metric.NewMeterConfig( + metric.WithInstrumentationAttributeSet(alice), + metric.WithInstrumentationAttributeSet(bob), + ) + assert.Equal(t, bob, c.InstrumentationAttributes(), + "Later values for the same key should overwrite earlier ones.") + }) + + t.Run("DifferentKeysWithSet", func(t *testing.T) { + c := metric.NewMeterConfig( + metric.WithInstrumentationAttributeSet(alice), + metric.WithInstrumentationAttributeSet(attribute.NewSet(adminAttr)), + ) + assert.Equal(t, aliceAdmin, c.InstrumentationAttributes(), + "Different keys should be merged.") + }) + + t.Run("MixedWithSet", func(t *testing.T) { + c := metric.NewMeterConfig( + metric.WithInstrumentationAttributeSet(aliceAdmin), + metric.WithInstrumentationAttributeSet(bob), + ) + assert.Equal(t, bobAdmin, c.InstrumentationAttributes(), + "Combination of same and different keys should be merged.") + }) + + t.Run("MergedEmptyWithSet", func(t *testing.T) { + c := metric.NewMeterConfig( + metric.WithInstrumentationAttributeSet(alice), + metric.WithInstrumentationAttributeSet(attribute.NewSet()), + ) + assert.Equal(t, alice, c.InstrumentationAttributes(), + "Empty attribute set should not affect existing ones.") + }) + + t.Run("MixedAttributesAndSet", func(t *testing.T) { + c := metric.NewMeterConfig( + metric.WithInstrumentationAttributes(aliceAttr), + metric.WithInstrumentationAttributeSet(attribute.NewSet(bobAttr, adminAttr)), + ) + assert.Equal(t, bobAdmin, c.InstrumentationAttributes(), + "Attributes and attribute sets should be merged together.") + }) +} + +func BenchmarkNewMeterConfig(b *testing.B) { + for _, bb := range []struct { + name string + options []metric.MeterOption + }{ + { + name: "with no options", + }, + { + name: "with an instrumentation version", + options: []metric.MeterOption{ + metric.WithInstrumentationVersion("testing version"), + }, + }, + { + name: "with a schema url", + options: []metric.MeterOption{ + metric.WithSchemaURL("testing URL"), + }, + }, + { + name: "with instrumentation attribute", + options: []metric.MeterOption{ + metric.WithInstrumentationAttributes(attribute.String("key", "value")), + }, + }, + { + name: "with instrumentation attribute set", + options: []metric.MeterOption{ + metric.WithInstrumentationAttributeSet(attribute.NewSet(attribute.String("key", "value"))), + }, + }, + } { + b.Run(bb.name, func(b *testing.B) { + b.ReportAllocs() + b.ResetTimer() + + for b.Loop() { + metric.NewMeterConfig(bb.options...) + } + }) + } } diff --git a/trace/config.go b/trace/config.go index 7f80214a3..4d83fb059 100644 --- a/trace/config.go +++ b/trace/config.go @@ -316,13 +316,16 @@ func mergeSets(a, b attribute.Set) attribute.Set { return attribute.NewSet(merged...) } -// WithInstrumentationAttributes sets the instrumentation attributes. +// WithInstrumentationAttributes adds the instrumentation attributes. // // The passed attributes will be de-duplicated. // -// If multiple WithInstrumentationAttributes 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. +// Note that [WithInstrumentationAttributeSet] is recommended as +// it is more efficient and also allows safely reusing the passed argument. +// +// 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 { @@ -341,6 +344,28 @@ func WithInstrumentationAttributes(attr ...attribute.KeyValue) TracerOption { }) } +// WithInstrumentationAttributeSet adds the instrumentation attributes. +// +// 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 WithInstrumentationAttributeSet(set attribute.Set) TracerOption { + if set.Len() == 0 { + return tracerOptionFunc(func(config TracerConfig) TracerConfig { + return config + }) + } + + return tracerOptionFunc(func(config TracerConfig) TracerConfig { + if config.attrs.Len() == 0 { + config.attrs = set + } else { + config.attrs = mergeSets(config.attrs, set) + } + return config + }) +} + // WithSchemaURL sets the schema URL for the Tracer. func WithSchemaURL(schemaURL string) TracerOption { return tracerOptionFunc(func(cfg TracerConfig) TracerConfig { diff --git a/trace/config_test.go b/trace/config_test.go index 4192c95f1..e88415025 100644 --- a/trace/config_test.go +++ b/trace/config_test.go @@ -231,12 +231,24 @@ func TestTracerConfig(t *testing.T) { assert.Equal(t, attrs, c.InstrumentationAttributes(), "instrumentation attributes") } +func TestWithInstrumentationAttributeSet(t *testing.T) { + attrs := attribute.NewSet( + attribute.String("service", "test"), + attribute.Int("three", 3), + ) + + c := NewTracerConfig( + WithInstrumentationAttributeSet(attrs), + ) + + assert.Equal(t, attrs, c.InstrumentationAttributes(), "instrumentation attributes") +} + // Save benchmark results to a file level var to avoid the compiler optimizing // away the actual work. var ( - tracerConfig TracerConfig - spanConfig SpanConfig - eventConfig EventConfig + spanConfig SpanConfig + eventConfig EventConfig ) func BenchmarkNewTracerConfig(b *testing.B) { @@ -259,13 +271,25 @@ func BenchmarkNewTracerConfig(b *testing.B) { WithSchemaURL("testing URL"), }, }, + { + name: "with instrumentation attribute", + options: []TracerOption{ + WithInstrumentationAttributes(attribute.String("key", "value")), + }, + }, + { + name: "with instrumentation attribute set", + options: []TracerOption{ + WithInstrumentationAttributeSet(attribute.NewSet(attribute.String("key", "value"))), + }, + }, } { b.Run(bb.name, func(b *testing.B) { b.ReportAllocs() b.ResetTimer() - for i := 0; i < b.N; i++ { - tracerConfig = NewTracerConfig(bb.options...) + for b.Loop() { + NewTracerConfig(bb.options...) } }) } @@ -457,4 +481,49 @@ func TestWithInstrumentationAttributesMerge(t *testing.T) { assert.Equal(t, alice, c.InstrumentationAttributes(), "Empty attributes should not affect existing ones.") }) + + t.Run("SameKeyWithSet", func(t *testing.T) { + c := NewTracerConfig( + WithInstrumentationAttributeSet(alice), + WithInstrumentationAttributeSet(bob), + ) + assert.Equal(t, bob, c.InstrumentationAttributes(), + "Later values for the same key should overwrite earlier ones.") + }) + + t.Run("DifferentKeysWithSet", func(t *testing.T) { + c := NewTracerConfig( + WithInstrumentationAttributeSet(alice), + WithInstrumentationAttributeSet(attribute.NewSet(adminAttr)), + ) + assert.Equal(t, aliceAdmin, c.InstrumentationAttributes(), + "Different keys should be merged.") + }) + + t.Run("MixedWithSet", func(t *testing.T) { + c := NewTracerConfig( + WithInstrumentationAttributeSet(aliceAdmin), + WithInstrumentationAttributeSet(bob), + ) + assert.Equal(t, bobAdmin, c.InstrumentationAttributes(), + "Combination of same and different keys should be merged.") + }) + + t.Run("MergedEmptyWithSet", func(t *testing.T) { + c := NewTracerConfig( + WithInstrumentationAttributeSet(alice), + WithInstrumentationAttributeSet(attribute.NewSet()), + ) + assert.Equal(t, alice, c.InstrumentationAttributes(), + "Empty attribute set should not affect existing ones.") + }) + + t.Run("MixedAttributesAndSet", func(t *testing.T) { + c := NewTracerConfig( + WithInstrumentationAttributes(aliceAttr), + WithInstrumentationAttributeSet(attribute.NewSet(bobAttr, adminAttr)), + ) + assert.Equal(t, bobAdmin, c.InstrumentationAttributes(), + "Attributes and attribute sets should be merged together.") + }) }