fix(ddm): Fix custom metrics querying via mqb (#60000)

Author: Riccardo Busetti

src/sentry/snuba/metrics/datasource.py

@@ -131,7 +131,7 @@ def get_available_derived_metrics(
 
     # Initially, we need all derived metrics to be able to support derived metrics that are not
     # private but might have private constituent metrics
-    all_derived_metrics = get_derived_metrics(exclude_private=False)
+    all_derived_metrics = get_derived_metrics()
 
     for derived_metric_mri, derived_metric_obj in all_derived_metrics.items():
         try:
@@ -159,8 +159,8 @@ def get_available_derived_metrics(
         if single_entity_constituents.issubset(found_derived_metrics):
             found_derived_metrics.add(composite_derived_metric_obj.metric_mri)
 
-    public_derived_metrics = set(get_derived_metrics(exclude_private=True).keys())
-    return found_derived_metrics.intersection(public_derived_metrics)
+    all_derived_metrics = set(get_derived_metrics().keys())
+    return found_derived_metrics.intersection(all_derived_metrics)
 
 
 def get_metrics_meta(projects: Sequence[Project], use_case_id: UseCaseID) -> Sequence[MetricMeta]:
@@ -294,7 +294,7 @@ def _get_metrics_filter_ids(
     org_id = org_id_from_projects(projects)
 
     metric_mris_deque = deque(metric_mris)
-    all_derived_metrics = get_derived_metrics(exclude_private=False)
+    all_derived_metrics = get_derived_metrics()
 
     while metric_mris_deque:
         mri = metric_mris_deque.popleft()
@@ -329,9 +329,9 @@ def _validate_requested_derived_metrics_in_input_metrics(
     SingleEntityDerivedMetric was incorrectly setup with constituent metrics that span multiple
     entities
     """
-    public_derived_metrics = get_derived_metrics(exclude_private=True)
+    all_derived_metrics = get_derived_metrics()
     requested_derived_metrics = {
-        metric_mri for metric_mri in metric_mris if metric_mri in public_derived_metrics
+        metric_mri for metric_mri in metric_mris if metric_mri in all_derived_metrics
     }
     found_derived_metrics = get_available_derived_metrics(
         projects, supported_metric_ids_in_entities, use_case_id
@@ -381,13 +381,6 @@ def _fetch_tags_or_values_for_mri(
     """
     org_id = projects[0].organization_id
 
-    if metric_mris is not None:
-        private_derived_metrics = set(get_derived_metrics(exclude_private=False).keys()) - set(
-            get_derived_metrics(exclude_private=True).keys()
-        )
-        if set(metric_mris).intersection(private_derived_metrics) != set():
-            raise InvalidParams(f"Metric MRIs {metric_mris} do not exist")
-
     try:
         metric_ids = _get_metrics_filter_ids(
             projects=projects, metric_mris=metric_mris, use_case_id=use_case_id
@@ -526,9 +519,9 @@ def get_single_metric_info(
     }
 
     metric_mri = get_mri(metric_name)
-    public_derived_metrics = get_derived_metrics(exclude_private=True)
-    if metric_mri in public_derived_metrics:
-        derived_metric = public_derived_metrics[metric_mri]
+    all_derived_metrics = get_derived_metrics()
+    if metric_mri in all_derived_metrics:
+        derived_metric = all_derived_metrics[metric_mri]
         response_dict.update(
             {
                 "operations": derived_metric.generate_available_operations(),

src/sentry/snuba/metrics/fields/base.py

@@ -77,7 +77,7 @@
     uniq_aggregation_on_metric,
     uniq_if_column_snql,
 )
-from sentry.snuba.metrics.naming_layer.mapping import get_public_name_from_mri, is_private_mri
+from sentry.snuba.metrics.naming_layer.mapping import get_public_name_from_mri
 from sentry.snuba.metrics.naming_layer.mri import SessionMRI, SpanMRI, TransactionMRI
 from sentry.snuba.metrics.utils import (
     DEFAULT_AGGREGATES,
@@ -1896,7 +1896,7 @@ def metric_object_factory(
     # that no private derived metrics are required. The query builder requires access to all
     # derived metrics to be able to compute derived metrics that are not private but might have
     # private constituents
-    derived_metrics = get_derived_metrics(exclude_private=False)
+    derived_metrics = get_derived_metrics()
     if metric_mri in derived_metrics:
         return derived_metrics[metric_mri]
 
@@ -1929,13 +1929,5 @@ def generate_bottom_up_dependency_tree_for_metrics(
     return dependency_list
 
 
-def get_derived_metrics(exclude_private: bool = True) -> Mapping[str, DerivedMetricExpression]:
-    return (
-        {
-            mri: expression
-            for (mri, expression) in DERIVED_METRICS.items()
-            if not is_private_mri(mri)
-        }
-        if exclude_private
-        else DERIVED_METRICS
-    )
+def get_derived_metrics() -> Mapping[str, DerivedMetricExpression]:
+    return DERIVED_METRICS

src/sentry/snuba/metrics/naming_layer/mapping.py

@@ -77,7 +77,10 @@ def get_mri(external_name: Union[Enum, str]) -> str:
 
 
 def get_public_name_from_mri(internal_name: Union[TransactionMRI, SessionMRI, str]) -> str:
-    """Returns the public name from a MRI if it has a mapping to a public metric name, otherwise raise an exception"""
+    """
+    Returns the public name from a MRI if it has a mapping to a public metric name, otherwise return the internal
+    name.
+    """
     if not len(MRI_TO_NAME):
         create_name_mapping_layers()
 
@@ -90,15 +93,13 @@ def get_public_name_from_mri(internal_name: Union[TransactionMRI, SessionMRI, st
     elif (alias := _extract_name_from_custom_metric_mri(internal_name)) is not None:
         return alias
     else:
-        raise InvalidParams(f"Unable to find a mri reverse mapping for '{internal_name}'.")
+        return internal_name
 
 
 def is_private_mri(internal_name: Union[TransactionMRI, SessionMRI, str]) -> bool:
-    try:
-        get_public_name_from_mri(internal_name)
-        return False
-    except InvalidParams:
-        return True
+    public_name = get_public_name_from_mri(internal_name)
+    # If the public name is the same as internal name it means that the internal is "private".
+    return public_name == internal_name
 
 
 def _extract_name_from_custom_metric_mri(mri: str) -> Optional[str]:

src/sentry/snuba/metrics/query.py

@@ -41,7 +41,6 @@ class MetricField:
         Dict[str, Union[None, str, int, float, Sequence[Tuple[Union[str, int], ...]]]]
     ] = None
     alias: Optional[str] = None
-    allow_private: bool = False
 
     def __post_init__(self) -> None:
         # Validate that it is a valid MRI format
@@ -57,9 +56,6 @@ def __post_init__(self) -> None:
 
     @property
     def _metric_name(self) -> str:
-        if self.allow_private:
-            return self.metric_mri
-
         return get_public_name_from_mri(self.metric_mri)
 
     def __str__(self) -> str:
@@ -193,7 +189,7 @@ def _use_case_id(metric_mri: str) -> UseCaseID:
 
     @staticmethod
     def _validate_field(field: MetricField) -> None:
-        derived_metrics_mri = get_derived_metrics(exclude_private=True)
+        all_derived_metrics = get_derived_metrics()
 
         # Validate the validity of the expression meaning that if an operation is present, then it needs to be one of
         # of the supported operations and that the metric mri should be one of the aggregated derived metrics
@@ -202,14 +198,9 @@ def _validate_field(field: MetricField) -> None:
                 raise InvalidParams(
                     f"Invalid operation '{field.op}'. Must be one of {', '.join(OPERATIONS)}"
                 )
-            if field.metric_mri in derived_metrics_mri:
-                metric_name = (
-                    field.metric_mri
-                    if field.allow_private
-                    else get_public_name_from_mri(field.metric_mri)
-                )
+            if field.metric_mri in all_derived_metrics:
                 raise DerivedMetricParseException(
-                    f"Failed to parse {field.op}({metric_name}). No operations can be "
+                    f"Failed to parse {field.op}({get_public_name_from_mri(field.metric_mri)}). No operations can be "
                     f"applied on this field as it is already a derived metric with an "
                     f"aggregation applied to it."
                 )

src/sentry/snuba/metrics/query_builder.py

@@ -110,16 +110,16 @@ def _strip_project_id(condition: Condition) -> Optional[Condition]:
     return None
 
 
-def parse_field(field: str, allow_mri: bool = False, allow_private: bool = False) -> MetricField:
+def parse_field(field: str, allow_mri: bool = False) -> MetricField:
     if allow_mri:
         mri_matches = MRI_SCHEMA_REGEX.match(field) or MRI_EXPRESSION_REGEX.match(field)
         if mri_matches:
-            return parse_mri_field(field, allow_private)
+            return parse_mri_field(field)
 
     return parse_public_field(field)
 
 
-def parse_mri_field(field: str, allow_private: bool = False) -> MetricField:
+def parse_mri_field(field: str) -> MetricField:
     matches = MRI_EXPRESSION_REGEX.match(field)
 
     try:
@@ -129,7 +129,7 @@ def parse_mri_field(field: str, allow_private: bool = False) -> MetricField:
         operation = None
         mri = field
 
-    return MetricField(op=operation, metric_mri=mri, allow_private=allow_private)
+    return MetricField(op=operation, metric_mri=mri)
 
 
 def parse_public_field(field: str) -> MetricField:
@@ -524,7 +524,6 @@ def __init__(
             parse_field(
                 key,
                 allow_mri=allow_mri,
-                allow_private=query_params.get("allowPrivate") == "true",
             )
             for key in query_params.getlist("field", [])
         ]

src/sentry/snuba/metrics_enhanced_performance.py

@@ -148,6 +148,8 @@ def timeseries_query(
         # any remaining errors mean we should try again with discover
         except IncompatibleMetricsQuery as error:
             sentry_sdk.set_tag("performance.mep_incompatible", str(error))
+            # We want to capture this exception to have more visibility in case the query can't be satisfied by metrics.
+            sentry_sdk.capture_exception(error)
             metrics_compatible = False
         except Exception as error:
             raise error

tests/sentry/api/endpoints/test_organization_metric_data.py

@@ -1630,7 +1630,6 @@ def test_gauges(self):
             per_page=3,
             useCase="custom",
             includeSeries="1",
-            allowPrivate="true",
         )
         groups = response.data["groups"]
 
@@ -1639,20 +1638,20 @@ def test_gauges(self):
             {
                 "by": {},
                 "series": {
-                    "count(g:custom/page_load@millisecond)": [2, 3],
-                    "max(g:custom/page_load@millisecond)": [20.0, 21.0],
-                    "min(g:custom/page_load@millisecond)": [1.0, 2.0],
-                    "last(g:custom/page_load@millisecond)": [20.0, 4.0],
-                    "sum(g:custom/page_load@millisecond)": [21.0, 21.0],
-                    "avg(g:custom/page_load@millisecond)": [10.5, 7.0],
+                    "count(page_load)": [2, 3],
+                    "max(page_load)": [20.0, 21.0],
+                    "min(page_load)": [1.0, 2.0],
+                    "last(page_load)": [20.0, 4.0],
+                    "sum(page_load)": [21.0, 21.0],
+                    "avg(page_load)": [10.5, 7.0],
                 },
                 "totals": {
-                    "count(g:custom/page_load@millisecond)": 5,
-                    "max(g:custom/page_load@millisecond)": 21.0,
-                    "min(g:custom/page_load@millisecond)": 1.0,
-                    "last(g:custom/page_load@millisecond)": 4.0,
-                    "sum(g:custom/page_load@millisecond)": 42.0,
-                    "avg(g:custom/page_load@millisecond)": 8.4,
+                    "count(page_load)": 5,
+                    "max(page_load)": 21.0,
+                    "min(page_load)": 1.0,
+                    "last(page_load)": 4.0,
+                    "sum(page_load)": 42.0,
+                    "avg(page_load)": 8.4,
                 },
             }
         ]

tests/sentry/snuba/metrics/test_metrics_layer/test_metrics_enhanced_performance.py

@@ -411,49 +411,6 @@ def test_custom_measurement_query_with_valid_mri(self):
             key=lambda elem: elem["name"],
         )
 
-    def test_custom_measurement_query_with_invalid_mri(self):
-        invalid_mris = [
-            "d:sessions/measurements.speed@millisecond",
-            "s:transactions/measurements.speed@millisecond",
-        ]
-
-        for value, invalid_mri in zip([100, 200], invalid_mris):
-            self.store_performance_metric(
-                name=invalid_mri,
-                tags={},
-                value=value,
-            )
-
-        for invalid_mri in invalid_mris:
-            with pytest.raises(
-                InvalidParams,
-                match=f"Unable to find a mri reverse mapping for '{invalid_mri}'.",
-            ):
-                # We keep the query in order to add more context to the test, even though the actual test
-                # is testing for the '__post_init__' inside 'MetricField'.
-                metrics_query = self.build_metrics_query(
-                    before_now="1h",
-                    granularity="1h",
-                    select=[
-                        MetricField(
-                            op="count",
-                            metric_mri=invalid_mri,
-                        ),
-                    ],
-                    groupby=[],
-                    orderby=[],
-                    limit=Limit(limit=2),
-                    offset=Offset(offset=0),
-                    include_series=False,
-                )
-
-                get_series(
-                    [self.project],
-                    metrics_query=metrics_query,
-                    include_meta=True,
-                    use_case_id=UseCaseID.TRANSACTIONS,
-                )
-
     def test_query_with_order_by_valid_str_field(self):
         project_2 = self.create_project()
         project_3 = self.create_project()

tests/sentry/snuba/metrics/test_naming_layer.py

@@ -2,13 +2,9 @@
 
 import pytest
 
-from sentry.snuba.metrics.naming_layer import create_name_mapping_layers
-from sentry.snuba.metrics.naming_layer.mapping import MRI_TO_NAME, is_private_mri
 from sentry.snuba.metrics.naming_layer.mri import (
     MRI_SCHEMA_REGEX,
     ParsedMRI,
-    SessionMRI,
-    TransactionMRI,
     is_custom_measurement,
     parse_mri,
 )
@@ -144,12 +140,3 @@ def test_parse_mri(name, expected):
 )
 def test_is_custom_measurement(parsed_mri, expected):
     assert is_custom_measurement(parsed_mri) == expected
-
-
-@pytest.mark.parametrize("mri", (list(TransactionMRI) + list(SessionMRI)))
-def test_is_private_mri(mri):
-    create_name_mapping_layers()
-
-    public_mris = set(MRI_TO_NAME.keys())
-    expected_private = False if mri.value in public_mris else True
-    assert is_private_mri(mri) == expected_private

tests/sentry/snuba/metrics/test_query.py

@@ -570,27 +570,6 @@ def test_validate_metric_field_mri():
         )
 
 
-@pytest.mark.parametrize(
-    "alias",
-    [
-        pytest.param(
-            None,
-            id="No alias provided",
-        ),
-        pytest.param(
-            "ahmed_alias",
-            id="alias is provided",
-        ),
-    ],
-)
-def test_validate_metric_field_mri_is_public(alias):
-    with pytest.raises(
-        InvalidParams,
-        match=f"Unable to find a mri reverse mapping for '{SessionMRI.ERRORED_ALL.value}'.",
-    ):
-        MetricField(op=None, metric_mri=SessionMRI.ERRORED_ALL.value, alias=alias)
-
-
 @pytest.mark.parametrize(
     "select, interval, series",
     [