ref(metrics_extraction): Test improvements + deprecate constant (#65012)

The following changes:

* Drop `APDEX_THRESHOLD_DEFAULT` and use `DEFAULT_PROJECT_THRESHOLD`
* Clean up some tests for improved readability
* Split up `_on_demand_query_check` for single responsibility principle

src/sentry/constants.py

@@ -637,7 +637,6 @@ REQUIRE_SCRUB_IP_ADDRESS_DEFAULT = False
 SCRAPE_JAVASCRIPT_DEFAULT = True
 TRUSTED_RELAYS_DEFAULT = None
 JOIN_REQUESTS_DEFAULT = True
-APDEX_THRESHOLD_DEFAULT = 300
 AI_SUGGESTED_SOLUTION = True
 GITHUB_COMMENT_BOT_DEFAULT = True
 

src/sentry/snuba/metrics/extraction.py

@@ -21,7 +21,7 @@ from sentry.api.event_search import (
     SearchKey,
     SearchValue,
 )
-from sentry.constants import APDEX_THRESHOLD_DEFAULT, DataCategory
+from sentry.constants import DataCategory
 from sentry.discover.arithmetic import is_equation
 from sentry.exceptions import InvalidSearchQuery
 from sentry.models.organization import Organization
@@ -29,7 +29,7 @@ from sentry.models.project import Project
 from sentry.models.transaction_threshold import ProjectTransactionThreshold, TransactionMetric
 from sentry.search.events import fields
 from sentry.search.events.builder import UnresolvedQuery
-from sentry.search.events.constants import VITAL_THRESHOLDS
+from sentry.search.events.constants import DEFAULT_PROJECT_THRESHOLD, VITAL_THRESHOLDS
 from sentry.snuba.dataset import Dataset
 from sentry.snuba.metrics.naming_layer.mri import ParsedMRI, parse_mri
 from sentry.snuba.metrics.utils import MetricOperationType
@@ -229,6 +229,7 @@ _SEARCH_TO_METRIC_AGGREGATES: dict[str, MetricOperationType] = {
 }
 
 # Maps plain Discover functions to derived metric functions which are understood by the metrics layer.
+# XXX: We need to support count_miserable
 _SEARCH_TO_DERIVED_METRIC_AGGREGATES: dict[str, MetricOperationType] = {
     "failure_count": "on_demand_failure_count",
     "failure_rate": "on_demand_failure_rate",
@@ -1513,7 +1514,7 @@ def _get_satisfactory_threshold_and_metric(project: Project) -> tuple[int, str]:
 
     if len(result) == 0:
         # We use the default threshold shown in the UI.
-        threshold = APDEX_THRESHOLD_DEFAULT
+        threshold = DEFAULT_PROJECT_THRESHOLD
         metric = TransactionMetric.DURATION.value
     else:
         # We technically don't use this threshold since we extract it from the apdex(x) field

tests/sentry/snuba/metrics/test_extraction.py

@@ -34,73 +34,39 @@ def test_equality_of_specs(default_project) -> None:
     [
         ("count()", "release:a", False),  # supported by standard metrics
         ("failure_rate()", "release:a", False),  # supported by standard metrics
-        ("count_unique(geo.city)", "release:a", False),
         # geo.city not supported by standard metrics, but also not by on demand
-        (
-            "count()",
-            "transaction.duration:>1",
-            True,
-        ),  # transaction.duration not supported by standard metrics
+        ("count_unique(geo.city)", "release:a", False),
+        # transaction.duration not supported by standard metrics
+        ("count()", "transaction.duration:>1", True),
         ("failure_count()", "transaction.duration:>1", True),  # supported by on demand
         ("failure_rate()", "transaction.duration:>1", True),  # supported by on demand
         ("apdex(10)", "", True),  # every apdex query is on-demand
         ("apdex(10)", "transaction.duration:>10", True),  # supported by on demand
-        (
-            "count_if(transaction.duration,equals,0)",
-            "release:a",
-            False,
-        ),  # count_if supported by standard metrics
+        # count_if supported by standard metrics
+        ("count_if(transaction.duration,equals,0)", "release:a", False),
         ("p75(transaction.duration)", "release:a", False),  # supported by standard metrics
-        (
-            "p75(transaction.duration)",
-            "transaction.duration:>1",
-            True,
-        ),  # transaction.duration query is on-demand
+        # transaction.duration query is on-demand
+        ("p75(transaction.duration)", "transaction.duration:>1", True),
         ("p90(transaction.duration)", "release:a", False),  # supported by standard metrics
-        (
-            "p90(transaction.duration)",
-            "transaction.duration:>1",
-            True,
-        ),  # transaction.duration query is on-demand
-        (
-            "percentile(transaction.duration, 0.9)",
-            "release:a",
-            False,
-        ),  # supported by standard metrics
-        (
-            "percentile(transaction.duration, 0.9)",
-            "transaction.duration:>1",
-            True,
-        ),  # transaction.duration query is on-demand
-        (
-            "percentile(transaction.duration, 0.90)",
-            "release:a",
-            False,
-        ),  # supported by standard metrics
-        (
-            "percentile(transaction.duration, 0.90)",
-            "transaction.duration:>1",
-            True,
-        ),
+        # transaction.duration query is on-demand
+        ("p90(transaction.duration)", "transaction.duration:>1", True),
+        # supported by standard metrics
+        ("percentile(transaction.duration, 0.9)", "release:a", False),
+        # transaction.duration query is on-demand
+        ("percentile(transaction.duration, 0.9)", "transaction.duration:>1", True),
+        # supported by standard metrics
+        ("percentile(transaction.duration, 0.90)", "release:a", False),
+        ("percentile(transaction.duration, 0.90)", "transaction.duration:>1", True),
         ("count()", "", False),  # Malformed aggregate should return false
-        (
-            "count()",
-            "event.type:error transaction.duration:>0",
-            False,
-        ),  # event.type:error not supported by metrics
-        (
-            "count()",
-            "event.type:default transaction.duration:>0",
-            False,
-        ),  # event.type:error not supported by metrics
-        (
-            "count()",
-            "error.handled:true transaction.duration:>0",
-            False,
-        ),  # error.handled is an error search term
+        # event.type:error not supported by metrics
+        ("count()", "event.type:error transaction.duration:>0", False),
+        # event.type:error not supported by metrics
+        ("count()", "event.type:default transaction.duration:>0", False),
+        # error.handled is an error search term
+        ("count()", "error.handled:true transaction.duration:>0", False),
     ],
 )
-def test_should_use_on_demand(agg, query, result) -> None:
+def test_should_use_on_demand(agg: str, query: str, result: bool) -> None:
     assert should_use_on_demand_metrics(Dataset.PerformanceMetrics, agg, query) is result
 
 
@@ -109,26 +75,10 @@ def test_should_use_on_demand(agg, query, result) -> None:
     [
         ("sum(c:custom/page_load@millisecond)", "release:a", False),
         ("sum(c:custom/page_load@millisecond)", "transaction.duration:>0", False),
-        (
-            "p75(d:transactions/measurements.fcp@millisecond)",
-            "release:a",
-            False,
-        ),
-        (
-            "p75(d:transactions/measurements.fcp@millisecond)",
-            "transaction.duration:>0",
-            False,
-        ),
-        (
-            "p95(d:spans/duration@millisecond)",
-            "release:a",
-            False,
-        ),
-        (
-            "p95(d:spans/duration@millisecond)",
-            "transaction.duration:>0",
-            False,
-        ),
+        ("p75(d:transactions/measurements.fcp@millisecond)", "release:a", False),
+        ("p75(d:transactions/measurements.fcp@millisecond)", "transaction.duration:>0", False),
+        ("p95(d:spans/duration@millisecond)", "release:a", False),
+        ("p95(d:spans/duration@millisecond)", "transaction.duration:>0", False),
     ],
 )
 def test_should_use_on_demand_with_mri(agg, query, result) -> None:

