Browse Source

feat(metrics): Search on non zero span groups in another query (#66912)

The spans samples query currently only searches on non zero span groups.
Here we allow a second query to search on the zero span group. It's
possible this second query can time out but we'll have to test to see.

This change also has the implication that the sorted spans query will
get slower but that query already times out quite frequently already.
Tony Xiao 1 year ago
parent
commit
fe2b1a3e18

+ 70 - 38
src/sentry/sentry_metrics/querying/samples_list.py

@@ -539,49 +539,66 @@ class SpansSamplesListExecutor(AbstractSamplesListExecutor):
     def get_unsorted_span_keys(self, offset: int, limit: int) -> list[tuple[str, str, str]]:
     def get_unsorted_span_keys(self, offset: int, limit: int) -> list[tuple[str, str, str]]:
         column = self.mri_to_column(self.mri)
         column = self.mri_to_column(self.mri)
 
 
-        builder = SpansIndexedQueryBuilder(
-            Dataset.SpansIndexed,
-            self.params,
-            snuba_params=self.snuba_params,
-            query=self.query,
-            selected_columns=[
-                f"rounded_timestamp({self.rollup})",
-                f"examples({column}, {self.num_samples}) AS examples",
-            ],
-            limit=limit,
-            offset=offset,
-            sample_rate=options.get("metrics.sample-list.sample-rate"),
-            config=QueryBuilderConfig(functions_acl=["rounded_timestamp", "examples"]),
-        )
+        for dataset_segmentation_condition_fn in self.dataset_segmentation_conditions():
+            builder = SpansIndexedQueryBuilder(
+                Dataset.SpansIndexed,
+                self.params,
+                snuba_params=self.snuba_params,
+                query=self.query,
+                selected_columns=[
+                    f"rounded_timestamp({self.rollup})",
+                    f"examples({column}, {self.num_samples}) AS examples",
+                ],
+                limit=limit,
+                offset=offset,
+                sample_rate=options.get("metrics.sample-list.sample-rate"),
+                config=QueryBuilderConfig(functions_acl=["rounded_timestamp", "examples"]),
+            )
 
 
-        additional_conditions = self.get_additional_conditions(builder)
+            segmentation_conditions = dataset_segmentation_condition_fn(builder)
 
 
-        assert column is not None
-        min_max_conditions = self.get_min_max_conditions(builder.resolve_column(column))
+            additional_conditions = self.get_additional_conditions(builder)
 
 
-        builder.add_conditions([*additional_conditions, *min_max_conditions])
+            assert column is not None
+            min_max_conditions = self.get_min_max_conditions(builder.resolve_column(column))
 
 
-        query_results = builder.run_query(self.referrer.value)
-        result = builder.process_results(query_results)
+            builder.add_conditions(
+                [
+                    *segmentation_conditions,
+                    *additional_conditions,
+                    *min_max_conditions,
+                ]
+            )
 
 
-        metric_key = lambda example: example[3]  # sort by metric
-        for row in result["data"]:
-            row["examples"] = pick_samples(row["examples"], metric_key=metric_key)
+            query_results = builder.run_query(self.referrer.value)
+            result = builder.process_results(query_results)
 
 
-        return [
-            (
-                example[0],  # group
-                example[1],  # timestamp
-                example[2],  # span_id
-            )
-            for row in result["data"]
-            for example in row["examples"]
-        ][:limit]
+            if not result["data"]:
+                continue
+
+            metric_key = lambda example: example[3]  # sort by metric
+            for row in result["data"]:
+                row["examples"] = pick_samples(row["examples"], metric_key=metric_key)
+
+            return [
+                (
+                    example[0],  # group
+                    example[1],  # timestamp
+                    example[2],  # span_id
+                )
+                for row in result["data"]
+                for example in row["examples"]
+            ][:limit]
+
+        return []
 
 
     @abstractmethod
     @abstractmethod
     def get_additional_conditions(self, builder: QueryBuilder) -> list[Condition]:
     def get_additional_conditions(self, builder: QueryBuilder) -> list[Condition]:
         raise NotImplementedError
         raise NotImplementedError
 
 
+    def dataset_segmentation_conditions(self) -> list[Callable[[QueryBuilder], list[Condition]]]:
+        return [lambda builder: []]
+
     def get_min_max_conditions(self, column: SelectType) -> list[Condition]:
     def get_min_max_conditions(self, column: SelectType) -> list[Condition]:
         conditions = []
         conditions = []
 
 
@@ -604,14 +621,29 @@ class SpansTimingsSamplesListExecutor(SpansSamplesListExecutor):
         return cls.MRI_MAPPING.get(mri)
         return cls.MRI_MAPPING.get(mri)
 
 
     def get_additional_conditions(self, builder: QueryBuilder) -> list[Condition]:
     def get_additional_conditions(self, builder: QueryBuilder) -> list[Condition]:
+        return []
+
+    def dataset_segmentation_conditions(self) -> list[Callable[[QueryBuilder], list[Condition]]]:
         return [
         return [
-            # The `00` group is used for spans not used within the
-            # new starfish experience. It's effectively the group
-            # for other. It is a massive group, so we've chosen
-            # to exclude it here.
+            # This grouping makes the assumption that spans are divided into 2 groups right now.
+            # Those that are classified with a non zero group, and those that are unclassified
+            # with a zero group.
             #
             #
-            # In the future, we will want to look into exposing them
-            Condition(builder.column("span.group"), Op.NEQ, "00")
+            # In the future, if all span groups are classified, this segmentation should change
+            # to reflect that.
+            lambda builder: [
+                # The `00` group is used for spans not used within the
+                # new starfish experience. It's effectively the group
+                # for other. It is a massive group, so we've chosen
+                # to exclude it here.
+                Condition(builder.column("span.group"), Op.NEQ, "00"),
+            ],
+            lambda builder: [
+                # If the previous query contained no results, we'll
+                # have to search the `00` group which is slower but
+                # unfortunately necessary here.
+                Condition(builder.column("span.group"), Op.EQ, "00"),
+            ],
         ]
         ]
 
 
 
 

+ 1 - 7
tests/sentry/api/endpoints/test_organization_metrics.py

@@ -190,7 +190,7 @@ class OrganizationMetricsSamplesEndpointTest(BaseSpansTestCase, APITestCase):
             "detail": ErrorDetail(string="Unsupported sort: id for MRI", code="parse_error")
             "detail": ErrorDetail(string="Unsupported sort: id for MRI", code="parse_error")
         }
         }
 
 
