ref(metrics-layer): Cleanup metric percentiles (#54869)

- This removes the percentile_range function which isn't being used as
far as I can tell
- This moves the percentile implementation from the metrics_layer and
span_metrics datasets to function_aliases since they're identical
- Add more test coverage to the span metrics dataset

src/sentry/search/events/datasets/function_aliases.py

@@ -218,3 +218,34 @@ def resolve_metrics_percentile(
             alias,
         )
     )
+
+
+def resolve_metrics_layer_percentile(
+    args: Mapping[str, Union[str, Column, SelectType, int, float]],
+    alias: str,
+    resolve_mri: Callable[[str], Column],
+    fixed_percentile: Optional[float] = None,
+):
+    # TODO: rename to just resolve_metrics_percentile once the non layer code can be retired
+    if fixed_percentile is None:
+        fixed_percentile = args["percentile"]
+    if fixed_percentile not in constants.METRIC_PERCENTILES:
+        raise IncompatibleMetricsQuery("Custom quantile incompatible with metrics")
+    column = resolve_mri(args["column"])
+    return (
+        Function(
+            "max",
+            [
+                column,
+            ],
+            alias,
+        )
+        if fixed_percentile == 1
+        else Function(
+            f"p{int(fixed_percentile * 100)}",
+            [
+                column,
+            ],
+            alias,
+        )
+    )

src/sentry/search/events/datasets/metrics_layer.py

@@ -8,7 +8,7 @@ from snuba_sdk import AliasedExpression, Column, Condition, Function, Op
 from sentry.api.event_search import SearchFilter
 from sentry.exceptions import IncompatibleMetricsQuery, InvalidSearchQuery
 from sentry.search.events import builder, constants, fields
-from sentry.search.events.datasets import field_aliases, filter_aliases
+from sentry.search.events.datasets import field_aliases, filter_aliases, function_aliases
 from sentry.search.events.datasets.metrics import MetricsDatasetConfig
 from sentry.search.events.types import SelectType, WhereType
 from sentry.snuba.metrics.naming_layer.mri import SessionMRI, TransactionMRI
@@ -120,8 +120,8 @@ class MetricsLayerDatasetConfig(MetricsDatasetConfig):
                             ),
                         ),
                     ],
-                    snql_metric_layer=lambda args, alias: self._resolve_percentile(
-                        args, alias, 0.5
+                    snql_metric_layer=lambda args, alias: function_aliases.resolve_metrics_layer_percentile(
+                        args=args, alias=alias, resolve_mri=self.resolve_mri, fixed_percentile=0.5
                     ),
                     is_percentile=True,
                     result_type_fn=self.reflective_result_type(),
@@ -137,8 +137,8 @@ class MetricsLayerDatasetConfig(MetricsDatasetConfig):
                             ),
                         ),
                     ],
-                    snql_metric_layer=lambda args, alias: self._resolve_percentile(
-                        args, alias, 0.75
+                    snql_metric_layer=lambda args, alias: function_aliases.resolve_metrics_layer_percentile(
+                        args=args, alias=alias, resolve_mri=self.resolve_mri, fixed_percentile=0.75
                     ),
                     is_percentile=True,
                     result_type_fn=self.reflective_result_type(),
@@ -154,8 +154,8 @@ class MetricsLayerDatasetConfig(MetricsDatasetConfig):
                             ),
                         ),
                     ],
-                    snql_metric_layer=lambda args, alias: self._resolve_percentile(
-                        args, alias, 0.90
+                    snql_metric_layer=lambda args, alias: function_aliases.resolve_metrics_layer_percentile(
+                        args=args, alias=alias, resolve_mri=self.resolve_mri, fixed_percentile=0.90
                     ),
                     is_percentile=True,
                     result_type_fn=self.reflective_result_type(),
@@ -171,8 +171,8 @@ class MetricsLayerDatasetConfig(MetricsDatasetConfig):
                             ),
                         ),
                     ],
-                    snql_metric_layer=lambda args, alias: self._resolve_percentile(
-                        args, alias, 0.95
+                    snql_metric_layer=lambda args, alias: function_aliases.resolve_metrics_layer_percentile(
+                        args=args, alias=alias, resolve_mri=self.resolve_mri, fixed_percentile=0.95
                     ),
                     is_percentile=True,
                     result_type_fn=self.reflective_result_type(),
@@ -188,8 +188,8 @@ class MetricsLayerDatasetConfig(MetricsDatasetConfig):
                             ),
                         ),
                     ],