tests/snuba/api/endpoints/test_organization_events_mep.py

@@ -3137,14 +3137,11 @@ class OrganizationEventsMetricsEnhancedPerformanceEndpointTestWithOnDemandMetric
         self.url = reverse(self.viewname, kwargs={"organization_slug": self.organization.slug})
         self.features = {"organizations:on-demand-metrics-extraction-widgets": True}
 
-    def _on_demand_query_check(
-        self,
-        params: dict[str, Any],
-        groupbys: list[str] | None = None,
-        expected_on_demand_query: bool | None = True,
-        expected_dataset: str | None = "metricsEnhanced",
-    ) -> Response:
-        """Do a request to the events endpoint with metrics enhanced and on-demand enabled."""
+    def _create_specs(
+        self, params: dict[str, Any], groupbys: list[str] | None = None
+    ) -> list[OnDemandMetricSpec]:
+        """Creates all specs based on the parameters that would be passed to the endpoint."""
+        specs = []
         for field in params["field"]:
             spec = OnDemandMetricSpec(
                 field=field,
@@ -3153,43 +3150,55 @@ class OrganizationEventsMetricsEnhancedPerformanceEndpointTestWithOnDemandMetric
                 groupbys=groupbys,
                 spec_type=MetricSpecType.DYNAMIC_QUERY,
             )
