-
-
Notifications
You must be signed in to change notification settings - Fork 54
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(meta) Add configuration for meta tables in generic metrics #5824
Conversation
This adds all the YAML files for the storages and entities required to query the meta tables. Note that this is 4 copies of the same files, just replacing the metric type in each one (counters/sets/gauges/distributions). A required column for the meta table is the use case ID, but that is a string column. This also slightly modifies the validator to allow string columns as well as integer columns. The intent was to keep the strictness of the original check (which specifically looks for ints), so this added a new similar check.
Test Failures Detected: Due to failing tests, we cannot provide coverage reports at this time. ❌ Failed Test Results:Completed 840 tests with View the full list of failed tests
|
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 just some changes to allocation policy suggested and another test
snuba/datasets/configuration/generic_metrics/entities/counters_meta.yaml
Show resolved
Hide resolved
snuba/datasets/configuration/generic_metrics/storages/counters_meta.yaml
Outdated
Show resolved
Hide resolved
snuba/datasets/configuration/generic_metrics/storages/counters_meta.yaml
Outdated
Show resolved
Hide resolved
snuba/datasets/configuration/generic_metrics/storages/counters_meta.yaml
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.
LGTM. Not sure if you plan to do this in another PR, but you would also need to reference the new entities in the generic metrics dataset YAML.
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
This adds all the YAML files for the storages and entities required to query
the meta tables. Note that this is 4 copies of the same files, just replacing
the metric type in each one (counters/sets/gauges/distributions).
A required column for the meta table is the use case ID, but that is a string
column. This also slightly modifies the validator to allow string columns as
well as integer columns. The intent was to keep the strictness of the original
check (which specifically looks for ints), so this added a new similar check.
For reviewers: This is a big PR, but there are really only 4 new files added.
So just reviewing the files for counters would be sufficient.