From 76bbad8a31b2024af47a2027bce4520d3726d000 Mon Sep 17 00:00:00 2001 From: Joshua MacDonald Date: Thu, 10 Oct 2024 10:17:01 -0700 Subject: [PATCH 01/13] internal/global/meter.go: Use the proper unwrap method --- internal/global/meter.go | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/internal/global/meter.go b/internal/global/meter.go index e3db438a09f..d567a874812 100644 --- a/internal/global/meter.go +++ b/internal/global/meter.go @@ -487,16 +487,12 @@ func (m *meter) RegisterCallback(f metric.Callback, insts ...metric.Observable) return reg, nil } -type wrapped interface { - unwrap() metric.Observable -} - func unwrapInstruments(instruments []metric.Observable) []metric.Observable { out := make([]metric.Observable, 0, len(instruments)) for _, inst := range instruments { - if in, ok := inst.(wrapped); ok { - out = append(out, in.unwrap()) + if in, ok := inst.(unwrapper); ok { + out = append(out, in.Unwrap()) } else { out = append(out, inst) } From 6866675d8478e8f93c9cb42ddb5e2218eb9699c3 Mon Sep 17 00:00:00 2001 From: Joshua MacDonald Date: Thu, 10 Oct 2024 10:32:26 -0700 Subject: [PATCH 02/13] unwrap callback instrument arguments for global async instruments --- internal/global/meter.go | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/internal/global/meter.go b/internal/global/meter.go index d567a874812..df8162744a2 100644 --- a/internal/global/meter.go +++ b/internal/global/meter.go @@ -5,6 +5,7 @@ package global // import "go.opentelemetry.io/otel/internal/global" import ( "container/list" + "context" "reflect" "sync" @@ -511,6 +512,23 @@ type registration struct { unregMu sync.Mutex } +type unwrapObs struct { + embedded.Observer + obs metric.Observer +} + +func (uo *unwrapObs) ObserveFloat64(inst metric.Float64Observable, value float64, opts ...metric.ObserveOption) { + uo.obs.ObserveFloat64(inst.(unwrapper).Unwrap().(metric.Float64Observable), value, opts...) +} + +func (uo *unwrapObs) ObserveInt64(inst metric.Int64Observable, value int64, opts ...metric.ObserveOption) { + uo.obs.ObserveInt64(inst.(unwrapper).Unwrap().(metric.Int64Observable), value, opts...) +} + +func (c *registration) unwrappedCallback(ctx context.Context, obs metric.Observer) error { + return c.function(ctx, &unwrapObs{obs: obs}) +} + func (c *registration) setDelegate(m metric.Meter) { insts := unwrapInstruments(c.instruments) @@ -522,7 +540,7 @@ func (c *registration) setDelegate(m metric.Meter) { return } - reg, err := m.RegisterCallback(c.function, insts...) + reg, err := m.RegisterCallback(c.unwrappedCallback, insts...) if err != nil { GetErrorHandler().Handle(err) return From 62c688973641bcab202edec99850d28cd73598e1 Mon Sep 17 00:00:00 2001 From: Joshua MacDonald Date: Thu, 10 Oct 2024 10:33:12 -0700 Subject: [PATCH 03/13] new test --- internal/global/alt_sdk_test.go | 166 ++++++++++++++++++++++++++++++++ 1 file changed, 166 insertions(+) create mode 100644 internal/global/alt_sdk_test.go diff --git a/internal/global/alt_sdk_test.go b/internal/global/alt_sdk_test.go new file mode 100644 index 00000000000..91e9e7697d9 --- /dev/null +++ b/internal/global/alt_sdk_test.go @@ -0,0 +1,166 @@ +// Copyright The OpenTelemetry Authors +// SPDX-License-Identifier: Apache-2.0 + +package global // import "go.opentelemetry.io/otel/internal/global" + +import ( + "context" + "testing" + + "github.com/stretchr/testify/require" + + "go.opentelemetry.io/otel/metric" + "go.opentelemetry.io/otel/metric/embedded" +) + +type altMeterProvider struct { + t *testing.T + meters []*altMeter + embedded.MeterProvider +} + +var _ metric.MeterProvider = &altMeterProvider{} + +func (amp *altMeterProvider) Meter(name string, opts ...metric.MeterOption) metric.Meter { + am := &altMeter{ + provider: amp, + } + amp.meters = append(amp.meters, am) + return am +} + +type altMeter struct { + provider *altMeterProvider + cbs []metric.Callback + embedded.Meter +} + +var _ metric.Meter = &altMeter{} + +type testAsyncCounter struct { + meter *altMeter + embedded.Int64ObservableCounter + metric.Int64Observable +} + +var _ metric.Int64ObservableCounter = &testAsyncCounter{} + +type altRegistration struct { + cb metric.Callback + embedded.Registration +} + +type altObserver struct { + t *testing.T + embedded.Observer +} + +func (*altRegistration) Unregister() error { + return nil +} + +func (am *altMeter) Int64Counter(name string, options ...metric.Int64CounterOption) (metric.Int64Counter, error) { + return nil, nil +} +func (am *altMeter) Int64UpDownCounter(name string, options ...metric.Int64UpDownCounterOption) (metric.Int64UpDownCounter, error) { + return nil, nil +} +func (am *altMeter) Int64Histogram(name string, options ...metric.Int64HistogramOption) (metric.Int64Histogram, error) { + return nil, nil +} + +func (am *altMeter) Int64Gauge(name string, options ...metric.Int64GaugeOption) (metric.Int64Gauge, error) { + return nil, nil +} + +func (am *altMeter) Int64ObservableCounter(name string, options ...metric.Int64ObservableCounterOption) (metric.Int64ObservableCounter, error) { + return &testAsyncCounter{ + meter: am, + }, nil +} + +func (am *altMeter) Int64ObservableUpDownCounter(name string, options ...metric.Int64ObservableUpDownCounterOption) (metric.Int64ObservableUpDownCounter, error) { + return nil, nil +} + +func (am *altMeter) Int64ObservableGauge(name string, options ...metric.Int64ObservableGaugeOption) (metric.Int64ObservableGauge, error) { + return nil, nil +} +func (am *altMeter) Float64Counter(name string, options ...metric.Float64CounterOption) (metric.Float64Counter, error) { + return nil, nil +} +func (am *altMeter) Float64UpDownCounter(name string, options ...metric.Float64UpDownCounterOption) (metric.Float64UpDownCounter, error) { + return nil, nil +} +func (am *altMeter) Float64Histogram(name string, options ...metric.Float64HistogramOption) (metric.Float64Histogram, error) { + return nil, nil +} +func (am *altMeter) Float64Gauge(name string, options ...metric.Float64GaugeOption) (metric.Float64Gauge, error) { + return nil, nil +} +func (am *altMeter) Float64ObservableCounter(name string, options ...metric.Float64ObservableCounterOption) (metric.Float64ObservableCounter, error) { + return nil, nil +} +func (am *altMeter) Float64ObservableUpDownCounter(name string, options ...metric.Float64ObservableUpDownCounterOption) (metric.Float64ObservableUpDownCounter, error) { + return nil, nil +} +func (am *altMeter) Float64ObservableGauge(name string, options ...metric.Float64ObservableGaugeOption) (metric.Float64ObservableGauge, error) { + // Note: The global delegation also breaks when we return nil in one of these! + return nil, nil +} + +func (am *altMeter) RegisterCallback(f metric.Callback, instruments ...metric.Observable) (metric.Registration, error) { + for _, inst := range instruments { + switch inst.(type) { + case *testAsyncCounter: + // OK! + default: + am.provider.t.Errorf("unexpected type %T", inst) + } + } + am.cbs = append(am.cbs, f) + return &altRegistration{cb: f}, nil +} + +func (ao *altObserver) ObserveFloat64(inst metric.Float64Observable, _ float64, _ ...metric.ObserveOption) { + ao.observe(inst) +} + +func (ao *altObserver) ObserveInt64(inst metric.Int64Observable, _ int64, _ ...metric.ObserveOption) { + ao.observe(inst) +} + +func (ao *altObserver) observe(inst any) { + switch inst.(type) { + case *testAsyncCounter: + // OK! + default: + ao.t.Errorf("unexpected type %T", inst) + } +} + +func TestMeterDelegation(t *testing.T) { + ResetForTest(t) + + amp := &altMeterProvider{t: t} + + gm := MeterProvider().Meter("test") + ai, err := gm.Int64ObservableCounter("test_counter") + require.NoError(t, err) + + _, err = gm.RegisterCallback(func(_ context.Context, obs metric.Observer) error { + obs.ObserveInt64(ai, 10) + return nil + }, ai) + require.NoError(t, err) + + SetMeterProvider(amp) + + ctx := context.Background() + ao := &altObserver{t: t} + for _, meter := range amp.meters { + for _, cb := range meter.cbs { + cb(ctx, ao) + } + } +} From a577bf4752e5ec90b38cee5dbaa12eb05f973c11 Mon Sep 17 00:00:00 2001 From: Joshua MacDonald Date: Thu, 10 Oct 2024 11:55:25 -0700 Subject: [PATCH 04/13] let unwrap be private --- internal/global/instruments.go | 14 +++++++------- internal/global/instruments_test.go | 12 ++++++------ internal/global/meter.go | 26 ++++++++++++++++---------- sdk/metric/meter.go | 27 --------------------------- 4 files changed, 29 insertions(+), 50 deletions(-) diff --git a/internal/global/instruments.go b/internal/global/instruments.go index 3a0cc42f6a4..ae92a425166 100644 --- a/internal/global/instruments.go +++ b/internal/global/instruments.go @@ -13,7 +13,7 @@ import ( // unwrapper unwraps to return the underlying instrument implementation. type unwrapper interface { - Unwrap() metric.Observable + unwrap() metric.Observable } type afCounter struct { @@ -40,7 +40,7 @@ func (i *afCounter) setDelegate(m metric.Meter) { i.delegate.Store(ctr) } -func (i *afCounter) Unwrap() metric.Observable { +func (i *afCounter) unwrap() metric.Observable { if ctr := i.delegate.Load(); ctr != nil { return ctr.(metric.Float64ObservableCounter) } @@ -71,7 +71,7 @@ func (i *afUpDownCounter) setDelegate(m metric.Meter) { i.delegate.Store(ctr) } -func (i *afUpDownCounter) Unwrap() metric.Observable { +func (i *afUpDownCounter) unwrap() metric.Observable { if ctr := i.delegate.Load(); ctr != nil { return ctr.(metric.Float64ObservableUpDownCounter) } @@ -102,7 +102,7 @@ func (i *afGauge) setDelegate(m metric.Meter) { i.delegate.Store(ctr) } -func (i *afGauge) Unwrap() metric.Observable { +func (i *afGauge) unwrap() metric.Observable { if ctr := i.delegate.Load(); ctr != nil { return ctr.(metric.Float64ObservableGauge) } @@ -133,7 +133,7 @@ func (i *aiCounter) setDelegate(m metric.Meter) { i.delegate.Store(ctr) } -func (i *aiCounter) Unwrap() metric.Observable { +func (i *aiCounter) unwrap() metric.Observable { if ctr := i.delegate.Load(); ctr != nil { return ctr.(metric.Int64ObservableCounter) } @@ -164,7 +164,7 @@ func (i *aiUpDownCounter) setDelegate(m metric.Meter) { i.delegate.Store(ctr) } -func (i *aiUpDownCounter) Unwrap() metric.Observable { +func (i *aiUpDownCounter) unwrap() metric.Observable { if ctr := i.delegate.Load(); ctr != nil { return ctr.(metric.Int64ObservableUpDownCounter) } @@ -195,7 +195,7 @@ func (i *aiGauge) setDelegate(m metric.Meter) { i.delegate.Store(ctr) } -func (i *aiGauge) Unwrap() metric.Observable { +func (i *aiGauge) unwrap() metric.Observable { if ctr := i.delegate.Load(); ctr != nil { return ctr.(metric.Int64ObservableGauge) } diff --git a/internal/global/instruments_test.go b/internal/global/instruments_test.go index 48c772e3b8a..74a89892bb8 100644 --- a/internal/global/instruments_test.go +++ b/internal/global/instruments_test.go @@ -57,19 +57,19 @@ func TestAsyncInstrumentSetDelegateConcurrentSafe(t *testing.T) { t.Run("Float64", func(t *testing.T) { t.Run("Counter", func(t *testing.T) { delegate := &afCounter{} - f := func(float64) { _ = delegate.Unwrap() } + f := func(float64) { _ = delegate.unwrap() } testFloat64ConcurrentSafe(f, delegate.setDelegate) }) t.Run("UpDownCounter", func(t *testing.T) { delegate := &afUpDownCounter{} - f := func(float64) { _ = delegate.Unwrap() } + f := func(float64) { _ = delegate.unwrap() } testFloat64ConcurrentSafe(f, delegate.setDelegate) }) t.Run("Gauge", func(t *testing.T) { delegate := &afGauge{} - f := func(float64) { _ = delegate.Unwrap() } + f := func(float64) { _ = delegate.unwrap() } testFloat64ConcurrentSafe(f, delegate.setDelegate) }) }) @@ -79,19 +79,19 @@ func TestAsyncInstrumentSetDelegateConcurrentSafe(t *testing.T) { t.Run("Int64", func(t *testing.T) { t.Run("Counter", func(t *testing.T) { delegate := &aiCounter{} - f := func(int64) { _ = delegate.Unwrap() } + f := func(int64) { _ = delegate.unwrap() } testInt64ConcurrentSafe(f, delegate.setDelegate) }) t.Run("UpDownCounter", func(t *testing.T) { delegate := &aiUpDownCounter{} - f := func(int64) { _ = delegate.Unwrap() } + f := func(int64) { _ = delegate.unwrap() } testInt64ConcurrentSafe(f, delegate.setDelegate) }) t.Run("Gauge", func(t *testing.T) { delegate := &aiGauge{} - f := func(int64) { _ = delegate.Unwrap() } + f := func(int64) { _ = delegate.unwrap() } testInt64ConcurrentSafe(f, delegate.setDelegate) }) }) diff --git a/internal/global/meter.go b/internal/global/meter.go index df8162744a2..9a38823dcd2 100644 --- a/internal/global/meter.go +++ b/internal/global/meter.go @@ -473,8 +473,7 @@ func (m *meter) RegisterCallback(f metric.Callback, insts ...metric.Observable) defer m.mtx.Unlock() if m.delegate != nil { - insts = unwrapInstruments(insts) - return m.delegate.RegisterCallback(f, insts...) + return m.delegate.RegisterCallback(unwrapCallback(f), unwrapInstruments(insts)...) } reg := ®istration{instruments: insts, function: f} @@ -493,7 +492,7 @@ func unwrapInstruments(instruments []metric.Observable) []metric.Observable { for _, inst := range instruments { if in, ok := inst.(unwrapper); ok { - out = append(out, in.Unwrap()) + out = append(out, in.unwrap()) } else { out = append(out, inst) } @@ -518,20 +517,27 @@ type unwrapObs struct { } func (uo *unwrapObs) ObserveFloat64(inst metric.Float64Observable, value float64, opts ...metric.ObserveOption) { - uo.obs.ObserveFloat64(inst.(unwrapper).Unwrap().(metric.Float64Observable), value, opts...) + if un, ok := inst.(unwrapper); ok { + inst = un.unwrap().(metric.Float64Observable) + } + + uo.obs.ObserveFloat64(inst, value, opts...) } func (uo *unwrapObs) ObserveInt64(inst metric.Int64Observable, value int64, opts ...metric.ObserveOption) { - uo.obs.ObserveInt64(inst.(unwrapper).Unwrap().(metric.Int64Observable), value, opts...) + if un, ok := inst.(unwrapper); ok { + inst = un.unwrap().(metric.Int64Observable) + } + uo.obs.ObserveInt64(inst, value, opts...) } -func (c *registration) unwrappedCallback(ctx context.Context, obs metric.Observer) error { - return c.function(ctx, &unwrapObs{obs: obs}) +func unwrapCallback(f metric.Callback) metric.Callback { + return func(ctx context.Context, obs metric.Observer) error { + return f(ctx, &unwrapObs{obs: obs}) + } } func (c *registration) setDelegate(m metric.Meter) { - insts := unwrapInstruments(c.instruments) - c.unregMu.Lock() defer c.unregMu.Unlock() @@ -540,7 +546,7 @@ func (c *registration) setDelegate(m metric.Meter) { return } - reg, err := m.RegisterCallback(c.unwrappedCallback, insts...) + reg, err := m.RegisterCallback(unwrapCallback(c.function), unwrapInstruments(c.instruments)...) if err != nil { GetErrorHandler().Handle(err) return diff --git a/sdk/metric/meter.go b/sdk/metric/meter.go index abff4650e1a..44d39b35dfe 100644 --- a/sdk/metric/meter.go +++ b/sdk/metric/meter.go @@ -444,13 +444,6 @@ func (m *meter) RegisterCallback(f metric.Callback, insts ...metric.Observable) reg := newObserver() var errs multierror for _, inst := range insts { - // Unwrap any global. - if u, ok := inst.(interface { - Unwrap() metric.Observable - }); ok { - inst = u.Unwrap() - } - switch o := inst.(type) { case int64Observable: if err := o.registerable(m); err != nil { @@ -521,16 +514,6 @@ func (r observer) ObserveFloat64(o metric.Float64Observable, v float64, opts ... switch conv := o.(type) { case float64Observable: oImpl = conv - case interface { - Unwrap() metric.Observable - }: - // Unwrap any global. - async := conv.Unwrap() - var ok bool - if oImpl, ok = async.(float64Observable); !ok { - global.Error(errUnknownObserver, "failed to record asynchronous") - return - } default: global.Error(errUnknownObserver, "failed to record") return @@ -556,16 +539,6 @@ func (r observer) ObserveInt64(o metric.Int64Observable, v int64, opts ...metric switch conv := o.(type) { case int64Observable: oImpl = conv - case interface { - Unwrap() metric.Observable - }: - // Unwrap any global. - async := conv.Unwrap() - var ok bool - if oImpl, ok = async.(int64Observable); !ok { - global.Error(errUnknownObserver, "failed to record asynchronous") - return - } default: global.Error(errUnknownObserver, "failed to record") return From 9e49a96e7e50f0ac2b9c3c43a31d715956c57de8 Mon Sep 17 00:00:00 2001 From: Joshua MacDonald Date: Thu, 10 Oct 2024 11:58:46 -0700 Subject: [PATCH 05/13] chlog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 63237296913..21d8fcd2ee8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -34,6 +34,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm - Fix timer channel drain to avoid hanging on Go 1.23. (#5868) - Fix delegation for global meter providers. (#5827) Change the `reflect.TypeOf` to use a nil pointer to not allocate on the heap unless necessary. +- Global MeterProvider registration correctly unwraps global instrument stubs. (#5881) From 9419fd1dd104442370e21ecd06845166d8b951fe Mon Sep 17 00:00:00 2001 From: Joshua MacDonald Date: Thu, 10 Oct 2024 12:03:54 -0700 Subject: [PATCH 06/13] lint --- internal/global/alt_sdk_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/global/alt_sdk_test.go b/internal/global/alt_sdk_test.go index 91e9e7697d9..d4aaadd4c49 100644 --- a/internal/global/alt_sdk_test.go +++ b/internal/global/alt_sdk_test.go @@ -160,7 +160,7 @@ func TestMeterDelegation(t *testing.T) { ao := &altObserver{t: t} for _, meter := range amp.meters { for _, cb := range meter.cbs { - cb(ctx, ao) + require.NoError(t, cb(ctx, ao)) } } } From c4fffde6537f3afc4f8f93bf5b170fb285bfe60b Mon Sep 17 00:00:00 2001 From: Joshua MacDonald Date: Thu, 10 Oct 2024 12:06:37 -0700 Subject: [PATCH 07/13] format --- internal/global/alt_sdk_test.go | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/internal/global/alt_sdk_test.go b/internal/global/alt_sdk_test.go index d4aaadd4c49..326eec97e5c 100644 --- a/internal/global/alt_sdk_test.go +++ b/internal/global/alt_sdk_test.go @@ -62,9 +62,11 @@ func (*altRegistration) Unregister() error { func (am *altMeter) Int64Counter(name string, options ...metric.Int64CounterOption) (metric.Int64Counter, error) { return nil, nil } + func (am *altMeter) Int64UpDownCounter(name string, options ...metric.Int64UpDownCounterOption) (metric.Int64UpDownCounter, error) { return nil, nil } + func (am *altMeter) Int64Histogram(name string, options ...metric.Int64HistogramOption) (metric.Int64Histogram, error) { return nil, nil } @@ -86,24 +88,31 @@ func (am *altMeter) Int64ObservableUpDownCounter(name string, options ...metric. func (am *altMeter) Int64ObservableGauge(name string, options ...metric.Int64ObservableGaugeOption) (metric.Int64ObservableGauge, error) { return nil, nil } + func (am *altMeter) Float64Counter(name string, options ...metric.Float64CounterOption) (metric.Float64Counter, error) { return nil, nil } + func (am *altMeter) Float64UpDownCounter(name string, options ...metric.Float64UpDownCounterOption) (metric.Float64UpDownCounter, error) { return nil, nil } + func (am *altMeter) Float64Histogram(name string, options ...metric.Float64HistogramOption) (metric.Float64Histogram, error) { return nil, nil } + func (am *altMeter) Float64Gauge(name string, options ...metric.Float64GaugeOption) (metric.Float64Gauge, error) { return nil, nil } + func (am *altMeter) Float64ObservableCounter(name string, options ...metric.Float64ObservableCounterOption) (metric.Float64ObservableCounter, error) { return nil, nil } + func (am *altMeter) Float64ObservableUpDownCounter(name string, options ...metric.Float64ObservableUpDownCounterOption) (metric.Float64ObservableUpDownCounter, error) { return nil, nil } + func (am *altMeter) Float64ObservableGauge(name string, options ...metric.Float64ObservableGaugeOption) (metric.Float64ObservableGauge, error) { // Note: The global delegation also breaks when we return nil in one of these! return nil, nil From 2271c1ab0489c32560f3334d4da27c7e2549e666 Mon Sep 17 00:00:00 2001 From: David Ashpole Date: Fri, 11 Oct 2024 15:52:08 -0400 Subject: [PATCH 08/13] Update CHANGELOG.md --- CHANGELOG.md | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d9bed67018e..acd16229528 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,8 +10,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm ### Fixed -- Fix delegation for global meter providers, and panic when calling otel.SetMeterProvider. (#5827) -- Change the `reflect.TypeOf` to use a nil pointer to not allocate on the heap unless necessary. (#5827) +- Global MeterProvider registration correctly unwraps global instrument stubs. (#5881) @@ -42,9 +41,8 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm - The race condition for multiple `FixedSize` exemplar reservoirs identified in #5814 is resolved. (#5819) - Fix log records duplication in case of heterogeneous resource attributes by correctly mapping each log record to it's resource and scope. (#5803) - Fix timer channel drain to avoid hanging on Go 1.23. (#5868) -- Fix delegation for global meter providers. (#5827) - Change the `reflect.TypeOf` to use a nil pointer to not allocate on the heap unless necessary. -- Global MeterProvider registration correctly unwraps global instrument stubs. (#5881) +- Fix delegation for global meter providers, and panic when calling otel.SetMeterProvider. (#5827) +- Change the `reflect.TypeOf` to use a nil pointer to not allocate on the heap unless necessary. (#5827) From 892766602b958a46a052291fe66af6cf601b3253 Mon Sep 17 00:00:00 2001 From: David Ashpole Date: Fri, 11 Oct 2024 15:52:29 -0400 Subject: [PATCH 09/13] Update CHANGELOG.md --- CHANGELOG.md | 3 --- 1 file changed, 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index acd16229528..969176abc9f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -44,9 +44,6 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm - Fix delegation for global meter providers, and panic when calling otel.SetMeterProvider. (#5827) - Change the `reflect.TypeOf` to use a nil pointer to not allocate on the heap unless necessary. (#5827) - - - ## [1.30.0/0.52.0/0.6.0/0.0.9] 2024-09-09 ### Added From 9a86e2b98190f8af3fb3598140f1ea18f887124a Mon Sep 17 00:00:00 2001 From: Joshua MacDonald Date: Fri, 18 Oct 2024 10:21:57 -0700 Subject: [PATCH 10/13] use a checked type conversion --- internal/global/alt_sdk_test.go | 133 +++++++++++++++++++++++++------- internal/global/meter.go | 44 +++++++++-- 2 files changed, 140 insertions(+), 37 deletions(-) diff --git a/internal/global/alt_sdk_test.go b/internal/global/alt_sdk_test.go index 326eec97e5c..428534178a3 100644 --- a/internal/global/alt_sdk_test.go +++ b/internal/global/alt_sdk_test.go @@ -11,8 +11,15 @@ import ( "go.opentelemetry.io/otel/metric" "go.opentelemetry.io/otel/metric/embedded" + "go.opentelemetry.io/otel/metric/noop" ) +// Below, an alternate meter provider is constructed specifically to +// test the asynchronous instrument path. The alternative SDK uses +// no-op implementations for its synchronous instruments, and the six +// asynchronous instrument types are created here to test that +// instruments and callbacks are unwrapped inside this library. + type altMeterProvider struct { t *testing.T meters []*altMeter @@ -37,13 +44,53 @@ type altMeter struct { var _ metric.Meter = &altMeter{} -type testAsyncCounter struct { +type testAiCounter struct { meter *altMeter embedded.Int64ObservableCounter metric.Int64Observable } -var _ metric.Int64ObservableCounter = &testAsyncCounter{} +var _ metric.Int64ObservableCounter = &testAiCounter{} + +type testAfCounter struct { + meter *altMeter + embedded.Float64ObservableCounter + metric.Float64Observable +} + +var _ metric.Float64ObservableCounter = &testAfCounter{} + +type testAiUpDownCounter struct { + meter *altMeter + embedded.Int64ObservableUpDownCounter + metric.Int64Observable +} + +var _ metric.Int64ObservableUpDownCounter = &testAiUpDownCounter{} + +type testAfUpDownCounter struct { + meter *altMeter + embedded.Float64ObservableUpDownCounter + metric.Float64Observable +} + +var _ metric.Float64ObservableUpDownCounter = &testAfUpDownCounter{} + +type testAiGauge struct { + meter *altMeter + embedded.Int64ObservableGauge + metric.Int64Observable +} + +var _ metric.Int64ObservableGauge = &testAiGauge{} + +type testAfGauge struct { + meter *altMeter + embedded.Float64ObservableGauge + metric.Float64Observable +} + +var _ metric.Float64ObservableGauge = &testAfGauge{} type altRegistration struct { cb metric.Callback @@ -59,70 +106,81 @@ func (*altRegistration) Unregister() error { return nil } -func (am *altMeter) Int64Counter(name string, options ...metric.Int64CounterOption) (metric.Int64Counter, error) { - return nil, nil +func (am *altMeter) Int64Counter(name string, _ ...metric.Int64CounterOption) (metric.Int64Counter, error) { + return noop.NewMeterProvider().Meter("noop").Int64Counter(name) } -func (am *altMeter) Int64UpDownCounter(name string, options ...metric.Int64UpDownCounterOption) (metric.Int64UpDownCounter, error) { - return nil, nil +func (am *altMeter) Int64UpDownCounter(name string, _ ...metric.Int64UpDownCounterOption) (metric.Int64UpDownCounter, error) { + return noop.NewMeterProvider().Meter("noop").Int64UpDownCounter(name) } -func (am *altMeter) Int64Histogram(name string, options ...metric.Int64HistogramOption) (metric.Int64Histogram, error) { - return nil, nil +func (am *altMeter) Int64Histogram(name string, _ ...metric.Int64HistogramOption) (metric.Int64Histogram, error) { + return noop.NewMeterProvider().Meter("noop").Int64Histogram(name) } -func (am *altMeter) Int64Gauge(name string, options ...metric.Int64GaugeOption) (metric.Int64Gauge, error) { - return nil, nil +func (am *altMeter) Int64Gauge(name string, _ ...metric.Int64GaugeOption) (metric.Int64Gauge, error) { + return noop.NewMeterProvider().Meter("noop").Int64Gauge(name) } func (am *altMeter) Int64ObservableCounter(name string, options ...metric.Int64ObservableCounterOption) (metric.Int64ObservableCounter, error) { - return &testAsyncCounter{ + return &testAiCounter{ meter: am, }, nil } func (am *altMeter) Int64ObservableUpDownCounter(name string, options ...metric.Int64ObservableUpDownCounterOption) (metric.Int64ObservableUpDownCounter, error) { - return nil, nil + return &testAiUpDownCounter{ + meter: am, + }, nil } func (am *altMeter) Int64ObservableGauge(name string, options ...metric.Int64ObservableGaugeOption) (metric.Int64ObservableGauge, error) { - return nil, nil + return &testAiGauge{ + meter: am, + }, nil } -func (am *altMeter) Float64Counter(name string, options ...metric.Float64CounterOption) (metric.Float64Counter, error) { - return nil, nil +func (am *altMeter) Float64Counter(name string, _ ...metric.Float64CounterOption) (metric.Float64Counter, error) { + return noop.NewMeterProvider().Meter("noop").Float64Counter(name) } -func (am *altMeter) Float64UpDownCounter(name string, options ...metric.Float64UpDownCounterOption) (metric.Float64UpDownCounter, error) { - return nil, nil +func (am *altMeter) Float64UpDownCounter(name string, _ ...metric.Float64UpDownCounterOption) (metric.Float64UpDownCounter, error) { + return noop.NewMeterProvider().Meter("noop").Float64UpDownCounter(name) } func (am *altMeter) Float64Histogram(name string, options ...metric.Float64HistogramOption) (metric.Float64Histogram, error) { - return nil, nil + return noop.NewMeterProvider().Meter("noop").Float64Histogram(name) } func (am *altMeter) Float64Gauge(name string, options ...metric.Float64GaugeOption) (metric.Float64Gauge, error) { - return nil, nil + return noop.NewMeterProvider().Meter("noop").Float64Gauge(name) } func (am *altMeter) Float64ObservableCounter(name string, options ...metric.Float64ObservableCounterOption) (metric.Float64ObservableCounter, error) { - return nil, nil + return &testAfCounter{ + meter: am, + }, nil } func (am *altMeter) Float64ObservableUpDownCounter(name string, options ...metric.Float64ObservableUpDownCounterOption) (metric.Float64ObservableUpDownCounter, error) { - return nil, nil + return &testAfUpDownCounter{ + meter: am, + }, nil } func (am *altMeter) Float64ObservableGauge(name string, options ...metric.Float64ObservableGaugeOption) (metric.Float64ObservableGauge, error) { - // Note: The global delegation also breaks when we return nil in one of these! - return nil, nil + return &testAfGauge{ + meter: am, + }, nil } func (am *altMeter) RegisterCallback(f metric.Callback, instruments ...metric.Observable) (metric.Registration, error) { for _, inst := range instruments { switch inst.(type) { - case *testAsyncCounter: - // OK! + case *testAiCounter, *testAfCounter, + *testAiUpDownCounter, *testAfUpDownCounter, + *testAiGauge, *testAfGauge: + // OK! default: am.provider.t.Errorf("unexpected type %T", inst) } @@ -141,7 +199,9 @@ func (ao *altObserver) ObserveInt64(inst metric.Int64Observable, _ int64, _ ...m func (ao *altObserver) observe(inst any) { switch inst.(type) { - case *testAsyncCounter: + case *testAiCounter, *testAfCounter, + *testAiUpDownCounter, *testAfUpDownCounter, + *testAiGauge, *testAfGauge: // OK! default: ao.t.Errorf("unexpected type %T", inst) @@ -154,13 +214,28 @@ func TestMeterDelegation(t *testing.T) { amp := &altMeterProvider{t: t} gm := MeterProvider().Meter("test") - ai, err := gm.Int64ObservableCounter("test_counter") + aic, err := gm.Int64ObservableCounter("test_counter_i") + require.NoError(t, err) + afc, err := gm.Float64ObservableCounter("test_counter_f") + require.NoError(t, err) + aiu, err := gm.Int64ObservableUpDownCounter("test_updowncounter_i") + require.NoError(t, err) + afu, err := gm.Float64ObservableUpDownCounter("test_updowncounter_f") + require.NoError(t, err) + aig, err := gm.Int64ObservableGauge("test_gauge_i") + require.NoError(t, err) + afg, err := gm.Float64ObservableGauge("test_gauge_f") require.NoError(t, err) _, err = gm.RegisterCallback(func(_ context.Context, obs metric.Observer) error { - obs.ObserveInt64(ai, 10) + obs.ObserveInt64(aic, 10) + obs.ObserveFloat64(afc, 10) + obs.ObserveInt64(aiu, 10) + obs.ObserveFloat64(afu, 10) + obs.ObserveInt64(aig, 10) + obs.ObserveFloat64(afg, 10) return nil - }, ai) + }, aic, afc, aiu, afu, aig, afg) require.NoError(t, err) SetMeterProvider(amp) diff --git a/internal/global/meter.go b/internal/global/meter.go index 9a38823dcd2..cc9b7246fd2 100644 --- a/internal/global/meter.go +++ b/internal/global/meter.go @@ -516,19 +516,47 @@ type unwrapObs struct { obs metric.Observer } -func (uo *unwrapObs) ObserveFloat64(inst metric.Float64Observable, value float64, opts ...metric.ObserveOption) { - if un, ok := inst.(unwrapper); ok { - inst = un.unwrap().(metric.Float64Observable) +// unwrapFloat64Observable returns an expected metric.Float64Observable after +// unwrapping the global object. +func unwrapFloat64Observable(inst metric.Float64Observable) metric.Float64Observable { + if unwrapped, ok := inst.(unwrapper); ok { + if floatObs, ok := unwrapped.unwrap().(metric.Float64Observable); ok { + // Note: if the unwrapped object does not + // unwrap as an observable for either of the + // branches here, it means an internal bug in + // this package. We avoid logging an error in + // this case, because the SDK has to try its + // own type conversion on the object. The SDK + // will see this and be forced to respond with + // its own error. + // + // This code uses a double-nested if statement + // to avoid creating a branch that is + // impossible to cover. + inst = floatObs + } } + return inst +} - uo.obs.ObserveFloat64(inst, value, opts...) +// unwrapInt64Observable returns an expected metric.Int64Observable after +// unwrapping the global object. +func unwrapInt64Observable(inst metric.Int64Observable) metric.Int64Observable { + if unwrapped, ok := inst.(unwrapper); ok { + if unint, ok := unwrapped.unwrap().(metric.Int64Observable); ok { + // See the comment in unwrapFloat64Observable(). + inst = unint + } + } + return inst +} + +func (uo *unwrapObs) ObserveFloat64(inst metric.Float64Observable, value float64, opts ...metric.ObserveOption) { + uo.obs.ObserveFloat64(unwrapFloat64Observable(inst), value, opts...) } func (uo *unwrapObs) ObserveInt64(inst metric.Int64Observable, value int64, opts ...metric.ObserveOption) { - if un, ok := inst.(unwrapper); ok { - inst = un.unwrap().(metric.Int64Observable) - } - uo.obs.ObserveInt64(inst, value, opts...) + uo.obs.ObserveInt64(unwrapInt64Observable(inst), value, opts...) } func unwrapCallback(f metric.Callback) metric.Callback { From e0ff0af7740aa584116b9a9a5a5f9a90dee8a33d Mon Sep 17 00:00:00 2001 From: Joshua MacDonald Date: Fri, 18 Oct 2024 10:22:18 -0700 Subject: [PATCH 11/13] rename alternate_meter_test.go --- internal/global/{alt_sdk_test.go => alternate_meter_test.go} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename internal/global/{alt_sdk_test.go => alternate_meter_test.go} (100%) diff --git a/internal/global/alt_sdk_test.go b/internal/global/alternate_meter_test.go similarity index 100% rename from internal/global/alt_sdk_test.go rename to internal/global/alternate_meter_test.go From 5a7caf716d9d75a7297fd21ff6127d8a30e7e7ba Mon Sep 17 00:00:00 2001 From: Joshua MacDonald Date: Fri, 18 Oct 2024 10:23:15 -0700 Subject: [PATCH 12/13] comment --- internal/global/meter.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/global/meter.go b/internal/global/meter.go index cc9b7246fd2..78520e8d6e9 100644 --- a/internal/global/meter.go +++ b/internal/global/meter.go @@ -523,7 +523,7 @@ func unwrapFloat64Observable(inst metric.Float64Observable) metric.Float64Observ if floatObs, ok := unwrapped.unwrap().(metric.Float64Observable); ok { // Note: if the unwrapped object does not // unwrap as an observable for either of the - // branches here, it means an internal bug in + // predicates here, it means an internal bug in // this package. We avoid logging an error in // this case, because the SDK has to try its // own type conversion on the object. The SDK From 99a6b401281750137ca55378f79a9e53ad783f96 Mon Sep 17 00:00:00 2001 From: Joshua MacDonald Date: Fri, 18 Oct 2024 10:24:53 -0700 Subject: [PATCH 13/13] changelog --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 62e3d007337..5388df8dfac 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,7 +18,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm ### Fixed -- Global MeterProvider registration correctly unwraps global instrument stubs. (#5881) +- Global MeterProvider registration unwraps global instrument Observers, the undocumented Unwrap() methods are now private. (#5881)