diff --git a/CHANGELOG.md b/CHANGELOG.md index d3f8170b023..85f1ec63320 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -41,6 +41,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm - Log an error for calls to `NewView` in `go.opentelemetry.io/otel/sdk/metric` that have empty criteria. (#4307) - Fix `resource.WithHostID()` to not set an empty `host.id`. (#4317) - Use the instrument identifying fields to cache aggregators and determine duplicate instrument registrations in `go.opentelemetry.io/otel/sdk/metric`. (#4337) +- Detect duplicate instruments for case-insensitive names in `go.opentelemetry.io/otel/sdk/metric`. (#4338) ## [1.16.0/0.39.0] 2023-05-18 diff --git a/sdk/metric/go.mod b/sdk/metric/go.mod index ebec07c04d0..fcdfdfa5b74 100644 --- a/sdk/metric/go.mod +++ b/sdk/metric/go.mod @@ -4,6 +4,7 @@ go 1.19 require ( github.com/go-logr/logr v1.2.4 + github.com/go-logr/stdr v1.2.2 github.com/stretchr/testify v1.8.4 go.opentelemetry.io/otel v1.16.0 go.opentelemetry.io/otel/metric v1.16.0 @@ -12,7 +13,6 @@ require ( require ( github.com/davecgh/go-spew v1.1.1 // indirect - github.com/go-logr/stdr v1.2.2 // indirect github.com/pmezard/go-difflib v1.0.0 // indirect go.opentelemetry.io/otel/trace v1.16.0 // indirect golang.org/x/sys v0.10.0 // indirect diff --git a/sdk/metric/pipeline.go b/sdk/metric/pipeline.go index d6af04c6e27..ad52aedfe68 100644 --- a/sdk/metric/pipeline.go +++ b/sdk/metric/pipeline.go @@ -353,7 +353,10 @@ func (i *inserter[N]) cachedAggregator(scope instrumentation.Scope, kind Instrum // 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. func (i *inserter[N]) logConflict(id instID) { - existing := i.views.Lookup(id.Name, func() instID { return id }) + // 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) + 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 dd30a35e065..58916079429 100644 --- a/sdk/metric/pipeline_test.go +++ b/sdk/metric/pipeline_test.go @@ -17,12 +17,19 @@ package metric // import "go.opentelemetry.io/otel/sdk/metric" import ( "context" "fmt" + "log" + "os" + "strings" "sync" "testing" + "github.com/go-logr/logr" + "github.com/go-logr/logr/funcr" + "github.com/go-logr/stdr" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "go.opentelemetry.io/otel" "go.opentelemetry.io/otel/attribute" "go.opentelemetry.io/otel/sdk/instrumentation" "go.opentelemetry.io/otel/sdk/metric/metricdata" @@ -166,3 +173,73 @@ func testDefaultViewImplicit[N int64 | float64]() func(t *testing.T) { } } } + +func TestLogConflictName(t *testing.T) { + testcases := []struct { + existing, name string + conflict bool + }{ + { + existing: "requestCount", + name: "requestCount", + conflict: false, + }, + { + existing: "requestCount", + name: "requestDuration", + conflict: false, + }, + { + existing: "requestCount", + name: "requestcount", + conflict: true, + }, + { + existing: "requestCount", + name: "REQUESTCOUNT", + conflict: true, + }, + { + existing: "requestCount", + name: "rEqUeStCoUnT", + conflict: true, + }, + } + + var msg string + t.Cleanup(func(orig logr.Logger) func() { + otel.SetLogger(funcr.New(func(_, args string) { + msg = args + }, funcr.Options{Verbosity: 20})) + return func() { otel.SetLogger(orig) } + }(stdr.New(log.New(os.Stderr, "", log.LstdFlags|log.Lshortfile)))) + + for _, tc := range testcases { + var vc cache[string, instID] + + name := strings.ToLower(tc.existing) + _ = vc.Lookup(name, func() instID { + return instID{Name: tc.existing} + }) + + i := newInserter[int64](newPipeline(nil, nil, nil), &vc) + i.logConflict(instID{Name: tc.name}) + + if tc.conflict { + assert.Containsf( + t, msg, "duplicate metric stream definitions", + "warning not logged for conflicting names: %s, %s", + tc.existing, tc.name, + ) + } else { + assert.Equalf( + t, msg, "", + "warning logged for non-conflicting names: %s, %s", + tc.existing, tc.name, + ) + } + + // Reset. + msg = "" + } +}