fix(stats): Disable timestamp from top events (#92547)

- This is so we throw an error if users try to use timestamp as a
groupby in timeseries requests

Author: wmak

src/sentry/api/endpoints/organization_events_stats.py

@@ -3,7 +3,7 @@
 from typing import Any
 
 import sentry_sdk
-from rest_framework.exceptions import ValidationError
+from rest_framework.exceptions import ParseError, ValidationError
 from rest_framework.request import Request
 from rest_framework.response import Response
 
@@ -223,12 +223,15 @@ def _get_event_stats(
                 final_columns = transform_query_columns_for_error_upsampling(query_columns)
 
             if top_events > 0:
+                raw_groupby = self.get_field_list(organization, request)
+                if "timestamp" in raw_groupby:
+                    raise ParseError("Cannot group by timestamp")
                 if use_rpc:
                     return scoped_dataset.run_top_events_timeseries_query(
                         params=snuba_params,
                         query_string=query,
                         y_axes=final_columns,
-                        raw_groupby=self.get_field_list(organization, request),
+                        raw_groupby=raw_groupby,
                         orderby=self.get_orderby(request),
                         limit=top_events,
                         referrer=referrer,
@@ -243,7 +246,7 @@ def _get_event_stats(
                     )
                 return scoped_dataset.top_events_timeseries(
                     timeseries_columns=final_columns,
-                    selected_columns=self.get_field_list(organization, request),
+                    selected_columns=raw_groupby,
                     equations=self.get_equation_list(organization, request),
                     user_query=query,
                     snuba_params=snuba_params,

src/sentry/api/endpoints/organization_events_timeseries.py

@@ -234,12 +234,15 @@ def get_event_stats(
         )
 
         if top_events > 0:
+            raw_groupby = self.get_field_list(organization, request, param_name="groupBy")
+            if "timestamp" in raw_groupby:
+                raise ParseError("Cannot group by timestamp")
             if dataset in {spans_rpc, ourlogs}:
                 return dataset.run_top_events_timeseries_query(
                     params=snuba_params,
                     query_string=query,
                     y_axes=query_columns,
-                    raw_groupby=self.get_field_list(organization, request, param_name="groupBy"),
+                    raw_groupby=raw_groupby,
                     orderby=self.get_orderby(request),
                     limit=top_events,
                     referrer=referrer,
@@ -254,7 +257,7 @@ def get_event_stats(
                 )
             return dataset.top_events_timeseries(
                 timeseries_columns=query_columns,
-                selected_columns=self.get_field_list(organization, request, param_name="groupBy"),
+                selected_columns=raw_groupby,
                 equations=self.get_equation_list(organization, request),
                 user_query=query,
                 snuba_params=snuba_params,

tests/snuba/api/endpoints/test_organization_events_stats.py

@@ -2254,41 +2254,6 @@ def test_top_events_with_error_unhandled(self):
         assert response.status_code == 200, response.content
         assert len(data) == 2
 
-    def test_top_events_with_timestamp(self):
-        with self.feature(self.enabled_features):
-            response = self.client.get(
-                self.url,
-                data={
-                    "start": self.day_ago.isoformat(),
-                    "end": (self.day_ago + timedelta(hours=2)).isoformat(),
-                    "interval": "1h",
-                    "yAxis": "count()",
-                    "orderby": ["-count()"],
-                    "query": "event.type:default",
-                    "field": ["count()", "message", "timestamp"],
-                    "topEvents": "5",
-                },
-                format="json",
-            )
-
-        data = response.data
-        assert response.status_code == 200, response.content
-        assert len(data) == 6
-        # Transactions won't be in the results because of the query
-        del self.events[4]
-        del self.event_data[4]
-
-        for index, event in enumerate(self.events[:5]):
-            results = data[",".join([event.message, event.timestamp])]
-            assert results["order"] == index
-            assert [{"count": self.event_data[index]["count"]}] in [
-                attrs for time, attrs in results["data"]
-            ]
-
-        other = data["Other"]
-        assert other["order"] == 5
-        assert [{"count": 1}] in [attrs for _, attrs in other["data"]]
-
     def test_top_events_with_int(self):
         with self.feature(self.enabled_features):
             response = self.client.get(
@@ -2715,40 +2680,6 @@ def test_invalid_interval(self, mock_raw_query, mock_bulk_query):
         # Should've default to 24h's default of 5m
         assert mock_raw_query.mock_calls[5].args[0].query.granularity.granularity == 300
 
-    def test_top_events_timestamp_fields(self):
-        with self.feature(self.enabled_features):
-            response = self.client.get(
-                self.url,
-                format="json",
-                data={
-                    "start": self.day_ago.isoformat(),
-                    "end": (self.day_ago + timedelta(hours=2)).isoformat(),
-                    "interval": "1h",
-                    "yAxis": "count()",
-                    "orderby": ["-count()"],
-                    "field": ["count()", "timestamp", "timestamp.to_hour", "timestamp.to_day"],
-                    "topEvents": "5",
-                },
-            )
-        assert response.status_code == 200
-        data = response.data
-        assert len(data) == 3
-
-        # these are the timestamps corresponding to the events stored
-        timestamps = [
-            self.day_ago + timedelta(minutes=2),
-            self.day_ago + timedelta(hours=1, minutes=2),
-            self.day_ago + timedelta(minutes=4),
-        ]
-        timestamp_hours = [timestamp.replace(minute=0, second=0) for timestamp in timestamps]
-        timestamp_days = [timestamp.replace(hour=0, minute=0, second=0) for timestamp in timestamps]
-
-        for ts, ts_hr, ts_day in zip(timestamps, timestamp_hours, timestamp_days):
-            key = f"{ts.isoformat()},{ts_day.isoformat()},{ts_hr.isoformat()}"
-            count = sum(e["count"] for e in self.event_data if e["data"]["timestamp"] == ts)
-            results = data[key]
-            assert [{"count": count}] in [attrs for time, attrs in results["data"]]
-
     def test_top_events_other_with_matching_columns(self):
         with self.feature(self.enabled_features):
             response = self.client.get(

tests/snuba/api/endpoints/test_organization_events_stats_span_indexed.py

@@ -2193,6 +2193,24 @@ def test_datetime_unaligned_with_regular_buckets(self):
         # The timestamp of the last event should be 22:00 and there should also only be 1 event
         assert data[-1] == ((self.day_ago + timedelta(hours=12)).timestamp(), [{"count": 1}])
 
+    def test_top_events_with_timestamp(self):
+        """Users shouldn't groupby timestamp for top events"""
+        response = self._do_request(
+            data={
+                "start": self.day_ago,
+                "end": self.day_ago + timedelta(minutes=6),
+                "interval": "1m",
+                "yAxis": "count()",
+                "field": ["timestamp", "sum(span.self_time)"],
+                "orderby": ["-sum_span_self_time"],
+                "project": self.project.id,
+                "dataset": self.dataset,
+                "excludeOther": 0,
+                "topEvents": 2,
+            },
+        )
+        assert response.status_code == 400, response.content
+
     def test_simple_equation(self):
         self.store_spans(
             [

tests/snuba/api/endpoints/test_organization_events_timeseries_spans.py

@@ -2101,3 +2101,22 @@ def test_disable_extrapolation(self):
             "valueType": "integer",
             "interval": 3_600_000,
         }
+
+    def test_top_events_with_timestamp(self):
+        """Users shouldn't groupby timestamp for top events"""
+        response = self._do_request(
+            data={
+                "start": self.start,
+                "end": self.end,
+                "interval": "1m",
+                "yAxis": "count(span.self_time)",
+                "groupBy": ["timestamp", "count(span.self_time)"],
+                "query": "count(span.self_time):>4",
+                "orderby": ["-count(span.self_time)"],
+                "project": self.project.id,
+                "dataset": "spans",
+                "excludeOther": 0,
+                "topEvents": 5,
+            },
+        )
+        assert response.status_code == 400, response.content