-
Notifications
You must be signed in to change notification settings - Fork 885
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
[POC] Add a MetricsRecorder plugin for metrics reporting #8628
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Liyun Xiu <[email protected]>
❌ Empty Changelog SectionThe Changelog section in your PR description is empty. Please add a valid changelog entry or entries. If you did add a changelog entry, check to make sure that it was not accidentally included inside the comment block in the Changelog section. |
Signed-off-by: Liyun Xiu <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #8628 +/- ##
=======================================
Coverage 60.95% 60.95%
=======================================
Files 3792 3793 +1
Lines 90355 90362 +7
Branches 14170 14172 +2
=======================================
+ Hits 55072 55078 +6
+ Misses 31822 31820 -2
- Partials 3461 3464 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Can we use telemetry plugin? |
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.
Lets not rush a new reporting plugin without due consideration given to the original one. I dont agree with some of the assumptions made in the original issue. I'll call out my concerns there. Lets have a discussion on the original issue before proceeding with this change in core. lets discuss the drawbacks with the useage collection plugin and why we cant find workarounds before adding a new one here.
telemetry plugin is built on top of usage_collection and it only collects metrics scattered in different places, it doesn't have mechanism to store metrics. |
❌ Empty Changelog SectionThe Changelog section in your PR description is empty. Please add a valid changelog entry or entries. If you did add a changelog entry, check to make sure that it was not accidentally included inside the comment block in the Changelog section. |
@seraphjiang @zengyan-amazon could you take a look? |
Description
Add a MetricsRecorder plugin for metrics reporting.
Issues Resolved
#8625
Screenshot
Testing the changes
Changelog
Check List
yarn test:jest
yarn test:jest_integration