+            specs.append(spec)
+        return specs
+
+    def _make_on_demand_request(self, params: dict[str, Any]) -> Response:
+        """Ensures that the required parameters for an on-demand request are included."""
         # Expected parameters for this helper function
         params["dataset"] = "metricsEnhanced"
         params["useOnDemandMetrics"] = "true"
         params["onDemandType"] = "dynamic_query"
+        return self.do_request(params)
 
-        self.store_on_demand_metric(1, spec=spec)
-        response = self.do_request(params)
-
+    def _assert_on_demand_response(
+        self,
+        response: Response,
+        expected_on_demand_query: bool | None = True,
+        expected_dataset: str | None = "metricsEnhanced",
+    ) -> None:
+        """Basic assertions for an on-demand request."""
         assert response.status_code == 200, response.content
         meta = response.data["meta"]
         assert meta.get("isMetricsExtractedData", False) is expected_on_demand_query
         assert meta["dataset"] == expected_dataset
 
-        return response
-
     def test_is_metrics_extracted_data_is_included(self) -> None:
-        self._on_demand_query_check(
-            {"field": ["count()"], "query": "transaction.duration:>=91", "yAxis": "count()"}
-        )
+        params = {"field": ["count()"], "query": "transaction.duration:>=91", "yAxis": "count()"}
+        specs = self._create_specs(params)
+        for spec in specs:
+            self.store_on_demand_metric(1, spec=spec)
+        response = self._make_on_demand_request(params)
+        self._assert_on_demand_response(response)
 
     def test_transaction_user_misery(self) -> None:
-        resp = self._on_demand_query_check(
-            {
-                "field": ["user_misery(300)", "apdex(300)"],
-                "project": self.project.id,
-                "query": "",
-                "sort": "-user_misery(300)",
-                "per_page": "20",
-                "referrer": "api.dashboards.tablewidget",
-            },
-            groupbys=["transaction"],
-        )
-        assert resp.data == {
-            "data": [{"user_misery(300)": 0.0, "apdex(300)": 0.0}],
+        user_misery_field = "user_misery(300)"
+        apdex_field = "apdex(300)"
+        params = {
+            "field": [user_misery_field, apdex_field],
+            "project": self.project.id,
+            "query": "",
+        }
+        specs = self._create_specs(params, groupbys=["transaction"])
+        for spec in specs:
+            self.store_on_demand_metric(1, spec=spec)
+        response = self._make_on_demand_request(params)
+        self._assert_on_demand_response(response, expected_on_demand_query=True)
+        assert response.data == {
+            "data": [{user_misery_field: 0.0, apdex_field: 0.0}],
             "meta": {
-                "fields": {"user_misery(300)": "number", "apdex(300)": "number"},
-                "units": {"user_misery(300)": None, "apdex(300)": None},
+                "fields": {user_misery_field: "number", apdex_field: "number"},
+                "units": {user_misery_field: None, apdex_field: None},
                 "isMetricsData": True,
                 "isMetricsExtractedData": True,
                 "tips": {},