From d2b46b43a84613191db9880469703a81d6d2d698 Mon Sep 17 00:00:00 2001 From: ZHENK Date: Wed, 20 Dec 2023 03:22:00 +1300 Subject: [PATCH 1/9] Fix observable instrument not registered on drop aggregation --- sdk/metric/instrument.go | 5 +++-- sdk/metric/meter.go | 12 ++++++++++++ 2 files changed, 15 insertions(+), 2 deletions(-) diff --git a/sdk/metric/instrument.go b/sdk/metric/instrument.go index d549dc17a20..a4cfcbb95f1 100644 --- a/sdk/metric/instrument.go +++ b/sdk/metric/instrument.go @@ -295,8 +295,9 @@ type observable[N int64 | float64] struct { metric.Observable observablID[N] - meter *meter - measures measures[N] + meter *meter + measures measures[N] + dropAggregation bool } func newObservable[N int64 | float64](m *meter, kind InstrumentKind, name, desc, u string) *observable[N] { diff --git a/sdk/metric/meter.go b/sdk/metric/meter.go index 423cba8bdf9..02697ce35f7 100644 --- a/sdk/metric/meter.go +++ b/sdk/metric/meter.go @@ -117,6 +117,7 @@ func (m *meter) int64ObservableInstrument(id Instrument, callbacks []metric.Int6 } // Drop aggregation if len(in) == 0 { + inst.dropAggregation = true continue } inst.appendMeasures(in) @@ -233,6 +234,7 @@ func (m *meter) float64ObservableInstrument(id Instrument, callbacks []metric.Fl } // Drop aggregation if len(in) == 0 { + inst.dropAggregation = true continue } inst.appendMeasures(in) @@ -436,6 +438,11 @@ func (r observer) ObserveFloat64(o metric.Float64Observable, v float64, opts ... return } + if oImpl.dropAggregation { + // Drop aggregation + return + } + if _, registered := r.float64[oImpl.observablID]; !registered { global.Error(errUnregObserver, "failed to record", "name", oImpl.name, @@ -469,6 +476,11 @@ func (r observer) ObserveInt64(o metric.Int64Observable, v int64, opts ...metric return } + if oImpl.dropAggregation { + // Drop aggregation + return + } + if _, registered := r.int64[oImpl.observablID]; !registered { global.Error(errUnregObserver, "failed to record", "name", oImpl.name, From 5f575fce59a2654c33dce6b611fb6e62ba945bb8 Mon Sep 17 00:00:00 2001 From: ZHENK Date: Wed, 20 Dec 2023 21:18:50 +1300 Subject: [PATCH 2/9] Add TestObservableDropAggregation --- sdk/metric/go.mod | 1 + sdk/metric/go.sum | 1 + sdk/metric/meter_test.go | 189 +++++++++++++++++++++++++++++++++++++++ 3 files changed, 191 insertions(+) diff --git a/sdk/metric/go.mod b/sdk/metric/go.mod index 24325464289..5fac032a82a 100644 --- a/sdk/metric/go.mod +++ b/sdk/metric/go.mod @@ -5,6 +5,7 @@ go 1.20 require ( github.com/go-logr/logr v1.3.0 github.com/go-logr/stdr v1.2.2 + github.com/google/go-cmp v0.6.0 github.com/stretchr/testify v1.8.4 go.opentelemetry.io/otel v1.21.0 go.opentelemetry.io/otel/metric v1.21.0 diff --git a/sdk/metric/go.sum b/sdk/metric/go.sum index 94020957893..939feb405bb 100644 --- a/sdk/metric/go.sum +++ b/sdk/metric/go.sum @@ -6,6 +6,7 @@ github.com/go-logr/logr v1.3.0/go.mod h1:9T104GzyrTigFIr8wt5mBrctHMim0Nb2HLGrmQ4 github.com/go-logr/stdr v1.2.2 h1:hSWxHoqTgW2S2qGc0LTAI563KZ5YKYRhT3MFKZMbjag= github.com/go-logr/stdr v1.2.2/go.mod h1:mMo/vtBO5dYbehREoey6XUKy/eSumjCCveDpRre4VKE= github.com/google/go-cmp v0.6.0 h1:ofyhxvXcZhMsU5ulbFiLKl/XBFqE1GSq7atu8tAmTRI= +github.com/google/go-cmp v0.6.0/go.mod h1:17dUlkBOakJ0+DkrSSNjCkIjxS6bF9zb3elmeNGIjoY= github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZbAQM= github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4= github.com/stretchr/testify v1.8.4 h1:CcVxjf3Q8PM0mHUKJCdn+eZZtm5yQwehR5yeSVQQcUk= diff --git a/sdk/metric/meter_test.go b/sdk/metric/meter_test.go index adf3cd251e2..7ff3e89c27d 100644 --- a/sdk/metric/meter_test.go +++ b/sdk/metric/meter_test.go @@ -16,6 +16,7 @@ package metric import ( "context" + "encoding/json" "errors" "fmt" "strings" @@ -23,7 +24,10 @@ import ( "testing" "github.com/go-logr/logr" + "github.com/go-logr/logr/funcr" "github.com/go-logr/logr/testr" + "github.com/google/go-cmp/cmp" + "github.com/google/go-cmp/cmp/cmpopts" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -2064,3 +2068,188 @@ func TestHistogramBucketPrecedenceOrdering(t *testing.T) { }) } } + +func TestObservableDropAggregation(t *testing.T) { + testcases := []struct { + name string + views []View + expectedObservableName []string + expectedLogs []map[string]interface{} + }{ + { + name: "default", + views: []View{}, + expectedObservableName: []string{ + "observable.int64.counter", + "observable.int64.up.down.counter", + "observable.int64.gauge", + "observable.float64.counter", + "observable.float64.up.down.counter", + "observable.float64.gauge", + }, + expectedLogs: []map[string]interface{}{ + { + "error": "observable instrument not registered for callback", + "msg": "failed to record", + "name": "unregistered.observable.int64.counter", + "number": "int64", + }, + { + "error": "observable instrument not registered for callback", + "msg": "failed to record", + "name": "unregistered.observable.float64.counter", + "number": "float64", + }, + }, + }, + { + name: "drop all metrics", + views: []View{ + func(i Instrument) (Stream, bool) { + return Stream{Aggregation: AggregationDrop{}}, true + }, + }, + expectedObservableName: []string{}, + expectedLogs: []map[string]interface{}{}, + }, + { + name: "drop float64 observable", + views: []View{ + func(i Instrument) (Stream, bool) { + if strings.HasPrefix(i.Name, "observable.float64") { + return Stream{Aggregation: AggregationDrop{}}, true + } + return Stream{}, false + }, + }, + expectedObservableName: []string{ + "observable.int64.counter", + "observable.int64.up.down.counter", + "observable.int64.gauge", + }, + expectedLogs: []map[string]interface{}{ + { + "error": "observable instrument not registered for callback", + "msg": "failed to record", + "name": "unregistered.observable.int64.counter", + "number": "int64", + }, + { + "error": "observable instrument not registered for callback", + "msg": "failed to record", + "name": "unregistered.observable.float64.counter", + "number": "float64", + }, + }, + }, + { + name: "drop int64 observable", + views: []View{ + func(i Instrument) (Stream, bool) { + if strings.HasPrefix(i.Name, "observable.int64") { + return Stream{Aggregation: AggregationDrop{}}, true + } + return Stream{}, false + }, + }, + expectedObservableName: []string{ + "observable.float64.counter", + "observable.float64.up.down.counter", + "observable.float64.gauge", + }, + expectedLogs: []map[string]interface{}{ + { + "error": "observable instrument not registered for callback", + "msg": "failed to record", + "name": "unregistered.observable.int64.counter", + "number": "int64", + }, + { + "error": "observable instrument not registered for callback", + "msg": "failed to record", + "name": "unregistered.observable.float64.counter", + "number": "float64", + }, + }, + }, + } + for _, tt := range testcases { + t.Run(tt.name, func(t *testing.T) { + logEntries := []map[string]interface{}{} + global.SetLogger( + funcr.NewJSON( + func(obj string) { + var entry map[string]interface{} + _ = json.Unmarshal([]byte(obj), &entry) + logEntries = append(logEntries, entry) + }, + funcr.Options{Verbosity: 0}, + ), + ) + + reader := NewManualReader() + meter := NewMeterProvider(WithView(tt.views...), WithReader(reader)).Meter("TestObservableDropAggregation") + + aiCounter, err := meter.Int64ObservableCounter("observable.int64.counter") + require.NoError(t, err) + aiUpDownCounter, err := meter.Int64ObservableUpDownCounter("observable.int64.up.down.counter") + require.NoError(t, err) + aiGauge, err := meter.Int64ObservableGauge("observable.int64.gauge") + require.NoError(t, err) + + afCounter, err := meter.Float64ObservableCounter("observable.float64.counter") + require.NoError(t, err) + afUpDownCounter, err := meter.Float64ObservableUpDownCounter("observable.float64.up.down.counter") + require.NoError(t, err) + afGauge, err := meter.Float64ObservableGauge("observable.float64.gauge") + require.NoError(t, err) + + unregisterediCounter, err := meter.Int64ObservableCounter("unregistered.observable.int64.counter") + require.NoError(t, err) + unregisteredfCounter, err := meter.Float64ObservableCounter("unregistered.observable.float64.counter") + require.NoError(t, err) + + _, err = meter.RegisterCallback( + func(ctx context.Context, obs metric.Observer) error { + obs.ObserveInt64(aiCounter, 1) + obs.ObserveInt64(aiUpDownCounter, 1) + obs.ObserveInt64(aiGauge, 1) + obs.ObserveFloat64(afCounter, 1) + obs.ObserveFloat64(afUpDownCounter, 1) + obs.ObserveFloat64(afGauge, 1) + + obs.ObserveInt64(unregisterediCounter, 1) + obs.ObserveFloat64(unregisteredfCounter, 1) + + return nil + }, + aiCounter, aiUpDownCounter, aiGauge, afCounter, afUpDownCounter, afGauge, + ) + require.NoError(t, err) + + var rm metricdata.ResourceMetrics + err = reader.Collect(context.Background(), &rm) + require.NoError(t, err) + + if len(tt.expectedObservableName) == 0 { + require.Len(t, rm.ScopeMetrics, 0) + return + } + + require.Len(t, rm.ScopeMetrics, 1) + require.Len(t, rm.ScopeMetrics[0].Metrics, len(tt.expectedObservableName)) + + for i, m := range rm.ScopeMetrics[0].Metrics { + assert.Equal(t, tt.expectedObservableName[i], m.Name) + } + + if diff := cmp.Diff(logEntries, tt.expectedLogs, + cmpopts.IgnoreMapEntries(func(_ string, v interface{}) bool { + return v.(string) == "" // skip comparing empty values + }), + ); diff != "" { + t.Errorf("Diff%v", diff) + } + }) + } +} From 7d10cef30c61f916ce94b4dc4f7612f9ab7aff54 Mon Sep 17 00:00:00 2001 From: ZHENK Date: Wed, 20 Dec 2023 21:28:49 +1300 Subject: [PATCH 3/9] Add testcase for dropping unregistered observable --- sdk/metric/meter_test.go | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/sdk/metric/meter_test.go b/sdk/metric/meter_test.go index 7ff3e89c27d..59952afb52c 100644 --- a/sdk/metric/meter_test.go +++ b/sdk/metric/meter_test.go @@ -2172,6 +2172,26 @@ func TestObservableDropAggregation(t *testing.T) { }, }, }, + { + name: "drop unregistered observable", + views: []View{ + func(i Instrument) (Stream, bool) { + if strings.HasPrefix(i.Name, "unregistered.observable") { + return Stream{Aggregation: AggregationDrop{}}, true + } + return Stream{}, false + }, + }, + expectedObservableName: []string{ + "observable.int64.counter", + "observable.int64.up.down.counter", + "observable.int64.gauge", + "observable.float64.counter", + "observable.float64.up.down.counter", + "observable.float64.gauge", + }, + expectedLogs: []map[string]interface{}{}, + }, } for _, tt := range testcases { t.Run(tt.name, func(t *testing.T) { From 74fc0edd423edd404ead4297414f0c1bad33ff8f Mon Sep 17 00:00:00 2001 From: ZHENK Date: Wed, 20 Dec 2023 21:39:21 +1300 Subject: [PATCH 4/9] Update CHANGELOG --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5cab19a25be..f6b031f93bc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -33,6 +33,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm ### Fixed - Fix `Parse` in `go.opentelemetry.io/otel/baggage` to validate member value before percent-decoding. (#4755) +- Fix `observable` not registered error during drop aggregation in `go.opentelemetry.io/otel/sdk/metric`. (#4760) ## [1.21.0/0.44.0] 2023-11-16 From 1793b43197eb30e75d930000764652aa5a24595f Mon Sep 17 00:00:00 2001 From: ZHENK Date: Thu, 21 Dec 2023 02:21:46 +1300 Subject: [PATCH 5/9] Add observable name const + suggestions --- CHANGELOG.md | 2 +- sdk/metric/go.mod | 1 - sdk/metric/go.sum | 1 - sdk/metric/meter_test.go | 187 +++++++++++++++++++-------------------- 4 files changed, 94 insertions(+), 97 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f6b031f93bc..b13d276cea3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -33,7 +33,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm ### Fixed - Fix `Parse` in `go.opentelemetry.io/otel/baggage` to validate member value before percent-decoding. (#4755) -- Fix `observable` not registered error during drop aggregation in `go.opentelemetry.io/otel/sdk/metric`. (#4760) +- Fix observable not registered error when the asynchronous instrument has a drop aggregation in `go.opentelemetry.io/otel/sdk/metric`. (#4772) ## [1.21.0/0.44.0] 2023-11-16 diff --git a/sdk/metric/go.mod b/sdk/metric/go.mod index 5fac032a82a..24325464289 100644 --- a/sdk/metric/go.mod +++ b/sdk/metric/go.mod @@ -5,7 +5,6 @@ go 1.20 require ( github.com/go-logr/logr v1.3.0 github.com/go-logr/stdr v1.2.2 - github.com/google/go-cmp v0.6.0 github.com/stretchr/testify v1.8.4 go.opentelemetry.io/otel v1.21.0 go.opentelemetry.io/otel/metric v1.21.0 diff --git a/sdk/metric/go.sum b/sdk/metric/go.sum index 939feb405bb..94020957893 100644 --- a/sdk/metric/go.sum +++ b/sdk/metric/go.sum @@ -6,7 +6,6 @@ github.com/go-logr/logr v1.3.0/go.mod h1:9T104GzyrTigFIr8wt5mBrctHMim0Nb2HLGrmQ4 github.com/go-logr/stdr v1.2.2 h1:hSWxHoqTgW2S2qGc0LTAI563KZ5YKYRhT3MFKZMbjag= github.com/go-logr/stdr v1.2.2/go.mod h1:mMo/vtBO5dYbehREoey6XUKy/eSumjCCveDpRre4VKE= github.com/google/go-cmp v0.6.0 h1:ofyhxvXcZhMsU5ulbFiLKl/XBFqE1GSq7atu8tAmTRI= -github.com/google/go-cmp v0.6.0/go.mod h1:17dUlkBOakJ0+DkrSSNjCkIjxS6bF9zb3elmeNGIjoY= github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZbAQM= github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4= github.com/stretchr/testify v1.8.4 h1:CcVxjf3Q8PM0mHUKJCdn+eZZtm5yQwehR5yeSVQQcUk= diff --git a/sdk/metric/meter_test.go b/sdk/metric/meter_test.go index 59952afb52c..7b0e6d31501 100644 --- a/sdk/metric/meter_test.go +++ b/sdk/metric/meter_test.go @@ -26,8 +26,6 @@ import ( "github.com/go-logr/logr" "github.com/go-logr/logr/funcr" "github.com/go-logr/logr/testr" - "github.com/google/go-cmp/cmp" - "github.com/google/go-cmp/cmp/cmpopts" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -2070,35 +2068,46 @@ func TestHistogramBucketPrecedenceOrdering(t *testing.T) { } func TestObservableDropAggregation(t *testing.T) { + const ( + intPrefix = "observable.int64." + intCntName = "observable.int64.counter" + intUDCntName = "observable.int64.up.down.counter" + intGaugeName = "observable.int64.gauge" + floatPrefix = "observable.float64." + floatCntName = "observable.float64.counter" + floatUDCntName = "observable.float64.up.down.counter" + floatGaugeName = "observable.float64.gauge" + unregPrefix = "unregistered.observable." + unregIntCntName = "unregistered.observable.int64.counter" + unregFloatCntName = "unregistered.observable.float64.counter" + ) + + type log struct { + name string + number string + } + testcases := []struct { - name string - views []View - expectedObservableName []string - expectedLogs []map[string]interface{} + name string + views []View + wantObservables []string + wantUnregLogs []log }{ { name: "default", - views: []View{}, - expectedObservableName: []string{ - "observable.int64.counter", - "observable.int64.up.down.counter", - "observable.int64.gauge", - "observable.float64.counter", - "observable.float64.up.down.counter", - "observable.float64.gauge", - }, - expectedLogs: []map[string]interface{}{ + views: nil, + wantObservables: []string{ + intCntName, intUDCntName, intGaugeName, + floatCntName, floatUDCntName, floatGaugeName, + }, + wantUnregLogs: []log{ { - "error": "observable instrument not registered for callback", - "msg": "failed to record", - "name": "unregistered.observable.int64.counter", - "number": "int64", + name: unregIntCntName, + number: "int64", }, { - "error": "observable instrument not registered for callback", - "msg": "failed to record", - "name": "unregistered.observable.float64.counter", - "number": "float64", + name: unregFloatCntName, + number: "float64", }, }, }, @@ -2109,36 +2118,30 @@ func TestObservableDropAggregation(t *testing.T) { return Stream{Aggregation: AggregationDrop{}}, true }, }, - expectedObservableName: []string{}, - expectedLogs: []map[string]interface{}{}, + wantObservables: nil, + wantUnregLogs: nil, }, { name: "drop float64 observable", views: []View{ func(i Instrument) (Stream, bool) { - if strings.HasPrefix(i.Name, "observable.float64") { + if strings.HasPrefix(i.Name, floatPrefix) { return Stream{Aggregation: AggregationDrop{}}, true } return Stream{}, false }, }, - expectedObservableName: []string{ - "observable.int64.counter", - "observable.int64.up.down.counter", - "observable.int64.gauge", + wantObservables: []string{ + intCntName, intUDCntName, intGaugeName, }, - expectedLogs: []map[string]interface{}{ + wantUnregLogs: []log{ { - "error": "observable instrument not registered for callback", - "msg": "failed to record", - "name": "unregistered.observable.int64.counter", - "number": "int64", + name: unregIntCntName, + number: "int64", }, { - "error": "observable instrument not registered for callback", - "msg": "failed to record", - "name": "unregistered.observable.float64.counter", - "number": "float64", + name: unregFloatCntName, + number: "float64", }, }, }, @@ -2146,29 +2149,23 @@ func TestObservableDropAggregation(t *testing.T) { name: "drop int64 observable", views: []View{ func(i Instrument) (Stream, bool) { - if strings.HasPrefix(i.Name, "observable.int64") { + if strings.HasPrefix(i.Name, intPrefix) { return Stream{Aggregation: AggregationDrop{}}, true } return Stream{}, false }, }, - expectedObservableName: []string{ - "observable.float64.counter", - "observable.float64.up.down.counter", - "observable.float64.gauge", + wantObservables: []string{ + floatCntName, floatUDCntName, floatGaugeName, }, - expectedLogs: []map[string]interface{}{ + wantUnregLogs: []log{ { - "error": "observable instrument not registered for callback", - "msg": "failed to record", - "name": "unregistered.observable.int64.counter", - "number": "int64", + name: unregIntCntName, + number: "int64", }, { - "error": "observable instrument not registered for callback", - "msg": "failed to record", - "name": "unregistered.observable.float64.counter", - "number": "float64", + name: unregFloatCntName, + number: "float64", }, }, }, @@ -2176,74 +2173,83 @@ func TestObservableDropAggregation(t *testing.T) { name: "drop unregistered observable", views: []View{ func(i Instrument) (Stream, bool) { - if strings.HasPrefix(i.Name, "unregistered.observable") { + if strings.HasPrefix(i.Name, unregPrefix) { return Stream{Aggregation: AggregationDrop{}}, true } return Stream{}, false }, }, - expectedObservableName: []string{ - "observable.int64.counter", - "observable.int64.up.down.counter", - "observable.int64.gauge", - "observable.float64.counter", - "observable.float64.up.down.counter", - "observable.float64.gauge", + wantObservables: []string{ + intCntName, intUDCntName, intGaugeName, + floatCntName, floatUDCntName, floatGaugeName, }, - expectedLogs: []map[string]interface{}{}, + wantUnregLogs: nil, }, } for _, tt := range testcases { t.Run(tt.name, func(t *testing.T) { - logEntries := []map[string]interface{}{} + var unregLogs []log global.SetLogger( funcr.NewJSON( func(obj string) { var entry map[string]interface{} _ = json.Unmarshal([]byte(obj), &entry) - logEntries = append(logEntries, entry) + + // All unregistered observables should log `errUnregObserver` error. + // A observable with drop aggregation is also unregistered, + // however this is expected and should not log an error. + require.Equal(t, errUnregObserver.Error(), entry["error"]) + + unregLogs = append(unregLogs, log{ + name: fmt.Sprintf("%v", entry["name"]), + number: fmt.Sprintf("%v", entry["number"]), + }) }, funcr.Options{Verbosity: 0}, ), ) + defer global.SetLogger(logr.Discard()) reader := NewManualReader() meter := NewMeterProvider(WithView(tt.views...), WithReader(reader)).Meter("TestObservableDropAggregation") - aiCounter, err := meter.Int64ObservableCounter("observable.int64.counter") + intCnt, err := meter.Int64ObservableCounter(intCntName) require.NoError(t, err) - aiUpDownCounter, err := meter.Int64ObservableUpDownCounter("observable.int64.up.down.counter") + intUDCnt, err := meter.Int64ObservableUpDownCounter(intUDCntName) require.NoError(t, err) - aiGauge, err := meter.Int64ObservableGauge("observable.int64.gauge") + intGaugeCnt, err := meter.Int64ObservableGauge(intGaugeName) require.NoError(t, err) - afCounter, err := meter.Float64ObservableCounter("observable.float64.counter") + floatCnt, err := meter.Float64ObservableCounter(floatCntName) require.NoError(t, err) - afUpDownCounter, err := meter.Float64ObservableUpDownCounter("observable.float64.up.down.counter") + floatUDCnt, err := meter.Float64ObservableUpDownCounter(floatUDCntName) require.NoError(t, err) - afGauge, err := meter.Float64ObservableGauge("observable.float64.gauge") + floatGaugeCnt, err := meter.Float64ObservableGauge(floatGaugeName) require.NoError(t, err) - unregisterediCounter, err := meter.Int64ObservableCounter("unregistered.observable.int64.counter") + unregIntCnt, err := meter.Int64ObservableCounter(unregIntCntName) require.NoError(t, err) - unregisteredfCounter, err := meter.Float64ObservableCounter("unregistered.observable.float64.counter") + unregFloatCnt, err := meter.Float64ObservableCounter(unregFloatCntName) require.NoError(t, err) _, err = meter.RegisterCallback( func(ctx context.Context, obs metric.Observer) error { - obs.ObserveInt64(aiCounter, 1) - obs.ObserveInt64(aiUpDownCounter, 1) - obs.ObserveInt64(aiGauge, 1) - obs.ObserveFloat64(afCounter, 1) - obs.ObserveFloat64(afUpDownCounter, 1) - obs.ObserveFloat64(afGauge, 1) - - obs.ObserveInt64(unregisterediCounter, 1) - obs.ObserveFloat64(unregisteredfCounter, 1) + obs.ObserveInt64(intCnt, 1) + obs.ObserveInt64(intUDCnt, 1) + obs.ObserveInt64(intGaugeCnt, 1) + obs.ObserveFloat64(floatCnt, 1) + obs.ObserveFloat64(floatUDCnt, 1) + obs.ObserveFloat64(floatGaugeCnt, 1) + // We deliberately call observe to unregistered observables + obs.ObserveInt64(unregIntCnt, 1) + obs.ObserveFloat64(unregFloatCnt, 1) return nil }, - aiCounter, aiUpDownCounter, aiGauge, afCounter, afUpDownCounter, afGauge, + intCnt, intUDCnt, intGaugeCnt, + floatCnt, floatUDCnt, floatGaugeCnt, + // We deliberately do not register `unregIntCnt` and `unregFloatCnt` + // to test that `errUnregObserver` is logged when observed by callback. ) require.NoError(t, err) @@ -2251,25 +2257,18 @@ func TestObservableDropAggregation(t *testing.T) { err = reader.Collect(context.Background(), &rm) require.NoError(t, err) - if len(tt.expectedObservableName) == 0 { + if len(tt.wantObservables) == 0 { require.Len(t, rm.ScopeMetrics, 0) return } require.Len(t, rm.ScopeMetrics, 1) - require.Len(t, rm.ScopeMetrics[0].Metrics, len(tt.expectedObservableName)) + require.Len(t, rm.ScopeMetrics[0].Metrics, len(tt.wantObservables)) for i, m := range rm.ScopeMetrics[0].Metrics { - assert.Equal(t, tt.expectedObservableName[i], m.Name) - } - - if diff := cmp.Diff(logEntries, tt.expectedLogs, - cmpopts.IgnoreMapEntries(func(_ string, v interface{}) bool { - return v.(string) == "" // skip comparing empty values - }), - ); diff != "" { - t.Errorf("Diff%v", diff) + assert.Equal(t, tt.wantObservables[i], m.Name) } + assert.Equal(t, tt.wantUnregLogs, unregLogs) }) } } From 7ba4c574b7cf2e93f40047b3778136ef71a96450 Mon Sep 17 00:00:00 2001 From: ZHENK Date: Thu, 21 Dec 2023 19:08:18 +1300 Subject: [PATCH 6/9] Add suggestions --- sdk/metric/meter_test.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/sdk/metric/meter_test.go b/sdk/metric/meter_test.go index 7b0e6d31501..d068ecd4bad 100644 --- a/sdk/metric/meter_test.go +++ b/sdk/metric/meter_test.go @@ -2189,7 +2189,7 @@ func TestObservableDropAggregation(t *testing.T) { for _, tt := range testcases { t.Run(tt.name, func(t *testing.T) { var unregLogs []log - global.SetLogger( + otel.SetLogger( funcr.NewJSON( func(obj string) { var entry map[string]interface{} @@ -2198,7 +2198,7 @@ func TestObservableDropAggregation(t *testing.T) { // All unregistered observables should log `errUnregObserver` error. // A observable with drop aggregation is also unregistered, // however this is expected and should not log an error. - require.Equal(t, errUnregObserver.Error(), entry["error"]) + assert.Equal(t, errUnregObserver.Error(), entry["error"]) unregLogs = append(unregLogs, log{ name: fmt.Sprintf("%v", entry["name"]), @@ -2208,7 +2208,7 @@ func TestObservableDropAggregation(t *testing.T) { funcr.Options{Verbosity: 0}, ), ) - defer global.SetLogger(logr.Discard()) + defer otel.SetLogger(logr.Discard()) reader := NewManualReader() meter := NewMeterProvider(WithView(tt.views...), WithReader(reader)).Meter("TestObservableDropAggregation") From 875fca6bd0fb97b17f54db717a67b097adea2130 Mon Sep 17 00:00:00 2001 From: sync-common-bot Date: Wed, 10 Jan 2024 15:44:29 +1300 Subject: [PATCH 7/9] Only error if the instrument is not dropped --- sdk/metric/meter.go | 39 +++++++++++++++++---------------------- 1 file changed, 17 insertions(+), 22 deletions(-) diff --git a/sdk/metric/meter.go b/sdk/metric/meter.go index 02697ce35f7..76f1e70a3d1 100644 --- a/sdk/metric/meter.go +++ b/sdk/metric/meter.go @@ -23,6 +23,7 @@ import ( "go.opentelemetry.io/otel/metric" "go.opentelemetry.io/otel/metric/embedded" "go.opentelemetry.io/otel/sdk/instrumentation" + "go.opentelemetry.io/otel/sdk/metric/internal/aggregate" ) @@ -438,18 +439,15 @@ func (r observer) ObserveFloat64(o metric.Float64Observable, v float64, opts ... return } - if oImpl.dropAggregation { - // Drop aggregation - return - } - if _, registered := r.float64[oImpl.observablID]; !registered { - global.Error(errUnregObserver, "failed to record", - "name", oImpl.name, - "description", oImpl.description, - "unit", oImpl.unit, - "number", fmt.Sprintf("%T", float64(0)), - ) + if !oImpl.dropAggregation { + global.Error(errUnregObserver, "failed to record", + "name", oImpl.name, + "description", oImpl.description, + "unit", oImpl.unit, + "number", fmt.Sprintf("%T", float64(0)), + ) + } return } c := metric.NewObserveConfig(opts) @@ -476,18 +474,15 @@ func (r observer) ObserveInt64(o metric.Int64Observable, v int64, opts ...metric return } - if oImpl.dropAggregation { - // Drop aggregation - return - } - if _, registered := r.int64[oImpl.observablID]; !registered { - global.Error(errUnregObserver, "failed to record", - "name", oImpl.name, - "description", oImpl.description, - "unit", oImpl.unit, - "number", fmt.Sprintf("%T", int64(0)), - ) + if !oImpl.dropAggregation { + global.Error(errUnregObserver, "failed to record", + "name", oImpl.name, + "description", oImpl.description, + "unit", oImpl.unit, + "number", fmt.Sprintf("%T", int64(0)), + ) + } return } c := metric.NewObserveConfig(opts) From 9e7e7729bfacc5fcfc4a5cbd9fe18109f6364a23 Mon Sep 17 00:00:00 2001 From: sync-common-bot Date: Thu, 11 Jan 2024 22:47:10 +1300 Subject: [PATCH 8/9] Decrease indentation --- sdk/metric/meter.go | 32 ++++++++++++++------------------ 1 file changed, 14 insertions(+), 18 deletions(-) diff --git a/sdk/metric/meter.go b/sdk/metric/meter.go index 76f1e70a3d1..4d816c0c562 100644 --- a/sdk/metric/meter.go +++ b/sdk/metric/meter.go @@ -439,15 +439,13 @@ func (r observer) ObserveFloat64(o metric.Float64Observable, v float64, opts ... return } - if _, registered := r.float64[oImpl.observablID]; !registered { - if !oImpl.dropAggregation { - global.Error(errUnregObserver, "failed to record", - "name", oImpl.name, - "description", oImpl.description, - "unit", oImpl.unit, - "number", fmt.Sprintf("%T", float64(0)), - ) - } + if _, registered := r.float64[oImpl.observablID]; !registered && !oImpl.dropAggregation { + global.Error(errUnregObserver, "failed to record", + "name", oImpl.name, + "description", oImpl.description, + "unit", oImpl.unit, + "number", fmt.Sprintf("%T", float64(0)), + ) return } c := metric.NewObserveConfig(opts) @@ -474,15 +472,13 @@ func (r observer) ObserveInt64(o metric.Int64Observable, v int64, opts ...metric return } - if _, registered := r.int64[oImpl.observablID]; !registered { - if !oImpl.dropAggregation { - global.Error(errUnregObserver, "failed to record", - "name", oImpl.name, - "description", oImpl.description, - "unit", oImpl.unit, - "number", fmt.Sprintf("%T", int64(0)), - ) - } + if _, registered := r.int64[oImpl.observablID]; !registered && !oImpl.dropAggregation { + global.Error(errUnregObserver, "failed to record", + "name", oImpl.name, + "description", oImpl.description, + "unit", oImpl.unit, + "number", fmt.Sprintf("%T", int64(0)), + ) return } c := metric.NewObserveConfig(opts) From 773248e59b6ff183cce71ade19a05803cef345f9 Mon Sep 17 00:00:00 2001 From: sync-common-bot Date: Fri, 12 Jan 2024 12:18:54 +1300 Subject: [PATCH 9/9] Revert "Decrease indentation" This reverts commit 9e7e7729bfacc5fcfc4a5cbd9fe18109f6364a23. --- sdk/metric/meter.go | 32 ++++++++++++++++++-------------- 1 file changed, 18 insertions(+), 14 deletions(-) diff --git a/sdk/metric/meter.go b/sdk/metric/meter.go index 4d816c0c562..76f1e70a3d1 100644 --- a/sdk/metric/meter.go +++ b/sdk/metric/meter.go @@ -439,13 +439,15 @@ func (r observer) ObserveFloat64(o metric.Float64Observable, v float64, opts ... return } - if _, registered := r.float64[oImpl.observablID]; !registered && !oImpl.dropAggregation { - global.Error(errUnregObserver, "failed to record", - "name", oImpl.name, - "description", oImpl.description, - "unit", oImpl.unit, - "number", fmt.Sprintf("%T", float64(0)), - ) + if _, registered := r.float64[oImpl.observablID]; !registered { + if !oImpl.dropAggregation { + global.Error(errUnregObserver, "failed to record", + "name", oImpl.name, + "description", oImpl.description, + "unit", oImpl.unit, + "number", fmt.Sprintf("%T", float64(0)), + ) + } return } c := metric.NewObserveConfig(opts) @@ -472,13 +474,15 @@ func (r observer) ObserveInt64(o metric.Int64Observable, v int64, opts ...metric return } - if _, registered := r.int64[oImpl.observablID]; !registered && !oImpl.dropAggregation { - global.Error(errUnregObserver, "failed to record", - "name", oImpl.name, - "description", oImpl.description, - "unit", oImpl.unit, - "number", fmt.Sprintf("%T", int64(0)), - ) + if _, registered := r.int64[oImpl.observablID]; !registered { + if !oImpl.dropAggregation { + global.Error(errUnregObserver, "failed to record", + "name", oImpl.name, + "description", oImpl.description, + "unit", oImpl.unit, + "number", fmt.Sprintf("%T", int64(0)), + ) + } return } c := metric.NewObserveConfig(opts)