Browse Source

fix(discover): Apdex potentially nan (#37509)

- The denominator of apdex could potentially be 0 resulting in apdex
  being nan. Using `resolve_division` instead which checks that the
  denominator isn't 0 before dividing.
- Adds a param to set the fallback value, this is cause otherwise apdex
  for that bucket will be `None`, and not get zerofilled.
William Mak 2 years ago
parent
commit
bfbe93ac53

+ 4 - 2
src/sentry/search/events/builder.py

@@ -657,7 +657,9 @@ class QueryBuilder:
             return snql_function.snql_aggregate(arguments, alias)
             return snql_function.snql_aggregate(arguments, alias)
         return None
         return None
 
 
-    def resolve_division(self, dividend: SelectType, divisor: SelectType, alias: str) -> SelectType:
+    def resolve_division(
+        self, dividend: SelectType, divisor: SelectType, alias: str, fallback: Optional[Any] = None
+    ) -> SelectType:
         return Function(
         return Function(
             "if",
             "if",
             [
             [
@@ -672,7 +674,7 @@ class QueryBuilder:
                         divisor,
                         divisor,
                     ],
                     ],
                 ),
                 ),
-                None,
+                fallback,
             ],
             ],
             alias,
             alias,
         )
         )

+ 12 - 12
src/sentry/search/events/datasets/discover.py

@@ -1255,19 +1255,19 @@ class DiscoverDatasetConfig(DatasetConfig):
             "countIf", [Function("greaterOrEquals", [column, 0])]
             "countIf", [Function("greaterOrEquals", [column, 0])]
         )
         )
 
 
-        return Function(  # (satisfied + tolerable/2)/(total)
-            "divide",
-            [
-                Function(
-                    "plus",
-                    [
-                        count_satisfaction,
-                        count_tolerable_div_2,
-                    ],
-                ),
-                count_total,
-            ],
+        return self.builder.resolve_division(  # (satisfied + tolerable/2)/(total)
+            Function(
+                "plus",
+                [
+                    count_satisfaction,
+                    count_tolerable_div_2,
+                ],
+            ),
+            count_total,
             alias,
             alias,
+            # TODO(zerofill): This behaviour is incorrect if we remove zerofilling
+            # But need to do something reasonable here since we'll get a null row otherwise
+            fallback=0,
         )
         )
 
 
     def _resolve_web_vital_function(
     def _resolve_web_vital_function(

+ 39 - 0
tests/snuba/api/endpoints/test_organization_events_stats.py

@@ -195,6 +195,45 @@ class OrganizationEventsStatsEndpointTest(APITestCase, SnubaTestCase):
         )
         )
         assert response.status_code == 200, response.content
         assert response.status_code == 200, response.content
 
 
+    def test_apdex_divide_by_zero(self):
+        ProjectTransactionThreshold.objects.create(
+            project=self.project,
+            organization=self.project.organization,
+            threshold=600,
+            metric=TransactionMetric.LCP.value,
+        )
+
+        # Shouldn't count towards apdex
+        data = load_data(
+            "transaction",
+            start_timestamp=self.day_ago + timedelta(minutes=(1)),
+            timestamp=self.day_ago + timedelta(minutes=(3)),
+        )
+        data["transaction"] = "/apdex/new/"
+        data["user"] = {"email": "1@example.com"}
+        data["measurements"] = {}
+        self.store_event(data, project_id=self.project.id)
+
+        response = self.do_request(
+            data={
+                "start": iso_format(self.day_ago),
+                "end": iso_format(self.day_ago + timedelta(hours=2)),
+                "interval": "1h",
+                "yAxis": "apdex()",
+                "project": [self.project.id],
+            },
+        )
+
+        assert response.status_code == 200, response.content
+        assert len(response.data["data"]) == 2
+        data = response.data["data"]
+        print(data)
+        # 0 transactions with LCP 0/0
+        assert [attrs for time, attrs in response.data["data"]] == [
+            [{"count": 0}],
+            [{"count": 0}],
+        ]
+
     def test_aggregate_function_apdex(self):
     def test_aggregate_function_apdex(self):
         project1 = self.create_project()
         project1 = self.create_project()
         project2 = self.create_project()
         project2 = self.create_project()