-                    snql_metric_layer=lambda args, alias: self._resolve_percentile(
-                        args, alias, 0.99
+                    snql_metric_layer=lambda args, alias: function_aliases.resolve_metrics_layer_percentile(
+                        args=args, alias=alias, resolve_mri=self.resolve_mri, fixed_percentile=0.99
                     ),
                     is_percentile=True,
                     result_type_fn=self.reflective_result_type(),
@@ -206,7 +206,9 @@ class MetricsLayerDatasetConfig(MetricsDatasetConfig):
                         ),
                     ],
                     # Not marked as a percentile as this is equivalent to just `max`
-                    snql_metric_layer=lambda args, alias: self._resolve_percentile(args, alias, 1),
+                    snql_metric_layer=lambda args, alias: function_aliases.resolve_metrics_layer_percentile(
+                        args=args, alias=alias, resolve_mri=self.resolve_mri, fixed_percentile=1
+                    ),
                     result_type_fn=self.reflective_result_type(),
                     default_result_type="duration",
                 ),
@@ -281,7 +283,12 @@ class MetricsLayerDatasetConfig(MetricsDatasetConfig):
                         fields.NumberRange("percentile", 0, 1),
                     ],
                     is_percentile=True,
-                    snql_metric_layer=self._resolve_percentile,
+                    snql_metric_layer=lambda args, alias: function_aliases.resolve_metrics_layer_percentile(
+                        args=args,
+                        alias=alias,
+                        resolve_mri=self.resolve_mri,
+                        fixed_percentile=args["percentile"],
+                    ),
                     default_result_type="duration",
                 ),
                 fields.MetricsFunction(
@@ -510,35 +517,6 @@ class MetricsLayerDatasetConfig(MetricsDatasetConfig):
         return Condition(self.builder.resolve_column("transaction"), Op(operator), value)
 
     # Query Functions
-    def _resolve_percentile(
-        self,
-        args: Mapping[str, Union[str, Column, SelectType, int, float]],
-        alias: str,
-        fixed_percentile: Optional[float] = None,
-    ) -> SelectType:
-        if fixed_percentile is None:
-            fixed_percentile = args["percentile"]
-        if fixed_percentile not in constants.METRIC_PERCENTILES:
-            raise IncompatibleMetricsQuery("Custom quantile incompatible with metrics")
-        column = self.resolve_mri(args["column"])
-        return (
-            Function(
-                "max",
-                [
-                    column,
-                ],
-                alias,
-            )
-            if fixed_percentile == 1
-            else Function(
-                f"p{int(fixed_percentile * 100)}",
-                [
-                    column,
-                ],
-                alias,
-            )
-        )
-
     def _resolve_apdex_function(
         self,
         args: Mapping[str, Union[str, Column, SelectType, int, float]],

src/sentry/search/events/datasets/spans_metrics.py

@@ -299,36 +299,6 @@ class SpansMetricsDatasetConfig(DatasetConfig):
                     snql_distribution=self._resolve_http_error_count,
                     default_result_type="integer",
                 ),
-                fields.MetricsFunction(
-                    "percentile_range",
-                    required_args=[
-                        fields.MetricArg(
-                            "column",
-                            allowed_columns=constants.SPAN_METRIC_DURATION_COLUMNS,
-                            allow_custom_measurements=False,
-                        ),
-                        fields.NumberRange("percentile", 0, 1),
-                        fields.ConditionArg("condition"),
-                        fields.SnQLDateArg("middle"),
-                    ],
-                    calculated_args=[resolve_metric_id],
-                    snql_distribution=lambda args, alias: function_aliases.resolve_metrics_percentile(
-                        args=args,
-                        alias=alias,
-                        fixed_percentile=args["percentile"],
-                        extra_conditions=[
-                            Function(
-                                args["condition"],
-                                [
-                                    Function("toDateTime", [args["middle"]]),
-                                    self.builder.column("timestamp"),
-                                ],
-                            ),
-                        ],
-                    ),
-                    is_percentile=True,
-                    default_result_type="duration",
-                ),
             ]
         }
 
