Add default_granularity to metric spec
#291
Conversation
courtneyholcomb
requested review from
QMalcolm,
graciegoheen and
a team
as code owners
|
I think this branch needs an update, will hold off until the cumulative metrics pieces are through. |
courtneyholcomb
changed the title
default_grain to metric specdefault_granularity to metric spec
courtneyholcomb
force-pushed
the
court/default-grain
branch
from
ad6949a to
0e55d39
Compare
After discussing with core team members, we've decided this name is more consistent with other fields
courtneyholcomb
force-pushed
the
court/default-grain
branch
from
7c2ea37 to
3005c7d
Compare
| @@ -228,3 +238,31 @@ def input_metrics(self) -> Sequence[PydanticMetricInput]: | |||
| return (self.type_params.numerator, self.type_params.denominator) | |||
| else: | |||
| assert_values_exhausted(self.type) | |||
|
|
|||
| @staticmethod | |||
| def all_input_measures_for_metric( | |||
There was a problem hiding this comment.
should this method be in the protocol?
There was a problem hiding this comment.
I think not. I don't think we want to require implementing this logic any place that implements a metric.
| measures.add(metric.type_params.measure) | ||
| elif metric.type is MetricType.DERIVED or metric.type is MetricType.RATIO: | ||
| for input_metric in metric.input_metrics: | ||
| nested_metric = metric_index.get(MetricReference(input_metric.name)) |
There was a problem hiding this comment.
could just use the input_metric.as_reference property
| assert ( | ||
| metric.type_params.measure is not None | ||
| ), f"Metric {metric.name} should have a measure defined, but it does not." | ||
| measures.add(metric.type_params.measure) |
There was a problem hiding this comment.
Not blocking, but heads up that this could end up with duplicate measures.
Ie.,
if a metric that has 2 measures is the same but has a different input filter, then the set() wouldn't count them as duplicates as the MetricInputMeasure will have a different filter field even tho it's referencing the same measure.
If you don't want duplicate measures showing up in this, you could return Set[MeasureReference] instead.
There was a problem hiding this comment.
Good call!!
Resolves #290
Description
Now that we are enabling sub-daily granularities, the current behavior of
metric_timewill likely be unexpected to users. If they change any of their time dimensions to use a grain smaller than DAY,metric_timewill default to that grain. To avoid this confusion, add a parameter that allows users to choose which grain they want their metric to default to. This will default to DAY if not set to avoid the unexpected behavior change. (If DAY is smaller than the defined granularity, will default to the smallest available granularity.)Checklist
changie newto create a changelog entry