Browse Source

fix(performance): Remove aggregate filter conditions on vitals (#26749)

Don't use aggregate filter on the vitals cards because
when filtering on aggregate conditions, we do not want
to apply the filter on the aggregate data.
Shruthi 3 years ago
parent
commit
04d76bed5a

+ 2 - 2
src/sentry/api/endpoints/organization_events_vitals.py

@@ -54,8 +54,8 @@ class OrganizationEventsVitalsEndpoint(OrganizationEventsV2EndpointBase):
                 limit=1,
                 referrer="api.events.vitals",
                 auto_fields=True,
-                auto_aggregations=True,
-                use_aggregate_conditions=True,
+                auto_aggregations=False,
+                use_aggregate_conditions=False,
             )
 
         results = {}

+ 5 - 3
src/sentry/search/events/fields.py

@@ -449,8 +449,7 @@ def resolve_field_list(
                     aggregate_fields[format_column_as_key(function.aggregate[1])].add(field)
 
     # Only auto aggregate when there's one other so the group by is not unexpectedly changed
-    auto_aggregate = auto_aggregations and snuba_filter.having and len(aggregations) > 0
-    if auto_aggregate:
+    if auto_aggregations and snuba_filter.having and len(aggregations) > 0:
         for agg in snuba_filter.condition_aggregates:
             if agg not in snuba_filter.aliases:
                 function = resolve_field(agg, snuba_filter.params, functions_acl)
@@ -464,8 +463,11 @@ def resolve_field_list(
                         if function.details.instance.redundant_grouping:
                             aggregate_fields[format_column_as_key(function.aggregate[1])].add(field)
 
+    check_aggregations = (
+        snuba_filter.having and len(aggregations) > 0 and snuba_filter.condition_aggregates
+    )
     snuba_filter_condition_aggregates = (
-        set(snuba_filter.condition_aggregates or []) if auto_aggregate else set()
+        set(snuba_filter.condition_aggregates) if check_aggregations else set()
     )
     for field in set(fields[:]).union(snuba_filter_condition_aggregates):
         if isinstance(field, str) and field in {

+ 46 - 11
tests/snuba/api/endpoints/test_organization_events_vitals.py

@@ -2,6 +2,7 @@ from datetime import timedelta
 
 from django.urls import reverse
 
+from sentry.models.transaction_threshold import ProjectTransactionThreshold, TransactionMetric
 from sentry.testutils import APITestCase, SnubaTestCase
 from sentry.testutils.helpers.datetime import before_now, iso_format
 from sentry.utils.samples import load_data
@@ -92,25 +93,59 @@ class OrganizationEventsVitalsEndpointTest(APITestCase, SnubaTestCase):
         }
 
     def test_simple_with_refining_user_misery_filter(self):
-        self.query.update({"query": "user_misery():>0.050"})
+        project1 = self.create_project(organization=self.organization)
+        project2 = self.create_project(organization=self.organization)
+        ProjectTransactionThreshold.objects.create(
+            project=project1,
+            organization=project1.organization,
+            threshold=100,
+            metric=TransactionMetric.LCP.value,
+        )
+
+        ProjectTransactionThreshold.objects.create(
+            project=project2,
+            organization=project2.organization,
+            threshold=1000,
+            metric=TransactionMetric.LCP.value,
+        )
 
         data = self.transaction_data.copy()
-        for lcp in [2000, 3000, 5000]:
-            self.store_event(
-                data,
-                {"lcp": lcp},
-                project_id=self.project.id,
-            )
+
+        for project in [project1, project2]:
+            for lcp in [2000, 3000, 5000]:
+                self.store_event(
+                    data,
+                    {"lcp": lcp},
+                    project_id=project.id,
+                )
 
         self.query.update({"vital": ["measurements.lcp"]})
-        response = self.do_request()
+        response = self.do_request(
+            features={"organizations:global-views": True, "organizations:discover-basic": True}
+        )
+
         assert response.status_code == 200, response.content
         assert response.data["measurements.lcp"] == {
-            "good": 1,
+            "good": 0,
             "meh": 1,
             "poor": 1,
-            "total": 3,
-            "p75": 4000,
+            "total": 2,
+            "p75": 4500,
+        }
+
+        self.query.update({"query": "user_misery():<0.04"})
+        response = self.do_request(
+            features={"organizations:global-views": True, "organizations:discover-basic": True}
+        )
+
+        assert response.status_code == 200, response.content
+        assert len(response.data) == 1
+        assert response.data["measurements.lcp"] == {
+            "good": 0,
+            "meh": 1,
+            "poor": 1,
+            "total": 2,
+            "p75": 4500,
         }
 
     def test_grouping(self):