feat(alerts): common field support for on-demand metrics (#52612)

src/sentry/snuba/metrics/extraction.py

@@ -19,6 +19,7 @@ from typing_extensions import NotRequired
 
 from sentry.api import event_search
 from sentry.api.event_search import ParenExpression, SearchFilter
+from sentry.exceptions import InvalidSearchQuery
 from sentry.search.events import fields
 from sentry.snuba.dataset import Dataset
 from sentry.snuba.metrics.utils import MetricOperationType
@@ -35,10 +36,10 @@ QUERY_HASH_KEY = "query_hash"
 # TODO: Streamline with dynamic sampling.
 RuleCondition = Union["LogicalRuleCondition", "ComparingRuleCondition", "NotRuleCondition"]
 
-
 # Maps from Discover's field names to event protocol paths. See Relay's
 # ``FieldValueProvider`` for supported fields. All fields need to be prefixed
 # with "event.".
+# List of UI supported search fields is defined in sentry/static/app/utils/fields/index.ts
 _SEARCH_TO_PROTOCOL_FIELDS = {
     # Top-level fields
     "release": "release",
@@ -63,7 +64,6 @@ _SEARCH_TO_PROTOCOL_FIELDS = {
     "os.name": "contexts.os.name",
     "os.version": "contexts.os.version",
     "browser.name": "contexts.browser.name",
-    "browser.version": "contexts.browser.version",
     "transaction.op": "contexts.trace.op",
     "transaction.status": "contexts.trace.status",
     "http.status_code": "contexts.response.status_code",
@@ -110,9 +110,29 @@ _AGGREGATE_TO_METRIC_TYPE = {
     "p99": "d",
 }
 
+# Query fields that on their own do not require on-demand metric extraction.
+_STANDARD_METRIC_FIELDS = [
+    "release",
+    "dist",
+    "environment",
+    "transaction",
+    "platform",
+    "transaction.status",
+    "transaction.op",
+    "http.method",
+    "http.status_code",
+    "browser.name",
+    "os.name",
+    "geo.country_code",
+    "event.type",
+]
+
 # Operators used in ``ComparingRuleCondition``.
 CompareOp = Literal["eq", "gt", "gte", "lt", "lte", "glob"]
 
+QueryOp = Literal["AND", "OR"]
+QueryToken = Union[SearchFilter, QueryOp, ParenExpression]
+
 
 class ComparingRuleCondition(TypedDict):
     """RuleCondition that compares a named field to a reference value."""
@@ -182,11 +202,41 @@ def is_on_demand_snuba_query(snuba_query: SnubaQuery) -> bool:
 
 
 def is_on_demand_query(dataset: Optional[Union[str, Dataset]], query: Optional[str]) -> bool:
-    """Returns ``True`` if the dataset and query combination can't be supported by standard metrics."""
+    """Returns ``True`` if the dataset is performance metrics and query contains non-standard search fields."""
+
     if not dataset or not query:
         return False
 
-    return Dataset(dataset) == Dataset.PerformanceMetrics and "transaction.duration" in query
+    if Dataset(dataset) != Dataset.PerformanceMetrics:
+        return False
+
+    try:
+        return not _is_standard_metrics_compatible(event_search.parse_search_query(query))
+    except InvalidSearchQuery:
+        logger.error(f"Failed to parse search query: {query}", exc_info=True)
+        return False
+
+
+def _is_standard_metrics_compatible(tokens: Sequence[QueryToken]) -> bool:
+    """
+    Recursively checks if any of the supplied token contain search filters that can't be handled by standard metrics.
+    """
+
+    for token in tokens:
+        if not _is_standard_metrics_search_filter(token):
+            return False
+
+    return True
+
+
+def _is_standard_metrics_search_filter(token: QueryToken) -> bool:
+    if isinstance(token, SearchFilter):
+        return token.key.name in _STANDARD_METRIC_FIELDS
+
+    if isinstance(token, ParenExpression):
+        return _is_standard_metrics_compatible(token.children)
+
+    return True
 
 
 class OndemandMetricSpec:
@@ -277,8 +327,6 @@ def _map_field_name(search_key: str) -> str:
     raise ValueError(f"Unsupported query field {search_key}")
 
 
-QueryOp = Literal["AND", "OR"]
-QueryToken = Union[SearchFilter, QueryOp, ParenExpression]
 T = TypeVar("T")
 
 

tests/sentry/relay/config/test_metric_extraction.py

@@ -19,12 +19,6 @@ def test_empty_query():
     assert convert_query_to_metric(alert.snuba_query) is None
 
 
-def test_standard_metric_query():
-    alert = create_alert("transaction:/my/api/url/")
-
-    assert convert_query_to_metric(alert.snuba_query) is None
-
-
 def test_simple_query_count():
     snuba_query = SnubaQuery(
         aggregate="count()",

tests/sentry/snuba/test_extraction.py

@@ -0,0 +1,47 @@
+from sentry.snuba.dataset import Dataset
+from sentry.snuba.metrics.extraction import is_on_demand_query
+
+
+def test_is_on_demand_query_wrong_dataset():
+    assert is_on_demand_query(Dataset.Transactions, "geo.city:=Vienna") is False
+    assert is_on_demand_query(Dataset.Metrics, "browser.version:=1 os.name:=android") is False
+
+
+def test_is_on_demand_query_no_query():
+    assert is_on_demand_query(Dataset.PerformanceMetrics, "") is False
+
+
+def test_is_on_demand_query_invalid_query():
+    assert is_on_demand_query(Dataset.PerformanceMetrics, "AND") is False
+    assert is_on_demand_query(Dataset.PerformanceMetrics, "(AND transaction.duration:>=1") is False
+    assert is_on_demand_query(Dataset.PerformanceMetrics, "transaction.duration:>=abc") is False
+
+
+def test_is_on_demand_query_true():
+    dataset = Dataset.PerformanceMetrics
+
+    # transaction.duration is a non-standard field
+    assert is_on_demand_query(dataset, "transaction.duration:>=1") is True
+    # transaction.duration is a non-standard field
+    assert is_on_demand_query(dataset, "geo.city:=Vienna") is True
+    # os.name is a standard field, browser.version is not
+    assert is_on_demand_query(dataset, "browser.version:=1 os.name:=android") is True
+    # os.version is not a standard field
+    assert (
+        is_on_demand_query(dataset, "(release:=a OR transaction.op:=b) transaction.duration:>1s")
+        is True
+    )
+
+
+def test_is_on_demand_query_false():
+    dataset = Dataset.PerformanceMetrics
+
+    assert is_on_demand_query(dataset, "") is False
+    assert is_on_demand_query(dataset, "environment:=dev") is False
+    assert is_on_demand_query(dataset, "release:=initial OR os.name:=android") is False
+    assert (
+        is_on_demand_query(
+            dataset, "(http.method:=POST OR http.status_code:=404) browser.name:=chrome"
+        )
+        is False
+    )