diff --git a/CHANGELOG.md b/CHANGELOG.md index f5b9d37f2f4..690bc7bdc33 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -65,6 +65,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm - Do not block the metric SDK when OTLP metric exports are blocked in `go.opentelemetry.io/otel/exporters/otlp/otlpmetric/otlpmetricgrpc` and `go.opentelemetry.io/otel/exporters/otlp/otlpmetric/otlpmetrichttp`. (#3925, #4395) - Do not append _total if the counter already ends in total `go.opentelemetry.io/otel/exporter/prometheus`. (#4373) - Fix resource detection data race in `go.opentelemetry.io/otel/sdk/resource`. (#4409) +- Use the first-seen instrument name during instrument name conflicts in `go.opentelemetry.io/otel/sdk/metric`. (#4428) ### Deprecated diff --git a/sdk/metric/instrument.go b/sdk/metric/instrument.go index eff2f179a51..67300d489f6 100644 --- a/sdk/metric/instrument.go +++ b/sdk/metric/instrument.go @@ -20,6 +20,7 @@ import ( "context" "errors" "fmt" + "strings" "go.opentelemetry.io/otel/attribute" "go.opentelemetry.io/otel/metric" @@ -187,6 +188,16 @@ type instID struct { Number string } +// Returns a normalized copy of the instID i. +// +// Instrument names are considered case-insensitive. Standardize the instrument +// name to always be lowercase for the returned instID so it can be compared +// without the name casing affecting the comparison. +func (i instID) normalize() instID { + i.Name = strings.ToLower(i.Name) + return i +} + type int64Inst struct { measures []aggregate.Measure[int64] diff --git a/sdk/metric/pipeline.go b/sdk/metric/pipeline.go index fd28a4afc15..6d1934a76fc 100644 --- a/sdk/metric/pipeline.go +++ b/sdk/metric/pipeline.go @@ -329,7 +329,12 @@ func (i *inserter[N]) cachedAggregator(scope instrumentation.Scope, kind Instrum // If there is a conflict, the specification says the view should // still be applied and a warning should be logged. i.logConflict(id) - cv := i.aggregators.Lookup(id, func() aggVal[N] { + + // If there are requests for the same instrument with different name + // casing, the first-seen needs to be returned. Use a normalize ID for the + // cache lookup to ensure the correct comparison. + normID := id.normalize() + cv := i.aggregators.Lookup(normID, func() aggVal[N] { b := aggregate.Builder[N]{ Temporality: i.pipeline.reader.temporality(kind), } @@ -344,6 +349,8 @@ func (i *inserter[N]) cachedAggregator(scope instrumentation.Scope, kind Instrum return aggVal[N]{0, nil, nil} } i.pipeline.addSync(scope, instrumentSync{ + // Use the first-seen name casing for this and all subsequent + // requests of this instrument. name: stream.Name, description: stream.Description, unit: stream.Unit, @@ -355,12 +362,13 @@ func (i *inserter[N]) cachedAggregator(scope instrumentation.Scope, kind Instrum return cv.Measure, cv.ID, cv.Err } -// logConflict validates if an instrument with the same name as id has already -// been created. If that instrument conflicts with id, a warning is logged. +// logConflict validates if an instrument with the same case-insensitive name +// as id has already been created. If that instrument conflicts with id, a +// warning is logged. func (i *inserter[N]) logConflict(id instID) { // The API specification defines names as case-insensitive. If there is a // different casing of a name it needs to be a conflict. - name := strings.ToLower(id.Name) + name := id.normalize().Name existing := i.views.Lookup(name, func() instID { return id }) if id == existing { return diff --git a/sdk/metric/pipeline_test.go b/sdk/metric/pipeline_test.go index 32c1d0358c4..d30d0015c06 100644 --- a/sdk/metric/pipeline_test.go +++ b/sdk/metric/pipeline_test.go @@ -32,6 +32,7 @@ import ( "go.opentelemetry.io/otel" "go.opentelemetry.io/otel/attribute" "go.opentelemetry.io/otel/sdk/instrumentation" + "go.opentelemetry.io/otel/sdk/metric/aggregation" "go.opentelemetry.io/otel/sdk/metric/metricdata" "go.opentelemetry.io/otel/sdk/metric/metricdata/metricdatatest" "go.opentelemetry.io/otel/sdk/resource" @@ -358,3 +359,37 @@ func TestLogConflictSuggestView(t *testing.T) { msg = "" }) } + +func TestInserterCachedAggregatorNameConflict(t *testing.T) { + const name = "requestCount" + scope := instrumentation.Scope{Name: "pipeline_test"} + kind := InstrumentKindCounter + stream := Stream{ + Name: name, + Aggregation: aggregation.Sum{}, + } + + var vc cache[string, instID] + pipe := newPipeline(nil, NewManualReader(), nil) + i := newInserter[int64](pipe, &vc) + + _, origID, err := i.cachedAggregator(scope, kind, stream) + require.NoError(t, err) + + require.Len(t, pipe.aggregations, 1) + require.Contains(t, pipe.aggregations, scope) + iSync := pipe.aggregations[scope] + require.Len(t, iSync, 1) + require.Equal(t, name, iSync[0].name) + + stream.Name = "RequestCount" + _, id, err := i.cachedAggregator(scope, kind, stream) + require.NoError(t, err) + assert.Equal(t, origID, id, "multiple aggregators for equivalent name") + + assert.Len(t, pipe.aggregations, 1, "additional scope added") + require.Contains(t, pipe.aggregations, scope, "original scope removed") + iSync = pipe.aggregations[scope] + require.Len(t, iSync, 1, "registered instrumentSync changed") + assert.Equal(t, name, iSync[0].name, "stream name changed") +}