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

@@ -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
 -----

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 {}
 

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,
         }

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"
                 )

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(

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",
+    ),
 ]