Use first-seen instrument name for name conflicts (#4428)

* Add acceptance test

* Return the first-seen for instrument name conflicts

* Add changes to changelog

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
 

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]
 

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

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")
+}