Repository: getsentry/sentry
Pull request: feat(alerts): treat every apdex as on demand
Changes on demand check and considers every apdex alert as an on-demand alert. 
Closes: https://github.com/getsentry/sentry/issues/55767


Problem statement:
Support Apdex thresholds
**Problem**: If a user configures an alert based on Apdex where a threshold is different than the one configured in the Project Settings, this should be an on-demand metric.
```[tasklist]
### Tasks
- [ ] FE
- [ ] Extraction
- [ ] Querying
```

Hints from issue discussion:

Commit message: Merge branch 'master' into ogi/feat/on-demand-apdex-be

Patch to review:
diff --git a/src/sentry/snuba/metrics/extraction.py b/src/sentry/snuba/metrics/extraction.py
index efd6d204e2e420..99295c587f5520 100644
--- a/src/sentry/snuba/metrics/extraction.py
+++ b/src/sentry/snuba/metrics/extraction.py
@@ -153,7 +153,7 @@
     "browser.name",
     "os.name",
     "geo.country_code",
-    # These fields are skipped during on demand spec generation and will not be converted to metric extraction conditions
+    # These are skipped during on demand spec generation and will not be converted to metric extraction conditions
     "event.type",
     "project",
 ]
@@ -307,8 +307,8 @@ def _get_function_support(function: str) -> SupportedBy:
 
 def _get_args_support(function: str, args: Sequence[str]) -> SupportedBy:
     # apdex can have two variations, either apdex() or apdex(value).
-    if function == "apdex" and len(args) < 2:
-        return SupportedBy.both()
+    if function == "apdex":
+        return SupportedBy(on_demand_metrics=True, standard_metrics=False)
 
     if len(args) == 0:
         return SupportedBy.both()
@@ -489,8 +489,8 @@ def cleanup_query(tokens: Sequence[QueryToken]) -> Sequence[QueryToken]:
     For example removing the on demand filters from "transaction.duration:>=1s OR browser.version:1 AND environment:dev"
     would result in "OR AND environment:dev" which is not a valid query this should be cleaned to "environment:dev.
 
