-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Support for custom MetricsFetcher in Perf tooling. #28671
Conversation
39d0789
to
5d292a9
Compare
8efa637
to
194498a
Compare
194498a
to
cf5aa74
Compare
Codecov Report
@@ Coverage Diff @@
## master #28671 +/- ##
==========================================
+ Coverage 71.96% 72.20% +0.24%
==========================================
Files 680 684 +4
Lines 100172 101233 +1061
==========================================
+ Hits 72088 73099 +1011
- Misses 26509 26559 +50
Partials 1575 1575
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 27 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control |
@pranavbhandari24 any update? |
@Abacn @damondouglas FYI. |
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.
Thanks Anand! Overall LGTM, just a couple concerns
please follow the below structure. | ||
|
||
**NOTE**: The Change point analysis only supports reading the metric data from Big Query for now. | ||
**NOTE**: The Change point analysis only supports reading the metric data from `BigQuery` only. |
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.
nit: only
is repeated, we can omit it from the end of the sentence
|
||
for test_name, params in tests_config.items(): | ||
for test_id, params in tests_config.items(): |
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.
We had spoken about supporting multiple metric_name
for a single test_id
. These metric names will be provided as a list in the config.
Shouldn't we check if params['metric_name']
is a list and execute run_change_point_analysis
for each metric_name?
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 can be done by adding metric_name
as a parameter to run_change_point_analysis
instead of accessing it directly in the method?
I'll defer to your opinion
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.
Yes, passing metrics as list will be implemented in a different PR. I will tag you there once it is ready.
d06f934
to
34c5256
Compare
Fixes: #28668
MetricsFetcher
class where anyone can inherit from it, build a Custom metrics_fetcher and pass it to theperf_analuysis.run()
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
addresses #123
), if applicable. This will automatically add a link to the pull request in the issue. If you would like the issue to automatically close on merging the pull request, commentfixes #<ISSUE NUMBER>
instead.CHANGES.md
with noteworthy changes.See the Contributor Guide for more tips on how to make review process smoother.
To check the build health, please visit https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md
GitHub Actions Tests Status (on master branch)
See CI.md for more information about GitHub Actions CI or the workflows README to see a list of phrases to trigger workflows.