Use MetricTimeDefaultGranularityPattern to resolve metric_time granularity
#1324
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This change is intended to make it more clear what the difference is between MinimumTimeGrainPattern and MetricTimeDefaultTimeGranularityPattern, since 'base' might easily be confused with 'default'.
courtneyholcomb
changed the base branch from
court/default-granularity_2
to
court/default-granularity1
These reflect a behavior change in which we don't error if metric_time is queried for metrics with two different default granularities. Instead, we choose the larger of the two, which is guaranteed to work for both metrics.
courtneyholcomb
force-pushed
the
court/default-granularity3
branch
from
f9793a9 to
5091b26
Compare
courtneyholcomb
force-pushed
the
court/default-granularity1
branch
2 times, most recently
from
30d5b6c to
86f6109
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is the main behavior change for this feature. Here, we apply the
MetricTimeDefaultGranularityPatternfor group by and filter resolution so thatmetric_timedefaults to the default granularity instead of the minimum one.There is one case where we actually want the minimum granularity instead of the default granularity for
metric_time, which is when determining the granularity used to adjust time constraints. Theonly_use_minimum_grainflag is used for that purpose.Note that very few tests are impacted by this change because
default_granularitydefaults toDAYor the minimum granularity for a time dimension if it's not set on the metric. This is the same behavior as before since no time dimensions have been added to our tests with sub-daily granularity yet. In an upcoming PR I will have tests that adddefault_granularityto some metrics to see the changes.You will see a handful snapshot changes here. These reflect an intentional behavior change (agreed on by product) in which we don't error if
metric_timeis queried for metrics with two different default granularities. Instead, we use the larger of the twodefault_granularities, which is guaranteed to work for both metrics.