-
Notifications
You must be signed in to change notification settings - Fork 842
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
feat(metrics): multi-instrument async callback support #2966
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2966 +/- ##
==========================================
- Coverage 92.54% 90.51% -2.04%
==========================================
Files 183 68 -115
Lines 6025 1876 -4149
Branches 1284 400 -884
==========================================
- Hits 5576 1698 -3878
+ Misses 449 178 -271
|
22a5521
to
18760f7
Compare
18760f7
to
628ef3e
Compare
I'm marking this ready to be reviewed to let people drop a comment on the new observable APIs. But I'd still like the spec change open-telemetry/opentelemetry-specification#2538 to land first. |
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.
as always i'm not the most familiar with metrics (ajd this is a large PR) but code wise lgtm
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.
Huge effort thanks so much. I'm going to approve but there are some small things that you can do if you want:
- Add tests for the metrics API package for the changed lines. Looks like they're not included in the coverage
- Documentation on how this feature is used
- In the PR a short description of the design of this feature and if you copied another implementation for it might make it easier for others to review.
experimental/packages/opentelemetry-api-metrics/src/NoopMeter.ts
Outdated
Show resolved
Hide resolved
@dyladan thank you for the suggestion! I've added some new docs in the README of sdk-metrics-base and updated the short description in the OP. |
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.
Looks good as per my understanding of the spec. 🙂
Thanks for the great work! 🚀
All my questions are resolved. Up to you if you want to merge or wait for more reviews. |
I'll try to land this by the end of tomorrow to get things rolling. Thank you all for reviewing! |
Which problem is this PR solving?
Added multi-instrument async callback support.
Fixes #2956
Short description of the changes
meter.createObservableCounter
,meter.createObservableGauge
,meter.createObservableUpDownCounter
callback
Observable
object on which callbacks can be registered or unregistered.meter.addBatchObservableCallback
andmeter.removeBatchObservableCallback
.After the change, observable instruments can be used with like:
Type of change
How Has This Been Tested?
Checklist: