-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Validate metrics using metadata.csv #6027
Conversation
Codecov Report
|
1b65733
to
6bc8b23
Compare
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 is a bad idea to have as the default behavior since most environments would need to be extremely specialized to produce even the majority of metrics.
Instead, let's add a new method that asserts everything in metadata.csv
and add the ability to pass in a list of metrics to exclude.
WDYT?
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.
Let's use
def find_check_root(depth=0): |
On hold, need to move stubs to Related: #3700 (comment) |
f74fc4b
to
552a52f
Compare
552a52f
to
4cc6abb
Compare
I changed the implementation to avoid Now |
@@ -301,6 +301,45 @@ def assert_all_metrics_covered(self): | |||
msg += '\nMissing Metrics:{}{}'.format(prefix, prefix.join(sorted(self.not_asserted()))) | |||
assert condition, msg | |||
|
|||
def assert_metrics_using_metadata(self, metadata_metrics, check_metric_type=True, exclude=None): |
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.
wdyt? since more integrations would have integration tests and not e2e
def assert_metrics_using_metadata(self, metadata_metrics, check_metric_type=True, exclude=None): | |
def assert_metrics_using_metadata(self, metadata_metrics, check_metric_type=False, exclude=None): |
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.
@ofek
I think this method has the most value for e2e since check the type is quite valuable.
Having check_metric_type=True
by default will encourage that. WDYT?
Co-Authored-By: Ofek Lev <ofekmeister@gmail.com>
What does this PR do?
Validate metrics using metadata.csv.
See examples below:
Motivation
That will help validating that
metadata.csv
metric types and actual types match.Additional Notes
Review checklist (to be filled by reviewers)
changelog/
andintegration/
labels attached