otel: conceal unwrapping for global async instrument registration

[jmacd]

Two defects are fixed here. However, note that async instrument delegation appears to have been broken a long time.

Internalizes and tests the behavior of the Global MeterProvider. This moves the call to Unwrap() out of the SDK, fully concealing it within the internal/global package (using an un-exported method).

This adds a test for the new functionality. While this test is not comprehensive, because it doesn't test every instrument variation, it explicitly tests that both the NewCallback function and the Observe functions receive objects constructed by the alternate SDK.

Fixes #5827

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.6%. Comparing base (cd754a6) to head (2a906fb).

Additional details and impacted files

@@          Coverage Diff          @@
##            main   #5881   +/-   ##
=====================================
  Coverage   84.5%   84.6%           
=====================================
  Files        272     272           
  Lines      22840   22829   -11     
=====================================
+ Hits       19313   19314    +1     
+ Misses      3178    3170    -8     
+ Partials     349     345    -4     

see 3 files with indirect coverage changes

[dashpole]

Is this broken for our SDK as well?

opentelemetry-go/sdk/metric/meter.go

Lines 448 to 452 in a7d5c1a

	if u, ok := inst.(interface {
	Unwrap() metric.Observable
	}); ok {
	inst = u.Unwrap()
	}

[jmacd]

I'm mildly confused by this. My expectation was that the internal/global package would hide details about the delgation and that SDKs would not need to unwrap themselves. Investigating!

[jmacd]

@dashpole PTAL

You pointed me to more tests w/ more nuance. The TestGlobalInstRegisterCallback test in sdk/metric exercises a case with mixed use of pre-registration and post-registration instruments. I've corrected this PR to pass all tests.

The unwrap() method is non-exported again. SDKs should not need to call such a method.

[jmacd]

For what it's worth, I ran the tests in otel-launcher-go that had broken and they are fixed by this PR.

[dashpole]

I tried to figure out why the unwrapping was done in the SDK in #3584, but can't figure it out. This LGTM, but I would really like @MrAlias to review if he remembers why this was done

[jmacd]

@dashpole As we discussed, I'll leave this open.
I think it is safe to release this repo as-is. The fact that unwrap() is called, doing nothing, appears harmless for OTel-Go SDK users. For the alternate SDK user, I can work around this by calling Unwrap(). I would be glad to see this PR applied after the release, but I'll accept if you decide to keep calling Unwrap() the SDK's responsibility. If so, I'd propose to add a public Unwrap interface and documentation. I'll leave it up to you and @MrAlias. Thanks!

[dashpole]

I would like to wait to merge this until after the release.

[pellared]

Do we want to make a patch release after this is merged?

[dashpole]

Do we want to make a patch release after this is merged?

I don't think so. This fixes a bug for alternative SDKs. Based on #5881 (comment), @jmacd is going to work around the issue in the meantime.

[jmacd]

I worked around the problem as stated above, so I consider this a nice-to-have low priority fix. The workaround, in case anyone else cares, is

	if unwrapped, ok := inst.(interface {
		Unwrap() metric.Observable
	}); ok {
		inst = unwrapped.Unwrap()
	}

I needed to do this in two places related to asynchronous instrument handling. The global package hides this detail as far as synchronous instruments are concerned. Link.

[MrAlias]

This will panic if this is an invalid type conversion, whereas before it would be logged as an error.

Can we log this as an error? Or, better yet, include a specific unwrappableFloat64Observable type?

type unwrapFloat64Observabler interface {
	unwrapFloat64Observable() metric.Float64Observable
}

and similar for Int64Observable?