-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
otel: conceal unwrapping for global async instrument registration #5881
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
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 |
Is this broken for our SDK as well? opentelemetry-go/sdk/metric/meter.go Lines 448 to 452 in a7d5c1a
|
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! |
@dashpole PTAL You pointed me to more tests w/ more nuance. The The |
For what it's worth, I ran the tests in otel-launcher-go that had broken and they are fixed by this PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dashpole As we discussed, I'll leave this open. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would like to wait to merge this until after the release.
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. |
**Description:** Updates to OTel-Go 1.31 require minor LS Metric SDK fixes (see Unwrap()), discussed and fixed in the upstream PR open-telemetry/opentelemetry-go#5881. The Unwrap() calls here are to work around the problem until fully fixed upstream. **Link to tracking Issue:** open-telemetry/opentelemetry-go#5827 explains how we got here. Part of #794. **Testing:** Incomplete. Existing tests were logging messages about foreign instruments in the asynchronous call path, but nothing failed as a result.
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
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. |
|
||
func (uo *unwrapObs) ObserveFloat64(inst metric.Float64Observable, value float64, opts ...metric.ObserveOption) { | ||
if un, ok := inst.(unwrapper); ok { | ||
inst = un.unwrap().(metric.Float64Observable) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
?
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