-
Notifications
You must be signed in to change notification settings - Fork 821
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(sdk-metrics-base): meter shared states #2821
refactor(sdk-metrics-base): meter shared states #2821
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2821 +/- ##
==========================================
+ Coverage 93.25% 93.33% +0.08%
==========================================
Files 152 165 +13
Lines 4904 5594 +690
Branches 1039 1176 +137
==========================================
+ Hits 4573 5221 +648
- Misses 331 373 +42
|
e1fd1b8
to
9dad9d0
Compare
9dad9d0
to
27ce086
Compare
…ared-state # Conflicts: # experimental/packages/opentelemetry-sdk-metrics-base/src/Meter.ts # experimental/packages/opentelemetry-sdk-metrics-base/test/state/MetricCollector.test.ts
This was closed accidentally. Reopened, and rebased. |
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 great! 🙂
experimental/packages/opentelemetry-sdk-metrics-base/test/export/TestMetricExporter.ts
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.
This looks mostly like a "pure" refactoring to me and no new functionality right? LGTM
experimental/packages/opentelemetry-sdk-metrics-base/test/export/TestMetricExporter.ts
Show resolved
Hide resolved
Yeah, this PR doesn't introduce any new features. |
@open-telemetry/javascript-approvers please take a look at this if you have time |
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.
My moral support 🙂
@open-telemetry/javascript-approvers this pr still needs one more review, I'd appreciate it if anyone can take a look at this :) I've updated it on top of the main branch and added a changelog entry. |
I'll bug everyone in the SIG meeting :) |
@open-telemetry/javascript-approvers merged the latest main branch and resolved conflicts. we still need one more review on this PR :) |
1ca839b
to
03fa81f
Compare
Unexpected push... reverted to the previous version 03fa81f. We are still waiting for another review to merge :-) |
@vmarchaud thank you |
Which problem is this PR solving?
Refactors meter states so that we can safely add more meter internal fields like async instrument callback registrations.
Also hides the public
Meter.collect
method in the shared state interface so that it can only be exposed throughMetricReader
.This is extracted from #2785 as part of the async callback re-work.
Type of change
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Checklist: