From 607dda7b7d3e45dbec54b2632ee043d14f3a6e5d Mon Sep 17 00:00:00 2001 From: Edward Gou Date: Wed, 7 Aug 2024 16:14:42 -0400 Subject: [PATCH 01/10] Skip metrics_layer when creating a PerformanceMetricsEntitySubscription using insights related aggregates/queries --- src/sentry/snuba/entity_subscription.py | 22 ++++++++++++++- .../sentry/snuba/test_entity_subscriptions.py | 28 +++++++++++++++++++ 2 files changed, 49 insertions(+), 1 deletion(-) diff --git a/src/sentry/snuba/entity_subscription.py b/src/sentry/snuba/entity_subscription.py index 5d8fffe82fc3fd..d59a62a6388cad 100644 --- a/src/sentry/snuba/entity_subscription.py +++ b/src/sentry/snuba/entity_subscription.py @@ -53,6 +53,15 @@ CRASH_RATE_ALERT_AGGREGATE_RE = ( r"^percentage\([ ]*(sessions_crashed|users_crashed)[ ]*\,[ ]*(sessions|users)[ ]*\)" ) +INSIGHTS_AGGREGATE_RE_SET = { + r"^(spm|cache_miss_rate)\([ ]*\)", + r"^http_response_rate\([ ]*(3|4|5)[ ]*\)", + r"^(sum|avg)\([ ]*(span\.self_time|span\.duration|messaging\.message\.receive\.latency|measurements\.(time_to_initial_display|time_to_full_display|ai\.total_tokens\.used))[ ]*\)" + r"^((weighted_)?performance_score|count_scores)\([ ]*(measurements\.score\.(lcp|fcp|ttfb|cls|inp))[ ]*\)", +} +INSIGHTS_QUERY_RE_SET = { + r"span\.(category|module|op|description):", +} ALERT_BLOCKED_FIELDS = { "start", "end", @@ -283,6 +292,15 @@ def resolve_tag_values_if_needed(self, strings: Sequence[str]) -> Sequence[str | return resolve_tag_values(self._get_use_case_id(), self.org_id, strings) + def is_insights_query(self, query: str) -> bool: + for pattern in INSIGHTS_AGGREGATE_RE_SET: + if re.match(pattern, self.aggregate): + return True + for pattern in INSIGHTS_QUERY_RE_SET: + if re.match(pattern, query): + return True + return False + def build_query_builder( self, query: str, @@ -310,7 +328,9 @@ def build_query_builder( granularity=self.get_granularity(), config=QueryBuilderConfig( skip_time_conditions=True, - use_metrics_layer=self.use_metrics_layer, + use_metrics_layer=False + if self.is_insights_query(query) + else self.use_metrics_layer, on_demand_metrics_enabled=self.on_demand_metrics_enabled, on_demand_metrics_type=MetricSpecType.SIMPLE_QUERY, skip_field_validation_for_entity_subscription_deletion=skip_field_validation_for_entity_subscription_deletion, diff --git a/tests/sentry/snuba/test_entity_subscriptions.py b/tests/sentry/snuba/test_entity_subscriptions.py index 3c82d97a36ae48..2f85eb46aa0023 100644 --- a/tests/sentry/snuba/test_entity_subscriptions.py +++ b/tests/sentry/snuba/test_entity_subscriptions.py @@ -617,6 +617,34 @@ def test_get_entity_subscription_for_events_dataset_with_join(self) -> None: Condition(Column("project_id", entity=g_entity), Op.IN, [self.project.id]), ] + def test_get_entity_subscription_for_insights_queries(self) -> None: + with Feature("organizations:custom-metrics"): + cases = [ + ("count()", "", True), + ("avg(transaction.duration)", "", True), + ("count()", "span.module:http", False), + ("count()", "span.category:http", False), + ("count()", "span.op:http.client", False), + ("count()", "span.description:abc", False), + # TODO: The following functions are not supported in the discover metrics dataset yet. + # Uncomment these as we port them over. + # ("spm()", "", False), + # ("cache_miss_rate()", "", False), + # ("http_response_rate()", "", False), + # ("avg(span.self_time)", "", False), + # ("performance_score(measurements.score.lcp)", "", False), + ] + for aggregate, query, use_metrics_layer in cases: + entity_subscription = get_entity_subscription( + query_type=SnubaQuery.Type.PERFORMANCE, + dataset=Dataset.PerformanceMetrics, + aggregate=aggregate, + time_window=3600, + extra_fields={"org_id": self.organization.id}, + ) + builder = entity_subscription.build_query_builder(query, [self.project.id], None) + assert builder.builder_config.use_metrics_layer is use_metrics_layer + class GetEntitySubscriptionFromSnubaQueryTest(TestCase): def test(self): From 2f51183051486e9efae12642f482f314f856e9a1 Mon Sep 17 00:00:00 2001 From: Edward Gou Date: Thu, 8 Aug 2024 10:31:43 -0400 Subject: [PATCH 02/10] missing comma --- src/sentry/snuba/entity_subscription.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/sentry/snuba/entity_subscription.py b/src/sentry/snuba/entity_subscription.py index d59a62a6388cad..ae5ac4fe31ccc1 100644 --- a/src/sentry/snuba/entity_subscription.py +++ b/src/sentry/snuba/entity_subscription.py @@ -56,7 +56,7 @@ INSIGHTS_AGGREGATE_RE_SET = { r"^(spm|cache_miss_rate)\([ ]*\)", r"^http_response_rate\([ ]*(3|4|5)[ ]*\)", - r"^(sum|avg)\([ ]*(span\.self_time|span\.duration|messaging\.message\.receive\.latency|measurements\.(time_to_initial_display|time_to_full_display|ai\.total_tokens\.used))[ ]*\)" + r"^(sum|avg)\([ ]*(span\.self_time|span\.duration|messaging\.message\.receive\.latency|measurements\.(time_to_initial_display|time_to_full_display|ai\.total_tokens\.used))[ ]*\)", r"^((weighted_)?performance_score|count_scores)\([ ]*(measurements\.score\.(lcp|fcp|ttfb|cls|inp))[ ]*\)", } INSIGHTS_QUERY_RE_SET = { From 892f59e9c9e423e638eb6f7cdcb6a5ee96c676d8 Mon Sep 17 00:00:00 2001 From: Edward Gou Date: Thu, 8 Aug 2024 15:34:29 -0400 Subject: [PATCH 03/10] move logic to MetricsQueryBuilder and parse functions/filters instead of using regex --- src/sentry/search/events/builder/metrics.py | 24 ++++++++++++++++++- src/sentry/search/events/constants.py | 12 ++++++++++ src/sentry/search/events/types.py | 1 + src/sentry/snuba/entity_subscription.py | 23 ++---------------- .../sentry/snuba/test_entity_subscriptions.py | 6 ++--- 5 files changed, 41 insertions(+), 25 deletions(-) diff --git a/src/sentry/search/events/builder/metrics.py b/src/sentry/search/events/builder/metrics.py index 1ed4327d1da9c8..07f5bc3830777e 100644 --- a/src/sentry/search/events/builder/metrics.py +++ b/src/sentry/search/events/builder/metrics.py @@ -151,7 +151,7 @@ def load_config(self) -> DatasetConfig: return super().load_config() if self.dataset in [Dataset.Metrics, Dataset.PerformanceMetrics]: - if self.builder_config.use_metrics_layer: + if self.use_metrics_layer: return MetricsLayerDatasetConfig(self) else: return MetricsDatasetConfig(self) @@ -376,6 +376,23 @@ def validate_aggregate_arguments(self) -> None: if not self.use_metrics_layer: super().validate_aggregate_arguments() + @property + def is_insights_metric_query(self) -> bool: + for query in self.parse_query(self.query): + if query.key.name in constants.INSIGHTS_METRICS_TAGS: + return True + for column in self.selected_columns: + # Not using parse_function since it checks against function_converter + # which is not loaded yet and we also do not need it + match = fields.is_function(column) + func = match.group("function") if match else None + if func in constants.INSIGHTS_METRICS_FUNCTIONS: + return True + argument = match.group("columns") if match else None + if argument in constants.SPAN_METRICS_MAP.keys() - constants.METRICS_MAP.keys(): + return True + return False + @property def is_performance(self) -> bool: return self.dataset is Dataset.PerformanceMetrics @@ -396,6 +413,11 @@ def use_case_id(self) -> UseCaseID: def use_metrics_layer(self) -> bool: # We want to use the metrics layer only for normal metrics, since span metrics are currently # NOT supported. + if ( + self.builder_config.insights_metrics_override_metric_layer + and self.is_insights_metric_query + ): + return False return self.builder_config.use_metrics_layer and not self.spans_metrics_builder def resolve_query( diff --git a/src/sentry/search/events/constants.py b/src/sentry/search/events/constants.py index 70c7f6b294e1ef..3dd8f84aa7dfd1 100644 --- a/src/sentry/search/events/constants.py +++ b/src/sentry/search/events/constants.py @@ -56,6 +56,7 @@ SPAN_OP = "span.op" SPAN_DESCRIPTION = "span.description" SPAN_STATUS = "span.status" +SPAN_CATEGORY = "span.category" class ThresholdDict(TypedDict): @@ -431,3 +432,14 @@ class ThresholdDict(TypedDict): # The limit in snuba currently for a single query is 131,535bytes, including room for other parameters picking 120,000 # for now MAX_PARAMETERS_IN_ARRAY = 120_000 + +INSIGHTS_METRICS_TAGS = {SPAN_MODULE_ALIAS, SPAN_DESCRIPTION, SPAN_OP, SPAN_CATEGORY} + +INSIGHTS_METRICS_FUNCTIONS = { + "spm", + "cache_miss_rate", + "http_response_rate", + "performance_score", + "weighted_performance_score", + "count_scores", +} diff --git a/src/sentry/search/events/types.py b/src/sentry/search/events/types.py index 5198d74603c9b9..a359962396d7df 100644 --- a/src/sentry/search/events/types.py +++ b/src/sentry/search/events/types.py @@ -202,6 +202,7 @@ class QueryBuilderConfig: on_demand_metrics_type: Any | None = None skip_field_validation_for_entity_subscription_deletion: bool = False allow_metric_aggregates: bool | None = False + insights_metrics_override_metric_layer: bool = False @dataclass(frozen=True) diff --git a/src/sentry/snuba/entity_subscription.py b/src/sentry/snuba/entity_subscription.py index ae5ac4fe31ccc1..c4106650b2e522 100644 --- a/src/sentry/snuba/entity_subscription.py +++ b/src/sentry/snuba/entity_subscription.py @@ -53,15 +53,6 @@ CRASH_RATE_ALERT_AGGREGATE_RE = ( r"^percentage\([ ]*(sessions_crashed|users_crashed)[ ]*\,[ ]*(sessions|users)[ ]*\)" ) -INSIGHTS_AGGREGATE_RE_SET = { - r"^(spm|cache_miss_rate)\([ ]*\)", - r"^http_response_rate\([ ]*(3|4|5)[ ]*\)", - r"^(sum|avg)\([ ]*(span\.self_time|span\.duration|messaging\.message\.receive\.latency|measurements\.(time_to_initial_display|time_to_full_display|ai\.total_tokens\.used))[ ]*\)", - r"^((weighted_)?performance_score|count_scores)\([ ]*(measurements\.score\.(lcp|fcp|ttfb|cls|inp))[ ]*\)", -} -INSIGHTS_QUERY_RE_SET = { - r"span\.(category|module|op|description):", -} ALERT_BLOCKED_FIELDS = { "start", "end", @@ -292,15 +283,6 @@ def resolve_tag_values_if_needed(self, strings: Sequence[str]) -> Sequence[str | return resolve_tag_values(self._get_use_case_id(), self.org_id, strings) - def is_insights_query(self, query: str) -> bool: - for pattern in INSIGHTS_AGGREGATE_RE_SET: - if re.match(pattern, self.aggregate): - return True - for pattern in INSIGHTS_QUERY_RE_SET: - if re.match(pattern, query): - return True - return False - def build_query_builder( self, query: str, @@ -328,12 +310,11 @@ def build_query_builder( granularity=self.get_granularity(), config=QueryBuilderConfig( skip_time_conditions=True, - use_metrics_layer=False - if self.is_insights_query(query) - else self.use_metrics_layer, + use_metrics_layer=self.use_metrics_layer, on_demand_metrics_enabled=self.on_demand_metrics_enabled, on_demand_metrics_type=MetricSpecType.SIMPLE_QUERY, skip_field_validation_for_entity_subscription_deletion=skip_field_validation_for_entity_subscription_deletion, + insights_metrics_override_metric_layer=True, ), ) diff --git a/tests/sentry/snuba/test_entity_subscriptions.py b/tests/sentry/snuba/test_entity_subscriptions.py index 2f85eb46aa0023..ea5a572928c854 100644 --- a/tests/sentry/snuba/test_entity_subscriptions.py +++ b/tests/sentry/snuba/test_entity_subscriptions.py @@ -626,13 +626,13 @@ def test_get_entity_subscription_for_insights_queries(self) -> None: ("count()", "span.category:http", False), ("count()", "span.op:http.client", False), ("count()", "span.description:abc", False), + ("performance_score(measurements.score.lcp)", "", False), # TODO: The following functions are not supported in the discover metrics dataset yet. # Uncomment these as we port them over. # ("spm()", "", False), # ("cache_miss_rate()", "", False), # ("http_response_rate()", "", False), # ("avg(span.self_time)", "", False), - # ("performance_score(measurements.score.lcp)", "", False), ] for aggregate, query, use_metrics_layer in cases: entity_subscription = get_entity_subscription( @@ -642,8 +642,8 @@ def test_get_entity_subscription_for_insights_queries(self) -> None: time_window=3600, extra_fields={"org_id": self.organization.id}, ) - builder = entity_subscription.build_query_builder(query, [self.project.id], None) - assert builder.builder_config.use_metrics_layer is use_metrics_layer + builder = entity_subscription.build_query_builder(query, [self.project.id], None) + assert builder.use_metrics_layer is use_metrics_layer class GetEntitySubscriptionFromSnubaQueryTest(TestCase): From 3a3007910f827b7249357095000a019954ea6782 Mon Sep 17 00:00:00 2001 From: Edward Gou Date: Thu, 8 Aug 2024 15:48:19 -0400 Subject: [PATCH 04/10] fix test? --- tests/sentry/snuba/test_entity_subscriptions.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/sentry/snuba/test_entity_subscriptions.py b/tests/sentry/snuba/test_entity_subscriptions.py index ea5a572928c854..751c7b954b65f0 100644 --- a/tests/sentry/snuba/test_entity_subscriptions.py +++ b/tests/sentry/snuba/test_entity_subscriptions.py @@ -9,6 +9,7 @@ UnsupportedQuerySubscription, ) from sentry.models.group import GroupStatus +from sentry.search.events.builder.metrics import AlertMetricsQueryBuilder from sentry.search.events.constants import METRICS_MAP from sentry.sentry_metrics import indexer from sentry.sentry_metrics.use_case_id_registry import UseCaseID @@ -643,6 +644,7 @@ def test_get_entity_subscription_for_insights_queries(self) -> None: extra_fields={"org_id": self.organization.id}, ) builder = entity_subscription.build_query_builder(query, [self.project.id], None) + assert isinstance(builder, AlertMetricsQueryBuilder) assert builder.use_metrics_layer is use_metrics_layer From 4fdc077d2757f1510c0d839cf0dc5fd6a3f069e0 Mon Sep 17 00:00:00 2001 From: Edward Gou Date: Thu, 8 Aug 2024 16:37:08 -0400 Subject: [PATCH 05/10] Allows insight metrics to be used with the avg function in the discover metrics dataset --- src/sentry/search/events/datasets/metrics.py | 7 +++++-- tests/sentry/snuba/test_entity_subscriptions.py | 2 +- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/src/sentry/search/events/datasets/metrics.py b/src/sentry/search/events/datasets/metrics.py index b2465bf0987d2b..5228163725d566 100644 --- a/src/sentry/search/events/datasets/metrics.py +++ b/src/sentry/search/events/datasets/metrics.py @@ -65,7 +65,9 @@ def field_alias_converter(self) -> Mapping[str, Callable[[str], SelectType]]: } def resolve_metric(self, value: str) -> int: - metric_id = self.builder.resolve_metric_index(constants.METRICS_MAP.get(value, value)) + # SPAN_METRICS_MAP and METRICS_MAP have some overlapping keys + mri_map = constants.SPAN_METRICS_MAP | constants.METRICS_MAP + metric_id = self.builder.resolve_metric_index(mri_map.get(value, value)) if metric_id is None: # Maybe this is a custom measurment? for measurement in self.builder.custom_measurement_map: @@ -112,7 +114,8 @@ def function_converter(self) -> Mapping[str, fields.MetricsFunction]: required_args=[ fields.MetricArg( "column", - allowed_columns=constants.METRIC_DURATION_COLUMNS, + allowed_columns=constants.SPAN_METRIC_DURATION_COLUMNS + | constants.METRIC_DURATION_COLUMNS, ) ], calculated_args=[resolve_metric_id], diff --git a/tests/sentry/snuba/test_entity_subscriptions.py b/tests/sentry/snuba/test_entity_subscriptions.py index 751c7b954b65f0..b4fc641c806546 100644 --- a/tests/sentry/snuba/test_entity_subscriptions.py +++ b/tests/sentry/snuba/test_entity_subscriptions.py @@ -628,12 +628,12 @@ def test_get_entity_subscription_for_insights_queries(self) -> None: ("count()", "span.op:http.client", False), ("count()", "span.description:abc", False), ("performance_score(measurements.score.lcp)", "", False), + ("avg(span.self_time)", "", False), # TODO: The following functions are not supported in the discover metrics dataset yet. # Uncomment these as we port them over. # ("spm()", "", False), # ("cache_miss_rate()", "", False), # ("http_response_rate()", "", False), - # ("avg(span.self_time)", "", False), ] for aggregate, query, use_metrics_layer in cases: entity_subscription = get_entity_subscription( From 9f133ae82bf9d5368d3f060ed79e07a6d8e4c993 Mon Sep 17 00:00:00 2001 From: Edward Gou Date: Thu, 8 Aug 2024 21:33:54 -0400 Subject: [PATCH 06/10] better naming --- src/sentry/search/events/builder/metrics.py | 26 +++++++++++++++++---- src/sentry/search/events/constants.py | 7 ++++-- 2 files changed, 26 insertions(+), 7 deletions(-) diff --git a/src/sentry/search/events/builder/metrics.py b/src/sentry/search/events/builder/metrics.py index 07f5bc3830777e..eef8998efd1596 100644 --- a/src/sentry/search/events/builder/metrics.py +++ b/src/sentry/search/events/builder/metrics.py @@ -376,23 +376,39 @@ def validate_aggregate_arguments(self) -> None: if not self.use_metrics_layer: super().validate_aggregate_arguments() + # This property is used to determine if a query is using at least one of the fields in the spans namespace. @property - def is_insights_metric_query(self) -> bool: + def is_spans_metrics_query(self) -> bool: for query in self.parse_query(self.query): - if query.key.name in constants.INSIGHTS_METRICS_TAGS: + if query.key.name in constants.SPANS_METRICS_TAGS: return True for column in self.selected_columns: # Not using parse_function since it checks against function_converter # which is not loaded yet and we also do not need it match = fields.is_function(column) func = match.group("function") if match else None - if func in constants.INSIGHTS_METRICS_FUNCTIONS: + if func in constants.SPANS_METRICS_FUNCTIONS: return True argument = match.group("columns") if match else None if argument in constants.SPAN_METRICS_MAP.keys() - constants.METRICS_MAP.keys(): return True return False + # Some fields and functions cannot be translated to metrics layer queries. + # This property is used to determine if a query is using at least one of these fields or functions, and if so, we must not use the metrics layer. + @property + def is_unsupported_metrics_layer_query(self) -> bool: + if self.is_spans_metrics_query: + return True + for column in self.selected_columns: + # Not using parse_function since it checks against function_converter + # which is not loaded yet and we also do not need it + match = fields.is_function(column) + func = match.group("function") if match else None + if func in constants.METRICS_LAYER_UNSUPPORTED_TRANSACTION_METRICS_FUNCTIONS: + return True + return False + @property def is_performance(self) -> bool: return self.dataset is Dataset.PerformanceMetrics @@ -400,7 +416,7 @@ def is_performance(self) -> bool: @property def use_case_id(self) -> UseCaseID: - if self.spans_metrics_builder: + if self.spans_metrics_builder or self.is_spans_metrics_query: return UseCaseID.SPANS elif self.is_performance: return UseCaseID.TRANSACTIONS @@ -415,7 +431,7 @@ def use_metrics_layer(self) -> bool: # NOT supported. if ( self.builder_config.insights_metrics_override_metric_layer - and self.is_insights_metric_query + and self.is_unsupported_metrics_layer_query ): return False return self.builder_config.use_metrics_layer and not self.spans_metrics_builder diff --git a/src/sentry/search/events/constants.py b/src/sentry/search/events/constants.py index 3dd8f84aa7dfd1..89715eb33c56f8 100644 --- a/src/sentry/search/events/constants.py +++ b/src/sentry/search/events/constants.py @@ -433,12 +433,15 @@ class ThresholdDict(TypedDict): # for now MAX_PARAMETERS_IN_ARRAY = 120_000 -INSIGHTS_METRICS_TAGS = {SPAN_MODULE_ALIAS, SPAN_DESCRIPTION, SPAN_OP, SPAN_CATEGORY} +SPANS_METRICS_TAGS = {SPAN_MODULE_ALIAS, SPAN_DESCRIPTION, SPAN_OP, SPAN_CATEGORY} -INSIGHTS_METRICS_FUNCTIONS = { +SPANS_METRICS_FUNCTIONS = { "spm", "cache_miss_rate", "http_response_rate", +} + +METRICS_LAYER_UNSUPPORTED_TRANSACTION_METRICS_FUNCTIONS = { "performance_score", "weighted_performance_score", "count_scores", From e14750217016dcab0bfd9f934a6db5379e7f8774 Mon Sep 17 00:00:00 2001 From: Edward Gou Date: Fri, 9 Aug 2024 13:10:53 -0400 Subject: [PATCH 07/10] move use case logic --- src/sentry/search/events/builder/metrics.py | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/sentry/search/events/builder/metrics.py b/src/sentry/search/events/builder/metrics.py index eef8998efd1596..2150c57567d039 100644 --- a/src/sentry/search/events/builder/metrics.py +++ b/src/sentry/search/events/builder/metrics.py @@ -416,7 +416,7 @@ def is_performance(self) -> bool: @property def use_case_id(self) -> UseCaseID: - if self.spans_metrics_builder or self.is_spans_metrics_query: + if self.spans_metrics_builder: return UseCaseID.SPANS elif self.is_performance: return UseCaseID.TRANSACTIONS @@ -717,7 +717,11 @@ def resolve_snql_function( def resolve_metric_index(self, value: str) -> int | None: """Layer on top of the metric indexer so we'll only hit it at most once per value""" if value not in self._indexer_cache: - result = indexer.resolve(self.use_case_id, self.organization_id, value) + result = indexer.resolve( + UseCaseID.SPANS if self.is_spans_metrics_query else self.use_case_id, + self.organization_id, + value, + ) self._indexer_cache[value] = result return self._indexer_cache[value] From 8abbeeafb345c181dce9e52ac31723be39473211 Mon Sep 17 00:00:00 2001 From: Edward Gou Date: Fri, 9 Aug 2024 14:09:14 -0400 Subject: [PATCH 08/10] simpler parse_query --- src/sentry/search/events/builder/metrics.py | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/src/sentry/search/events/builder/metrics.py b/src/sentry/search/events/builder/metrics.py index 2150c57567d039..3cb60879516718 100644 --- a/src/sentry/search/events/builder/metrics.py +++ b/src/sentry/search/events/builder/metrics.py @@ -55,6 +55,7 @@ SnubaParams, WhereType, ) +from sentry.search.utils import parse_query from sentry.sentry_metrics import indexer from sentry.sentry_metrics.use_case_id_registry import UseCaseID from sentry.snuba.dataset import Dataset @@ -379,9 +380,13 @@ def validate_aggregate_arguments(self) -> None: # This property is used to determine if a query is using at least one of the fields in the spans namespace. @property def is_spans_metrics_query(self) -> bool: - for query in self.parse_query(self.query): - if query.key.name in constants.SPANS_METRICS_TAGS: - return True + if self.query is not None: + tags = parse_query( + self.params.projects, self.query, self.params.user, self.params.environments + )["tags"] + for tag in tags: + if tag in constants.SPANS_METRICS_TAGS: + return True for column in self.selected_columns: # Not using parse_function since it checks against function_converter # which is not loaded yet and we also do not need it @@ -416,7 +421,7 @@ def is_performance(self) -> bool: @property def use_case_id(self) -> UseCaseID: - if self.spans_metrics_builder: + if self.spans_metrics_builder or self.is_spans_metrics_query: return UseCaseID.SPANS elif self.is_performance: return UseCaseID.TRANSACTIONS @@ -718,7 +723,7 @@ def resolve_metric_index(self, value: str) -> int | None: """Layer on top of the metric indexer so we'll only hit it at most once per value""" if value not in self._indexer_cache: result = indexer.resolve( - UseCaseID.SPANS if self.is_spans_metrics_query else self.use_case_id, + self.use_case_id, self.organization_id, value, ) From 42da777d990cf5a053f42fe5797648953ddb764e Mon Sep 17 00:00:00 2001 From: Edward Gou Date: Fri, 9 Aug 2024 15:03:23 -0400 Subject: [PATCH 09/10] add test --- .../endpoints/test_organization_events_mep.py | 46 +++++++++++++++++++ 1 file changed, 46 insertions(+) diff --git a/tests/snuba/api/endpoints/test_organization_events_mep.py b/tests/snuba/api/endpoints/test_organization_events_mep.py index 438199e3045d44..eef0548d6b821a 100644 --- a/tests/snuba/api/endpoints/test_organization_events_mep.py +++ b/tests/snuba/api/endpoints/test_organization_events_mep.py @@ -3458,6 +3458,48 @@ def test_filtering_by_org_id_is_not_compatible(self): ) assert response.status_code == 400, response.content + def test_avg_span_self_time(self): + self.store_span_metric( + 1, + timestamp=self.min_ago, + ) + + self.store_span_metric( + 3, + timestamp=self.min_ago, + ) + + self.store_span_metric( + 3, + timestamp=self.min_ago, + ) + + self.store_span_metric( + 4, + timestamp=self.min_ago, + ) + + self.store_span_metric( + 5, + timestamp=self.min_ago, + ) + + response = self.do_request( + { + "field": [ + "avg(span.self_time)", + ], + "query": "", + "project": self.project.id, + "dataset": "metrics", + } + ) + + assert response.status_code == 200, response.content + data = response.data["data"] + assert len(data) == 1 + assert data[0]["avg(span.self_time)"] == 3.2 + class OrganizationEventsMetricsEnhancedPerformanceEndpointTestWithOnDemandMetrics( MetricsEnhancedPerformanceTestCase @@ -3946,3 +3988,7 @@ def test_timestamp_groupby(self): @pytest.mark.xfail(reason="Not implemented") def test_on_demand_with_mep(self): super().test_on_demand_with_mep() + + @pytest.mark.xfail(reason="Not implemented") + def test_avg_span_self_time(self): + super().test_avg_span_self_time() From 5279b83a3830cedec7cdb92049b6704bd778200c Mon Sep 17 00:00:00 2001 From: Edward Gou Date: Mon, 12 Aug 2024 10:36:49 -0400 Subject: [PATCH 10/10] add caching --- src/sentry/search/events/builder/metrics.py | 20 +++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-) diff --git a/src/sentry/search/events/builder/metrics.py b/src/sentry/search/events/builder/metrics.py index 3cb60879516718..ef314e38458e5a 100644 --- a/src/sentry/search/events/builder/metrics.py +++ b/src/sentry/search/events/builder/metrics.py @@ -122,6 +122,8 @@ def __init__( self._indexer_cache: dict[str, int | None] = {} self._use_default_tags: bool | None = None self._has_nullable: bool = False + self._is_spans_metrics_query_cache: bool | None = None + self._is_unsupported_metrics_layer_query_cache: bool | None = None # always true if this is being called config.has_metrics = True assert dataset is None or dataset in [Dataset.PerformanceMetrics, Dataset.Metrics] @@ -377,15 +379,18 @@ def validate_aggregate_arguments(self) -> None: if not self.use_metrics_layer: super().validate_aggregate_arguments() - # This property is used to determine if a query is using at least one of the fields in the spans namespace. @property def is_spans_metrics_query(self) -> bool: + """This property is used to determine if a query is using at least one of the fields in the spans namespace.""" + if self._is_spans_metrics_query_cache is not None: + return self._is_spans_metrics_query_cache if self.query is not None: tags = parse_query( self.params.projects, self.query, self.params.user, self.params.environments )["tags"] for tag in tags: if tag in constants.SPANS_METRICS_TAGS: + self._is_spans_metrics_query_cache = True return True for column in self.selected_columns: # Not using parse_function since it checks against function_converter @@ -393,17 +398,24 @@ def is_spans_metrics_query(self) -> bool: match = fields.is_function(column) func = match.group("function") if match else None if func in constants.SPANS_METRICS_FUNCTIONS: + self._is_spans_metrics_query_cache = True return True argument = match.group("columns") if match else None if argument in constants.SPAN_METRICS_MAP.keys() - constants.METRICS_MAP.keys(): + self._is_spans_metrics_query_cache = True return True + self._is_spans_metrics_query_cache = False return False - # Some fields and functions cannot be translated to metrics layer queries. - # This property is used to determine if a query is using at least one of these fields or functions, and if so, we must not use the metrics layer. @property def is_unsupported_metrics_layer_query(self) -> bool: + """Some fields and functions cannot be translated to metrics layer queries. + This property is used to determine if a query is using at least one of these fields or functions, and if so, we must not use the metrics layer. + """ + if self._is_unsupported_metrics_layer_query_cache is not None: + return self._is_unsupported_metrics_layer_query_cache if self.is_spans_metrics_query: + self._is_unsupported_metrics_layer_query_cache = True return True for column in self.selected_columns: # Not using parse_function since it checks against function_converter @@ -411,7 +423,9 @@ def is_unsupported_metrics_layer_query(self) -> bool: match = fields.is_function(column) func = match.group("function") if match else None if func in constants.METRICS_LAYER_UNSUPPORTED_TRANSACTION_METRICS_FUNCTIONS: + self._is_unsupported_metrics_layer_query_cache = True return True + self._is_unsupported_metrics_layer_query_cache = False return False @property