fix(discover): Top N queries do not support grouping by array fields (#96032)

- Fix a bug where top events did not correctly work with array fields, and instead just ignored their existence entirely

Author: gggritso

src/sentry/search/events/builder/discover.py

@@ -401,6 +401,7 @@ def resolve_top_event_conditions(
             resolved_field = self.resolve_column(self.prefixed_to_tag_map.get(field, field))
 
             values: set[Any] = set()
+            array_values: list[Any] = []
             for event in top_events:
                 if field in event:
                     alias = field
@@ -410,11 +411,12 @@ def resolve_top_event_conditions(
                     continue
 
                 # Note that because orderby shouldn't be an array field its not included in the values
-                if isinstance(event.get(alias), list):
-                    continue
+                event_value = event.get(alias)
+                if isinstance(event_value, list) and event_value not in array_values:
+                    array_values.append(event_value)
                 else:
-                    values.add(event.get(alias))
-            values_list = list(values)
+                    values.add(event_value)
+            values_list = list(values) + array_values
 
             if values_list:
                 if field == "timestamp" or field.startswith("timestamp.to_"):
@@ -451,6 +453,21 @@ def resolve_top_event_conditions(
                             conditions.append(And(conditions=[null_condition, non_none_condition]))
                     else:
                         conditions.append(null_condition)
+                elif any(isinstance(value, list) for value in values_list):
+                    list_conditions = []
+                    for values in values_list:
+                        # This depends on a weird clickhouse behaviour where the best way to compare arrays is to do
+                        # array("foo", "bar") IN array("foo", "bar") == 1
+                        list_conditions.append(
+                            Condition(resolved_field, Op.IN if not other else Op.NOT_IN, values)
+                        )
+                    if len(list_conditions) > 1:
+                        if not other:
+                            conditions.append(Or(conditions=list_conditions))
+                        else:
+                            conditions.append(And(conditions=list_conditions))
+                    else:
+                        conditions.extend(list_conditions)
                 else:
                     conditions.append(
                         Condition(resolved_field, Op.IN if not other else Op.NOT_IN, values_list)

src/sentry/snuba/discover.py

@@ -427,7 +427,10 @@ def create_groupby_dict(
             value = result_row.get(field)
             if isinstance(value, list):
                 if len(value) > 0:
-                    value = value[-1]
+                    # Even though frontend renders only the last element, this can cause key overlaps
+                    # For now lets just render this as a list to avoid that problem
+                    # TODO: timeseries can handle this correctly since this value isn't used as a dict key
+                    value = f"[{','.join(value)}]"
                 else:
                     value = ""
             values.append({"key": field, "value": str(value)})

tests/snuba/api/endpoints/test_organization_events_stats.py

@@ -3136,7 +3136,8 @@ def setUp(self):
                     "message": "poof",
                     "timestamp": (self.day_ago + timedelta(minutes=2)).isoformat(),
                     "user": {"email": self.user.email},
-                    "tags": {"shared-tag": "yup"},
+                    "tags": {"shared-tag": "yup", "env": "prod"},
+                    "exception": {"values": [{"type": "NameError"}, {"type": "FooError"}]},
                     "fingerprint": ["group1"],
                 },
                 "project": self.project2,
@@ -3148,7 +3149,8 @@ def setUp(self):
                     "timestamp": (self.day_ago + timedelta(hours=1, minutes=2)).isoformat(),
                     "fingerprint": ["group2"],
                     "user": {"email": self.user2.email},
-                    "tags": {"shared-tag": "yup"},
+                    "tags": {"shared-tag": "yup", "env": "prod"},
+                    "exception": {"values": [{"type": "NameError"}, {"type": "FooError"}]},
                 },
                 "project": self.project2,
                 "count": 6,
@@ -3159,7 +3161,8 @@ def setUp(self):
                     "timestamp": (self.day_ago + timedelta(minutes=2)).isoformat(),
                     "fingerprint": ["group3"],
                     "user": {"email": "[email protected]"},
-                    "tags": {"shared-tag": "yup"},
+                    "tags": {"shared-tag": "yup", "env": "prod"},
+                    "exception": {"values": [{"type": "NameError"}, {"type": "FooError"}]},
                 },
                 "project": self.project,
                 "count": 5,
@@ -3170,7 +3173,8 @@ def setUp(self):
                     "timestamp": (self.day_ago + timedelta(minutes=2)).isoformat(),
                     "fingerprint": ["group4"],
                     "user": {"email": "[email protected]"},
-                    "tags": {"shared-tag": "yup"},
+                    "tags": {"shared-tag": "yup", "env": "prod"},
+                    "exception": {"values": [{"type": "ValueError"}]},
                 },
                 "project": self.project,
                 "count": 4,
@@ -3180,7 +3184,8 @@ def setUp(self):
                     "message": "kinda bad",
                     "timestamp": (self.day_ago + timedelta(minutes=2)).isoformat(),
                     "user": {"email": self.user.email},
-                    "tags": {"shared-tag": "yup"},
+                    "tags": {"shared-tag": "yup", "env": "staging"},
+                    "exception": {"values": [{"type": "NameError"}, {"type": "FooError"}]},
                     "fingerprint": ["group7"],
                 },
                 "project": self.project,
@@ -3193,7 +3198,8 @@ def setUp(self):
                     "timestamp": (self.day_ago + timedelta(minutes=2)).isoformat(),
                     "fingerprint": ["group5"],
                     "user": {"email": "[email protected]"},
-                    "tags": {"shared-tag": "yup"},
+                    "tags": {"shared-tag": "yup", "env": "dev"},
+                    "exception": {"values": [{"type": "ValueError"}]},
                 },
                 "project": self.project,
                 "count": 2,