@@ -516,6 +486,8 @@ class SpansMetricsLayerDatasetConfig(DatasetConfig):
         self.total_span_duration: Optional[float] = None
 
     def resolve_mri(self, value) -> Column:
+        """Given the public facing column name resolve it to the MRI and return a Column"""
+        # If the query builder has not detected a transaction use the light self time metric to get a performance boost
         if value == "span.self_time" and not self.builder.has_transaction:
             return Column(constants.SELF_TIME_LIGHT)
         else:
@@ -529,7 +501,11 @@ class SpansMetricsLayerDatasetConfig(DatasetConfig):
 
     @property
     def field_alias_converter(self) -> Mapping[str, Callable[[str], SelectType]]:
-        return {constants.SPAN_MODULE_ALIAS: self._resolve_span_module}
+        return {
+            constants.SPAN_MODULE_ALIAS: lambda alias: field_aliases.resolve_span_module(
+                self.builder, alias
+            )
+        }
 
     @property
     def function_converter(self) -> Mapping[str, fields.MetricsFunction]:
@@ -643,7 +619,11 @@ class SpansMetricsLayerDatasetConfig(DatasetConfig):
                         ),
                         fields.NumberRange("percentile", 0, 1),
                     ],
-                    snql_metric_layer=self._resolve_percentile,
+                    snql_metric_layer=lambda args, alias: function_aliases.resolve_metrics_layer_percentile(
+                        args,
+                        alias,
+                        self.resolve_mri,
+                    ),
                     result_type_fn=self.reflective_result_type(),
                     default_result_type="duration",
                 ),
@@ -659,8 +639,8 @@ class SpansMetricsLayerDatasetConfig(DatasetConfig):
                             ),
                         ),
                     ],
-                    snql_metric_layer=lambda args, alias: self._resolve_percentile(
-                        args=args, alias=alias, fixed_percentile=0.50
+                    snql_metric_layer=lambda args, alias: function_aliases.resolve_metrics_layer_percentile(
+                        args=args, alias=alias, resolve_mri=self.resolve_mri, fixed_percentile=0.50
                     ),
                     default_result_type="duration",
                 ),
@@ -676,8 +656,8 @@ class SpansMetricsLayerDatasetConfig(DatasetConfig):
                             ),
                         ),
                     ],
-                    snql_metric_layer=lambda args, alias: self._resolve_percentile(
-                        args=args, alias=alias, fixed_percentile=0.75
+                    snql_metric_layer=lambda args, alias: function_aliases.resolve_metrics_layer_percentile(
+                        args=args, alias=alias, resolve_mri=self.resolve_mri, fixed_percentile=0.75
                     ),
                     default_result_type="duration",
                 ),
@@ -693,8 +673,8 @@ class SpansMetricsLayerDatasetConfig(DatasetConfig):
                             ),
                         ),
                     ],
-                    snql_metric_layer=lambda args, alias: self._resolve_percentile(
-                        args=args, alias=alias, fixed_percentile=0.95
+                    snql_metric_layer=lambda args, alias: function_aliases.resolve_metrics_layer_percentile(
+                        args=args, alias=alias, resolve_mri=self.resolve_mri, fixed_percentile=0.95
                     ),
                     default_result_type="duration",
                 ),
@@ -710,8 +690,8 @@ class SpansMetricsLayerDatasetConfig(DatasetConfig):
                             ),
                         ),
                     ],
-                    snql_metric_layer=lambda args, alias: self._resolve_percentile(
-                        args=args, alias=alias, fixed_percentile=0.99
+                    snql_metric_layer=lambda args, alias: function_aliases.resolve_metrics_layer_percentile(
+                        args=args, alias=alias, resolve_mri=self.resolve_mri, fixed_percentile=0.99
                     ),
                     default_result_type="duration",
                 ),
@@ -727,8 +707,8 @@ class SpansMetricsLayerDatasetConfig(DatasetConfig):
                             ),
                         ),
                     ],
-                    snql_metric_layer=lambda args, alias: self._resolve_percentile(
-                        args=args, alias=alias, fixed_percentile=1
+                    snql_metric_layer=lambda args, alias: function_aliases.resolve_metrics_layer_percentile(
+                        args=args, alias=alias, resolve_mri=self.resolve_mri, fixed_percentile=1.0
                     ),
                     default_result_type="duration",
                 ),
