-
Notifications
You must be signed in to change notification settings - Fork 14
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: add aggregation_units to metric and datasource #553
Conversation
I wonder if calling it |
To me I'm definitely open to a different name though, and @danielkberry and I discussed it a bit already ( |
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! A few small suggestions and questions. I'm excited to incorporate this into my mozanalysis PR.
lib/metric-config-parser/metric_config_parser/tests/test_analysis.py
Outdated
Show resolved
Hide resolved
There's just one open question, then I can re-review. |
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 like all the changes are made, thanks! LGTM
Step 1 towards supporting profile management feature.
aggregation_units
to metrics and data_sources, and defaults both to what is currently supported,client_id
only