-    "release:internal and browser.version:1 or os.name:android" => "release:internal or and os.name:android" which would be
-    cleaned to "release:internal or os.name:android"
+    "release:internal and browser.version:1 or os.name:android" => "release:internal or and os.name:android" which
+    would be cleaned to "release:internal or os.name:android"
     """
     tokens = list(tokens)
 
@@ -608,7 +608,10 @@ def apdex_tag_spec(project: Project, argument: Optional[str]) -> List[TagSpec]:
 }
 
 
-def is_derived_metric(op: MetricOperationType) -> bool:
+def is_derived_metric(op: Optional[MetricOperationType]) -> bool:
+    if op is None:
+        return False
+
     return op in _DERIVED_METRICS
 
 
@@ -617,6 +620,7 @@ class FieldParsingResult:
     function: str
     arguments: List[str]
     alias: str
+    op: Optional[MetricOperationType]
 
 
 @dataclass(frozen=True)
@@ -698,7 +702,7 @@ def _query_for_hash(self):
         return str(_deep_sorted(self.condition))
 
     @cached_property
-    def condition(self) -> RuleCondition:
+    def condition(self) -> Optional[RuleCondition]:
         """Returns a parent condition containing a list of other conditions which determine whether of not the metric
         is extracted."""
         return self._process_query()
@@ -717,14 +721,19 @@ def to_metric_spec(self, project: Project) -> MetricSpec:
         extended_tags_conditions = self.tags_conditions(project).copy()
         extended_tags_conditions.append({"key": QUERY_HASH_KEY, "value": self.query_hash})
 
-        return {
+        metric_spec: MetricSpec = {
             "category": DataCategory.TRANSACTION.api_name(),
             "mri": self.mri,
             "field": self.field_to_extract,
-            "condition": self.condition,
             "tags": extended_tags_conditions,
         }
 
+        condition = self.condition
+        if condition is not None:
+            metric_spec["condition"] = condition
+
+        return metric_spec
+
     def _process_field(self) -> Tuple[MetricOperationType, str, Optional[str]]:
         parsed_field = self._parse_field(self.field)
         if parsed_field is None:
@@ -735,7 +744,7 @@ def _process_field(self) -> Tuple[MetricOperationType, str, Optional[str]]:
 
         return op, metric_type, self._parse_argument(op, metric_type, parsed_field)
 
-    def _process_query(self) -> RuleCondition:
+    def _process_query(self) -> Optional[RuleCondition]:
         parsed_field = self._parse_field(self.field)
         if parsed_field is None:
             raise Exception(f"Unable to parse the field {self.field}")
@@ -751,7 +760,7 @@ def _process_query(self) -> RuleCondition:
         parsed_query = self._parse_query(self.query)
         # An on demand metric must have at least a condition, otherwise we can just use a classic metric.
         if parsed_query is None or len(parsed_query.conditions) == 0:
-            if count_if_rule_condition is None:
+            if count_if_rule_condition is None and parsed_field.function != "apdex":
                 raise Exception("This query should not use on demand metrics")
 
             return count_if_rule_condition
@@ -817,7 +826,7 @@ def _cleanup_query(query: str) -> str:
     def _parse_field(value: str) -> Optional[FieldParsingResult]:
         try:
             function, arguments, alias = fields.parse_function(value)
-            return FieldParsingResult(function=function, arguments=arguments, alias=alias)
+            return FieldParsingResult(function=function, arguments=arguments, alias=alias, op=None)
         except InvalidSearchQuery:
             return None
 
diff --git a/tests/sentry/snuba/test_extraction.py b/tests/sentry/snuba/test_extraction.py
index aa3d5b876d08ca..c1059065024a4b 100644
--- a/tests/sentry/snuba/test_extraction.py
+++ b/tests/sentry/snuba/test_extraction.py
@@ -26,7 +26,8 @@
             "transaction.duration:>1",
             True,
         ),  # transaction.duration not supported by standard metrics
-        ("failure_rate()", "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)",
@@ -60,6 +61,7 @@ class TestCreatesOndemandMetricSpec:
             ("p75(measurements.fp)", "transaction.duration:>0"),
             ("p75(transaction.duration)", "transaction.duration:>0"),
             ("count_if(transaction.duration,equals,0)", "transaction.duration:>0"),
+            ("count_if(transaction.duration,notEquals,0)", "transaction.duration:>0"),
             (
                 "count()",
                 "project:a-1 route.action:CloseBatch level:info",
@@ -69,6 +71,7 @@ class TestCreatesOndemandMetricSpec:
             ("count()", "foo:bar"),
             ("failure_rate()", "transaction.duration:>100"),
             ("apdex(10)", "transaction.duration:>100"),
+            ("apdex(10)", ""),
         ],
     )
     def test_creates_on_demand_spec(self, aggregate, query):
@@ -90,7 +93,6 @@ def test_creates_on_demand_spec(self, aggregate, query):
             ("p95(measurements.lcp)", ""),
             ("avg(spans.http)", ""),
             ("failure_rate()", "release:bar"),
-            ("apdex(10)", "release:foo"),
         ],
     )
     def test_does_not_create_on_demand_spec(self, aggregate, query):
@@ -296,6 +298,20 @@ def test_spec_apdex(_get_apdex_project_transaction_threshold, default_project):
     ]
 
 
+@django_db_all
+@patch("sentry.snuba.metrics.extraction._get_apdex_project_transaction_threshold")
+def test_spec_apdex_without_condition(_get_apdex_project_transaction_threshold, default_project):
+    _get_apdex_project_transaction_threshold.return_value = 100, "transaction.duration"
+
+    spec = OnDemandMetricSpec("apdex(10)", "")
+
+    assert spec._metric_type == "c"
+    assert spec.field_to_extract is None
+    assert spec.op == "on_demand_apdex"
+    assert spec.condition is None
+    assert len(spec.tags_conditions(default_project)) == 3
+
+
 def test_spec_custom_tag():
     custom_tag_spec = OnDemandMetricSpec("count()", "foo:bar")
 


Code review comments:
diff_hunk: @@ -751,7 +760,7 @@ def _process_query(self) -> RuleCondition:
         parsed_query = self._parse_query(self.query)
         # An on demand metric must have at least a condition, otherwise we can just use a classic metric.
         if parsed_query is None or len(parsed_query.conditions) == 0:
-            if count_if_rule_condition is None:
+            if count_if_rule_condition is None and parsed_field.function != "apdex":
path: src/sentry/snuba/metrics/extraction.py
text: @user1:
This condition without explanation is a bit tough to understand. What I would do it actually make a list of functions for which we don't require conditions and early return.

@user1:
```suggestion
            if parsed_field.function in CONDITIONLESS_FUNCTIONS:
            	return None
            	
            # Logic as before          	
```

@user1:
This way you really have an `Optional[RuleCondition]`, without a return `None` you will have a valid `RuleCondition` always, since the other cases will result in an exception being thrown.

@author:
Refactored in [f9e32c2](https://github.com/getsentry/sentry/pull/55999/commits/f9e32c244d801050f9dd9c35e2528a59c23097fc)

diff_hunk: @@ -735,7 +744,7 @@ def _process_field(self) -> Tuple[MetricOperationType, str, Optional[str]]:
 
         return op, metric_type, self._parse_argument(op, metric_type, parsed_field)
 
-    def _process_query(self) -> RuleCondition:
+    def _process_query(self) -> Optional[RuleCondition]:
path: src/sentry/snuba/metrics/extraction.py
text: @user1:
This type is not `Optional`, same applies for upstream changes.