Skip to content

Commit

Permalink
Detect duplicate instruments for case-insensitive names (#4338)
Browse files Browse the repository at this point in the history
* 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
open-telemetry/opentelemetry-specification#3606.

* Add changes to changelog

* Reset global logger after test
  • Loading branch information
MrAlias authored Jul 19, 2023
1 parent 84b2e54 commit e08359f
Show file tree
Hide file tree
Showing 4 changed files with 83 additions and 2 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
2 changes: 1 addition & 1 deletion sdk/metric/go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand Down
5 changes: 4 additions & 1 deletion sdk/metric/pipeline.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down
77 changes: 77 additions & 0 deletions sdk/metric/pipeline_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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 = ""
}
}

0 comments on commit e08359f

Please sign in to comment.