Browse Source

feat(on-demand): support known custom percentiles (#58430)

Ogi 1 year ago
parent
commit
48405f3524

+ 52 - 17
src/sentry/snuba/metrics/extraction.py

@@ -59,8 +59,8 @@ _SEARCH_TO_PROTOCOL_FIELDS = {
     # Top-level structures ("interfaces")
     "user.email": "user.email",
     "user.id": "user.id",
-    "user.ip_address": "user.ip_address",
-    "user.name": "user.name",
+    "user.ip": "user.ip_address",
+    "user.username": "user.name",
     "user.segment": "user.segment",
     "geo.city": "user.geo.city",
     "geo.country_code": "user.geo.country_code",
@@ -115,8 +115,10 @@ _SEARCH_TO_METRIC_AGGREGATES: Dict[str, MetricOperationType] = {
     "max": "max",
     "p50": "p50",
     "p75": "p75",
+    "p90": "p90",
     "p95": "p95",
     "p99": "p99",
+    "p100": "p100"
     # generic percentile is not supported by metrics layer.
 }
 
@@ -138,8 +140,11 @@ _AGGREGATE_TO_METRIC_TYPE = {
     "max": "d",
     "p50": "d",
     "p75": "d",
+    "p90": "d",
     "p95": "d",
     "p99": "d",
+    "p100": "d",
+    "percentile": "d",
     # With on demand metrics, evaluated metrics are actually stored, thus we have to choose a concrete metric type.
     "failure_count": "c",
     "failure_rate": "c",
@@ -311,7 +316,7 @@ def _get_aggregate_supported_by(aggregate: str) -> SupportedBy:
             raise InvalidSearchQuery(f"Invalid characters in field {aggregate}")
 
         function, _, args, _ = query_builder.parse_function(match)
-        function_support = _get_function_support(function)
+        function_support = _get_function_support(function, args)
         args_support = _get_args_support(function, args)
 
         return SupportedBy.combine(function_support, args_support)
@@ -322,7 +327,10 @@ def _get_aggregate_supported_by(aggregate: str) -> SupportedBy:
     return SupportedBy.neither()
 
 
-def _get_function_support(function: str) -> SupportedBy:
+def _get_function_support(function: str, args: Sequence[str]) -> SupportedBy:
+    if function == "percentile":
+        return _get_percentile_support(args)
+
     return SupportedBy(
         standard_metrics=True,
         on_demand_metrics=(
@@ -333,6 +341,38 @@ def _get_function_support(function: str) -> SupportedBy:
     )
 
 
+def _get_percentile_support(args: Sequence[str]) -> SupportedBy:
+    if len(args) != 2:
+        return SupportedBy.neither()
+
+    if not _get_percentile_op(args):
+        return SupportedBy.neither()
+
+    return SupportedBy(standard_metrics=False, on_demand_metrics=True)
+
+
+def _get_percentile_op(args: Sequence[str]) -> Optional[MetricOperationType]:
+    if len(args) != 2:
+        raise ValueError("Percentile function should have 2 arguments")
+
+    percentile = args[1]
+
+    if percentile in ["0.5", "0.50"]:
+        return "p50"
+    if percentile == "0.75":
+        return "p75"
+    if percentile in ["0.9", "0.90"]:
+        return "p90"
+    if percentile == "0.95":
+        return "p95"
+    if percentile == "0.99":
+        return "p99"
+    if percentile in ["1", "1.0"]:
+        return "p100"
+
+    return None
+
+
 def _get_args_support(function: str, args: Sequence[str]) -> SupportedBy:
     if len(args) == 0:
         return SupportedBy.both()
@@ -810,7 +850,7 @@ class OnDemandMetricSpec:
         if parsed_field is None:
             raise Exception(f"Unable to parse the field {self.field}")
 
-        op = self._get_op(parsed_field.function)
+        op = self._get_op(parsed_field.function, parsed_field.arguments)
         metric_type = self._get_metric_type(parsed_field.function)
 
         return op, metric_type, self._parse_arguments(op, metric_type, parsed_field)
@@ -829,7 +869,7 @@ class OnDemandMetricSpec:
         if parsed_query is None or len(parsed_query.conditions) == 0:
             if aggregate_conditions is None:
                 # derived metrics have their conditions injected in the tags
-                if self._get_op(parsed_field.function) in _DERIVED_METRICS:
+                if self._get_op(parsed_field.function, parsed_field.arguments) in _DERIVED_METRICS:
                     return None
                 raise Exception("This query should not use on demand metrics")
 
@@ -876,7 +916,12 @@ class OnDemandMetricSpec:
         return [_map_field_name(first_argument)] if map_argument else arguments
 
     @staticmethod
-    def _get_op(function: str) -> MetricOperationType:
+    def _get_op(function: str, args: Sequence[str]) -> MetricOperationType:
+        if function == "percentile":
+            percentile_op = _get_percentile_op(args)
+            if percentile_op is not None:
+                function = cast(str, percentile_op)
+
         op = _SEARCH_TO_METRIC_AGGREGATES.get(function) or _SEARCH_TO_DERIVED_METRIC_AGGREGATES.get(
             function
         )
@@ -893,16 +938,6 @@ class OnDemandMetricSpec:
 
         raise Exception(f"Unsupported aggregate function {function}")
 
-    # @staticmethod
-    # def _cleanup_query(query: str) -> str:
-    #     regexes = [r"event\.type:transaction\s*", r"project:[\w\d\"\-_]+\s*"]
-    #
-    #     new_query = query
-    #     for regex in regexes:
-    #         new_query = re.sub(regex, "", new_query)
-    #
-    #     return new_query
-
     @staticmethod
     def _parse_field(value: str) -> Optional[FieldParsingResult]:
         try:

+ 1 - 0
src/sentry/snuba/metrics/utils.py

@@ -85,6 +85,7 @@ MetricOperationType = Literal[
     "p90",
     "p95",
     "p99",
+    "p100",
     "percentage",
     "histogram",
     "rate",

+ 13 - 0
tests/sentry/snuba/test_extraction.py

@@ -63,10 +63,21 @@ class TestCreatesOndemandMetricSpec:
         [
             # transaction duration not supported by standard metrics
             ("count()", "transaction.duration:>0"),
+            ("count()", "user.ip:192.168.0.1"),
+            ("count()", "user.username:foobar"),
             ("count()", "transaction.duration:>0 event.type:transaction project:abc"),
             ("count()", "(transaction.duration:>0) AND (event.type:transaction)"),
             ("p75(measurements.fp)", "transaction.duration:>0"),
             ("p75(transaction.duration)", "transaction.duration:>0"),
+            ("p100(transaction.duration)", "transaction.duration:>0"),
+            # we dont support custom percentiles that can be mapped to one of standard percentiles
+            ("percentile(transaction.duration, 0.5)", "transaction.duration>0"),
+            ("percentile(transaction.duration, 0.50)", "transaction.duration>0"),
+            ("percentile(transaction.duration, 0.9)", "transaction.duration>0"),
+            ("percentile(transaction.duration, 0.90)", "transaction.duration>0"),
+            ("percentile(transaction.duration, 0.95)", "transaction.duration>0"),
+            ("percentile(transaction.duration, 0.99)", "transaction.duration>0"),
+            ("percentile(transaction.duration, 1)", "transaction.duration>0"),
             ("count_if(transaction.duration,equals,0)", "transaction.duration:>0"),
             ("count_if(transaction.duration,notEquals,0)", "transaction.duration:>0"),
             (
@@ -100,6 +111,8 @@ class TestCreatesOndemandMetricSpec:
             ("last_seen()", "transaction.duration:>0"),  # last_seen not supported by on demand
             ("any(user)", "transaction.duration:>0"),  # any not supported by on demand
             ("p95(transaction.duration)", ""),  # p95 without query is supported by standard metrics
+            # we do not support custom percentiles that can not be mapped to one of standard percentiles
+            ("percentile(transaction.duration, 0.123)", "transaction.duration>0"),
             (
                 "count()",
                 "p75(transaction.duration):>0",