@@ -3204,7 +3210,8 @@ def setUp(self):
                     "timestamp": (self.day_ago + timedelta(minutes=2)).isoformat(),
                     "fingerprint": ["group6"],
                     "user": {"email": "[email protected]"},
-                    "tags": {"shared-tag": "yup"},
+                    "tags": {"shared-tag": "yup", "env": "dev"},
+                    "exception": {"values": [{"type": "ValueError"}]},
                 },
                 "project": self.project,
                 "count": 1,
@@ -3250,8 +3257,14 @@ def test_simple_top_events(self):
 
         for index, event in enumerate(self.events[:5]):
             message = event.message or event.transaction
+            exception = event.get_event_metadata()["type"]
             results = data[
-                ",".join([message, self.event_data[index]["data"]["user"].get("email", "None")])
+                ",".join(
+                    [
+                        f"{message} {exception}",
+                        self.event_data[index]["data"]["user"].get("email", "None"),
+                    ]
+                )
             ]
             assert results["order"] == index
             assert [{"count": self.event_data[index]["count"]}] in [
@@ -3262,6 +3275,38 @@ def test_simple_top_events(self):
         assert other["order"] == 5
         assert [{"count": 3}] in [attrs for _, attrs in other["data"]]
 
+    def test_top_events_with_array_field(self):
+        """
+        Test that when doing a qurey on top events with an array field that its handled correctly
+        """
+
+        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",
+                    "project": self.project.id,
+                    "query": "!error.type:*Exception*",
+                    "yAxis": "count_unique(user)",
+                    "orderby": ["-count_unique(user)"],
+                    "field": ["error.type", "count_unique(user)"],
+                    "topEvents": "2",
+                    "dataset": "errors",
+                },
+                format="json",
+            )
+
+        assert response.status_code == 200, response.content
+
+        data = response.data
+        assert len(data) == 2
+        assert "[NameError,FooError]" in data
+        assert "[ValueError]" in data
+        assert [attrs[0]["count"] for _, attrs in data["[NameError,FooError]"]["data"]] == [2, 0]
+        assert [attrs[0]["count"] for _, attrs in data["[ValueError]"]["data"]] == [1, 0]
+
     def test_top_events_with_projects_other(self):
         with self.feature(self.enabled_features):
             response = self.client.get(
@@ -3318,13 +3363,14 @@ def test_top_events_with_issue(self):
 
         for index, event in enumerate(self.events[:4]):
             message = event.message
+            exception = event.get_event_metadata()["type"]
             # Because we deleted the group for event 0
             if index == 0 or event.group is None:
                 issue = "unknown"
             else:
                 issue = event.group.qualified_short_id
 
-            results = data[",".join([issue, message])]
+            results = data[",".join([issue, f"{message} {exception}"])]
             assert results["order"] == index
             assert [{"count": self.event_data[index]["count"]}] in [
                 attrs for time, attrs in results["data"]

tests/snuba/api/endpoints/test_organization_events_stats_mep.py

@@ -889,7 +889,7 @@ def test_split_decision_for_top_events_errors_widget(self):
         assert len(response.data) == 1
 
         # Results are grouped by the error type
-        assert response.data.get("test_error").get("meta").get(
+        assert response.data.get("[test_error]").get("meta").get(
             "discoverSplitDecision"
         ) is DashboardWidgetTypes.get_type_name(DashboardWidgetTypes.ERROR_EVENTS)