From dcf14aa937fa5d3fa97af91245fed7235dae0d82 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20Paj=C4=85k?= Date: Mon, 8 Sep 2025 10:46:08 +0200 Subject: [PATCH] trace,metric,log: WithInstrumentationAttributes options to merge attributes (#7300) ## What 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. ## Why Per https://github.com/open-telemetry/opentelemetry-go/pull/7287#pullrequestreview-3181379062 and https://github.com/open-telemetry/opentelemetry-go/pull/7287#issuecomment-3250085450 Not that this does not address https://github.com/open-telemetry/opentelemetry-go/issues/7217. This is left to a seperate PR to not scope creep this one. CC @axw --------- Co-authored-by: Damien Mathieu <42@dmathieu.com> --- CHANGELOG.md | 4 ++++ log/logger.go | 29 +++++++++++++++++++++++++- log/logger_test.go | 47 ++++++++++++++++++++++++++++++++++++++++++ metric/config.go | 17 ++++++++++++++- metric/config_test.go | 48 +++++++++++++++++++++++++++++++++++++++++++ trace/config.go | 29 +++++++++++++++++++++++++- trace/config_test.go | 47 ++++++++++++++++++++++++++++++++++++++++++ 7 files changed, 218 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b84e38f8d..dee7836d0 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] +### 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) + ### Removed - Drop support for [Go 1.23]. (#7274) diff --git a/log/logger.go b/log/logger.go index 8441ca884..3cd60ed55 100644 --- a/log/logger.go +++ b/log/logger.go @@ -114,13 +114,40 @@ func WithInstrumentationVersion(version string) LoggerOption { }) } +// mergeSets returns the union of keys between a and b. Any duplicate keys will +// use the value associated with b. +func mergeSets(a, b attribute.Set) attribute.Set { + // NewMergeIterator uses the first value for any duplicates. + iter := attribute.NewMergeIterator(&b, &a) + merged := make([]attribute.KeyValue, 0, a.Len()+b.Len()) + for iter.Next() { + merged = append(merged, iter.Attribute()) + } + return attribute.NewSet(merged...) +} + // WithInstrumentationAttributes returns a [LoggerOption] that sets the // instrumentation attributes of a [Logger]. // // 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. func WithInstrumentationAttributes(attr ...attribute.KeyValue) LoggerOption { + if len(attr) == 0 { + return loggerOptionFunc(func(config LoggerConfig) LoggerConfig { + return config + }) + } + return loggerOptionFunc(func(config LoggerConfig) LoggerConfig { - config.attrs = attribute.NewSet(attr...) + newAttrs := attribute.NewSet(attr...) + if config.attrs.Len() == 0 { + config.attrs = newAttrs + } else { + config.attrs = mergeSets(config.attrs, newAttrs) + } return config }) } diff --git a/log/logger_test.go b/log/logger_test.go index add175df4..3da344f5c 100644 --- a/log/logger_test.go +++ b/log/logger_test.go @@ -30,3 +30,50 @@ func TestNewLoggerConfig(t *testing.T) { assert.Equal(t, schemaURL, c.SchemaURL(), "schema URL") assert.Equal(t, attr, c.InstrumentationAttributes(), "instrumentation attributes") } + +func TestWithInstrumentationAttributesMerge(t *testing.T) { + aliceAttr := attribute.String("user", "Alice") + bobAttr := attribute.String("user", "Bob") + adminAttr := attribute.Bool("admin", true) + + alice := attribute.NewSet(aliceAttr) + bob := attribute.NewSet(bobAttr) + aliceAdmin := attribute.NewSet(aliceAttr, adminAttr) + bobAdmin := attribute.NewSet(bobAttr, adminAttr) + + t.Run("SameKey", func(t *testing.T) { + c := log.NewLoggerConfig( + log.WithInstrumentationAttributes(aliceAttr), + log.WithInstrumentationAttributes(bobAttr), + ) + assert.Equal(t, bob, c.InstrumentationAttributes(), + "Later values for the same key should overwrite earlier ones.") + }) + + t.Run("DifferentKeys", func(t *testing.T) { + c := log.NewLoggerConfig( + log.WithInstrumentationAttributes(aliceAttr), + log.WithInstrumentationAttributes(adminAttr), + ) + assert.Equal(t, aliceAdmin, c.InstrumentationAttributes(), + "Different keys should be merged.") + }) + + t.Run("Mixed", func(t *testing.T) { + c := log.NewLoggerConfig( + log.WithInstrumentationAttributes(aliceAttr, adminAttr), + log.WithInstrumentationAttributes(bobAttr), + ) + assert.Equal(t, bobAdmin, c.InstrumentationAttributes(), + "Combination of same and different keys should be merged.") + }) + + t.Run("MergedEmpty", func(t *testing.T) { + c := log.NewLoggerConfig( + log.WithInstrumentationAttributes(aliceAttr), + log.WithInstrumentationAttributes(), + ) + assert.Equal(t, alice, c.InstrumentationAttributes(), + "Empty attributes should not affect existing ones.") + }) +} diff --git a/metric/config.go b/metric/config.go index d9e3b13e4..8c1c3fa2b 100644 --- a/metric/config.go +++ b/metric/config.go @@ -65,9 +65,24 @@ func WithInstrumentationVersion(version string) MeterOption { // WithInstrumentationAttributes sets 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. func WithInstrumentationAttributes(attr ...attribute.KeyValue) MeterOption { + if len(attr) == 0 { + return meterOptionFunc(func(config MeterConfig) MeterConfig { + return config + }) + } + return meterOptionFunc(func(config MeterConfig) MeterConfig { - config.attrs = attribute.NewSet(attr...) + newAttrs := attribute.NewSet(attr...) + if config.attrs.Len() == 0 { + config.attrs = newAttrs + } else { + config.attrs = mergeSets(config.attrs, newAttrs) + } return config }) } diff --git a/metric/config_test.go b/metric/config_test.go index fda2b48b2..bd4305701 100644 --- a/metric/config_test.go +++ b/metric/config_test.go @@ -30,3 +30,51 @@ func TestConfig(t *testing.T) { assert.Equal(t, schemaURL, c.SchemaURL(), "schema URL") assert.Equal(t, attr, c.InstrumentationAttributes(), "instrumentation attributes") } + +func TestWithInstrumentationAttributesMerge(t *testing.T) { + aliceAttr := attribute.String("user", "Alice") + bobAttr := attribute.String("user", "Bob") + adminAttr := attribute.Bool("admin", true) + + alice := attribute.NewSet(aliceAttr) + bob := attribute.NewSet(bobAttr) + aliceAdmin := attribute.NewSet(aliceAttr, adminAttr) + bobAdmin := attribute.NewSet(bobAttr, adminAttr) + + t.Run("SameKey", func(t *testing.T) { + c := metric.NewMeterConfig( + metric.WithInstrumentationAttributes(aliceAttr), + metric.WithInstrumentationAttributes(bobAttr), + ) + assert.Equal(t, bob, c.InstrumentationAttributes(), + "Later values for the same key should overwrite earlier ones.") + }) + + t.Run("DifferentKeys", func(t *testing.T) { + // Different keys should be merged + c := metric.NewMeterConfig( + metric.WithInstrumentationAttributes(aliceAttr), + metric.WithInstrumentationAttributes(adminAttr), + ) + assert.Equal(t, aliceAdmin, c.InstrumentationAttributes(), + "Different keys should be merged.") + }) + + t.Run("Mixed", func(t *testing.T) { + c := metric.NewMeterConfig( + metric.WithInstrumentationAttributes(aliceAttr, adminAttr), + metric.WithInstrumentationAttributes(bobAttr), + ) + assert.Equal(t, bobAdmin, c.InstrumentationAttributes(), + "Combination of same and different keys should be merged.") + }) + + t.Run("MergedEmpty", func(t *testing.T) { + c := metric.NewMeterConfig( + metric.WithInstrumentationAttributes(aliceAttr), + metric.WithInstrumentationAttributes(), + ) + assert.Equal(t, alice, c.InstrumentationAttributes(), + "Empty attributes should not affect existing ones.") + }) +} diff --git a/trace/config.go b/trace/config.go index aea11a2b5..7f80214a3 100644 --- a/trace/config.go +++ b/trace/config.go @@ -304,12 +304,39 @@ func WithInstrumentationVersion(version string) TracerOption { }) } +// mergeSets returns the union of keys between a and b. Any duplicate keys will +// use the value associated with b. +func mergeSets(a, b attribute.Set) attribute.Set { + // NewMergeIterator uses the first value for any duplicates. + iter := attribute.NewMergeIterator(&b, &a) + merged := make([]attribute.KeyValue, 0, a.Len()+b.Len()) + for iter.Next() { + merged = append(merged, iter.Attribute()) + } + return attribute.NewSet(merged...) +} + // WithInstrumentationAttributes sets 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. func WithInstrumentationAttributes(attr ...attribute.KeyValue) TracerOption { + if len(attr) == 0 { + return tracerOptionFunc(func(config TracerConfig) TracerConfig { + return config + }) + } + return tracerOptionFunc(func(config TracerConfig) TracerConfig { - config.attrs = attribute.NewSet(attr...) + newAttrs := attribute.NewSet(attr...) + if config.attrs.Len() == 0 { + config.attrs = newAttrs + } else { + config.attrs = mergeSets(config.attrs, newAttrs) + } return config }) } diff --git a/trace/config_test.go b/trace/config_test.go index 00e6507e8..4192c95f1 100644 --- a/trace/config_test.go +++ b/trace/config_test.go @@ -411,3 +411,50 @@ func BenchmarkNewEventConfig(b *testing.B) { }) } } + +func TestWithInstrumentationAttributesMerge(t *testing.T) { + aliceAttr := attribute.String("user", "Alice") + bobAttr := attribute.String("user", "Bob") + adminAttr := attribute.Bool("admin", true) + + alice := attribute.NewSet(aliceAttr) + bob := attribute.NewSet(bobAttr) + aliceAdmin := attribute.NewSet(aliceAttr, adminAttr) + bobAdmin := attribute.NewSet(bobAttr, adminAttr) + + t.Run("SameKey", func(t *testing.T) { + c := NewTracerConfig( + WithInstrumentationAttributes(aliceAttr), + WithInstrumentationAttributes(bobAttr), + ) + assert.Equal(t, bob, c.InstrumentationAttributes(), + "Later values for the same key should overwrite earlier ones.") + }) + + t.Run("DifferentKeys", func(t *testing.T) { + c := NewTracerConfig( + WithInstrumentationAttributes(aliceAttr), + WithInstrumentationAttributes(adminAttr), + ) + assert.Equal(t, aliceAdmin, c.InstrumentationAttributes(), + "Different keys should be merged") + }) + + t.Run("Mixed", func(t *testing.T) { + c := NewTracerConfig( + WithInstrumentationAttributes(aliceAttr, adminAttr), + WithInstrumentationAttributes(bobAttr), + ) + assert.Equal(t, bobAdmin, c.InstrumentationAttributes(), + "Combination of same and different keys should be merged.") + }) + + t.Run("MergedEmpty", func(t *testing.T) { + c := NewTracerConfig( + WithInstrumentationAttributes(aliceAttr), + WithInstrumentationAttributes(), + ) + assert.Equal(t, alice, c.InstrumentationAttributes(), + "Empty attributes should not affect existing ones.") + }) +}