From 93357e02f54d3c2a682bd16e867d2986c679a484 Mon Sep 17 00:00:00 2001 From: Tyler Yahn Date: Wed, 9 Aug 2023 09:53:16 -0700 Subject: [PATCH 1/3] Add acceptance test --- sdk/metric/pipeline_test.go | 35 +++++++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+) 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") +} From c14e9868ba4506887ea54ddd74d573ce075a129c Mon Sep 17 00:00:00 2001 From: Tyler Yahn Date: Wed, 9 Aug 2023 09:55:36 -0700 Subject: [PATCH 2/3] Return the first-seen for instrument name conflicts --- sdk/metric/instrument.go | 11 +++++++++++ sdk/metric/pipeline.go | 16 ++++++++++++---- 2 files changed, 23 insertions(+), 4 deletions(-) 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 From 17359d66eaf1f48399e7969b7f6d2872b859d0d0 Mon Sep 17 00:00:00 2001 From: Tyler Yahn Date: Wed, 9 Aug 2023 10:00:46 -0700 Subject: [PATCH 3/3] Add changes to changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) 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