From e08359f30c881d6174bb8f750f3526b3774d4539 Mon Sep 17 00:00:00 2001
From: Tyler Yahn <MrAlias@users.noreply.github.com>
Date: Wed, 19 Jul 2023 10:17:38 -0700
Subject: [PATCH] Detect duplicate instruments for case-insensitive names
 (#4338)

* Detect dup inst for case-insensitive names

Resolve #3835

Detect duplicate instrument registrations for instruments that have the
same case-insensitive names. Continue to return the instruments with
different names, but log a warning. This is the solution proposed in
https://github.com/open-telemetry/opentelemetry-specification/pull/3606.

* Add changes to changelog

* Reset global logger after test
---
 CHANGELOG.md                |  1 +
 sdk/metric/go.mod           |  2 +-
 sdk/metric/pipeline.go      |  5 ++-
 sdk/metric/pipeline_test.go | 77 +++++++++++++++++++++++++++++++++++++
 4 files changed, 83 insertions(+), 2 deletions(-)

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