Skip to content
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

Fix for delayed global registration #2784

Merged
merged 6 commits into from Apr 14, 2022

Conversation

MadVikingGod
Copy link
Contributor

Fixes: #2777

What happened to cause this?

Before the global package, the only way to create an instrument was via the actual SDK. The global package introduced the idea of a delegated instrument, which when set forwarded all calls to the underlying instrument. But part of the SDK checks that the instruments passed to RegisterCallback were created by this SDK. The problem is that it does this poorly (eg just that they are of an internal type, so it won't guard against 2 instances of SDK interchanging instruments.)

So when you SetGlobalMeterProvider is called, it delegates all of the instruments and callbacks to the provided MP, but it will call RegisterCallback with a delegated type not the SDK type (it's stored inside the delegate).

How does this fix the problem?
Added an internal Unwrap to the delegated types, and before calling RegisterCallback it will unwrap the delegates so the SDK's type should be called.

@codecov
Copy link

codecov bot commented Apr 12, 2022

Codecov Report

Merging #2784 (44a365a) into main (57a248d) will decrease coverage by 0.1%.
The diff coverage is 39.5%.

Impacted file tree graph

@@           Coverage Diff           @@
##            main   #2784     +/-   ##
=======================================
- Coverage   75.9%   75.7%   -0.2%     
=======================================
  Files        178     178             
  Lines      11665   11707     +42     
=======================================
+ Hits        8855    8871     +16     
- Misses      2585    2611     +26     
  Partials     225     225             
Impacted Files Coverage Δ
metric/internal/global/instruments.go 57.2% <13.3%> (-9.3%) ⬇️
metric/internal/global/meter.go 96.4% <100.0%> (+0.2%) ⬆️

CHANGELOG.md Show resolved Hide resolved
@lucasoares
Copy link

lucasoares commented Apr 13, 2022

Tested and this PR solves #2777.

@MadVikingGod MadVikingGod merged commit 46a10bb into open-telemetry:main Apr 14, 2022
@MadVikingGod MadVikingGod deleted the mvg/fix-2777 branch April 14, 2022 00:49
@MrAlias MrAlias added this to the Release v1.7.0 milestone Apr 25, 2022
@MrAlias MrAlias mentioned this pull request Apr 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bad instrument using meter.AsyncInt64().Gauge and callback
4 participants