-
Notifications
You must be signed in to change notification settings - Fork 819
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
refactor: batch observer to be independent from metric types #1709
Conversation
bf8ec5c
to
01d2034
Compare
- BatchObservers do not have to be created with names - BatchObservers do not have instruments for their own
01d2034
to
4ccaed5
Compare
Codecov Report
@@ Coverage Diff @@
## master #1709 +/- ##
==========================================
+ Coverage 92.13% 92.15% +0.02%
==========================================
Files 166 166
Lines 5591 5573 -18
Branches 1194 1194
==========================================
- Hits 5151 5136 -15
+ Misses 440 437 -3
|
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.
lgtm, just need to fix the build
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.
Nice change, please fix the return type, have some concerns with regards to new BatchObserver which still uses the old interface for its options, shouldn't this be changed too ?
# Conflicts: # packages/opentelemetry-api/src/metrics/Meter.ts # packages/opentelemetry-metrics/src/BatchObserver.ts # packages/opentelemetry-metrics/src/Meter.ts
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.
lgtm
…lemetry#1709) * refactor: batch observer to be independent from metric types - BatchObservers do not have to be created with names - BatchObservers do not have instruments for their own * fixup: nodejs v8 compatibilities * fixup: replace all batch observer metric occurrences with batch observer
…lemetry#1709) * refactor: batch observer to be independent from metric types - BatchObservers do not have to be created with names - BatchObservers do not have instruments for their own * fixup: nodejs v8 compatibilities * fixup: replace all batch observer metric occurrences with batch observer
Which problem is this PR solving?
Short description of the changes