Browse Source

fix(search): skip timestamp search term when querying for issues and issues-stats (#48198)

Saw some invalid queries where users search on
`timestamp:>2023-04-19T19:57:00+05:30
timestamp:<2023-04-19T20:10:00+05:30`. This resulted in a 500 internal
server error when querying for issue-stats (not totally sure why
querying for issues worked here). The cause of the internal error was
due to duplicate and overlapping `timestamp` where clauses in the snuba
query.

This PR special-cases when filtering on `timestamp`, similar to what we
do with `date`, by excluding the timestamp search filter and merges it
with the `start` and `end` times.

Resolves: SENTRY-Y4Q
Gilbert Szeto 1 year ago
parent
commit
baecb8d0de

+ 20 - 3
src/sentry/api/serializers/models/group.py

@@ -900,7 +900,8 @@ class GroupSerializerSnuba(GroupSerializerBase):
         *SKIP_SNUBA_FIELDS,
         "last_seen",
         "times_seen",
-        "date",  # We merge this with start/end, so don't want to include it as its own
+        "date",
+        "timestamp",  # We merge this with start/end, so don't want to include it as its own
         # condition
         # We don't need to filter by release stage again here since we're
         # filtering to specific groups. Saves us making a second query to
@@ -928,12 +929,28 @@ class GroupSerializerSnuba(GroupSerializerBase):
         # should try and encapsulate this logic, but if you're changing this, change it
         # there as well.
         self.start = None
-        start_params = [_f for _f in [start, get_search_filter(search_filters, "date", ">")] if _f]
+        start_params = [
+            _f
+            for _f in [
+                start,
+                get_search_filter(search_filters, "date", ">"),
+                get_search_filter(search_filters, "timestamp", ">"),
+            ]
+            if _f
+        ]
         if start_params:
             self.start = max(_f for _f in start_params if _f)
 
         self.end = None
-        end_params = [_f for _f in [end, get_search_filter(search_filters, "date", "<")] if _f]
+        end_params = [
+            _f
+            for _f in [
+                end,
+                get_search_filter(search_filters, "date", "<"),
+                get_search_filter(search_filters, "timestamp", "<"),
+            ]
+            if _f
+        ]
         if end_params:
             self.end = min(end_params)
 

+ 17 - 4
src/sentry/search/snuba/executors.py

@@ -334,7 +334,7 @@ class AbstractQueryExecutor(metaclass=ABCMeta):
             sf
             for sf in search_filters or ()
             # remove any search_filters that are only available in postgres, we special case date
-            if not (sf.key.name in self.postgres_only_fields or sf.key.name == "date")
+            if not (sf.key.name in self.postgres_only_fields.union(["date", "timestamp"]))
         ]
 
         # common pinned parameters that won't change based off datasource
@@ -490,7 +490,15 @@ class PostgresSnubaQueryExecutor(AbstractQueryExecutor):
         now = timezone.now()
         end = None
         paginator_options = {} if paginator_options is None else paginator_options
-        end_params = [_f for _f in [date_to, get_search_filter(search_filters, "date", "<")] if _f]
+        end_params = [
+            _f
+            for _f in [
+                date_to,
+                get_search_filter(search_filters, "date", "<"),
+                get_search_filter(search_filters, "timestamp", "<"),
+            ]
+            if _f
+        ]
         if end_params:
             end = min(end_params)
 
@@ -507,7 +515,12 @@ class PostgresSnubaQueryExecutor(AbstractQueryExecutor):
         # apparently `retention_window_start` can be None(?), so we need a
         # fallback.
         retention_date = max(_f for _f in [retention_window_start, now - timedelta(days=90)] if _f)
-        start_params = [date_from, retention_date, get_search_filter(search_filters, "date", ">")]
+        start_params = [
+            date_from,
+            retention_date,
+            get_search_filter(search_filters, "date", ">"),
+            get_search_filter(search_filters, "timestamp", ">"),
+        ]
         start = max(_f for _f in start_params if _f)
         end = max([retention_date, end])
 
