Browse Source

feat(snuba-search): snuba group dataset search handle additional columns p2 (#70812)

This PR handles additional filters for the snuba-heavy search:

* `issue:`
* `project:`
* `date:`
* `first-release`
* `has:x`
* Generic tags like `server:`

I'm attempting to use as many pieces from existing search code as
possible which is why I'm leveraging the discover query builder as well
as `format_search_filter`. It's kind of a Frankenstein monster of
composed parts, however.
Stephen Cefali 10 months ago
parent
commit
45a6bfc46d

+ 1 - 0
src/sentry/issues/endpoints/organization_group_index.py

@@ -53,6 +53,7 @@ allowed_inbox_search_terms = frozenset(["date", "status", "for_review", "assigne
 UNSUPPORTED_SNUBA_FILTERS = [
     "issue.priority",  # coming soon
     "firstRelease",  # coming soon
+    "first_release",  # coming soon
 ]
 
 

+ 3 - 0
src/sentry/search/events/builder/discover.py

@@ -1322,6 +1322,9 @@ class BaseQueryBuilder:
                 1,
             )
         else:
+            # pull out the aliased expression if it exists
+            if isinstance(lhs, AliasedExpression):
+                lhs = lhs.exp
             condition = Condition(lhs, Op(search_filter.operator), value)
 
         if is_null_condition:

+ 45 - 29
src/sentry/search/snuba/executors.py

@@ -57,7 +57,6 @@ from sentry.models.organization import Organization
 from sentry.models.project import Project
 from sentry.models.team import Team
 from sentry.models.user import User
-from sentry.rules.conditions.event_attribute import ATTR_CHOICES
 from sentry.search.events.builder.discover import UnresolvedQuery
 from sentry.search.events.datasets.discover import DiscoverDatasetConfig
 from sentry.search.events.filter import convert_search_filter_to_snuba_query, format_search_filter
@@ -106,6 +105,15 @@ POSTGRES_ONLY_SEARCH_FIELDS = [
 ]
 
 
+def map_field_name_from_format_search_filter(field: str) -> str:
+    """
+    Maps the field name we get from the format_search_filter to the field used in Suba
+    """
+    if field == "date":
+        return "timestamp"
+    return field
+
+
 @dataclass
 class TrendsParams:
     # (event or issue age_hours) / (event or issue halflife hours)
@@ -1166,16 +1174,6 @@ class GroupAttributesPostgresSnubaQueryExecutor(PostgresSnubaQueryExecutor):
             search_filter.value.raw_value,
         )
 
-    def get_first_seen_filter(
-        self, search_filter: SearchFilter, joined_entity: Entity
-    ) -> Condition:
-        # use the group first seen from the group dataset
-        return Condition(
-            Column("group_first_seen", self.entities["attrs"]),
-            Op(search_filter.operator),
-            search_filter.value.raw_value,
-        )
-
     def get_basic_group_snuba_condition(
         self, search_filter: SearchFilter, joined_entity: Entity
     ) -> Condition:
@@ -1184,7 +1182,7 @@ class GroupAttributesPostgresSnubaQueryExecutor(PostgresSnubaQueryExecutor):
         """
         return Condition(
             Column(f"group_{search_filter.key.name}", self.entities["attrs"]),
-            Op.IN,
+            Op(search_filter.operator),
             search_filter.value.raw_value,
         )
 
@@ -1200,7 +1198,7 @@ class GroupAttributesPostgresSnubaQueryExecutor(PostgresSnubaQueryExecutor):
         Returns the basic lookup for a search filter.
         """
         # note this might hit postgres to do queries on releases
-        raw_conditions = convert_search_filter_to_snuba_query(
+        raw_conditions, projects_to_filter, group_ids = format_search_filter(
             search_filter,
             params={
                 "organization_id": organization_id,
@@ -1215,19 +1213,36 @@ class GroupAttributesPostgresSnubaQueryExecutor(PostgresSnubaQueryExecutor):
         if not isinstance(item, list):
             raw_conditions = [raw_conditions]
 
+        query_builder = self.def_get_query_builder(joined_entity)
+        query_builder.default_filter_converter(search_filter)
+
         output_conditions = []
         for item in raw_conditions:
-            column_name = item[0]
-            # do some name mapping for snuba
-            column_name = column_name.replace("stack.", "stacktrace.")
-            if ATTR_CHOICES.get(column_name) is not None:
-                raw_column = ATTR_CHOICES.get(column_name)
-                column_name = raw_column.value.event_name
-
-            column = Column(column_name, joined_entity)
+            lhs = item[0]
+            if isinstance(lhs, str):
+                raw_column = map_field_name_from_format_search_filter(lhs)
+                lhs = query_builder.resolve_column(raw_column)
+            else:
+                # right now we are assuming lhs looks like ['isNull', ['user']]
+                # if there are more complex expressions we will need to handle them
+                raw_column = map_field_name_from_format_search_filter(lhs[1][0])
+                rhs = [query_builder.resolve_column(raw_column)]
+                if len(lhs[1]) > 1:
+                    # example item here: [['ifNull', ['date', "''"]], '>=', 1715707188000]
+                    # which has lhs ['ifNull', ['date', "''"]]
+                    # we need this to become Function('ifNull', [Column('date', entity), ''])
+                    rhs.append(lhs[1][1])
+                lhs = Function(lhs[0], rhs)
+
             operator = Op(item[1])
             value = item[2]
-            output_conditions.append(Condition(column, operator, value))
+            output_conditions.append(Condition(lhs, operator, value))
+
+        for entity in [joined_entity, self.entities["attrs"]]:
+            for name, value in [("project_id", projects_to_filter), ("group_id", group_ids)]:
+                if value:
+                    output_conditions.append(Condition(Column(name, entity), Op.IN, value))
+
         if len(output_conditions) == 1:
             return output_conditions[0]
         return BooleanCondition(op=BooleanOp.AND, conditions=output_conditions)
@@ -1512,7 +1527,7 @@ class GroupAttributesPostgresSnubaQueryExecutor(PostgresSnubaQueryExecutor):
         "assigned_or_suggested": (get_assigned_or_suggested, Clauses.WHERE),
         "assigned_to": (get_assigned, Clauses.WHERE),
         "message": (get_message_condition, Clauses.WHERE),
-        "first_seen": (get_first_seen_filter, Clauses.WHERE),
+        "first_seen": (get_basic_group_snuba_condition, Clauses.WHERE),
         "last_seen": (get_last_seen_filter, Clauses.HAVING),
         "times_seen": (get_times_seen_filter, Clauses.HAVING),
     }
@@ -1669,8 +1684,9 @@ class GroupAttributesPostgresSnubaQueryExecutor(PostgresSnubaQueryExecutor):
                 fn = lookup[0] if lookup else None
                 clause = lookup[1] if lookup else Clauses.WHERE
 
-                # skip these
-                if search_filter.key.name in ["issue.category", "issue.type"]:
+                # issue.category and issue.type are handled specially
+                # we handle the date filter with calculate_start_end
+                if search_filter.key.name in ["issue.category", "issue.type", "date"]:
                     pass
                 elif fn:
                     # dynamic lookup of what clause to use
@@ -1681,11 +1697,11 @@ class GroupAttributesPostgresSnubaQueryExecutor(PostgresSnubaQueryExecutor):
                     else:
                         raise InvalidQueryForExecutor(f"Invalid clause {clause}")
                 else:
-                    where_conditions.append(
-                        self.get_basic_event_snuba_condition(
-                            search_filter, joined_entity, organization.id, project_ids, environments
-                        )
+                    condition = self.get_basic_event_snuba_condition(
+                        search_filter, joined_entity, organization.id, project_ids, environments
                     )
+                    if condition is not None:
+                        where_conditions.append(condition)
 
             # handle types based on issue.type and issue.category
             if not is_errors:

+ 91 - 15
tests/sentry/issues/endpoints/test_organization_group_index.py

@@ -303,6 +303,7 @@ class GroupListTest(APITestCase, SnubaTestCase, SearchIssueTestMixin):
         )
         assert [item["id"] for item in response.data] == [str(group_1.id), str(group_2.id)]
 
+    @override_options({"issues.group_attributes.send_kafka": True})
     def test_trace_search(self) -> None:
         event = self.store_event(
             data={
@@ -329,6 +330,14 @@ class GroupListTest(APITestCase, SnubaTestCase, SearchIssueTestMixin):
         assert len(response.data) == 1
         assert response.data[0]["id"] == str(event.group.id)
 
+        response = self.get_success_response(
+            sort_by="date",
+            query="is:unresolved trace:a7d67cf796774551a95be6543cacd459",
+            useGroupSnubaDataset=1,
+        )
+        assert len(response.data) == 1
+        assert response.data[0]["id"] == str(event.group.id)
+
     def test_feature_gate(self) -> None:
         # ensure there are two or more projects
         self.create_project(organization=self.project.organization)
@@ -470,6 +479,7 @@ class GroupListTest(APITestCase, SnubaTestCase, SearchIssueTestMixin):
         response = self.get_response(environment="garbage")
         assert response.status_code == 404
 
+    @override_options({"issues.group_attributes.send_kafka": True})
     def test_project(self) -> None:
         self.store_event(
             data={
@@ -485,6 +495,11 @@ class GroupListTest(APITestCase, SnubaTestCase, SearchIssueTestMixin):
         response = self.get_success_response(query=f"project:{project.slug}")
         assert len(response.data) == 1
 
+        response = self.get_success_response(
+            query=f"project:{project.slug}", useGroupSnubaDataset=1
+        )
+        assert len(response.data) == 1
+
     def test_auto_resolved(self) -> None:
         project = self.project
         project.update_option("sentry:resolve_age", 1)
@@ -603,6 +618,7 @@ class GroupListTest(APITestCase, SnubaTestCase, SearchIssueTestMixin):
         assert len(response.data) == 1
         assert response["X-Sentry-Direct-Hit"] == "1"
 
+    @override_options({"issues.group_attributes.send_kafka": True})
     def test_lookup_by_multiple_short_id_alias(self) -> None:
         self.login_as(self.user)
         project = self.project
@@ -623,6 +639,15 @@ class GroupListTest(APITestCase, SnubaTestCase, SearchIssueTestMixin):
         assert len(response.data) == 2
         assert response.get("X-Sentry-Direct-Hit") != "1"
 
+        with self.feature("organizations:global-views"):
+            response = self.get_success_response(
+                query=f"issue:[{event.group.qualified_short_id},{event2.group.qualified_short_id}]",
+                shortIdLookup=1,
+                useGroupSnubaDataset=1,
+            )
+        assert len(response.data) == 2
+        assert response.get("X-Sentry-Direct-Hit") != "1"
+
     def test_lookup_by_short_id_ignores_project_list(self) -> None:
         organization = self.create_organization()
         project = self.create_project(organization=organization)
@@ -675,6 +700,7 @@ class GroupListTest(APITestCase, SnubaTestCase, SearchIssueTestMixin):
         response = self.get_response(group=[group.id])
         assert response.status_code == 403
 
+    @override_options({"issues.group_attributes.send_kafka": True})
     def test_lookup_by_first_release(self) -> None:
         self.login_as(self.user)
         project = self.project
@@ -700,6 +726,15 @@ class GroupListTest(APITestCase, SnubaTestCase, SearchIssueTestMixin):
         assert int(issues[0]["id"]) == event2.group.id
         assert int(issues[1]["id"]) == event.group.id
 
+        with self.feature("organizations:global-views"):
+            response = self.get_success_response(
+                **{"query": 'first-release:"%s"' % release.version}, useGroupSnubaDataset=1
+            )
+        issues = json.loads(response.content)
+        assert len(issues) == 2
+        assert int(issues[0]["id"]) == event2.group.id
+        assert int(issues[1]["id"]) == event.group.id
+
     def test_lookup_by_release(self) -> None:
         self.login_as(self.user)
         project = self.project
@@ -924,6 +959,7 @@ class GroupListTest(APITestCase, SnubaTestCase, SearchIssueTestMixin):
         response = self.get_response(limit=10, query="assigned:[me, none]")
         assert len(response.data) == 4
 
+    @override_options({"issues.group_attributes.send_kafka": True})
     def test_seen_stats(self) -> None:
         self.store_event(
             data={"timestamp": iso_format(before_now(seconds=500)), "fingerprint": ["group-1"]},
@@ -936,7 +972,7 @@ class GroupListTest(APITestCase, SnubaTestCase, SearchIssueTestMixin):
             project_id=self.project.id,
         )
         group2 = event2.group
-        group2.first_seen = before_now_350_seconds
+        group2.first_seen = datetime.fromisoformat(before_now_350_seconds)
         group2.times_seen = 55
         group2.save()
         before_now_250_seconds = iso_format(before_now(seconds=250))
@@ -1020,6 +1056,15 @@ class GroupListTest(APITestCase, SnubaTestCase, SearchIssueTestMixin):
             before_now_100_seconds
         ).replace(tzinfo=UTC)
 
+        # now with useGroupSnubaDataset = 1
+        response = self.get_response(
+            sort_by="date", limit=10, query="server:example.com", useGroupSnubaDataset=1
+        )
+
+        assert response.status_code == 200
+        assert len(response.data) == 2
+        assert int(response.data[0]["id"]) == group2.id
+
     def test_semver_seen_stats(self) -> None:
         release_1 = self.create_release(version="test@1.2.3")
         release_2 = self.create_release(version="test@1.2.4")
@@ -1963,19 +2008,6 @@ class GroupListTest(APITestCase, SnubaTestCase, SearchIssueTestMixin):
         )
         assert response.data[0]["owners"][2]["type"] == GROUP_OWNER_TYPE[GroupOwnerType.CODEOWNERS]
 
-    def test_filter_not_unresolved(self) -> None:
-        event = self.store_event(
-            data={"timestamp": iso_format(before_now(seconds=500)), "fingerprint": ["group-1"]},
-            project_id=self.project.id,
-        )
-        event.group.update(status=GroupStatus.RESOLVED, substatus=None)
-        self.login_as(user=self.user)
-        response = self.get_response(
-            sort_by="date", limit=10, query="!is:unresolved", expand="inbox", collapse="stats"
-        )
-        assert response.status_code == 200
-        assert [int(r["id"]) for r in response.data] == [event.group.id]
-
     def test_default_search(self) -> None:
         event1 = self.store_event(
             data={"timestamp": iso_format(before_now(seconds=500)), "fingerprint": ["group-1"]},
@@ -3433,7 +3465,7 @@ class GroupListTest(APITestCase, SnubaTestCase, SearchIssueTestMixin):
         response = self.get_response(
             sort_by="date",
             limit=10,
-            query="times_seen:>0 last_seen:-1h",
+            query="times_seen:>0 last_seen:-1h date:-1h",
             useGroupSnubaDataset=1,
         )
 
@@ -3485,6 +3517,50 @@ class GroupListTest(APITestCase, SnubaTestCase, SearchIssueTestMixin):
         assert response.data[0]["inbox"] is not None
         assert response.data[0]["inbox"]["reason"] == GroupInboxReason.NEW.value
 
+    @patch("sentry.analytics.record")
+    @override_options({"issues.group_attributes.send_kafka": True})
+    def test_snuba_heavy_advanced_search_errors(self, mock_record):
+        self.login_as(user=self.user)
+        response = self.get_response(sort_by="date", query="!has:user", useGroupSnubaDataset=1)
+        assert response.status_code == 200, response.data
+        assert not any(
+            c[0][0] == "advanced_search.feature_gated" for c in mock_record.call_args_list
+        )
+
+        with self.feature({"organizations:advanced-search": False}):
+            response = self.get_response(sort_by="date", query="!has:user", useGroupSnubaDataset=1)
+            assert response.status_code == 400, response.data
+            assert (
+                "You need access to the advanced search feature to use negative "
+                "search" == response.data["detail"]
+            )
+
+            mock_record.assert_called_with(
+                "advanced_search.feature_gated",
+                user_id=self.user.id,
+                default_user_id=self.user.id,
+                organization_id=self.organization.id,
+            )
+
+    @override_options({"issues.group_attributes.send_kafka": True})
+    def test_snuba_heavy_filter_not_unresolved(self) -> None:
+        event = self.store_event(
+            data={"timestamp": iso_format(before_now(seconds=500)), "fingerprint": ["group-1"]},
+            project_id=self.project.id,
+        )
+        event.group.update(status=GroupStatus.RESOLVED, substatus=None)
+        self.login_as(user=self.user)
+        response = self.get_response(
+            sort_by="date",
+            limit=10,
+            query="!is:unresolved",
+            expand="inbox",
+            collapse="stats",
+            useGroupSnubaDataset=1,
+        )
+        assert response.status_code == 200
+        assert [int(r["id"]) for r in response.data] == [event.group.id]
+
 
 class GroupUpdateTest(APITestCase, SnubaTestCase):
     endpoint = "sentry-api-0-organization-group-index"