Browse Source

feat(web-vitals): adds a 24 hour org level cache (#36716)

This PR adds a 24 hour org level cache for the new vitals overview endpoint. Even though the projects for each user within an org might be different, we can still get a cache hit because we store all project data in the cache and filter later.
Stephen Cefali 2 years ago
parent
commit
e5f907708b

+ 48 - 23
src/sentry/api/endpoints/organization_vitals_overview.py

@@ -1,12 +1,15 @@
 from datetime import timedelta
+from typing import Sequence
 
 from django.conf import settings
+from django.core.cache import cache
 from django.http import Http404
 from django.utils import timezone
 from rest_framework.request import Request
 from rest_framework.response import Response
 
 from sentry import experiments
+from sentry.api.base import ONE_DAY
 from sentry.api.bases import OrganizationEventsEndpointBase
 from sentry.api.serializers.models.project import get_access_by_project
 from sentry.models import Organization, Project, ProjectStatus
@@ -53,6 +56,47 @@ NO_RESULT_RESPONSE = {
 }
 
 
+def get_vital_data_for_org_no_cache(organization: Organization, projects: Sequence[Project]):
+    project_ids = list(map(lambda x: x.id, projects))
+
+    def get_discover_result(columns, referrer):
+        result = discover.query(
+            query="transaction.duration:<15m transaction.op:pageload event.type:transaction",
+            selected_columns=columns,
+            limit=settings.ORGANIZATION_VITALS_OVERVIEW_PROJECT_LIMIT,
+            params={
+                "start": timezone.now() - timedelta(days=7),
+                "end": timezone.now(),
+                "organization_id": organization.id,
+                "project_id": list(project_ids),
+            },
+            referrer=referrer,
+        )
+        return result["data"]
+
+    org_data = get_discover_result(BASIC_COLUMNS, "api.organization-vitals")
+    # no data at all for any vital
+    if not org_data:
+        return (None, None)
+
+    # get counts by project
+    project_data = get_discover_result(
+        ["project_id"] + BASIC_COLUMNS, "api.organization-vitals-per-project"
+    )
+    return (org_data, project_data)
+
+
+def get_vital_data_for_org(organization: Organization, projects: Sequence[Project]):
+    # cache is unique to an org
+    cache_key = f"organization-vitals-overview:{organization.id}"
+    cache_value = cache.get(cache_key)
+    # cache miss, lookup and store value
+    if cache_value is None:
+        cache_value = get_vital_data_for_org_no_cache(organization, projects)
+        cache.set(cache_key, cache_value, ONE_DAY)
+    return cache_value
+
+
 class OrganizationVitalsOverviewEndpoint(OrganizationEventsEndpointBase):
     private = True
 
@@ -73,38 +117,19 @@ class OrganizationVitalsOverviewEndpoint(OrganizationEventsEndpointBase):
         if len(projects) >= settings.ORGANIZATION_VITALS_OVERVIEW_PROJECT_LIMIT:
             return self.respond(NO_RESULT_RESPONSE)
 
-        project_ids = list(map(lambda x: x.id, projects))
-
-        def get_discover_result(columns, referrer):
-            result = discover.query(
-                query="transaction.duration:<15m transaction.op:pageload event.type:transaction",
-                selected_columns=columns,
-                limit=settings.ORGANIZATION_VITALS_OVERVIEW_PROJECT_LIMIT,
-                params={
-                    "start": timezone.now() - timedelta(days=7),
-                    "end": timezone.now(),
-                    "organization_id": organization.id,
-                    "project_id": list(project_ids),
-                },
-                referrer=referrer,
-            )
-            return result["data"]
-
         with self.handle_query_errors():
-            org_data = get_discover_result(BASIC_COLUMNS, "api.organization-vitals")
+            # find data we might have cached
+            org_data, project_data = get_vital_data_for_org(organization, projects)
             # no data at all for any vital
             if not org_data:
                 return self.respond(NO_RESULT_RESPONSE)
+
+            # take data and transform output
             output = {}
             # only a single result
             for key, val in org_data[0].items():
                 output[NAME_MAPPING[key]] = val
 
-            # get counts by project
-            project_data = get_discover_result(
-                ["project_id"] + BASIC_COLUMNS, "api.organization-vitals-per-project"
-            )
-
             # check access for project level data
             access_by_project = get_access_by_project(projects, request.user)
             projects_with_access = list(

+ 54 - 0
tests/sentry/api/endpoints/test_organization_vitals_overview.py

@@ -214,3 +214,57 @@ class OrganizationVitalsOverviewTest(APITestCase):
             "projectData": [],
         }
         assert mock_query.call_count == 0
+
+    @mock.patch("sentry.api.endpoints.organization_vitals_overview.experiments.get", return_value=1)
+    @mock.patch(
+        "sentry.api.endpoints.organization_vitals_overview.discover.query",
+    )
+    def test_with_cache(self, mock_query, mock_experiment_get):
+        mock_query.side_effect = self.mock_discover_query
+        self.get_response(self.organization.slug)
+        # call again to hit the cache
+        response = self.get_response(self.organization.slug)
+        assert response.status_code == 200
+        assert response.data == {
+            "FCP": 1000,
+            "LCP": 2000,
+            "appStartWarm": 3000,
+            "appStartCold": 5000,
+            "fcpCount": 10,
+            "lcpCount": 20,
+            "appWarmStartCount": 30,
+            "appColdStartCount": 40,
+            "projectData": [
+                {
+                    "projectId": self.project.id,
+                    "FCP": 1000,
+                    "LCP": 2000,
+                    "appStartWarm": None,
+                    "appStartCold": None,
+                    "fcpCount": 10,
+                    "lcpCount": 20,
+                    "appWarmStartCount": 0,
+                    "appColdStartCount": 0,
+                },
+                {
+                    "projectId": self.project2.id,
+                    "FCP": None,
+                    "LCP": None,
+                    "appStartWarm": 3000,
+                    "appStartCold": 5000,
+                    "fcpCount": 0,
+                    "lcpCount": 0,
+                    "appWarmStartCount": 30,
+                    "appColdStartCount": 40,
+                },
+            ],
+        }
+        assert mock_query.call_count == 2
+        assert set(mock_query.call_args.kwargs["params"]["project_id"]) == {
+            self.project.id,
+            self.project2.id,
+        }
+        mock_experiment_get.assert_called_with(
+            "VitalsAlertExperiment", self.organization, self.user
+        )
+        assert mock_experiment_get.call_count == 2

+ 3 - 3
tests/snuba/search/test_backend.py

@@ -2137,7 +2137,7 @@ class CdcEventsSnubaSearchTest(TestCase, SnubaTestCase):
             [self.group2, self.group1],
             None,
             sort_by="freq",
-            # Change the date range to bust the cache
+            # Change the date range to bust the
             date_from=self.base_datetime - timedelta(days=29),
         )
 
@@ -2163,7 +2163,7 @@ class CdcEventsSnubaSearchTest(TestCase, SnubaTestCase):
             [group3, self.group2, self.group1],
             None,
             sort_by="new",
-            # Change the date range to bust the cache
+            # Change the date range to bust the
             date_from=self.base_datetime - timedelta(days=29),
         )
 
@@ -2222,7 +2222,7 @@ class CdcEventsSnubaSearchTest(TestCase, SnubaTestCase):
             [self.group2, self.group1, group3],
             None,
             sort_by="user",
-            # Change the date range to bust the cache
+            # Change the date range to bust the
             date_from=self.base_datetime - timedelta(days=29),
         )