Skip to content

Commit

Permalink
Detect dup inst for case-insensitive names
Browse files Browse the repository at this point in the history
Resolve open-telemetry#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
open-telemetry/opentelemetry-specification#3606.
  • Loading branch information
MrAlias committed Jul 18, 2023
1 parent 9b0c4d2 commit 14e552b
Show file tree
Hide file tree
Showing 2 changed files with 74 additions and 1 deletion.
5 changes: 4 additions & 1 deletion sdk/metric/pipeline.go
Original file line number Diff line number Diff line change
Expand Up @@ -351,7 +351,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 streamID) {
existing := i.views.Lookup(id.Name, func() streamID { 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() streamID { return id })
if id == existing {
return
}
Expand Down
70 changes: 70 additions & 0 deletions sdk/metric/pipeline_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,12 +17,15 @@ package metric // import "go.opentelemetry.io/otel/sdk/metric"
import (
"context"
"fmt"
"strings"
"sync"
"testing"

"github.com/go-logr/logr/funcr"
"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"
Expand Down Expand Up @@ -166,3 +169,70 @@ 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
otel.SetLogger(funcr.New(func(_, args string) {
msg = args
}, funcr.Options{Verbosity: 20}))

for _, tc := range testcases {
var vc cache[string, streamID]

name := strings.ToLower(tc.existing)
_ = vc.Lookup(name, func() streamID {
return streamID{Name: tc.existing}
})

i := newInserter[int64](newPipeline(nil, nil, nil), &vc)
i.logConflict(streamID{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 = ""
}
}

0 comments on commit 14e552b

Please sign in to comment.