From 146286428895305d4ea48f1bf66de97f2f8dc507 Mon Sep 17 00:00:00 2001 From: Evan Hicks Date: Mon, 23 Oct 2023 11:37:18 -0400 Subject: [PATCH] ref(metrics): Make granularity optional on the Rollup (#125) The granularity can be automatically inferred by the API, so it needs to be left optional when constructing the query. However it is still required in order to actually send the query through. --- CHANGES.rst | 5 +++++ snuba_sdk/metrics_query_visitors.py | 6 ++++++ snuba_sdk/metrics_visitors.py | 14 ++++++++------ snuba_sdk/timeseries.py | 7 ++----- tests/test_metrics_expressions.py | 7 ++++++- tests/test_metrics_query.py | 25 +++++++++++++++++++++++++ 6 files changed, 52 insertions(+), 12 deletions(-) diff --git a/CHANGES.rst b/CHANGES.rst index eab91486..6f8b17de 100644 --- a/CHANGES.rst +++ b/CHANGES.rst @@ -1,6 +1,11 @@ Changelog and versioning ========================== +2.0.6 +----- +- Make the `granularity` field optional in the Rollup, so that it can + be automatically inferred by the API layer + 2.0.5 ----- diff --git a/snuba_sdk/metrics_query_visitors.py b/snuba_sdk/metrics_query_visitors.py index 5105911f..4c68ac47 100644 --- a/snuba_sdk/metrics_query_visitors.py +++ b/snuba_sdk/metrics_query_visitors.py @@ -250,6 +250,12 @@ def _visit_rollup(self, rollup: Rollup | None) -> Mapping[str, None]: raise InvalidMetricsQueryError("rollup is required for a metrics query") elif not isinstance(rollup, Rollup): raise InvalidMetricsQueryError("rollup must be a Rollup object") + + # Since the granularity is inferred by the API, it can be initially None, but must be present when + # the query is ultimately serialized and sent to Snuba. + if rollup.granularity is None: + raise InvalidMetricsQueryError("granularity must be set on the rollup") + rollup.validate() return {} diff --git a/snuba_sdk/metrics_visitors.py b/snuba_sdk/metrics_visitors.py index 78d2ff7d..ba609e0a 100644 --- a/snuba_sdk/metrics_visitors.py +++ b/snuba_sdk/metrics_visitors.py @@ -159,11 +159,13 @@ def __init__(self, translator: Translation | None = None) -> None: self.translator = translator or Translation() def visit(self, rollup: Rollup) -> Mapping[str, str]: - condition = Condition( - lhs=Column("granularity"), - op=Op.EQ, - rhs=rollup.granularity, - ) + condition = None + if rollup.granularity is not None: + condition = Condition( + lhs=Column("granularity"), + op=Op.EQ, + rhs=rollup.granularity, + ) interval = "" orderby = "" @@ -189,7 +191,7 @@ def visit(self, rollup: Rollup) -> Mapping[str, str]: return { "orderby": orderby, - "filter": self.translator.visit(condition), + "filter": self.translator.visit(condition) if condition else "", "interval": interval, "with_totals": with_totals, } diff --git a/snuba_sdk/timeseries.py b/snuba_sdk/timeseries.py index b0c197b3..b26099de 100644 --- a/snuba_sdk/timeseries.py +++ b/snuba_sdk/timeseries.py @@ -189,10 +189,7 @@ def __post_init__(self) -> None: def validate(self) -> None: # The interval is used to determine how the timestamp is rolled up in the group by of the query. # The granularity is separate since it ultimately determines which data we retrieve. - # At a future point the granularity might be able to be inferred based on other attributes - # e.g. for totals queries, pick the highest possible granularity. For now it must be specified. - if self.granularity is None or self.granularity not in ALLOWED_GRANULARITIES: - # TODO: This could possibly be inferred from the interval, for now it's hardcoded + if self.granularity and self.granularity not in ALLOWED_GRANULARITIES: raise InvalidExpressionError( f"granularity must be an integer and one of {ALLOWED_GRANULARITIES}" ) @@ -201,7 +198,7 @@ def validate(self) -> None: _validate_int_literal( "interval", self.interval, 10, None ) # Minimum 10 seconds - if self.interval < self.granularity: + if self.granularity is not None and self.interval < self.granularity: raise InvalidExpressionError( "interval must be greater than or equal to granularity" ) diff --git a/tests/test_metrics_expressions.py b/tests/test_metrics_expressions.py index cf04faf6..cf5c2011 100644 --- a/tests/test_metrics_expressions.py +++ b/tests/test_metrics_expressions.py @@ -117,8 +117,13 @@ None, None, None, + { + "orderby": "time ASC", + "filter": "", + "interval": "toStartOfInterval(timestamp, toIntervalSecond(60), 'Universal') AS `time`", + "with_totals": "", + }, None, - InvalidExpressionError("granularity must be an integer"), id="10", ), pytest.param( diff --git a/tests/test_metrics_query.py b/tests/test_metrics_query.py index f60870c9..cdcd4d74 100644 --- a/tests/test_metrics_query.py +++ b/tests/test_metrics_query.py @@ -419,6 +419,31 @@ def test_query(query: MetricsQuery, translated: str | None) -> None: InvalidMetricsQueryError("scope must be a MetricsScope object"), id="bad scope type", ), + pytest.param( + MetricsQuery( + query=Timeseries( + metric=Metric( + "transaction.duration", + "d:transactions/duration@millisecond", + 123, + "metrics_sets", + ), + aggregate="quantiles", + aggregate_params=[0.5, 0.99], + ), + start=NOW, + end=NOW + timedelta(days=14), + groupby=[AliasedExpression(Column("tags[transaction]"), "transaction")], + rollup=Rollup(interval=60, totals=True), + scope=MetricsScope( + org_ids=[1], + project_ids=[11], + use_case_id="transactions", + ), + ), + InvalidMetricsQueryError("granularity must be set on the rollup"), + id="granularity must be present", + ), ]