@@ -744,36 +724,3 @@ class SpansMetricsLayerDatasetConfig(DatasetConfig):
     @property
     def orderby_converter(self) -> Mapping[str, OrderBy]:
         return {}
-
-    def _resolve_span_module(self, alias: str) -> SelectType:
-        return field_aliases.resolve_span_module(self.builder, alias)
-
-    # Query Functions
-    def _resolve_percentile(
-        self,
-        args: Mapping[str, Union[str, Column, SelectType, int, float]],
-        alias: str,
-        fixed_percentile: Optional[float] = None,
-    ) -> SelectType:
-        if fixed_percentile is None:
-            fixed_percentile = args["percentile"]
-        if fixed_percentile not in constants.METRIC_PERCENTILES:
-            raise IncompatibleMetricsQuery("Custom quantile incompatible with metrics")
-        column = self.resolve_mri(args["column"])
-        return (
-            Function(
-                "max",
-                [
-                    column,
-                ],
-                alias,
-            )
-            if fixed_percentile == 1
-            else Function(
-                f"p{int(fixed_percentile * 100)}",
-                [
-                    column,
-                ],
-                alias,
-            )
-        )

tests/snuba/api/endpoints/test_organization_events_span_metrics.py

@@ -150,47 +150,57 @@ class OrganizationEventsMetricsEnhancedPerformanceEndpointTest(MetricsEnhancedPe
         assert data[0]["percentile(span.self_time, 0.95)"] == 1
         assert meta["dataset"] == "spansMetrics"
 
-    def test_p50(self):
+    def test_fixed_percentile_functions(self):
         self.store_span_metric(
             1,
             internal_metric=constants.SELF_TIME_LIGHT,
             timestamp=self.min_ago,
         )
-        response = self.do_request(
-            {
-                "field": ["p50()"],
-                "query": "",
-                "project": self.project.id,
-                "dataset": "spansMetrics",
-            }
-        )
-        assert response.status_code == 200, response.content
-        data = response.data["data"]
-        meta = response.data["meta"]
-        assert len(data) == 1
-        assert data[0]["p50()"] == 1
-        assert meta["dataset"] == "spansMetrics"
-
-    def test_p50_with_duration(self):
+        for function in ["p50()", "p75()", "p95()", "p99()", "p100()"]:
+            response = self.do_request(
+                {
+                    "field": [function],
+                    "query": "",
+                    "project": self.project.id,
+                    "dataset": "spansMetrics",
+                }
+            )
+            assert response.status_code == 200, response.content
+            data = response.data["data"]
+            meta = response.data["meta"]
+            assert len(data) == 1
+            assert data[0][function] == 1, function
+            assert meta["dataset"] == "spansMetrics", function
+            assert meta["fields"][function] == "duration", function
+
+    def test_fixed_percentile_functions_with_duration(self):
         self.store_span_metric(
             1,
             internal_metric=constants.SPAN_METRICS_MAP["span.duration"],
             timestamp=self.min_ago,
         )
-        response = self.do_request(
-            {
-                "field": ["p50(span.duration)"],
-                "query": "",
-                "project": self.project.id,
-                "dataset": "spansMetrics",
-            }
-        )
-        assert response.status_code == 200, response.content
-        data = response.data["data"]
-        meta = response.data["meta"]
-        assert len(data) == 1
-        assert data[0]["p50(span.duration)"] == 1
-        assert meta["dataset"] == "spansMetrics"
+        for function in [
+            "p50(span.duration)",
+            "p75(span.duration)",
+            "p95(span.duration)",
+            "p99(span.duration)",
+            "p100(span.duration)",
+        ]:
+            response = self.do_request(
+                {
+                    "field": [function],
+                    "query": "",
+                    "project": self.project.id,
+                    "dataset": "spansMetrics",
+                }
+            )
+            assert response.status_code == 200, response.content
+            data = response.data["data"]
+            meta = response.data["meta"]
+            assert len(data) == 1, function
+            assert data[0][function] == 1, function
+            assert meta["dataset"] == "spansMetrics", function
+            assert meta["fields"][function] == "duration", function
 
     def test_avg(self):
         self.store_span_metric(