Browse Source

feat(metrics-layer): Add Results Converter in metrics layer (#62689)

### Overview
This PR is responsible for converting the snuba result of a metrics
(release-health) query back to its original tag values.
For example, the snuba result:

```
[{'aggregate_value': 1, 'release': '1003', 'time': '2024-01-05T17:02:00+00:00'}, 
{'aggregate_value': 1, 'release': '1003', 'time': '2024-01-05T17:04:00+00:00'}
```
should be 
```
[{'aggregate_value': 1, 'release': 'some_real_release_name', 'time': '2024-01-05T17:02:00+00:00'}, 
{'aggregate_value': 1, 'release': 'some_real_release_name', 'time': '2024-01-05T17:04:00+00:00'}
```


To accomplish this, a `ReverseMappings` class was added to keep track of
which tag_keys and tag_values need to be reverse resolved during the
initial query resolution. By saving this state, we can avoid attempting
to resolve non-tag columns (project_id, etc.). Additionally, we can
reduce the times we need to call the indexer when reverse resolving the
result. Finally, a `convert_snuba_result` was added to handle the post
processing of the snuba result.
Enoch Tang 1 year ago
parent
commit
bea8d3af42

+ 153 - 48
src/sentry/snuba/metrics_layer/query.py

@@ -23,7 +23,7 @@ from snuba_sdk.formula import FormulaParameterGroup
 from sentry import options
 from sentry.exceptions import InvalidParams
 from sentry.sentry_metrics.use_case_id_registry import UseCaseID
-from sentry.sentry_metrics.utils import resolve_weak, string_to_use_case_id
+from sentry.sentry_metrics.utils import resolve_weak, reverse_resolve_weak, string_to_use_case_id
 from sentry.snuba.dataset import Dataset, EntityKey
 from sentry.snuba.metrics.naming_layer.mapping import get_mri
 from sentry.snuba.metrics.naming_layer.mri import parse_mri
@@ -50,6 +50,34 @@ AGGREGATE_ALIASES = {
     "count_unique": ("uniq", None),
 }
 
+RELEASE_HEALTH_ENTITIES = {
+    "c": EntityKey.MetricsCounters,
+    "d": EntityKey.MetricsDistributions,
+    "s": EntityKey.MetricsSets,
+}
+
+GENERIC_ENTITIES = {
+    "c": EntityKey.GenericMetricsCounters,
+    "d": EntityKey.GenericMetricsDistributions,
+    "s": EntityKey.GenericMetricsSets,
+    "g": EntityKey.GenericMetricsGauges,
+}
+
+
+class ReverseMappings:
+    """
+    Used to keep track of which tag values need to be reverse resolved in the result for
+    metrics (release health) queries. Stores a set of tag_keys in which the tag values need
+    to be reverse resolved. Only tag keys which are resolved will be included in this set.
+    Therefore, columns like project_id is included. Reverse_mappings saves a dictionary of resolved integers
+    to their original string values when they are first resolved before query execution.
+    Groupby columns values will still need to access the indexer as those reverse mappings will not be present here.
+    """
+
+    def __init__(self) -> None:
+        self.tag_keys: set[str] = set()
+        self.reverse_mappings: dict[int, str] = dict()
+
 
 def run_query(request: Request) -> Mapping[str, Any]:
     """
@@ -179,7 +207,7 @@ def mql_query(request: Request, start: datetime, end: datetime) -> Mapping[str,
             request.dataset = Dataset.Metrics.value
         else:
             request.dataset = Dataset.PerformanceMetrics.value
-        indexer_mappings = _lookup_indexer_resolve(metrics_query, request.dataset)
+        indexer_mappings, reverse_mappings = _lookup_indexer_resolve(metrics_query, request.dataset)
         mappings.update(indexer_mappings)
         request.query = metrics_query.set_indexer_mappings(mappings)
         request.tenant_ids["use_case_id"] = metrics_query.scope.use_case_id
@@ -204,7 +232,14 @@ def mql_query(request: Request, start: datetime, end: datetime) -> Mapping[str,
         )
         raise
 
-    # TODO: Right now, if the query is release health, the tag values in the results are left unresolved. We need to fix this.
+    snuba_result = convert_snuba_result(
+        snuba_result,
+        reverse_mappings,
+        request.dataset,
+        metrics_query.scope.use_case_id,
+        metrics_query.scope.org_ids[0],
+    )
+
     # If we normalized the start/end, return those values in the response so the caller is aware
     results = {
         **snuba_result,
@@ -343,51 +378,54 @@ def _resolve_metrics_entity(mri: str) -> EntityKey:
     return GENERIC_ENTITIES[parsed_mri.entity]
 
 
-RELEASE_HEALTH_ENTITIES = {
-    "c": EntityKey.MetricsCounters,
-    "d": EntityKey.MetricsDistributions,
-    "s": EntityKey.MetricsSets,
-}
-
-GENERIC_ENTITIES = {
-    "c": EntityKey.GenericMetricsCounters,
-    "d": EntityKey.GenericMetricsDistributions,
-    "s": EntityKey.GenericMetricsSets,
-    "g": EntityKey.GenericMetricsGauges,
-}
-
-
-def _lookup_indexer_resolve(metrics_query: MetricsQuery, dataset: str) -> Mapping[str, str | int]:
+def _lookup_indexer_resolve(
+    metrics_query: MetricsQuery, dataset: str
+) -> tuple[Mapping[str, str | int], ReverseMappings]:
     """
     Returns an updated metrics query with all the indexer resolves complete. Also returns a mapping
     that shows all the strings that were resolved and what they were resolved too.
     """
+    reverse_mappings = ReverseMappings()
     org_id = metrics_query.scope.org_ids[0]
     use_case_id = string_to_use_case_id(metrics_query.scope.use_case_id)
-    return _lookup_indexer_resolve_exp(metrics_query.query, org_id, use_case_id, dataset)
+    indexer_mappings = _lookup_indexer_resolve_exp(
+        metrics_query.query, org_id, use_case_id, dataset, reverse_mappings
+    )
+    return indexer_mappings, reverse_mappings
 
 
 def _lookup_indexer_resolve_exp(
-    exp: Formula | Timeseries, org_id: int, use_case_id: UseCaseID, dataset: str
+    exp: Formula | Timeseries,
+    org_id: int,
+    use_case_id: UseCaseID,
+    dataset: str,
+    reverse_mappings: ReverseMappings,
 ) -> Mapping[str, str | int]:
     indexer_mappings: dict[str, str | int] = {}
-    new_mappings = _lookup_resolve_groupby(exp.groupby, use_case_id, org_id)
+    new_mappings = _lookup_resolve_groupby(exp.groupby, use_case_id, org_id, reverse_mappings)
     indexer_mappings.update(new_mappings)
-    new_mappings = _lookup_resolve_filters(exp.filters, use_case_id, org_id, dataset)
+    new_mappings = _lookup_resolve_filters(
+        exp.filters, use_case_id, org_id, dataset, reverse_mappings
+    )
     indexer_mappings.update(new_mappings)
 
     if isinstance(exp, Formula):
         parameters = exp.parameters
         for i, p in enumerate(parameters):
             if isinstance(p, (Formula, Timeseries)):
-                new_mappings = _lookup_indexer_resolve_exp(p, org_id, use_case_id, dataset)
+                new_mappings = _lookup_indexer_resolve_exp(
+                    p, org_id, use_case_id, dataset, reverse_mappings
+                )
                 indexer_mappings.update(new_mappings)
 
     return indexer_mappings
 
 
 def _lookup_resolve_groupby(
-    groupby: list[Column] | None, use_case_id: UseCaseID, org_id: int
+    groupby: list[Column] | None,
+    use_case_id: UseCaseID,
+    org_id: int,
+    reverse_mappings: ReverseMappings,
 ) -> Mapping[str, str | int]:
     """
     Go through the groupby columns and resolve any that need to be resolved.
@@ -402,12 +440,17 @@ def _lookup_resolve_groupby(
         resolved = resolve_weak(use_case_id, org_id, col.name)
         if resolved > -1:
             mappings[col.name] = resolved
+            reverse_mappings.tag_keys.add(col.name)
 
     return mappings
 
 
 def _lookup_resolve_filters(
-    filters: list[Condition | BooleanCondition], use_case_id: UseCaseID, org_id: int, dataset: str
+    filters: list[Condition | BooleanCondition],
+    use_case_id: UseCaseID,
+    org_id: int,
+    dataset: str,
+    reverse_mappings: ReverseMappings,
 ) -> Mapping[str, str | int]:
     """
     Go through the columns in the filter and resolve any that can be resolved.
@@ -419,41 +462,76 @@ def _lookup_resolve_filters(
 
     mappings = {}
 
-    def lookup_resolve_exp(exp: FilterTypes, dataset: str) -> None:
+    def lookup_resolve_exp(
+        exp: FilterTypes, dataset: str, reverse_mappings: ReverseMappings
+    ) -> None:
         if dataset == Dataset.Metrics.value and (isinstance(exp, str) or isinstance(exp, list)):
             if isinstance(exp, str):
                 resolved = resolve_weak(use_case_id, org_id, exp)
                 if resolved > -1:
                     mappings[exp] = resolved
+                    reverse_mappings.reverse_mappings[resolved] = exp
             elif isinstance(exp, list):
                 for value in exp:
                     assert isinstance(value, str)
                     resolved = resolve_weak(use_case_id, org_id, value)
                     if resolved > -1:
                         mappings[value] = resolved
+                        reverse_mappings.reverse_mappings[resolved] = value
             else:
                 raise InvalidParams("Invalid filter tag value type")
         elif isinstance(exp, Column):
             resolved = resolve_weak(use_case_id, org_id, exp.name)
             if resolved > -1:
                 mappings[exp.name] = resolved
+                if dataset == Dataset.Metrics.value:
+                    reverse_mappings.tag_keys.add(exp.name)
         elif isinstance(exp, CurriedFunction):
             for p in exp.parameters:
-                lookup_resolve_exp(p, dataset)
+                lookup_resolve_exp(p, dataset, reverse_mappings)
         elif isinstance(exp, BooleanCondition):
             for c in exp.conditions:
-                lookup_resolve_exp(c, dataset)
+                lookup_resolve_exp(c, dataset, reverse_mappings)
         elif isinstance(exp, Condition):
-            lookup_resolve_exp(exp.lhs, dataset)
-            # If the dataset is metrics, then we need to resolve the tag values as well
+            lookup_resolve_exp(exp.lhs, dataset, reverse_mappings)
+            # If the dataset is metrics, then we need to resolve the RHS tag values as well
             if dataset == Dataset.Metrics.value:
-                lookup_resolve_exp(exp.rhs, dataset)
+                lookup_resolve_exp(exp.rhs, dataset, reverse_mappings)
 
     for exp in filters:
-        lookup_resolve_exp(exp, dataset)
+        lookup_resolve_exp(exp, dataset, reverse_mappings)
     return mappings
 
 
+def convert_snuba_result(
+    snuba_result: Any,
+    reverse_mappings: ReverseMappings,
+    dataset: str,
+    use_case_id_str: str,
+    org_id: int,
+):
+    """
+    If the dataset is metrics (release-health), then we need to convert the resultant tag values from
+    their resolved integers back into their original strings.
+    """
+    if dataset == Dataset.PerformanceMetrics.value:
+        return snuba_result
+    for data_point in snuba_result["data"]:
+        for key in data_point:
+            if key in reverse_mappings.tag_keys:
+                if data_point[key] in reverse_mappings.reverse_mappings:
+                    data_point[key] = reverse_mappings.reverse_mappings[data_point[key]]
+                else:
+                    # Reverse mapping was not saved in initial resolve, this means column was only specfied in groupby.
+                    # We need to manually do a reverse resolve here.
+                    reverse_resolve = reverse_resolve_weak(
+                        string_to_use_case_id(use_case_id_str), org_id, int(data_point[key])
+                    )
+                    if reverse_resolve:
+                        data_point[key] = reverse_resolve
+    return snuba_result
+
+
 ####################
 # TO BE DEPRECATED #
 ####################
@@ -468,7 +546,9 @@ def snql_query(request: Request, start: datetime, end: datetime) -> Mapping[str,
     try:
         # Replace any aggregate aliases with the appropriate aggregate
         metrics_query = metrics_query.set_query(_resolve_aggregate_aliases(metrics_query.query))
-        resolved_metrics_query, mappings = _resolve_metrics_query(metrics_query, request.dataset)
+        resolved_metrics_query, mappings, reverse_mappings = _resolve_metrics_query(
+            metrics_query, request.dataset
+        )
         request.query = resolved_metrics_query.set_indexer_mappings(mappings)
         request.tenant_ids["use_case_id"] = resolved_metrics_query.scope.use_case_id
         # Release health AKA sessions uses a separate Dataset. Change the dataset based on the use case id.
@@ -495,7 +575,13 @@ def snql_query(request: Request, start: datetime, end: datetime) -> Mapping[str,
         )
         raise
 
-    # TODO: Right now, if the query is release health, the tag values in the results are left unresolved. We need to fix this.
+    snuba_results = convert_snuba_result(
+        snuba_results,
+        reverse_mappings,
+        request.dataset,
+        metrics_query.scope.use_case_id,
+        metrics_query.scope.org_ids[0],
+    )
 
     # If we normalized the start/end, return those values in the response so the caller is aware
     results = {
@@ -577,7 +663,7 @@ def _resolve_formula_metrics(
 
 def _resolve_metrics_query(
     metrics_query: MetricsQuery, dataset: str
-) -> tuple[MetricsQuery, Mapping[str, str | int]]:
+) -> tuple[MetricsQuery, Mapping[str, str | int], ReverseMappings]:
     """
     Returns an updated metrics query with all the indexer resolves complete. Also returns a mapping
     that shows all the strings that were resolved and what they were resolved too.
@@ -592,6 +678,7 @@ def _resolve_metrics_query(
 
     use_case_id = string_to_use_case_id(use_case_id_str)
     metrics_query, mappings = _resolve_query_metrics(metrics_query, use_case_id, org_id)
+    reverse_mappings = ReverseMappings()
 
     # Release health AKA sessions uses a separate Dataset. Change the dataset based on the use case id.
     # This is necessary here because the product code that uses this isn't aware of which feature is
@@ -602,7 +689,7 @@ def _resolve_metrics_query(
         dataset = Dataset.PerformanceMetrics.value
 
     new_groupby, new_mappings = _resolve_groupby(
-        metrics_query.query.groupby, use_case_id, org_id, dataset
+        metrics_query.query.groupby, use_case_id, org_id, dataset, reverse_mappings
     )
     metrics_query = metrics_query.set_query(metrics_query.query.set_groupby(new_groupby))
     mappings.update(new_mappings)
@@ -612,7 +699,7 @@ def _resolve_metrics_query(
         for i, p in enumerate(parameters):
             if isinstance(p, Timeseries):
                 new_groupby, new_mappings = _resolve_groupby(
-                    p.groupby, use_case_id, org_id, dataset
+                    p.groupby, use_case_id, org_id, dataset, reverse_mappings
                 )
                 parameters[i] = p.set_groupby(new_groupby)
                 mappings.update(new_mappings)
@@ -620,7 +707,7 @@ def _resolve_metrics_query(
         metrics_query = metrics_query.set_query(metrics_query.query.set_parameters(parameters))
 
     new_filters, new_mappings = _resolve_filters(
-        metrics_query.query.filters, use_case_id, org_id, dataset
+        metrics_query.query.filters, use_case_id, org_id, dataset, reverse_mappings
     )
     metrics_query = metrics_query.set_query(metrics_query.query.set_filters(new_filters))
     mappings.update(new_mappings)
@@ -630,18 +717,22 @@ def _resolve_metrics_query(
         for i, p in enumerate(parameters):
             if isinstance(p, Timeseries):
                 new_filters, new_mappings = _resolve_filters(
-                    p.filters, use_case_id, org_id, dataset
+                    p.filters, use_case_id, org_id, dataset, reverse_mappings
                 )
                 parameters[i] = p.set_filters(new_filters)
                 mappings.update(new_mappings)
 
         metrics_query = metrics_query.set_query(metrics_query.query.set_parameters(parameters))
 
-    return metrics_query, mappings
+    return metrics_query, mappings, reverse_mappings
 
 
 def _resolve_groupby(
-    groupby: list[Column] | None, use_case_id: UseCaseID, org_id: int, dataset: str
+    groupby: list[Column] | None,
+    use_case_id: UseCaseID,
+    org_id: int,
+    dataset: str,
+    reverse_mappings: ReverseMappings,
 ) -> tuple[list[Column] | None, Mapping[str, int]]:
     """
     Go through the groupby columns and resolve any that need to be resolved.
@@ -660,6 +751,7 @@ def _resolve_groupby(
                 new_groupby.append(
                     AliasedExpression(exp=replace(col, name=f"tags[{resolved}]"), alias=col.name)
                 )
+                reverse_mappings.tag_keys.add(col.name)
             else:
                 new_groupby.append(
                     AliasedExpression(
@@ -674,7 +766,11 @@ def _resolve_groupby(
 
 
 def _resolve_filters(
-    filters: list[Condition | BooleanCondition], use_case_id: UseCaseID, org_id: int, dataset: str
+    filters: list[Condition | BooleanCondition],
+    use_case_id: UseCaseID,
+    org_id: int,
+    dataset: str,
+    reverse_mappings: ReverseMappings,
 ) -> tuple[list[Condition | BooleanCondition] | None, Mapping[str, int]]:
     """
     Go through the columns in the filter and resolve any that can be resolved.
@@ -686,12 +782,15 @@ def _resolve_filters(
 
     mappings = {}
 
-    def resolve_exp(exp: FilterTypes, dataset: str) -> FilterTypes:
+    def resolve_exp(
+        exp: FilterTypes, dataset: str, reverse_mappings: ReverseMappings
+    ) -> FilterTypes:
         if dataset == Dataset.Metrics.value and (isinstance(exp, str) or isinstance(exp, list)):
             if isinstance(exp, str):
                 resolved = resolve_weak(use_case_id, org_id, exp)
                 if resolved > -1:
                     mappings[exp] = resolved
+                    reverse_mappings.reverse_mappings[resolved] = exp
                     return resolved
             elif isinstance(exp, list):
                 resolved_values: list[int] = []
@@ -701,6 +800,7 @@ def _resolve_filters(
                     if resolved > -1:
                         resolved_values.append(resolved)
                         mappings[value] = resolved
+                        reverse_mappings.reverse_mappings[resolved] = value
                     return resolved_values
             else:
                 raise InvalidParams("Invalid filter tag value type")
@@ -709,20 +809,25 @@ def _resolve_filters(
             if resolved > -1:
                 mappings[exp.name] = resolved
                 if dataset == Dataset.Metrics.value:
+                    reverse_mappings.tag_keys.add(exp.name)
                     return replace(exp, name=f"tags[{resolved}]")
                 else:
                     return replace(exp, name=f"tags_raw[{resolved}]")
         elif isinstance(exp, CurriedFunction):
-            return replace(exp, parameters=[resolve_exp(p, dataset) for p in exp.parameters])
+            return replace(
+                exp, parameters=[resolve_exp(p, dataset, reverse_mappings) for p in exp.parameters]
+            )
         elif isinstance(exp, BooleanCondition):
-            return replace(exp, conditions=[resolve_exp(c, dataset) for c in exp.conditions])
+            return replace(
+                exp, conditions=[resolve_exp(c, dataset, reverse_mappings) for c in exp.conditions]
+            )
         elif isinstance(exp, Condition):
-            exp = replace(exp, lhs=resolve_exp(exp.lhs, dataset))
+            exp = replace(exp, lhs=resolve_exp(exp.lhs, dataset, reverse_mappings))
             # If the dataset is metrics, then we need to resolve the tag values as well
             if dataset == Dataset.Metrics.value:
-                exp = replace(exp, rhs=resolve_exp(exp.rhs, dataset))
+                exp = replace(exp, rhs=resolve_exp(exp.rhs, dataset, reverse_mappings))
             return exp
         return exp
 
-    new_filters = [resolve_exp(exp, dataset) for exp in filters]
+    new_filters = [resolve_exp(exp, dataset, reverse_mappings) for exp in filters]
     return new_filters, mappings

+ 0 - 1
tests/sentry/sentry_metrics/querying/test_api.py

@@ -637,7 +637,6 @@ class MetricsAPITestCase(TestCase, BaseMetricsTestCase):
                 referrer="metrics.data.api",
             )
 
-    @pytest.mark.skip(reason="sessions are not supported in the new metrics layer")
     def test_with_sessions(self) -> None:
         self.store_session(
             self.build_session(

+ 101 - 7
tests/sentry/snuba/metrics/test_metrics_query_layer/test_metrics_query_layer.py

@@ -21,7 +21,7 @@ from snuba_sdk import (
 
 from sentry.sentry_metrics import indexer
 from sentry.sentry_metrics.use_case_id_registry import UseCaseID
-from sentry.snuba.metrics.naming_layer import TransactionMRI
+from sentry.snuba.metrics.naming_layer import SessionMRI, TransactionMRI
 from sentry.snuba.metrics_layer.query import _resolve_granularity, _resolve_metrics_query
 from sentry.testutils.cases import BaseMetricsLayerTestCase, TestCase
 from sentry.testutils.helpers.datetime import freeze_time
@@ -51,7 +51,9 @@ class MetricsQueryLayerTest(BaseMetricsLayerTestCase, TestCase):
             ),
         )
 
-        resolved_metrics_query, mappings = _resolve_metrics_query(metrics_query, "generic_metrics")
+        resolved_metrics_query, mappings, reverse_mapping = _resolve_metrics_query(
+            metrics_query, "generic_metrics"
+        )
         expected_metric_id = indexer.resolve(
             UseCaseID.TRANSACTIONS,
             self.project.organization_id,
@@ -59,6 +61,7 @@ class MetricsQueryLayerTest(BaseMetricsLayerTestCase, TestCase):
         )
         assert resolved_metrics_query.query.metric.id == expected_metric_id
         assert mappings[TransactionMRI.DURATION.value] == expected_metric_id
+        assert reverse_mapping.tag_keys == set()
 
     def test_resolve_formula_metrics_query(self):
         self.store_performance_metric(
@@ -82,7 +85,9 @@ class MetricsQueryLayerTest(BaseMetricsLayerTestCase, TestCase):
             ),
         )
 
-        resolved_metrics_query, mappings = _resolve_metrics_query(metrics_query, "generic_metrics")
+        resolved_metrics_query, mappings, reverse_mapping = _resolve_metrics_query(
+            metrics_query, "generic_metrics"
+        )
         expected_metric_id = indexer.resolve(
             UseCaseID.TRANSACTIONS,
             self.project.organization_id,
@@ -90,6 +95,7 @@ class MetricsQueryLayerTest(BaseMetricsLayerTestCase, TestCase):
         )
         assert resolved_metrics_query.query.parameters[0].metric.id == expected_metric_id
         assert mappings[TransactionMRI.DURATION.value] == expected_metric_id
+        assert reverse_mapping.tag_keys == set()
 
     def test_resolve_metrics_query_with_groupby(self):
         self.store_performance_metric(
@@ -121,7 +127,9 @@ class MetricsQueryLayerTest(BaseMetricsLayerTestCase, TestCase):
             "transaction",
         )
 
-        resolved_metrics_query, mappings = _resolve_metrics_query(metrics_query, "generic_metrics")
+        resolved_metrics_query, mappings, reverse_mapping = _resolve_metrics_query(
+            metrics_query, "generic_metrics"
+        )
         assert resolved_metrics_query.query.metric.public_name == "transaction.duration"
         assert resolved_metrics_query.query.metric.mri == TransactionMRI.DURATION.value
         assert resolved_metrics_query.query.metric.id == expected_metric_id
@@ -130,6 +138,7 @@ class MetricsQueryLayerTest(BaseMetricsLayerTestCase, TestCase):
         ]
         assert mappings[TransactionMRI.DURATION.value] == expected_metric_id
         assert mappings["transaction"] == expected_transaction_id
+        assert reverse_mapping.tag_keys == set()
 
     def test_resolve_formula_metrics_query_with_groupby(self):
         self.store_performance_metric(
@@ -177,7 +186,9 @@ class MetricsQueryLayerTest(BaseMetricsLayerTestCase, TestCase):
             "status_code",
         )
 
-        resolved_metrics_query, mappings = _resolve_metrics_query(metrics_query, "generic_metrics")
+        resolved_metrics_query, mappings, reverse_mapping = _resolve_metrics_query(
+            metrics_query, "generic_metrics"
+        )
         assert (
             resolved_metrics_query.query.parameters[0].metric.public_name == "transaction.duration"
         )
@@ -197,6 +208,7 @@ class MetricsQueryLayerTest(BaseMetricsLayerTestCase, TestCase):
         assert mappings[TransactionMRI.DURATION.value] == expected_metric_id
         assert mappings["transaction"] == expected_transaction_id
         assert mappings["status_code"] == expected_status_code_id
+        assert reverse_mapping.tag_keys == set()
 
     def test_resolve_metrics_query_with_filters(self):
         self.store_performance_metric(
@@ -242,7 +254,9 @@ class MetricsQueryLayerTest(BaseMetricsLayerTestCase, TestCase):
             "device",
         )
 
-        resolved_metrics_query, mappings = _resolve_metrics_query(metrics_query, "generic_metrics")
+        resolved_metrics_query, mappings, reverse_mapping = _resolve_metrics_query(
+            metrics_query, "generic_metrics"
+        )
         assert resolved_metrics_query.query.metric.id == expected_metric_id
         assert resolved_metrics_query.query.filters == [
             Condition(Column(f"tags_raw[{expected_transaction_id}]"), Op.EQ, "/checkout"),
@@ -256,6 +270,7 @@ class MetricsQueryLayerTest(BaseMetricsLayerTestCase, TestCase):
         assert mappings[TransactionMRI.DURATION.value] == expected_metric_id
         assert mappings["transaction"] == expected_transaction_id
         assert mappings["device"] == expected_device_id
+        assert reverse_mapping.tag_keys == set()
 
     def test_resolve_formula_metrics_query_with_filters(self):
         self.store_performance_metric(
@@ -326,7 +341,9 @@ class MetricsQueryLayerTest(BaseMetricsLayerTestCase, TestCase):
             "status_code",
         )
 
-        resolved_metrics_query, mappings = _resolve_metrics_query(metrics_query, "generic_metrics")
+        resolved_metrics_query, mappings, reverse_mapping = _resolve_metrics_query(
+            metrics_query, "generic_metrics"
+        )
         assert resolved_metrics_query.query.parameters[0].metric.id == expected_metric_id
         assert resolved_metrics_query.query.parameters[0].filters == [
             Condition(Column(f"tags_raw[{expected_transaction_id}]"), Op.EQ, "/checkout"),
@@ -353,6 +370,83 @@ class MetricsQueryLayerTest(BaseMetricsLayerTestCase, TestCase):
         assert mappings["transaction"] == expected_transaction_id
         assert mappings["device"] == expected_device_id
         assert mappings["status_code"] == expected_status_code_id
+        assert reverse_mapping.tag_keys == set()
+
+    def test_resolve_metrics_query_with_filters_release_health(self):
+        self.store_release_health_metric(
+            name=SessionMRI.RAW_DURATION.value,
+            project_id=self.project.id,
+            tags={"transaction": "/checkout", "device": "BlackBerry"},
+            value=1,
+        )
+        metrics_query = MetricsQuery(
+            query=Timeseries(
+                Metric(mri=SessionMRI.RAW_DURATION.value),
+                aggregate="count",
+                filters=[
+                    Condition(Column("transaction"), Op.EQ, "/checkout"),
+                    Or(
+                        [
+                            Condition(Column("device"), Op.EQ, "BlackBerry"),
+                            Condition(Column("device"), Op.EQ, "Nokia"),
+                        ]
+                    ),
+                ],
+                groupby=[Column("transaction")],
+            ),
+            scope=MetricsScope(
+                org_ids=[self.project.organization_id],
+                project_ids=[self.project.id],
+                use_case_id=UseCaseID.SESSIONS.value,
+            ),
+        )
+        expected_metric_id = indexer.resolve(
+            UseCaseID.SESSIONS,
+            self.project.organization_id,
+            SessionMRI.RAW_DURATION.value,
+        )
+        expected_transaction_id = indexer.resolve(
+            UseCaseID.SESSIONS,
+            self.project.organization_id,
+            "transaction",
+        )
+        expected_device_id = indexer.resolve(
+            UseCaseID.SESSIONS,
+            self.project.organization_id,
+            "device",
+        )
+        expected_checkout = indexer.resolve(
+            UseCaseID.SESSIONS,
+            self.project.organization_id,
+            "/checkout",
+        )
+        expected_blackberry = indexer.resolve(
+            UseCaseID.SESSIONS,
+            self.project.organization_id,
+            "BlackBerry",
+        )
+
+        resolved_metrics_query, mappings, reverse_mapping = _resolve_metrics_query(
+            metrics_query, "metrics"
+        )
+        assert resolved_metrics_query.query.metric.id == expected_metric_id
+        assert resolved_metrics_query.query.filters == [
+            Condition(Column(f"tags[{expected_transaction_id}]"), Op.EQ, expected_checkout),
+            Or(
+                [
+                    Condition(Column(f"tags[{expected_device_id}]"), Op.EQ, expected_blackberry),
+                    Condition(Column(f"tags[{expected_device_id}]"), Op.EQ, "Nokia"),
+                ]
+            ),
+        ]
+        assert mappings[SessionMRI.RAW_DURATION.value] == expected_metric_id
+        assert mappings["transaction"] == expected_transaction_id
+        assert mappings["device"] == expected_device_id
+        assert reverse_mapping.tag_keys == {"transaction", "device"}
+        assert set(reverse_mapping.reverse_mappings.keys()) == {
+            expected_checkout,
+            expected_blackberry,
+        }
 
 
 @pytest.mark.parametrize(

+ 42 - 6
tests/snuba/test_metrics_layer.py

@@ -475,7 +475,7 @@ class MQLTest(TestCase, BaseMetricsTestCase):
         assert len(result["data"]) == 10
         assert result["totals"]["aggregate_value"] == 9.0
 
-    def test_groupby_metrics(self) -> None:
+    def test_metrics_groupby(self) -> None:
         query = MetricsQuery(
             query=Timeseries(
                 metric=Metric(
@@ -504,9 +504,10 @@ class MQLTest(TestCase, BaseMetricsTestCase):
         result = self.run_query(request)
         assert request.dataset == "metrics"
         assert len(result["data"]) == 10
-        # TODO: check reverse resolved tags
+        for data_point in result["data"]:
+            assert data_point["release"] == "release_even" or data_point["release"] == "release_odd"
 
-    def test_filters_metrics(self) -> None:
+    def test_metrics_filters(self) -> None:
         query = MetricsQuery(
             query=Timeseries(
                 metric=Metric(
@@ -537,9 +538,8 @@ class MQLTest(TestCase, BaseMetricsTestCase):
         result = self.run_query(request)
         assert request.dataset == "metrics"
         assert len(result["data"]) == 5
-        # TODO: check reverse resolved tags
 
-    def test_complex_metrics(self) -> None:
+    def test_metrics_complex(self) -> None:
         query = MetricsQuery(
             query=Timeseries(
                 metric=Metric(
@@ -571,7 +571,43 @@ class MQLTest(TestCase, BaseMetricsTestCase):
         result = self.run_query(request)
         assert request.dataset == "metrics"
         assert len(result["data"]) == 5
-        # TODO: check reverse resolved tags
+        assert any(data_point["release"] == "release_even" for data_point in result["data"])
+
+    def test_metrics_correctly_reverse_resolved(self) -> None:
+        query = MetricsQuery(
+            query=Timeseries(
+                metric=Metric(
+                    None,
+                    SessionMRI.RAW_SESSION.value,
+                ),
+                aggregate="count",
+                groupby=[Column("release"), Column("project_id")],
+                filters=[
+                    Condition(Column("release"), Op.EQ, "release_even"),
+                    Condition(Column("project_id"), Op.EQ, self.project.id),
+                ],
+            ),
+            start=self.hour_ago,
+            end=self.now,
+            rollup=Rollup(interval=60, granularity=60),
+            scope=MetricsScope(
+                org_ids=[self.org_id],
+                project_ids=[self.project.id],
+                use_case_id=UseCaseID.SESSIONS.value,
+            ),
+        )
+
+        request = Request(
+            dataset="metrics",
+            app_id="tests",
+            query=query,
+            tenant_ids={"referrer": "metrics.testing.test", "organization_id": self.org_id},
+        )
+        result = self.run_query(request)
+        assert request.dataset == "metrics"
+        assert len(result["data"]) == 5
+        assert any(data_point["release"] == "release_even" for data_point in result["data"])
+        assert any(data_point["project_id"] == self.project.id for data_point in result["data"])
 
     @pytest.mark.skip(reason="This is not implemented in MQL")
     def test_failure_rate(self) -> None: