fix(metrics): Allow mris in metric tags endpoint (#57004)

src/sentry/api/endpoints/organization_metrics.py

@@ -13,11 +13,11 @@ from sentry.sentry_metrics.use_case_id_registry import UseCaseID
 from sentry.sentry_metrics.utils import string_to_use_case_id
 from sentry.snuba.metrics import (
     QueryDefinition,
+    get_all_tags,
     get_metrics_meta,
     get_series,
     get_single_metric_info,
     get_tag_values,
-    get_tags,
 )
 from sentry.snuba.metrics.utils import DerivedMetricException, DerivedMetricParseException
 from sentry.snuba.sessions_v2 import InvalidField
@@ -66,8 +66,8 @@ class OrganizationMetricDetailsEndpoint(OrganizationEndpoint):
     owner = ApiOwner.TELEMETRY_EXPERIENCE
 
     def get(self, request: Request, organization, metric_name) -> Response:
-
         projects = self.get_projects(request, organization)
+
         try:
             metric = get_single_metric_info(
                 projects,
@@ -94,19 +94,18 @@ class OrganizationMetricsTagsEndpoint(OrganizationEndpoint):
 
     If the ``metric`` query param is provided more than once, the *intersection*
     of available tags is used.
-
     """
 
     owner = ApiOwner.TELEMETRY_EXPERIENCE
 
     def get(self, request: Request, organization) -> Response:
-
-        metrics = request.GET.getlist("metric") or []
+        metric_names = request.GET.getlist("metric") or []
         projects = self.get_projects(request, organization)
+
         try:
-            tags = get_tags(
+            tags = get_all_tags(
                 projects,
-                metrics,
+                metric_names,
                 use_case_id=get_use_case_id(request),
             )
         except (InvalidParams, DerivedMetricParseException) as exc:
@@ -125,10 +124,9 @@ class OrganizationMetricsTagDetailsEndpoint(OrganizationEndpoint):
     owner = ApiOwner.TELEMETRY_EXPERIENCE
 
     def get(self, request: Request, organization, tag_name) -> Response:
-
         metric_names = request.GET.getlist("metric") or None
-
         projects = self.get_projects(request, organization)
+
         try:
             tag_values = get_tag_values(
                 projects,
@@ -137,12 +135,7 @@ class OrganizationMetricsTagDetailsEndpoint(OrganizationEndpoint):
                 use_case_id=get_use_case_id(request),
             )
         except (InvalidParams, DerivedMetricParseException) as exc:
-            msg = str(exc)
-            # TODO: Use separate error type once we have real data
-            if "Unknown tag" in msg:
-                raise ResourceDoesNotExist(f"tag '{tag_name}'")
-            else:
-                raise ParseError(msg)
+            raise ParseError(str(exc))
 
         return Response(tag_values, status=200)
 

src/sentry/snuba/metrics/datasource.py

@@ -8,7 +8,13 @@ until we have a proper metadata store set up. To keep things simple, and hopeful
 efficient, we only look at the past 24 hours.
 """
 
-__all__ = ("get_metrics_meta", "get_tags", "get_tag_values", "get_series", "get_single_metric_info")
+__all__ = (
+    "get_metrics_meta",
+    "get_all_tags",
+    "get_tag_values",
+    "get_series",
+    "get_single_metric_info",
+)
 
 import logging
 from collections import defaultdict, deque
@@ -38,9 +44,8 @@ from sentry.snuba.metrics.fields.base import (
     get_derived_metrics,
     org_id_from_projects,
 )
-from sentry.snuba.metrics.naming_layer.mapping import get_mri
+from sentry.snuba.metrics.naming_layer.mapping import get_mri, is_mri
 from sentry.snuba.metrics.naming_layer.mri import (
-    MRI_SCHEMA_REGEX,
     get_available_operations,
     is_custom_measurement,
     parse_mri,
@@ -313,9 +318,16 @@ def _fetch_tags_or_values_for_metrics(
     column: str,
     use_case_id: UseCaseID,
 ) -> Tuple[Union[Sequence[Tag], Sequence[TagValue]], Optional[str]]:
-    assert len({p.organization_id for p in projects}) == 1
-
-    metric_mris = [get_mri(metric_name) for metric_name in metric_names] if metric_names else []
+    metric_mris = []
+
+    # For now this function supports all MRIs but only the usage of public names for static MRIs. In case
+    # there will be the need, the support for custom metrics MRIs will have to be added but with additional
+    # complexity.
+    for metric_name in metric_names or ():
+        if is_mri(metric_name):
+            metric_mris.append(metric_name)
+        else:
+            metric_mris.append(get_mri(metric_name))
 
     return _fetch_tags_or_values_for_mri(projects, metric_mris, referrer, column, use_case_id)
 
@@ -369,7 +381,6 @@ def _fetch_tags_or_values_for_mri(
         metric_types = performance_metric_types
 
     for metric_type in metric_types:
-
         entity_key = METRIC_TYPE_TO_ENTITY[metric_type]
         rows = run_metrics_query(
             entity_key=entity_key,
@@ -495,31 +506,23 @@ def get_single_metric_info(
     return response_dict
 
 
-def get_tags(
-    projects: Sequence[Project], metrics: Optional[Sequence[str]], use_case_id: UseCaseID
+def get_all_tags(
+    projects: Sequence[Project], metric_names: Optional[Sequence[str]], use_case_id: UseCaseID
 ) -> Sequence[Tag]:
-    """Get all metric tags for the given projects and metric_names"""
+    """Get all metric tags for the given projects and metric_names."""
     assert projects
 
     try:
-        if len(metrics) and all(MRI_SCHEMA_REGEX.match(metric) for metric in metrics):
-            tags, _ = _fetch_tags_or_values_for_mri(
-                projects=projects,
-                metric_mris=metrics,
-                column="tags.key",
-                referrer="snuba.metrics.meta.get_tags",
-                use_case_id=use_case_id,
-            )
-        else:
-            tags, _ = _fetch_tags_or_values_for_metrics(
-                projects=projects,
-                metric_names=metrics,
-                column="tags.key",
-                referrer="snuba.metrics.meta.get_tags",
-                use_case_id=use_case_id,
-            )
+        tags, _ = _fetch_tags_or_values_for_metrics(
+            projects=projects,
+            metric_names=metric_names,
+            column="tags.key",
+            referrer="snuba.metrics.meta.get_tags",
+            use_case_id=use_case_id,
+        )
     except InvalidParams:
         return []
+
     return tags
 
 
@@ -529,15 +532,14 @@ def get_tag_values(
     metric_names: Optional[Sequence[str]],
     use_case_id: UseCaseID,
 ) -> Sequence[TagValue]:
-    """Get all known values for a specific tag"""
+    """Get all known values for a specific tag for the given projects and metric_names."""
     assert projects
 
-    org_id = org_id_from_projects(projects)
-
     if tag_name in UNALLOWED_TAGS:
         raise InvalidParams(f"Tag name {tag_name} is an unallowed tag")
 
     try:
+        org_id = org_id_from_projects(projects)
         tag_id = resolve_tag_key(use_case_id, org_id, tag_name)
     except MetricIndexNotFound:
         raise InvalidParams(f"Tag {tag_name} is not available in the indexer")
@@ -552,6 +554,7 @@ def get_tag_values(
         )
     except InvalidParams:
         return []
+
     return tags
 
 

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

@@ -53,20 +53,23 @@ NAME_TO_MRI: Dict[str, Enum] = {}
 MRI_TO_NAME: Dict[str, str] = {}
 
 
+def is_mri(value: str) -> bool:
+    return MRI_SCHEMA_REGEX.match(value) is not None
+
+
 def get_mri(external_name: Union[Enum, str]) -> str:
     if not len(NAME_TO_MRI):
         create_name_mapping_layers()
 
     if isinstance(external_name, Enum):
         external_name = external_name.value
-    assert isinstance(external_name, str)
 
     try:
+        assert isinstance(external_name, str)
         return cast(str, NAME_TO_MRI[external_name].value)
     except KeyError:
         raise InvalidParams(
-            f"Failed to parse '{external_name}'. Must be something like 'sum(my_metric)', "
-            f"or a supported aggregate derived metric like `session.crash_free_rate`"
+            f"Failed to parse '{external_name}'. The metric name must belong to a public metric."
         )
 
 
@@ -81,7 +84,7 @@ def get_public_name_from_mri(internal_name: Union[TransactionMRI, SessionMRI, st
 
     if internal_name in MRI_TO_NAME:
         return MRI_TO_NAME[internal_name]
-    elif (alias := extract_custom_metric_alias(internal_name)) is not None:
+    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}'.")
@@ -95,19 +98,24 @@ def is_private_mri(internal_name: Union[TransactionMRI, SessionMRI, str]) -> boo
         return True
 
 
-def extract_custom_metric_alias(internal_name: str) -> Optional[str]:
-    match = MRI_SCHEMA_REGEX.match(internal_name)
-    if (
-        match is not None
-        and match.group("entity") == "d"
-        and match.group("namespace") == "transactions"
-    ):
-        return match.group("name")
-    elif match is not None and match.group("namespace") == "custom":
-        return match.group("name")
-    else:
+def _extract_name_from_custom_metric_mri(mri: str) -> Optional[str]:
+    match = MRI_SCHEMA_REGEX.match(mri)
+    if match is None:
         return None
 
+    entity = match.group("entity")
+    namespace = match.group("namespace")
+
+    # Custom metrics are fully custom metrics that the sdks can emit.
+    is_custom_metric = namespace == "custom"
+    # Custom measurements are a special kind of custom metrics that are more limited and were existing
+    # before fully custom metrics.
+    is_custom_measurement = entity == "d" and namespace == "transactions"
+    if is_custom_metric or is_custom_measurement:
+        return match.group("name")
+
+    return None
+
 
 def get_operation_with_public_name(operation: Optional[str], metric_mri: str) -> str:
     return (

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

@@ -40,7 +40,7 @@ ENTITY_TYPE_REGEX = r"(c|s|d|g|e)"
 MRI_NAME_REGEX = r"([a-z_]+(?:\.[a-z_]+)*)"
 # ToDo(ahmed): Add a better regex for unit portion for MRI
 MRI_SCHEMA_REGEX_STRING = rf"(?P<entity>{ENTITY_TYPE_REGEX}):(?P<namespace>{NAMESPACE_REGEX})/(?P<name>{MRI_NAME_REGEX})@(?P<unit>[\w.]*)"
-MRI_SCHEMA_REGEX = re.compile(MRI_SCHEMA_REGEX_STRING)
+MRI_SCHEMA_REGEX = re.compile(rf"^{MRI_SCHEMA_REGEX_STRING}$")
 MRI_EXPRESSION_REGEX = re.compile(rf"^{OP_REGEX}\(({MRI_SCHEMA_REGEX_STRING})\)$")
 
 

tests/sentry/api/endpoints/test_organization_metric_data.py

@@ -1574,8 +1574,7 @@ class DerivedMetricsDataTest(MetricsAPIBaseTestCase):
         )
         assert response.status_code == 400
         assert response.json()["detail"] == (
-            "Failed to parse 'crash_free_fake'. Must be something like 'sum(my_metric)', "
-            "or a supported aggregate derived metric like `session.crash_free_rate`"
+            "Failed to parse 'crash_free_fake'. The metric name must belong to a public metric."
         )
 
     def test_crash_free_percentage(self):
@@ -2071,8 +2070,7 @@ class DerivedMetricsDataTest(MetricsAPIBaseTestCase):
         )
 
         assert response.data["detail"] == (
-            "Failed to parse 'transaction.all'. Must be something like 'sum(my_metric)', "
-            "or a supported aggregate derived metric like `session.crash_free_rate`"
+            "Failed to parse 'transaction.all'. The metric name must belong to a public metric."
         )
 
     def test_failure_rate_transaction(self):
@@ -2152,8 +2150,7 @@ class DerivedMetricsDataTest(MetricsAPIBaseTestCase):
                 interval="6m",
             )
             assert response.data["detail"] == (
-                f"Failed to parse '{private_name}'. Must be something like 'sum(my_metric)', "
-                "or a supported aggregate derived metric like `session.crash_free_rate`"
+                f"Failed to parse '{private_name}'. The metric name must belong to a public metric."
             )
 
     def test_apdex_transactions(self):

tests/sentry/snuba/metrics/test_datasource.py

@@ -0,0 +1,61 @@
+import pytest
+
+from sentry.sentry_metrics.use_case_id_registry import UseCaseID
+from sentry.snuba.metrics import get_tag_values
+from sentry.snuba.metrics.naming_layer import TransactionMetricKey, TransactionMRI
+from sentry.testutils.cases import BaseMetricsLayerTestCase, TestCase
+from sentry.testutils.helpers.datetime import freeze_time
+from sentry.testutils.skips import requires_snuba
+
+pytestmark = [pytest.mark.sentry_metrics, requires_snuba]
+
+
+@pytest.mark.snuba_ci
+@freeze_time(BaseMetricsLayerTestCase.MOCK_DATETIME)
+class DatasourceTestCase(BaseMetricsLayerTestCase, TestCase):
+    @property
+    def now(self):
+        return BaseMetricsLayerTestCase.MOCK_DATETIME
+
+    def test_get_tag_values_with_mri(self):
+        releases = ["1.0", "2.0"]
+        for release in ("1.0", "2.0"):
+            self.store_performance_metric(
+                name=TransactionMRI.DURATION.value,
+                tags={"release": release},
+                value=1,
+            )
+
+        values = get_tag_values(
+            [self.project], "release", [TransactionMRI.DURATION.value], UseCaseID.TRANSACTIONS
+        )
+        for release in releases:
+            assert {"key": "release", "value": release} in values
+
+    def test_get_tag_values_with_public_name(self):
+        satisfactions = ["miserable", "satisfied", "tolerable"]
+        for satisfaction in satisfactions:
+            self.store_performance_metric(
+                name=TransactionMRI.MEASUREMENTS_LCP.value,
+                tags={"satisfaction": satisfaction},
+                value=1,
+            )
+
+        # Valid public metric name.
+        values = get_tag_values(
+            [self.project],
+            "satisfaction",
+            [TransactionMetricKey.MEASUREMENTS_LCP.value],
+            UseCaseID.TRANSACTIONS,
+        )
+        for satisfaction in satisfactions:
+            assert {"key": "satisfaction", "value": satisfaction} in values
+
+        # Invalid public metric name.
+        values = get_tag_values(
+            [self.project],
+            "satisfaction",
+            ["transaction.measurements"],
+            UseCaseID.TRANSACTIONS,
+        )
+        assert values == []