@@ -536,7 +549,7 @@ class PostgresSnubaQueryExecutor(AbstractQueryExecutor):
             not [
                 sf
                 for sf in (search_filters or ())
-                if sf.key.name not in self.postgres_only_fields.union(["date"])
+                if sf.key.name not in self.postgres_only_fields.union(["date", "timestamp"])
             ]
         ):
             group_queryset = (

+ 30 - 0
tests/snuba/api/endpoints/test_organization_group_index_stats.py

@@ -123,3 +123,33 @@ class GroupListTest(APITestCase, SnubaTestCase, OccurrenceTestMixin):
 
         assert response.status_code == 200
         assert len(response.data) == 2
+
+    def test_query_timestamp(self):
+        self.store_event(
+            data={"timestamp": iso_format(before_now(seconds=500)), "fingerprint": ["group-1"]},
+            project_id=self.project.id,
+        )
+        event2 = self.store_event(
+            data={"timestamp": iso_format(before_now(seconds=1)), "fingerprint": ["group-a"]},
+            project_id=self.project.id,
+        )
+        self.store_event(
+            data={"timestamp": iso_format(before_now(seconds=2)), "fingerprint": ["group-b"]},
+            project_id=self.project.id,
+        )
+        event4 = self.store_event(
+            data={"timestamp": iso_format(before_now(seconds=3)), "fingerprint": ["group-c"]},
+            project_id=self.project.id,
+        )
+
+        group_a = event2.group
+        group_c = event4.group
+
+        self.login_as(user=self.user)
+        response = self.get_response(
+            query=f"timestamp:>{iso_format(before_now(seconds=3))} timestamp:<{iso_format(before_now(seconds=1))}",
+            groups=[group_a.id, group_c.id],
+        )
+
+        assert response.status_code == 200
+        assert len(response.data) == 2

+ 31 - 0
tests/snuba/api/serializers/test_group.py

@@ -4,6 +4,7 @@ from unittest.mock import patch
 import pytz
 from django.utils import timezone
 
+from sentry.api.event_search import SearchFilter, SearchKey, SearchValue
 from sentry.api.serializers import serialize
 from sentry.api.serializers.models.group import GroupSerializerSnuba
 from sentry.issues.grouptype import (
@@ -442,6 +443,36 @@ class GroupSerializerSnubaTest(APITestCase, SnubaTestCase):
 
             assert iso_format(start) == iso_format(before_now(days=expected))
 
+    def test_skipped_date_timestamp_filters(self):
+        group = self.create_group()
+        serializer = GroupSerializerSnuba(
+            search_filters=[
+                SearchFilter(
+                    SearchKey("timestamp"),
+                    ">",
+                    SearchValue(before_now(hours=1).replace(tzinfo=pytz.UTC)),
+                ),
+                SearchFilter(
+                    SearchKey("timestamp"),
+                    "<",
+                    SearchValue(before_now(seconds=1).replace(tzinfo=pytz.UTC)),
+                ),
+                SearchFilter(
+                    SearchKey("date"),
+                    ">",
+                    SearchValue(before_now(hours=1).replace(tzinfo=pytz.UTC)),
+                ),
+                SearchFilter(
+                    SearchKey("date"),
+                    "<",
+                    SearchValue(before_now(seconds=1).replace(tzinfo=pytz.UTC)),
+                ),
+            ]
+        )
+        assert not serializer.conditions
+        result = serialize(group, self.user, serializer=serializer)
+        assert result["id"] == str(group.id)
+
 
 @region_silo_test
 class PerformanceGroupSerializerSnubaTest(

+ 33 - 1
tests/snuba/api/serializers/test_group_stream.py

@@ -2,13 +2,15 @@ import time
 from datetime import timedelta
 from unittest import mock
 
+import pytz
 from django.utils import timezone
 
+from sentry.api.event_search import SearchFilter, SearchKey, SearchValue
 from sentry.api.serializers import serialize
 from sentry.api.serializers.models.group_stream import StreamGroupSerializerSnuba, snuba_tsdb
 from sentry.models import Environment
 from sentry.testutils import APITestCase, SnubaTestCase
-from sentry.testutils.helpers.datetime import iso_format
+from sentry.testutils.helpers.datetime import before_now, iso_format
 from sentry.testutils.silo import region_silo_test
 from sentry.utils.cache import cache
 from sentry.utils.hashlib import hash_values
@@ -226,3 +228,33 @@ class StreamGroupSerializerTestCase(APITestCase, SnubaTestCase):
         assert result[0]["sessionCount"] == 2
         # No sessions in project2
         assert result[1]["sessionCount"] is None
+
+    def test_skipped_date_timestamp_filters(self):
+        group = self.create_group()
+        serializer = StreamGroupSerializerSnuba(
+            search_filters=[
+                SearchFilter(
+                    SearchKey("timestamp"),
+                    ">",
+                    SearchValue(before_now(hours=1).replace(tzinfo=pytz.UTC)),
+                ),
+                SearchFilter(
+                    SearchKey("timestamp"),
+                    "<",
+                    SearchValue(before_now(seconds=1).replace(tzinfo=pytz.UTC)),
+                ),
+                SearchFilter(
+                    SearchKey("date"),
+                    ">",
+                    SearchValue(before_now(hours=1).replace(tzinfo=pytz.UTC)),
+                ),
+                SearchFilter(
+                    SearchKey("date"),
+                    "<",
+                    SearchValue(before_now(seconds=1).replace(tzinfo=pytz.UTC)),
+                ),
+            ]
+        )
+        assert not serializer.conditions
+        result = serialize([group], self.user, serializer=serializer)
+        assert result[0]["id"] == str(group.id)

+ 9 - 0
tests/snuba/search/test_backend.py

@@ -328,6 +328,15 @@ class EventsSnubaSearchTest(SharedSnubaTest):
         )
         assert set(results) == set()
 
+    def test_query_timestamp(self):
+        results = self.make_query(
+            [self.project],
+            environments=[self.environments["production"]],
+            search_filter_query=f"timestamp:>{iso_format(self.event1.datetime)} timestamp:<{iso_format(self.event3.datetime)}",
+        )
+
+        assert set(results) == {self.group1}
+
     def test_sort(self):
         results = self.make_query(sort_by="date")
         assert list(results) == [self.group1, self.group2]