-
Notifications
You must be signed in to change notification settings - Fork 796
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(views): Update addView() to disallow named views that select more than one instrument. #2820
feat(views): Update addView() to disallow named views that select more than one instrument. #2820
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2820 +/- ##
==========================================
+ Coverage 93.47% 93.51% +0.04%
==========================================
Files 163 163
Lines 5544 5570 +26
Branches 1168 1179 +11
==========================================
+ Hits 5182 5209 +27
+ Misses 362 361 -1
|
experimental/packages/opentelemetry-sdk-metrics-base/src/MeterProvider.ts
Outdated
Show resolved
Hide resolved
experimental/packages/opentelemetry-sdk-metrics-base/test/MeterProvider.test.ts
Outdated
Show resolved
Hide resolved
In case you missed it: this PR is still marked as WIP. You need to click the button "Ready for review" to gather attention from other approvers. |
4ddc031
to
e832d87
Compare
experimental/packages/opentelemetry-sdk-metrics-base/src/MeterProvider.ts
Outdated
Show resolved
Hide resolved
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.
Thank you for your work! Overall LGTM, just one suggestion left.
experimental/packages/opentelemetry-sdk-metrics-base/src/MeterProvider.ts
Outdated
Show resolved
Hide resolved
experimental/packages/opentelemetry-sdk-metrics-base/src/MeterProvider.ts
Outdated
Show resolved
Hide resolved
…= null' instead of '!== undefined'.
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 however you'll need to rebase
Which problem is this PR solving?
This PR simplifies the the public API for
MeterProvider.addView()
as mentioned in #2592 and makesaddView()
adhere to the following spec statement:Type of change
How Has This Been Tested?
Checklist: