fix(events-facets): Use the pagination helper (#51177)

The initial PR to expose page size and cursor allowed us to paginate,
but it's missing data about whether or not there are more results. The
pagination helper puts that information in the response header so we can
inspect it in the frontend and know if there's more data.

This PR is mostly a whitespace change because it wraps the original data
fetching logic in a function that gets called with varying `limit` and
`offset` values, derived from the `cursor` provided.

src/sentry/api/endpoints/organization_events_facets.py

@@ -7,6 +7,7 @@ from rest_framework.response import Response
 from sentry import tagstore
 from sentry.api.base import region_silo_endpoint
 from sentry.api.bases import NoProjects, OrganizationEventsV2EndpointBase
+from sentry.api.paginator import GenericOffsetPaginator
 from sentry.search.utils import DEVICE_CLASS
 from sentry.snuba import discover
 
@@ -22,59 +23,60 @@ class OrganizationEventsFacetsEndpoint(OrganizationEventsV2EndpointBase):
         except NoProjects:
             return Response([])
 
-        with sentry_sdk.start_span(op="discover.endpoint", description="discover_query"):
-            with self.handle_query_errors():
-                per_page = int(request.GET.get("per_page")) if request.GET.get("per_page") else None
-                cursor = int(request.GET.get("cursor")) if request.GET.get("cursor") else None
-                pagination_data = {}
-                if per_page:
-                    pagination_data["per_page"] = per_page
-                if cursor:
-                    pagination_data["cursor"] = cursor
-                facets = discover.get_facets(
-                    query=request.GET.get("query"),
-                    params=params,
-                    referrer="api.organization-events-facets.top-tags",
-                    **pagination_data,
-                )
+        def data_fn(offset, limit):
+            with sentry_sdk.start_span(op="discover.endpoint", description="discover_query"):
+                with self.handle_query_errors():
+                    facets = discover.get_facets(
+                        query=request.GET.get("query"),
+                        params=params,
+                        referrer="api.organization-events-facets.top-tags",
+                        per_page=limit,
+                        cursor=offset,
+                    )
 
-        with sentry_sdk.start_span(op="discover.endpoint", description="populate_results") as span:
-            span.set_data("facet_count", len(facets or []))
-            resp = defaultdict(lambda: {"key": "", "topValues": []})
-            for row in facets:
-                values = resp[row.key]
-                values["key"] = tagstore.get_standardized_key(row.key)
-                values["topValues"].append(
-                    {
-                        "name": tagstore.get_tag_value_label(row.key, row.value),
-                        "value": row.value,
-                        "count": row.count,
-                    }
-                )
+            with sentry_sdk.start_span(
+                op="discover.endpoint", description="populate_results"
+            ) as span:
+                span.set_data("facet_count", len(facets or []))
+                resp = defaultdict(lambda: {"key": "", "topValues": []})
+                for row in facets:
+                    values = resp[row.key]
+                    values["key"] = tagstore.get_standardized_key(row.key)
+                    values["topValues"].append(
+                        {
+                            "name": tagstore.get_tag_value_label(row.key, row.value),
+                            "value": row.value,
+                            "count": row.count,
+                        }
+                    )
 
-            if "project" in resp:
-                # Replace project ids with slugs as that is what we generally expose to users
-                # and filter out projects that the user doesn't have access too.
-                projects = {p.id: p.slug for p in self.get_projects(request, organization)}
-                filtered_values = []
-                for v in resp["project"]["topValues"]:
-                    if v["value"] in projects:
-                        name = projects[v["value"]]
-                        v.update({"name": name})
-                        filtered_values.append(v)
+                if "project" in resp:
+                    # Replace project ids with slugs as that is what we generally expose to users
+                    # and filter out projects that the user doesn't have access too.
+                    projects = {p.id: p.slug for p in self.get_projects(request, organization)}
+                    filtered_values = []
+                    for v in resp["project"]["topValues"]:
+                        if v["value"] in projects:
+                            name = projects[v["value"]]
+                            v.update({"name": name})
+                            filtered_values.append(v)
 
-                resp["project"]["topValues"] = filtered_values
+                    resp["project"]["topValues"] = filtered_values
 
-            if "device.class" in resp:
-                # Map device.class tag values to low, medium, or high
-                filtered_values = []
-                for v in resp["device.class"]["topValues"]:
-                    for key, value in DEVICE_CLASS.items():
-                        if v["value"] in value:
-                            v.update({"name": key, "value": key})
-                            filtered_values.append(v)
-                            continue
+                if "device.class" in resp:
+                    # Map device.class tag values to low, medium, or high
+                    filtered_values = []
+                    for v in resp["device.class"]["topValues"]:
+                        for key, value in DEVICE_CLASS.items():
+                            if v["value"] in value:
+                                v.update({"name": key, "value": key})
+                                filtered_values.append(v)
+                                continue
+
+                    resp["device.class"]["topValues"] = filtered_values
 
-                resp["device.class"]["topValues"] = filtered_values
+            return list(resp.values())
 
-        return Response(list(resp.values()))
+        return self.paginate(
+            request=request, paginator=GenericOffsetPaginator(data_fn=data_fn), default_per_page=10
+        )

src/sentry/snuba/discover.py

@@ -563,7 +563,7 @@ def get_facets(
     params (Dict[str, str]) Filtering parameters with start, end, project_id, environment
     referrer (str) A referrer string to help locate the origin of this query.
     per_page (int) The number of records to fetch.
-    cursor (int) Multiplied by per_page to determine the number of records to skip.
+    cursor (int) The number of records to skip.
 
     Returns Sequence[FacetResult]
     """
@@ -577,7 +577,7 @@ def get_facets(
             selected_columns=["tags_key", "count()"],
             orderby=["-count()", "tags_key"],
             limit=per_page,
-            offset=cursor * per_page,
+            offset=cursor,
             turbo=sample,
         )
         key_names = key_name_builder.run_query(referrer)
@@ -597,12 +597,14 @@ def get_facets(
 
     fetch_projects = False
     if len(params.get("project_id", [])) > 1:
+        # TODO(nar): Since we pop a result to fill it with project data, we need to account for this offset
         if len(top_tags) == per_page:
             top_tags.pop()
         fetch_projects = True
 
     results = []
-    if fetch_projects:
+    # Only inject project data on the first page
+    if fetch_projects and cursor == 0:
         with sentry_sdk.start_span(op="discover.discover", description="facets.projects"):
             project_value_builder = QueryBuilder(
                 Dataset.Discover,

tests/snuba/api/endpoints/test_organization_events_facets.py

@@ -2,6 +2,7 @@ from datetime import timedelta
 from unittest import mock
 from uuid import uuid4
 
+import requests
 from django.urls import reverse
 from django.utils import timezone
 from pytz import utc
@@ -670,39 +671,63 @@ class OrganizationEventsFacetsEndpointTest(SnubaTestCase, APITestCase):
         ]
         self.assert_facet(response, "device.class", expected)
 
-    def test_with_per_page_and_cursor_parameters(self):
+    def test_with_cursor_parameter(self):
         test_project = self.create_project()
-        self.store_event(
-            data={
-                "event_id": uuid4().hex,
-                "timestamp": self.min_ago_iso,
-                "tags": {
-                    "a": "one",
-                    "b": "two",
-                    "c": "three",
-                    "d": "four",
-                    "e": "five",
-                    "f": "six",
-                    "g": "seven",
-                    "h": "eight",
-                    "i": "nine",
-                    "j": "ten",
-                    "k": "eleven",
-                },
-            },
+        test_tags = {
+            "a": "one",
+            "b": "two",
+            "c": "three",
+            "d": "four",
+            "e": "five",
+            "f": "six",
+            "g": "seven",
+            "h": "eight",
+            "i": "nine",
+            "j": "ten",
+            "k": "eleven",
+        }
+
+        self.store_event(
+            data={"event_id": uuid4().hex, "timestamp": self.min_ago_iso, "tags": test_tags},
             project_id=test_project.id,
         )
 
+        # Test the default query fetches the first 10 results
+        with self.feature(self.features):
+            response = self.client.get(self.url, format="json", data={"project": test_project.id})
+            links = requests.utils.parse_header_links(
+                response.get("link", "").rstrip(">").replace(">,<", ",<")
+            )
+
+        assert response.status_code == 200, response.content
+        assert links[1]["results"] == "true"  # There are more results to be fetched
+        assert links[1]["cursor"] == "0:10:0"
+        assert len(response.data) == 10
+
+        # Loop over the first 10 tags to ensure they're in the results
+        for tag_key in list(test_tags.keys())[:10]:
+            expected = [
+                {"count": 1, "name": test_tags[tag_key], "value": test_tags[tag_key]},
+            ]
+            self.assert_facet(response, tag_key, expected)
+
+        # Get the next page
         with self.feature(self.features):
             response = self.client.get(
-                self.url,
-                format="json",
-                data={"project": test_project.id, "cursor": 5, "per_page": 1},
+                self.url, format="json", data={"project": test_project.id, "cursor": "0:10:0"}
+            )
+            links = requests.utils.parse_header_links(
+                response.get("link", "").rstrip(">").replace(">,<", ",<")
             )
 
         assert response.status_code == 200, response.content
-        assert len(response.data) == 1
+        assert links[1]["results"] == "false"  # There should be no more tags to fetch
+        assert len(response.data) == 2
+        expected = [
+            {"count": 1, "name": "eleven", "value": "eleven"},
+        ]
+        self.assert_facet(response, "k", expected)
         expected = [
-            {"count": 1, "name": "six", "value": "six"},
+            {"count": 1, "name": "error", "value": "error"},
         ]
-        self.assert_facet(response, "f", expected)
+        self.assert_facet(response, "level", expected)