Skip to content

Commit

Permalink
Merge branch 'main' into dep-otlptrace-internal
Browse files Browse the repository at this point in the history
  • Loading branch information
MrAlias committed Aug 10, 2023
2 parents bf20fe2 + 10d9038 commit a1da462
Show file tree
Hide file tree
Showing 4 changed files with 59 additions and 4 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
11 changes: 11 additions & 0 deletions sdk/metric/instrument.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import (
"context"
"errors"
"fmt"
"strings"

"go.opentelemetry.io/otel/attribute"
"go.opentelemetry.io/otel/metric"
Expand Down Expand Up @@ -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]

Expand Down
16 changes: 12 additions & 4 deletions sdk/metric/pipeline.go
Original file line number Diff line number Diff line change
Expand Up @@ -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),
}
Expand All @@ -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,
Expand All @@ -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
Expand Down
35 changes: 35 additions & 0 deletions sdk/metric/pipeline_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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")
}

0 comments on commit a1da462

Please sign in to comment.