Fix delegation for global MeterProviders

[Jesse-Bonfire]

Fixes #5827
Fixes #5852

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: Jesse-Bonfire / name: Jesse Bank (0e2a08a, e8e45fc, 5d01374, 084d4c5, 7ff5cff, 8aef950, b6fd66c, be103cd, 49e421c, 079c15e)
• ✅ login: dmathieu / name: Damien Mathieu (c1da265)
• ✅ login: pellared / name: Robert Pająk (8b2c127, 07ee290, 8ac96b3)
• ✅ login: dashpole / name: David Ashpole (4ce2556)

[dashpole]

Can you add a unit test that fails before the fix, and passes after?

[dmathieu]

You will also need to sign the CLA.

[Jesse-Bonfire]

Working with my company to get CLA approval. Also where should I put the unit test? internal/global/meter_test.go or is there a better spot?

[dashpole]

internal/global/meter_test.go is great

[blkt]

Hello folks, sorry to jump in, @MrAlias kindly pointed me to this PR while discussing a problem I had with the recent release of opentelemetry.

While this fix indeed solves the broken delegation problem in our case, the setup we have had the system panicking because it went through this line of code which is not followed by a return like similar occurrences, like this and this.
As a result, it ran reg.Unregister with a null pointer.

Since the code has been there for a while I assume this is the desired behavior, but it does look suspicious to me, so I wanted to put this on your radar.

[Jesse-Bonfire]

@dashpole @dmathieu what do you folks think? I can add the return in the error case so we don't get null pointers

[Jesse-Bonfire]

I had to add the unit test to sdk/metric/meter_test.go to have enough context to repro the issue in the unit test.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.6%. Comparing base (19877b1) to head (c1da265).
Report is 1 commits behind head on main.

Additional details and impacted files

@@          Coverage Diff          @@
##            main   #5828   +/-   ##
=====================================
  Coverage   84.5%   84.6%           
=====================================
  Files        272     272           
  Lines      22736   22779   +43     
=====================================
+ Hits       19224   19272   +48     
+ Misses      3169    3165    -4     
+ Partials     343     342    -1     

see 2 files with indirect coverage changes

[jmacd]

I can confirm that this fixes a regression related to #5754 #5758 #5780 in https://github.com/lightstep/otel-launcher-go.

Without this fix, SetMeterProvider() would reach a panic, e.g.,

=== RUN   TestSecureMetricsAltSDK
2024/09/27 10:14:42 asynchronous instrument does not belong to this SDK: *global.aiUpDownCounter
--- FAIL: TestSecureMetricsAltSDK (0.01s)
panic: runtime error: invalid memory address or nil pointer dereference [recovered]
	panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x2 addr=0x0 pc=0x102ab40cc]

goroutine 36 [running]:
testing.tRunner.func1.2({0x1030e8ba0, 0x103b5d330})
	/opt/homebrew/Cellar/go/1.23.1/libexec/src/testing/testing.go:1632 +0x1bc
testing.tRunner.func1()
	/opt/homebrew/Cellar/go/1.23.1/libexec/src/testing/testing.go:1635 +0x334
panic({0x1030e8ba0?, 0x103b5d330?})
	/opt/homebrew/Cellar/go/1.23.1/libexec/src/runtime/panic.go:785 +0x124
go.opentelemetry.io/otel/internal/global.(*registration).setDelegate(0x1400071c100, {0x1032b4058, 0x140003ce540})
	/Users/josh.macdonald/src/lightstep/go/pkg/mod/go.opentelemetry.io/otel@v1.30.0/internal/global/meter.go:492 +0x19c
go.opentelemetry.io/otel/internal/global.(*meter).setDelegate(0x1400070a120, {0x1032999c0?, 0x14000445040?})
	/Users/josh.macdonald/src/lightstep/go/pkg/mod/go.opentelemetry.io/otel@v1.30.0/internal/global/meter.go:138 +0x1d8
go.opentelemetry.io/otel/internal/global.(*meterProvider).setDelegate(0x14000437230, {0x1032999c0, 0x14000445040})
	/Users/josh.macdonald/src/lightstep/go/pkg/mod/go.opentelemetry.io/otel@v1.30.0/internal/global/meter.go:47 +0x168
go.opentelemetry.io/otel/internal/global.SetMeterProvider.func1()
	/Users/josh.macdonald/src/lightstep/go/pkg/mod/go.opentelemetry.io/otel@v1.30.0/internal/global/state.go:171 +0x3c
sync.(*Once).doSlow(0x140000b45e0?, 0x140000b45c0?)
	/opt/homebrew/Cellar/go/1.23.1/libexec/src/sync/once.go:76 +0xf8
sync.(*Once).Do(...)

and with this PR, the issue is resolved.

[Jesse-Bonfire]

This is my first time contributing, should I update the branch locally and push or do it in github?

[dashpole]

Either is fine. Pushing the update branch button on github is usually less work, but can't resolve conflicts if there are any

[pellared]

@sarathsp06, can you please confirm if it is also fixing #5852?

[sarathsp06]

@sarathsp06, can you please confirm if it is also fixing #5852?

It does . This would fix the problem

[sarathsp06]

LGTM

[jmacd]

👋 We are eager to see this go into a release. Since the OTel Collector always bumps its dependencies to the latest OTel-Go dependency, we now have a situation where anyone with a recent Collector and a custom OTel-Go SDK is blocked from using new Collector releases until after an OTel-Go release.

Please and thank you!

[krak3n]

ETA on a hot fix release? This regression has impact on your users upgrading to the current released version.