-    def test_span_exclusive_time_samples(self):
+    def test_span_exclusive_time_samples_zero_group(self):
         durations = [100, 200, 300]
         durations = [100, 200, 300]
         span_ids = [uuid4().hex[:16] for _ in durations]
         span_ids = [uuid4().hex[:16] for _ in durations]
         good_span_id = span_ids[1]
         good_span_id = span_ids[1]
@@ -206,7 +206,6 @@ class OrganizationMetricsSamplesEndpointTest(BaseSpansTestCase, APITestCase):
                 duration=duration,
                 duration=duration,
                 exclusive_time=duration,
                 exclusive_time=duration,
                 timestamp=ts,
                 timestamp=ts,
-                group=uuid4().hex[:16],  # we need a non 0 group
             )
             )
 
 
         query = {
         query = {
@@ -267,7 +266,6 @@ class OrganizationMetricsSamplesEndpointTest(BaseSpansTestCase, APITestCase):
                 span_id=span_id,
                 span_id=span_id,
                 duration=duration,
                 duration=duration,
                 timestamp=ts,
                 timestamp=ts,
-                group=uuid4().hex[:16],  # we need a non 0 group
                 measurements={
                 measurements={
                     measurement: duration + j + 1
                     measurement: duration + j + 1
                     for j, measurement in enumerate(
                     for j, measurement in enumerate(
@@ -289,7 +287,6 @@ class OrganizationMetricsSamplesEndpointTest(BaseSpansTestCase, APITestCase):
                 uuid4().hex,
                 uuid4().hex,
                 span_id=uuid4().hex[:16],
                 span_id=uuid4().hex[:16],
                 timestamp=ts,
                 timestamp=ts,
-                group=uuid4().hex[:16],  # we need a non 0 group
             )
             )
 
 
         for i, mri in enumerate(
         for i, mri in enumerate(
@@ -527,7 +524,6 @@ class OrganizationMetricsSamplesEndpointTest(BaseSpansTestCase, APITestCase):
                 span_id=span_id,
                 span_id=span_id,
                 duration=val,
                 duration=val,
                 timestamp=ts,
                 timestamp=ts,
-                group=uuid4().hex[:16],  # we need a non 0 group
                 store_metrics_summary={
                 store_metrics_summary={
                     mri: [
                     mri: [
                         {
                         {
@@ -547,7 +543,6 @@ class OrganizationMetricsSamplesEndpointTest(BaseSpansTestCase, APITestCase):
             uuid4().hex,
             uuid4().hex,
             span_id=uuid4().hex[:16],
             span_id=uuid4().hex[:16],
             timestamp=before_now(days=10, minutes=10).replace(microsecond=0),
             timestamp=before_now(days=10, minutes=10).replace(microsecond=0),
-            group=uuid4().hex[:16],  # we need a non 0 group
             store_metrics_summary={
             store_metrics_summary={
                 "d:custom/other@millisecond": [
                 "d:custom/other@millisecond": [
                     {
                     {
@@ -628,7 +623,6 @@ class OrganizationMetricsSamplesEndpointTest(BaseSpansTestCase, APITestCase):
                 duration=value,
                 duration=value,
                 exclusive_time=value,
                 exclusive_time=value,
                 timestamp=ts,
                 timestamp=ts,
-                group=uuid4().hex[:16],  # we need a non 0 group
                 measurements={"score.total": value},
                 measurements={"score.total": value},
                 store_metrics_summary={
                 store_metrics_summary={
                     custom_mri: [
                     custom_mri: [