From 9684af34118d202fe3435a928a6d43b33c682884 Mon Sep 17 00:00:00 2001 From: Ogi <86684834+obostjancic@users.noreply.github.com> Date: Fri, 14 Jul 2023 13:05:51 +0200 Subject: [PATCH 01/21] feat(alerts): on demand metric alert chart --- src/sentry/search/events/builder/metrics.py | 34 +++++++++++++-------- src/sentry/snuba/metrics/extraction.py | 6 ++-- 2 files changed, 24 insertions(+), 16 deletions(-) diff --git a/src/sentry/search/events/builder/metrics.py b/src/sentry/search/events/builder/metrics.py index c287561d2d0a8..a4a6846ba5443 100644 --- a/src/sentry/search/events/builder/metrics.py +++ b/src/sentry/search/events/builder/metrics.py @@ -98,7 +98,7 @@ def _resolve_on_demand_spec( self, dataset: Optional[Dataset], selected_cols: List[Optional[str]], query: str ) -> Optional[OndemandMetricSpec]: field = selected_cols[0] if selected_cols else None - if not self.is_alerts_query or not field: + if not field: return None if not is_on_demand_query(dataset, field, query): @@ -111,13 +111,19 @@ def _resolve_on_demand_spec( return None def _get_on_demand_metrics_query(self) -> Optional[MetricsQuery]: - if not self.is_performance or not self.is_alerts_query: + if not self.is_performance: return None spec = self._on_demand_spec + # TODO(ogi): TimeseriesQueryBuilder somehow has a limit of 10000 which is too high for get_series + if not self.limit or self.limit.limit >= 10000: + limit = None + else: + limit = self.limit + return MetricsQuery( - select=[MetricField(spec.op, spec.mri, alias=spec.mri)], + select=[MetricField(spec.op, spec.mri, alias="count")], where=[ Condition( lhs=Column(QUERY_HASH_KEY), @@ -125,16 +131,16 @@ def _get_on_demand_metrics_query(self) -> Optional[MetricsQuery]: rhs=spec.query_hash(), ), ], - # TODO(ogi): groupby and orderby - limit=self.limit, + limit=limit, offset=self.offset, granularity=self.resolve_granularity(), + # interval=60, is_alerts_query=self.is_alerts_query, org_id=self.params.organization.id, project_ids=[p.id for p in self.params.projects], # We do not need the series here, as later, we only extract the totals and assign it to the # request.query - include_series=False, + include_series=True, start=self.params.start, end=self.params.end, ) @@ -768,7 +774,6 @@ def run_query(self, referrer: str, use_cache: bool = False) -> Any: metric_query = transform_mqb_query_to_metrics_query( self.get_metrics_layer_snql_query().query, self.is_alerts_query ) - # metric_query.where = metric_query_ondemand.where with sentry_sdk.start_span(op="metric_layer", description="run_query"): metrics_data = get_series( projects=self.params.projects, @@ -792,7 +797,7 @@ def run_query(self, referrer: str, use_cache: bool = False) -> Any: data.update(group["totals"]) metric_layer_result["data"].append(data) for meta in metric_layer_result["meta"]: - if data[meta["name"]] is None: + if data.get(meta["name"]) is None: data[meta["name"]] = self.get_default_value(meta["type"]) return metric_layer_result @@ -1180,7 +1185,7 @@ def get_snql_query(self) -> List[Request]: return queries def run_query(self, referrer: str, use_cache: bool = False) -> Any: - if self.use_metrics_layer: + if self.use_metrics_layer or self._on_demand_spec: from sentry.snuba.metrics.datasource import get_series from sentry.snuba.metrics.mqb_query_transformer import ( transform_mqb_query_to_metrics_query, @@ -1189,13 +1194,16 @@ def run_query(self, referrer: str, use_cache: bool = False) -> Any: snuba_query = self.get_snql_query()[0].query try: with sentry_sdk.start_span(op="metric_layer", description="transform_query"): - metric_query = transform_mqb_query_to_metrics_query( - snuba_query, self.is_alerts_query - ) + if self._on_demand_spec: + metrics_query = self._get_on_demand_metrics_query() + else: + metrics_query = transform_mqb_query_to_metrics_query( + snuba_query, self.is_alerts_query + ) with sentry_sdk.start_span(op="metric_layer", description="run_query"): metrics_data = get_series( projects=self.params.projects, - metrics_query=metric_query, + metrics_query=metrics_query, use_case_id=UseCaseID.TRANSACTIONS if self.is_performance else UseCaseID.SESSIONS, diff --git a/src/sentry/snuba/metrics/extraction.py b/src/sentry/snuba/metrics/extraction.py index c747391311d3e..c4497241f96e6 100644 --- a/src/sentry/snuba/metrics/extraction.py +++ b/src/sentry/snuba/metrics/extraction.py @@ -212,7 +212,7 @@ def is_on_demand_snuba_query(snuba_query: SnubaQuery) -> bool: def is_on_demand_query( - dataset: Optional[Union[str, Dataset]], aggregate: str, query: Optional[str] + dataset: Optional[Union[str, Dataset]], aggregate: str, query: Optional[str] ) -> bool: """Returns ``True`` if the dataset is performance metrics and query contains non-standard search fields.""" @@ -306,7 +306,7 @@ def __init__(self, field: str, query: str): # On-demand metrics are implicitly transaction metrics. Remove the # filter from the query since it can't be translated to a RuleCondition. - self._query = re.sub(r"event\.type:transaction\s*", "", query) + self._query = re.sub(r"event\.type:transaction\s*", "", query).strip() self._init_aggregate(field) def _init_aggregate(self, aggregate: str) -> None: @@ -320,7 +320,7 @@ def _init_aggregate(self, aggregate: str) -> None: # TODO: Add support for derived metrics: failure_rate, apdex, eps, epm, tps, tpm function, arguments, _alias = fields.parse_function(aggregate) assert ( - function in _AGGREGATE_TO_METRIC_TYPE and function in _SEARCH_TO_METRIC_AGGREGATES + function in _AGGREGATE_TO_METRIC_TYPE and function in _SEARCH_TO_METRIC_AGGREGATES ), f"Unsupported aggregate function {function}" self.metric_type = _AGGREGATE_TO_METRIC_TYPE[function] self.op = _SEARCH_TO_METRIC_AGGREGATES[function] From 68586dd56049d3887521776d0e21482a06cca0c7 Mon Sep 17 00:00:00 2001 From: Ogi <86684834+obostjancic@users.noreply.github.com> Date: Fri, 14 Jul 2023 13:12:20 +0200 Subject: [PATCH 02/21] fixes --- src/sentry/search/events/builder/metrics.py | 1 + 1 file changed, 1 insertion(+) diff --git a/src/sentry/search/events/builder/metrics.py b/src/sentry/search/events/builder/metrics.py index a4a6846ba5443..a4000f520d11b 100644 --- a/src/sentry/search/events/builder/metrics.py +++ b/src/sentry/search/events/builder/metrics.py @@ -123,6 +123,7 @@ def _get_on_demand_metrics_query(self) -> Optional[MetricsQuery]: limit = self.limit return MetricsQuery( + # FIXME: count alias is required in order to render chart since the serializer searches for that exact dict key select=[MetricField(spec.op, spec.mri, alias="count")], where=[ Condition( From c4ecde810169d0adba83d2722c70cf9301a54ac8 Mon Sep 17 00:00:00 2001 From: Ogi <86684834+obostjancic@users.noreply.github.com> Date: Fri, 14 Jul 2023 13:13:20 +0200 Subject: [PATCH 03/21] remove comment --- src/sentry/search/events/builder/metrics.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/sentry/search/events/builder/metrics.py b/src/sentry/search/events/builder/metrics.py index a4000f520d11b..aa06f0eadc700 100644 --- a/src/sentry/search/events/builder/metrics.py +++ b/src/sentry/search/events/builder/metrics.py @@ -139,8 +139,6 @@ def _get_on_demand_metrics_query(self) -> Optional[MetricsQuery]: is_alerts_query=self.is_alerts_query, org_id=self.params.organization.id, project_ids=[p.id for p in self.params.projects], - # We do not need the series here, as later, we only extract the totals and assign it to the - # request.query include_series=True, start=self.params.start, end=self.params.end, From e1d4ec48ddc01d49c5003e744e9480d1855d1a59 Mon Sep 17 00:00:00 2001 From: "getsantry[bot]" <66042841+getsantry[bot]@users.noreply.github.com> Date: Mon, 17 Jul 2023 07:00:53 +0000 Subject: [PATCH 04/21] :hammer_and_wrench: apply pre-commit fixes --- src/sentry/snuba/metrics/extraction.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/sentry/snuba/metrics/extraction.py b/src/sentry/snuba/metrics/extraction.py index c4497241f96e6..4691ad6dfbdf6 100644 --- a/src/sentry/snuba/metrics/extraction.py +++ b/src/sentry/snuba/metrics/extraction.py @@ -212,7 +212,7 @@ def is_on_demand_snuba_query(snuba_query: SnubaQuery) -> bool: def is_on_demand_query( - dataset: Optional[Union[str, Dataset]], aggregate: str, query: Optional[str] + dataset: Optional[Union[str, Dataset]], aggregate: str, query: Optional[str] ) -> bool: """Returns ``True`` if the dataset is performance metrics and query contains non-standard search fields.""" @@ -320,7 +320,7 @@ def _init_aggregate(self, aggregate: str) -> None: # TODO: Add support for derived metrics: failure_rate, apdex, eps, epm, tps, tpm function, arguments, _alias = fields.parse_function(aggregate) assert ( - function in _AGGREGATE_TO_METRIC_TYPE and function in _SEARCH_TO_METRIC_AGGREGATES + function in _AGGREGATE_TO_METRIC_TYPE and function in _SEARCH_TO_METRIC_AGGREGATES ), f"Unsupported aggregate function {function}" self.metric_type = _AGGREGATE_TO_METRIC_TYPE[function] self.op = _SEARCH_TO_METRIC_AGGREGATES[function] From a6229acbaed2435af81bc43319d33ade97620f39 Mon Sep 17 00:00:00 2001 From: Ogi <86684834+obostjancic@users.noreply.github.com> Date: Mon, 17 Jul 2023 09:42:23 +0200 Subject: [PATCH 05/21] is_timeseries_query --- src/sentry/search/events/builder/metrics.py | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/src/sentry/search/events/builder/metrics.py b/src/sentry/search/events/builder/metrics.py index aa06f0eadc700..cd5fbf7b86229 100644 --- a/src/sentry/search/events/builder/metrics.py +++ b/src/sentry/search/events/builder/metrics.py @@ -50,6 +50,7 @@ class MetricsQueryBuilder(QueryBuilder): requires_organization_condition = True is_alerts_query = False + is_timeseries_query = False organization_column: str = "organization_id" def __init__( @@ -116,15 +117,16 @@ def _get_on_demand_metrics_query(self) -> Optional[MetricsQuery]: spec = self._on_demand_spec - # TODO(ogi): TimeseriesQueryBuilder somehow has a limit of 10000 which is too high for get_series - if not self.limit or self.limit.limit >= 10000: + # TimeseriesQueryBuilder specific parameters + if self.is_timeseries_query: limit = None + alias = "count" else: limit = self.limit + alias = spec.mri return MetricsQuery( - # FIXME: count alias is required in order to render chart since the serializer searches for that exact dict key - select=[MetricField(spec.op, spec.mri, alias="count")], + select=[MetricField(spec.op, spec.mri, alias=alias)], where=[ Condition( lhs=Column(QUERY_HASH_KEY), @@ -135,7 +137,6 @@ def _get_on_demand_metrics_query(self) -> Optional[MetricsQuery]: limit=limit, offset=self.offset, granularity=self.resolve_granularity(), - # interval=60, is_alerts_query=self.is_alerts_query, org_id=self.params.organization.id, project_ids=[p.id for p in self.params.projects], @@ -1042,6 +1043,7 @@ def resolve_split_granularity(self) -> Tuple[List[Condition], Optional[Granulari class TimeseriesMetricQueryBuilder(MetricsQueryBuilder): time_alias = "time" + is_timeseries_query = True def __init__( self, From 6da4e6eb75e70b4f12d53f2384870aad8859d7c5 Mon Sep 17 00:00:00 2001 From: Ogi <86684834+obostjancic@users.noreply.github.com> Date: Mon, 17 Jul 2023 10:34:43 +0200 Subject: [PATCH 06/21] test fix --- src/sentry/snuba/metrics/extraction.py | 1 + 1 file changed, 1 insertion(+) diff --git a/src/sentry/snuba/metrics/extraction.py b/src/sentry/snuba/metrics/extraction.py index 4691ad6dfbdf6..5a6cf70697d84 100644 --- a/src/sentry/snuba/metrics/extraction.py +++ b/src/sentry/snuba/metrics/extraction.py @@ -136,6 +136,7 @@ "os.name", "geo.country_code", "event.type", + "project", ] # Operators used in ``ComparingRuleCondition``. From eb939c55c060f1389db9b7388781bd4288345810 Mon Sep 17 00:00:00 2001 From: Ogi <86684834+obostjancic@users.noreply.github.com> Date: Mon, 17 Jul 2023 11:18:12 +0200 Subject: [PATCH 07/21] remove use_performance condition check --- src/sentry/search/events/builder/metrics.py | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/sentry/search/events/builder/metrics.py b/src/sentry/search/events/builder/metrics.py index cd5fbf7b86229..4fa03ff8672c3 100644 --- a/src/sentry/search/events/builder/metrics.py +++ b/src/sentry/search/events/builder/metrics.py @@ -112,9 +112,6 @@ def _resolve_on_demand_spec( return None def _get_on_demand_metrics_query(self) -> Optional[MetricsQuery]: - if not self.is_performance: - return None - spec = self._on_demand_spec # TimeseriesQueryBuilder specific parameters From b8fc1cda82b07d17d64b3c0d8ffb5683ad809ac4 Mon Sep 17 00:00:00 2001 From: Ogi <86684834+obostjancic@users.noreply.github.com> Date: Mon, 17 Jul 2023 13:58:27 +0200 Subject: [PATCH 08/21] imporve test coverage --- .../search/events/builder/test_metrics.py | 39 +++++++++++++++++++ tests/sentry/snuba/test_extraction.py | 1 + 2 files changed, 40 insertions(+) diff --git a/tests/sentry/search/events/builder/test_metrics.py b/tests/sentry/search/events/builder/test_metrics.py index a2a9593a9b50b..36f2572a41a0e 100644 --- a/tests/sentry/search/events/builder/test_metrics.py +++ b/tests/sentry/search/events/builder/test_metrics.py @@ -1953,6 +1953,45 @@ def test_no_error_if_aggregates_disallowed_but_no_aggregates_included(self): allow_metric_aggregates=False, ) + def test_on_demand_metrics(self): + query = TimeseriesMetricQueryBuilder( + self.params, + dataset=Dataset.PerformanceMetrics, + interval=900, + query="transaction.duration:>0", + selected_columns=["count()"], + ) + result = query.run_query("test_query") + assert result["data"][:5] == [ + { + "time": self.start.isoformat(), + "count": 0, + }, + { + "time": (self.start + datetime.timedelta(hours=1)).isoformat(), + "count": 0, + }, + { + "time": (self.start + datetime.timedelta(hours=2)).isoformat(), + "count": 0, + }, + { + "time": (self.start + datetime.timedelta(hours=3)).isoformat(), + "count": 0, + }, + { + "time": (self.start + datetime.timedelta(hours=4)).isoformat(), + "count": 0, + }, + ] + self.assertCountEqual( + result["meta"], + [ + {"name": "time", "type": "DateTime('Universal')"}, + {"name": "count", "type": "Float64"}, + ], + ) + class HistogramMetricQueryBuilderTest(MetricBuilderBaseTest): def test_histogram_columns_set_on_builder(self): diff --git a/tests/sentry/snuba/test_extraction.py b/tests/sentry/snuba/test_extraction.py index 917e4e6a24580..675dfbf56e111 100644 --- a/tests/sentry/snuba/test_extraction.py +++ b/tests/sentry/snuba/test_extraction.py @@ -19,6 +19,7 @@ def test_is_on_demand_query_invalid_query(): assert is_on_demand_query(dataset, "count()", "AND") is False assert is_on_demand_query(dataset, "count()", "(AND transaction.duration:>=1") is False assert is_on_demand_query(dataset, "count()", "transaction.duration:>=abc") is False + assert is_on_demand_query(dataset, "count_if(}", "") is False def test_is_on_demand_query_true(): From 1eb6e1d24bd2e0c225447b953fe00ef0341309be Mon Sep 17 00:00:00 2001 From: Ogi <86684834+obostjancic@users.noreply.github.com> Date: Tue, 18 Jul 2023 09:17:31 +0200 Subject: [PATCH 09/21] remove project field --- src/sentry/snuba/metrics/extraction.py | 2 +- tests/sentry/search/events/builder/test_metrics.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/sentry/snuba/metrics/extraction.py b/src/sentry/snuba/metrics/extraction.py index 5a6cf70697d84..eab5be778f6b1 100644 --- a/src/sentry/snuba/metrics/extraction.py +++ b/src/sentry/snuba/metrics/extraction.py @@ -135,8 +135,8 @@ "browser.name", "os.name", "geo.country_code", + # event.type is accepted as a valid search field but it will not me converted to a condition for metric extraction "event.type", - "project", ] # Operators used in ``ComparingRuleCondition``. diff --git a/tests/sentry/search/events/builder/test_metrics.py b/tests/sentry/search/events/builder/test_metrics.py index 36f2572a41a0e..af22e19c6a96a 100644 --- a/tests/sentry/search/events/builder/test_metrics.py +++ b/tests/sentry/search/events/builder/test_metrics.py @@ -1697,7 +1697,7 @@ def test_transaction_in_filter(self): def test_project_filter(self): query = TimeseriesMetricQueryBuilder( self.params, - dataset=Dataset.PerformanceMetrics, + dataset=Dataset.Metrics, interval=900, query=f"project:{self.project.slug}", selected_columns=["p95(transaction.duration)"], From b63451eac28abbd2f3d35da13b3c110b62305a1b Mon Sep 17 00:00:00 2001 From: Ogi <86684834+obostjancic@users.noreply.github.com> Date: Tue, 18 Jul 2023 13:20:14 +0200 Subject: [PATCH 10/21] add experimental feature flag --- src/sentry/snuba/metrics/extraction.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/sentry/snuba/metrics/extraction.py b/src/sentry/snuba/metrics/extraction.py index eab5be778f6b1..b1448dc1fc8f2 100644 --- a/src/sentry/snuba/metrics/extraction.py +++ b/src/sentry/snuba/metrics/extraction.py @@ -17,6 +17,7 @@ from typing_extensions import NotRequired +from sentry import features from sentry.api import event_search from sentry.api.event_search import ParenExpression, SearchFilter from sentry.exceptions import InvalidSearchQuery @@ -216,6 +217,8 @@ def is_on_demand_query( dataset: Optional[Union[str, Dataset]], aggregate: str, query: Optional[str] ) -> bool: """Returns ``True`` if the dataset is performance metrics and query contains non-standard search fields.""" + if features.has("organizations:on-demand-metrics-extraction-experimental", None): + return False if not dataset or Dataset(dataset) != Dataset.PerformanceMetrics: return False From b82d399ba09e7823e79e2c2a261eb1dc734a551a Mon Sep 17 00:00:00 2001 From: Ogi <86684834+obostjancic@users.noreply.github.com> Date: Wed, 19 Jul 2023 15:17:36 +0200 Subject: [PATCH 11/21] switched to feature flag --- .../endpoints/organization_events_stats.py | 3 + src/sentry/search/events/builder/discover.py | 2 + src/sentry/search/events/builder/metrics.py | 35 ++++--- src/sentry/snuba/entity_subscription.py | 5 + src/sentry/snuba/metrics/extraction.py | 50 +++++++--- .../search/events/builder/test_metrics.py | 96 +++++++++---------- tests/sentry/snuba/test_extraction.py | 9 +- 7 files changed, 125 insertions(+), 75 deletions(-) diff --git a/src/sentry/api/endpoints/organization_events_stats.py b/src/sentry/api/endpoints/organization_events_stats.py index c5eea994816c2..325f8f445bf5e 100644 --- a/src/sentry/api/endpoints/organization_events_stats.py +++ b/src/sentry/api/endpoints/organization_events_stats.py @@ -211,6 +211,9 @@ def get_event_stats( allow_metric_aggregates=allow_metric_aggregates, has_metrics=use_metrics, use_metrics_layer=batch_features.get("organizations:use-metrics-layer", False), + on_demand_metrics_enabled=batch_features.get( + "organizations:on-demand-metrics-extraction", False + ), ) try: diff --git a/src/sentry/search/events/builder/discover.py b/src/sentry/search/events/builder/discover.py index fb9d5abb523ed..de7c1168d05c8 100644 --- a/src/sentry/search/events/builder/discover.py +++ b/src/sentry/search/events/builder/discover.py @@ -219,6 +219,7 @@ def __init__( # Currently this is only used for avoiding conflicting values when doing the first query # of a top events request skip_tag_resolution: bool = False, + on_demand_metrics_enabled: bool = False, ): self.dataset = dataset @@ -234,6 +235,7 @@ def __init__( self.transform_alias_to_input_format = transform_alias_to_input_format self.raw_equations = equations self.use_metrics_layer = use_metrics_layer + self.on_demand_metrics_enabled = on_demand_metrics_enabled self.auto_fields = auto_fields self.query = query self.groupby_columns = groupby_columns diff --git a/src/sentry/search/events/builder/metrics.py b/src/sentry/search/events/builder/metrics.py index 4fa03ff8672c3..c486c929ba80c 100644 --- a/src/sentry/search/events/builder/metrics.py +++ b/src/sentry/search/events/builder/metrics.py @@ -75,10 +75,6 @@ def __init__( kwargs["has_metrics"] = True assert dataset is None or dataset in [Dataset.PerformanceMetrics, Dataset.Metrics] - self._on_demand_spec = self._resolve_on_demand_spec( - dataset, kwargs.get("selected_columns", []), kwargs.get("query", "") - ) - if granularity is not None: self._granularity = granularity super().__init__( @@ -88,6 +84,10 @@ def __init__( *args, **kwargs, ) + self._on_demand_spec = self._resolve_on_demand_spec( + dataset, kwargs.get("selected_columns", []), kwargs.get("query", "") + ) + org_id = self.filter_params.get("organization_id") if org_id is None and self.params.organization is not None: org_id = self.params.organization.id @@ -98,6 +98,9 @@ def __init__( def _resolve_on_demand_spec( self, dataset: Optional[Dataset], selected_cols: List[Optional[str]], query: str ) -> Optional[OndemandMetricSpec]: + if not self.on_demand_metrics_enabled: + return None + field = selected_cols[0] if selected_cols else None if not field: return None @@ -200,7 +203,7 @@ def resolve_column_name(self, col: str) -> str: col = tag_match.group("tag") if tag_match else col # on-demand metrics require metrics layer behavior - if self.use_metrics_layer or self._on_demand_spec: + if self.use_metrics_layer or self.on_demand_metrics_enabled: if col in ["project_id", "timestamp"]: return col # TODO: update resolve params so this isn't needed @@ -566,7 +569,7 @@ def get_metrics_layer_snql_query(self) -> Request: snuba SDK. """ - if not self.use_metrics_layer and not self._on_demand_spec: + if not self.use_metrics_layer and not self.on_demand_metrics_enabled: # The reasoning for this error is because if "use_metrics_layer" is false, the MQB will not generate the # snql dialect explained below as there is not need for that because it will directly generate normal snql # that can be returned via the "get_snql_query" method. @@ -757,7 +760,7 @@ def validate_orderby_clause(self) -> None: raise IncompatibleMetricsQuery("Can't orderby tags") def run_query(self, referrer: str, use_cache: bool = False) -> Any: - if self.use_metrics_layer or self._on_demand_spec: + if self.use_metrics_layer or self.on_demand_metrics_enabled: from sentry.snuba.metrics.datasource import get_series from sentry.snuba.metrics.mqb_query_transformer import ( transform_mqb_query_to_metrics_query, @@ -765,16 +768,16 @@ def run_query(self, referrer: str, use_cache: bool = False) -> Any: try: with sentry_sdk.start_span(op="metric_layer", description="transform_query"): - if self._on_demand_spec: - metric_query = self._get_on_demand_metrics_query() + if self.on_demand_metrics_enabled: + metrics_query = self._get_on_demand_metrics_query() else: - metric_query = transform_mqb_query_to_metrics_query( + metrics_query = transform_mqb_query_to_metrics_query( self.get_metrics_layer_snql_query().query, self.is_alerts_query ) with sentry_sdk.start_span(op="metric_layer", description="run_query"): metrics_data = get_series( projects=self.params.projects, - metrics_query=metric_query, + metrics_query=metrics_query, use_case_id=UseCaseID.TRANSACTIONS if self.is_performance else UseCaseID.SESSIONS, @@ -956,7 +959,7 @@ def get_snql_query(self) -> Request: and returns one or more equivalent snql query(ies). """ - if self.use_metrics_layer or self._on_demand_spec: + if self.use_metrics_layer or self.on_demand_metrics_enabled: from sentry.snuba.metrics import SnubaQueryBuilder from sentry.snuba.metrics.mqb_query_transformer import ( transform_mqb_query_to_metrics_query, @@ -964,7 +967,7 @@ def get_snql_query(self) -> Request: snuba_request = self.get_metrics_layer_snql_query() - if self._on_demand_spec: + if self.on_demand_metrics_enabled: metrics_query = self._get_on_demand_metrics_query() else: metrics_query = transform_mqb_query_to_metrics_query( @@ -1054,6 +1057,7 @@ def __init__( limit: Optional[int] = 10000, use_metrics_layer: Optional[bool] = False, groupby: Optional[Column] = None, + on_demand_metrics_enabled: Optional[bool] = False, ): super().__init__( params=params, @@ -1064,6 +1068,7 @@ def __init__( auto_fields=False, functions_acl=functions_acl, use_metrics_layer=use_metrics_layer, + on_demand_metrics_enabled=on_demand_metrics_enabled, ) if self.granularity.granularity > interval: for granularity in constants.METRICS_GRANULARITIES: @@ -1183,7 +1188,7 @@ def get_snql_query(self) -> List[Request]: return queries def run_query(self, referrer: str, use_cache: bool = False) -> Any: - if self.use_metrics_layer or self._on_demand_spec: + if self.use_metrics_layer or self.on_demand_metrics_enabled: from sentry.snuba.metrics.datasource import get_series from sentry.snuba.metrics.mqb_query_transformer import ( transform_mqb_query_to_metrics_query, @@ -1192,7 +1197,7 @@ def run_query(self, referrer: str, use_cache: bool = False) -> Any: snuba_query = self.get_snql_query()[0].query try: with sentry_sdk.start_span(op="metric_layer", description="transform_query"): - if self._on_demand_spec: + if self.on_demand_metrics_enabled: metrics_query = self._get_on_demand_metrics_query() else: metrics_query = transform_mqb_query_to_metrics_query( diff --git a/src/sentry/snuba/entity_subscription.py b/src/sentry/snuba/entity_subscription.py index 093df7111371c..b79a888560e40 100644 --- a/src/sentry/snuba/entity_subscription.py +++ b/src/sentry/snuba/entity_subscription.py @@ -305,6 +305,10 @@ def __init__( self.use_metrics_layer = features.has( "organizations:use-metrics-layer", Organization.objects.get_from_cache(id=self.org_id) ) + self.on_demand_metrics_enabled = features.has( + "organizations:on-demand-metrics-extraction", + Organization.objects.get_from_cache(id=self.org_id), + ) @abstractmethod def get_snql_aggregations(self) -> List[str]: @@ -378,6 +382,7 @@ def build_query_builder( skip_time_conditions=True, granularity=self.get_granularity(), use_metrics_layer=self.use_metrics_layer, + on_demand_metrics_enabled=self.on_demand_metrics_enabled, ) extra_conditions = self.get_snql_extra_conditions() diff --git a/src/sentry/snuba/metrics/extraction.py b/src/sentry/snuba/metrics/extraction.py index b1448dc1fc8f2..6e62a5303e6bf 100644 --- a/src/sentry/snuba/metrics/extraction.py +++ b/src/sentry/snuba/metrics/extraction.py @@ -17,7 +17,6 @@ from typing_extensions import NotRequired -from sentry import features from sentry.api import event_search from sentry.api.event_search import ParenExpression, SearchFilter from sentry.exceptions import InvalidSearchQuery @@ -122,7 +121,8 @@ "p99": "d", } -# Query fields that on their own do not require on-demand metric extraction. +# Query fields that on their own do not require on-demand metric extraction but if present in an on-demand query +# will be converted to metric extraction conditions. _STANDARD_METRIC_FIELDS = [ "release", "dist", @@ -136,8 +136,9 @@ "browser.name", "os.name", "geo.country_code", - # event.type is accepted as a valid search field but it will not me converted to a condition for metric extraction + # These fields are skipped during on demand spec generation and will not be converted to metric extraction conditions "event.type", + "project", ] # Operators used in ``ComparingRuleCondition``. @@ -217,8 +218,6 @@ def is_on_demand_query( dataset: Optional[Union[str, Dataset]], aggregate: str, query: Optional[str] ) -> bool: """Returns ``True`` if the dataset is performance metrics and query contains non-standard search fields.""" - if features.has("organizations:on-demand-metrics-extraction-experimental", None): - return False if not dataset or Dataset(dataset) != Dataset.PerformanceMetrics: return False @@ -226,9 +225,8 @@ def is_on_demand_query( for field in _get_aggregate_fields(aggregate): if not _is_standard_metrics_field(field): return True - try: - return not _is_standard_metrics_query(event_search.parse_search_query(query or "")) + return not is_standard_metrics_query(query) except InvalidSearchQuery: logger.error(f"Failed to parse search query: {query}", exc_info=True) return False @@ -254,6 +252,10 @@ def _get_aggregate_fields(aggregate: str) -> Sequence[str]: return [] +def is_standard_metrics_query(query: Optional[str] = "") -> bool: + return _is_standard_metrics_query(event_search.parse_search_query(query)) + + def _is_standard_metrics_query(tokens: Sequence[QueryToken]) -> bool: """ Recursively checks if any of the supplied token contain search filters that can't be handled by standard metrics. @@ -293,7 +295,7 @@ class OndemandMetricSpec: op: MetricOperationType # The original query used to construct the metric spec. - _query: str + query: Sequence[SearchFilter] # Rule condition parsed from the aggregate field expression. _field_condition: Optional[RuleCondition] @@ -308,11 +310,37 @@ def __init__(self, field: str, query: str): """ - # On-demand metrics are implicitly transaction metrics. Remove the - # filter from the query since it can't be translated to a RuleCondition. - self._query = re.sub(r"event\.type:transaction\s*", "", query).strip() + self._init__query(query) self._init_aggregate(field) + def _init__query(self, query: str) -> None: + # On-demand metrics are implicitly transaction metrics. Remove the + # filters from the query that can't be translated to a RuleCondition. + query = re.sub(r"event\.type:transaction\s*", "", query) + query = re.sub(r"project:\w+\s*", "", query) + + self._query = query.strip() + + def _is_standard_metrics_query(tokens: Sequence[QueryToken]) -> bool: + """ + Recursively checks if any of the supplied token contain search filters that can't be handled by standard metrics. + """ + + for token in tokens: + if not _is_standard_metrics_search_filter(token): + return False + + return True + + def _is_standard_metrics_search_filter(token: QueryToken) -> bool: + if isinstance(token, SearchFilter): + return _is_standard_metrics_field(token.key.name) + + if isinstance(token, ParenExpression): + return _is_standard_metrics_query(token.children) + + return True + def _init_aggregate(self, aggregate: str) -> None: """ Extracts the field name, metric type and metric operation from a Discover diff --git a/tests/sentry/search/events/builder/test_metrics.py b/tests/sentry/search/events/builder/test_metrics.py index af22e19c6a96a..0cf0eec4b3146 100644 --- a/tests/sentry/search/events/builder/test_metrics.py +++ b/tests/sentry/search/events/builder/test_metrics.py @@ -22,7 +22,6 @@ from sentry.snuba.dataset import Dataset from sentry.snuba.metrics.extraction import QUERY_HASH_KEY from sentry.testutils.cases import MetricsEnhancedPerformanceTestCase -from sentry.testutils.helpers import Feature pytestmark = pytest.mark.sentry_metrics @@ -1697,7 +1696,7 @@ def test_transaction_in_filter(self): def test_project_filter(self): query = TimeseriesMetricQueryBuilder( self.params, - dataset=Dataset.Metrics, + dataset=Dataset.PerformanceMetrics, interval=900, query=f"project:{self.project.slug}", selected_columns=["p95(transaction.duration)"], @@ -1960,6 +1959,7 @@ def test_on_demand_metrics(self): interval=900, query="transaction.duration:>0", selected_columns=["count()"], + on_demand_metrics_enabled=True, ) result = query.run_query("test_query") assert result["data"][:5] == [ @@ -2095,53 +2095,53 @@ def test_query_normal_distribution(self): class AlertMetricsQueryBuilderTest(MetricBuilderBaseTest): def test_get_snql_query(self): - with Feature("organizations:on-demand-metrics-extraction"): - query = AlertMetricsQueryBuilder( - self.params, - use_metrics_layer=True, - granularity=3600, - query="transaction.duration:>=100", - dataset=Dataset.PerformanceMetrics, - selected_columns=["p75(measurements.fp)"], - ) + query = AlertMetricsQueryBuilder( + self.params, + use_metrics_layer=True, + granularity=3600, + query="transaction.duration:>=100", + dataset=Dataset.PerformanceMetrics, + selected_columns=["p75(measurements.fp)"], + on_demand_metrics_enabled=True, + ) - snql_request = query.get_snql_query() - assert snql_request.dataset == "generic_metrics" - snql_query = snql_request.query - self.assertCountEqual( - [ - Function( - "arrayElement", - [ - Function( - "quantilesIf(0.75)", - [ - Column("value"), - Function( - "equals", - [ - Column("metric_id"), - indexer.resolve( - UseCaseID.TRANSACTIONS, - None, - "d:transactions/on_demand@none", - ), - ], - ), - ], - ), - 1, - ], - "d:transactions/on_demand@none", - ) - ], - snql_query.select, - ) + snql_request = query.get_snql_query() + assert snql_request.dataset == "generic_metrics" + snql_query = snql_request.query + self.assertCountEqual( + [ + Function( + "arrayElement", + [ + Function( + "quantilesIf(0.75)", + [ + Column("value"), + Function( + "equals", + [ + Column("metric_id"), + indexer.resolve( + UseCaseID.TRANSACTIONS, + None, + "d:transactions/on_demand@none", + ), + ], + ), + ], + ), + 1, + ], + "d:transactions/on_demand@none", + ) + ], + snql_query.select, + ) - query_hash_index = indexer.resolve(UseCaseID.TRANSACTIONS, None, QUERY_HASH_KEY) + query_hash_index = indexer.resolve(UseCaseID.TRANSACTIONS, None, QUERY_HASH_KEY) - query_hash_clause = Condition( - lhs=Column(name=f"tags_raw[{query_hash_index}]"), op=Op.EQ, rhs="80237309" - ) + query_hash_clause = Condition( + lhs=Column(name=f"tags_raw[{query_hash_index}]"), op=Op.EQ, rhs="80237309" + ) - assert query_hash_clause in snql_query.where + assert query_hash_clause in snql_query.where diff --git a/tests/sentry/snuba/test_extraction.py b/tests/sentry/snuba/test_extraction.py index 675dfbf56e111..977a7d5bceaa9 100644 --- a/tests/sentry/snuba/test_extraction.py +++ b/tests/sentry/snuba/test_extraction.py @@ -17,7 +17,7 @@ def test_is_on_demand_query_invalid_query(): dataset = Dataset.PerformanceMetrics assert is_on_demand_query(dataset, "count()", "AND") is False - assert is_on_demand_query(dataset, "count()", "(AND transaction.duration:>=1") is False + assert is_on_demand_query(dataset, "count()", ")AND transaction.duration:>=1") is False assert is_on_demand_query(dataset, "count()", "transaction.duration:>=abc") is False assert is_on_demand_query(dataset, "count_if(}", "") is False @@ -176,3 +176,10 @@ def test_spec_countif_with_query(): {"name": "event.duration", "op": "eq", "value": 300.0}, ], } + + +def test_ignore_fields(): + with_ignored_field = OndemandMetricSpec("count()", "transaction.duration:>=1 project:sentry") + without_ignored_field = OndemandMetricSpec("count()", "transaction.duration:>=1") + + assert with_ignored_field.condition() == without_ignored_field.condition() From b604e7368172f6ceddb1194e031ea9e80e097abe Mon Sep 17 00:00:00 2001 From: Ogi <86684834+obostjancic@users.noreply.github.com> Date: Wed, 19 Jul 2023 15:30:25 +0200 Subject: [PATCH 12/21] type fix --- src/sentry/snuba/metrics/extraction.py | 20 -------------------- 1 file changed, 20 deletions(-) diff --git a/src/sentry/snuba/metrics/extraction.py b/src/sentry/snuba/metrics/extraction.py index 6e62a5303e6bf..482b429fe9de6 100644 --- a/src/sentry/snuba/metrics/extraction.py +++ b/src/sentry/snuba/metrics/extraction.py @@ -321,26 +321,6 @@ def _init__query(self, query: str) -> None: self._query = query.strip() - def _is_standard_metrics_query(tokens: Sequence[QueryToken]) -> bool: - """ - Recursively checks if any of the supplied token contain search filters that can't be handled by standard metrics. - """ - - for token in tokens: - if not _is_standard_metrics_search_filter(token): - return False - - return True - - def _is_standard_metrics_search_filter(token: QueryToken) -> bool: - if isinstance(token, SearchFilter): - return _is_standard_metrics_field(token.key.name) - - if isinstance(token, ParenExpression): - return _is_standard_metrics_query(token.children) - - return True - def _init_aggregate(self, aggregate: str) -> None: """ Extracts the field name, metric type and metric operation from a Discover From 984bd99b5675e9339c2df67d92a5fce5901202be Mon Sep 17 00:00:00 2001 From: Ogi <86684834+obostjancic@users.noreply.github.com> Date: Wed, 19 Jul 2023 16:07:40 +0200 Subject: [PATCH 13/21] add default param --- src/sentry/snuba/discover.py | 1 + src/sentry/snuba/functions.py | 2 ++ src/sentry/snuba/issue_platform.py | 1 + src/sentry/snuba/metrics_enhanced_performance.py | 2 ++ src/sentry/snuba/metrics_performance.py | 3 +++ src/sentry/snuba/profiles.py | 2 ++ src/sentry/snuba/spans_indexed.py | 1 + src/sentry/snuba/spans_metrics.py | 1 + 8 files changed, 13 insertions(+) diff --git a/src/sentry/snuba/discover.py b/src/sentry/snuba/discover.py index 66cb295d8fe5d..a52bfe1dc4aab 100644 --- a/src/sentry/snuba/discover.py +++ b/src/sentry/snuba/discover.py @@ -262,6 +262,7 @@ def timeseries_query( allow_metric_aggregates=False, has_metrics=False, use_metrics_layer=False, + on_demand_metrics_enabled=False, ): """ High-level API for doing arbitrary user timeseries queries against events. diff --git a/src/sentry/snuba/functions.py b/src/sentry/snuba/functions.py index 98f9abca2952a..1c0ceba92eee2 100644 --- a/src/sentry/snuba/functions.py +++ b/src/sentry/snuba/functions.py @@ -37,6 +37,7 @@ def query( has_metrics: bool = False, functions_acl: Optional[List[str]] = None, use_metrics_layer: bool = False, + on_demand_metrics_enabled: bool = False, ) -> Any: if not selected_columns: raise InvalidSearchQuery("No columns selected") @@ -73,6 +74,7 @@ def timeseries_query( allow_metric_aggregates: bool = False, has_metrics: bool = False, use_metrics_layer: bool = False, + on_demand_metrics_enabled: bool = False, ) -> Any: builder = ProfileFunctionsTimeseriesQueryBuilder( dataset=Dataset.Functions, diff --git a/src/sentry/snuba/issue_platform.py b/src/sentry/snuba/issue_platform.py index c8918e67644f7..9aaa38b6a4db9 100644 --- a/src/sentry/snuba/issue_platform.py +++ b/src/sentry/snuba/issue_platform.py @@ -109,6 +109,7 @@ def timeseries_query( allow_metric_aggregates=False, has_metrics=False, use_metrics_layer=False, + on_demand_metrics_enabled=False, ): """ High-level API for doing arbitrary user timeseries queries against events. diff --git a/src/sentry/snuba/metrics_enhanced_performance.py b/src/sentry/snuba/metrics_enhanced_performance.py index 07e445a62194a..c27b2d0d0f1d5 100644 --- a/src/sentry/snuba/metrics_enhanced_performance.py +++ b/src/sentry/snuba/metrics_enhanced_performance.py @@ -31,6 +31,7 @@ def query( transform_alias_to_input_format=False, has_metrics: bool = True, use_metrics_layer: bool = False, + on_demand_metrics_enabled: bool = False, ): metrics_compatible = not equations @@ -105,6 +106,7 @@ def timeseries_query( functions_acl: Optional[List[str]] = None, has_metrics: bool = True, use_metrics_layer: bool = False, + on_demand_metrics_enabled: bool = False, ) -> SnubaTSResult: """ High-level API for doing arbitrary user timeseries queries against events. diff --git a/src/sentry/snuba/metrics_performance.py b/src/sentry/snuba/metrics_performance.py index a0209ce6c69cb..889c2bec34403 100644 --- a/src/sentry/snuba/metrics_performance.py +++ b/src/sentry/snuba/metrics_performance.py @@ -37,6 +37,7 @@ def query( transform_alias_to_input_format=False, has_metrics: bool = True, use_metrics_layer: bool = False, + on_demand_metrics_enabled: bool = False, granularity: Optional[int] = None, ): with sentry_sdk.start_span(op="mep", description="MetricQueryBuilder"): @@ -81,6 +82,7 @@ def bulk_timeseries_query( functions_acl: Optional[List[str]] = None, has_metrics: bool = True, use_metrics_layer: bool = False, + on_demand_metrics_enabled: bool = False, groupby: Optional[Column] = None, apply_formatting: Optional[bool] = True, ) -> SnubaTSResult: @@ -179,6 +181,7 @@ def timeseries_query( functions_acl: Optional[List[str]] = None, has_metrics: bool = True, use_metrics_layer: bool = False, + on_demand_metrics_enabled: bool = False, groupby: Optional[Column] = None, ) -> SnubaTSResult: """ diff --git a/src/sentry/snuba/profiles.py b/src/sentry/snuba/profiles.py index 2c0d868af39f0..a8d7c2f805572 100644 --- a/src/sentry/snuba/profiles.py +++ b/src/sentry/snuba/profiles.py @@ -28,6 +28,7 @@ def query( has_metrics: bool = False, functions_acl: Optional[List[str]] = None, use_metrics_layer: bool = False, + on_demand_metrics_enabled: bool = False, ) -> Any: if not selected_columns: raise InvalidSearchQuery("No columns selected") @@ -64,6 +65,7 @@ def timeseries_query( allow_metric_aggregates: bool = False, has_metrics: bool = False, use_metrics_layer: bool = False, + on_demand_metrics_enabled: bool = False, ) -> Any: builder = ProfilesTimeseriesQueryBuilder( dataset=Dataset.Profiles, diff --git a/src/sentry/snuba/spans_indexed.py b/src/sentry/snuba/spans_indexed.py index 7492158036cc5..d85900343ff32 100644 --- a/src/sentry/snuba/spans_indexed.py +++ b/src/sentry/snuba/spans_indexed.py @@ -78,6 +78,7 @@ def timeseries_query( functions_acl: Optional[List[str]] = None, has_metrics: bool = True, use_metrics_layer: bool = False, + on_demand_metrics_enabled: bool = False, ) -> SnubaTSResult: """ High-level API for doing arbitrary user timeseries queries against events. diff --git a/src/sentry/snuba/spans_metrics.py b/src/sentry/snuba/spans_metrics.py index eb71ef8f2d2b3..3d154b840e802 100644 --- a/src/sentry/snuba/spans_metrics.py +++ b/src/sentry/snuba/spans_metrics.py @@ -77,6 +77,7 @@ def timeseries_query( functions_acl: Optional[List[str]] = None, has_metrics: bool = True, use_metrics_layer: bool = False, + on_demand_metrics_enabled: bool = False, groupby: Optional[Column] = None, ) -> SnubaTSResult: """ From cfaf43174bd529d1e0b1991d9f14099115b51632 Mon Sep 17 00:00:00 2001 From: Ogi <86684834+obostjancic@users.noreply.github.com> Date: Wed, 19 Jul 2023 17:04:15 +0200 Subject: [PATCH 14/21] improve test coverage --- .../search/events/builder/test_metrics.py | 37 ++++++++++++++++++- 1 file changed, 36 insertions(+), 1 deletion(-) diff --git a/tests/sentry/search/events/builder/test_metrics.py b/tests/sentry/search/events/builder/test_metrics.py index 0cf0eec4b3146..1cdd9ff4dc60f 100644 --- a/tests/sentry/search/events/builder/test_metrics.py +++ b/tests/sentry/search/events/builder/test_metrics.py @@ -2094,10 +2094,45 @@ def test_query_normal_distribution(self): class AlertMetricsQueryBuilderTest(MetricBuilderBaseTest): + def test_standard_metrics(self): + query = AlertMetricsQueryBuilder( + self.params, + use_metrics_layer=False, + granularity=3600, + query="", + dataset=Dataset.PerformanceMetrics, + selected_columns=["p75(measurements.fp)"], + on_demand_metrics_enabled=True, + ) + + assert query.on_demand_metrics_enabled + assert not query._on_demand_spec + + def test_run_on_demand_query(self): + query = AlertMetricsQueryBuilder( + self.params, + use_metrics_layer=False, + granularity=3600, + query="transaction.duration:>=100", + dataset=Dataset.PerformanceMetrics, + selected_columns=["p75(measurements.fp)"], + on_demand_metrics_enabled=True, + ) + + result = query.run_query("test_query") + + assert len(result["data"]) == 1 + + meta = result["meta"] + + assert len(meta) == 2 + assert meta[0]["name"] == "bucketed_time" + assert meta[1]["name"] == "d:transactions/on_demand@none" + def test_get_snql_query(self): query = AlertMetricsQueryBuilder( self.params, - use_metrics_layer=True, + use_metrics_layer=False, granularity=3600, query="transaction.duration:>=100", dataset=Dataset.PerformanceMetrics, From 0e226b3f6b9636ee5b433ddbc63e0ea345429675 Mon Sep 17 00:00:00 2001 From: Ogi <86684834+obostjancic@users.noreply.github.com> Date: Thu, 20 Jul 2023 09:21:45 +0200 Subject: [PATCH 15/21] pass flag to timeseries query --- src/sentry/api/endpoints/organization_events_stats.py | 1 + src/sentry/snuba/metrics_performance.py | 1 + 2 files changed, 2 insertions(+) diff --git a/src/sentry/api/endpoints/organization_events_stats.py b/src/sentry/api/endpoints/organization_events_stats.py index 73eee0637599c..027222d68b7e8 100644 --- a/src/sentry/api/endpoints/organization_events_stats.py +++ b/src/sentry/api/endpoints/organization_events_stats.py @@ -97,6 +97,7 @@ def get_features(self, organization: Organization, request: Request) -> Mapping[ "organizations:mep-rollout-flag", "organizations:use-metrics-layer", "organizations:starfish-view", + "organizations:on-demand-metrics-extraction", ] batch_features = features.batch_has( feature_names, diff --git a/src/sentry/snuba/metrics_performance.py b/src/sentry/snuba/metrics_performance.py index 889c2bec34403..bade029baee15 100644 --- a/src/sentry/snuba/metrics_performance.py +++ b/src/sentry/snuba/metrics_performance.py @@ -205,6 +205,7 @@ def timeseries_query( allow_metric_aggregates=allow_metric_aggregates, use_metrics_layer=use_metrics_layer, groupby=groupby, + on_demand_metrics_enabled=on_demand_metrics_enabled, ) metrics_referrer = referrer + ".metrics-enhanced" result = metrics_query.run_query(metrics_referrer) From abd36251f2ae61dd71e1bdcea5c168fea9cd11a9 Mon Sep 17 00:00:00 2001 From: Ogi <86684834+obostjancic@users.noreply.github.com> Date: Thu, 20 Jul 2023 10:53:44 +0200 Subject: [PATCH 16/21] fix _query --- src/sentry/snuba/metrics/extraction.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/sentry/snuba/metrics/extraction.py b/src/sentry/snuba/metrics/extraction.py index 482b429fe9de6..bba0fd9308eec 100644 --- a/src/sentry/snuba/metrics/extraction.py +++ b/src/sentry/snuba/metrics/extraction.py @@ -295,7 +295,7 @@ class OndemandMetricSpec: op: MetricOperationType # The original query used to construct the metric spec. - query: Sequence[SearchFilter] + _query: str # Rule condition parsed from the aggregate field expression. _field_condition: Optional[RuleCondition] From 3b2fd6f2b2d91d6df09a3022cac1f4e1235330ee Mon Sep 17 00:00:00 2001 From: Ogi <86684834+obostjancic@users.noreply.github.com> Date: Thu, 20 Jul 2023 13:20:57 +0200 Subject: [PATCH 17/21] address pr comments --- src/sentry/snuba/metrics/extraction.py | 6 +----- tests/sentry/search/events/builder/test_metrics.py | 1 - 2 files changed, 1 insertion(+), 6 deletions(-) diff --git a/src/sentry/snuba/metrics/extraction.py b/src/sentry/snuba/metrics/extraction.py index bba0fd9308eec..0714584db19fa 100644 --- a/src/sentry/snuba/metrics/extraction.py +++ b/src/sentry/snuba/metrics/extraction.py @@ -226,7 +226,7 @@ def is_on_demand_query( if not _is_standard_metrics_field(field): return True try: - return not is_standard_metrics_query(query) + return not _is_standard_metrics_query(event_search.parse_search_query(query)) except InvalidSearchQuery: logger.error(f"Failed to parse search query: {query}", exc_info=True) return False @@ -252,10 +252,6 @@ def _get_aggregate_fields(aggregate: str) -> Sequence[str]: return [] -def is_standard_metrics_query(query: Optional[str] = "") -> bool: - return _is_standard_metrics_query(event_search.parse_search_query(query)) - - def _is_standard_metrics_query(tokens: Sequence[QueryToken]) -> bool: """ Recursively checks if any of the supplied token contain search filters that can't be handled by standard metrics. diff --git a/tests/sentry/search/events/builder/test_metrics.py b/tests/sentry/search/events/builder/test_metrics.py index 1cdd9ff4dc60f..babd916cf5115 100644 --- a/tests/sentry/search/events/builder/test_metrics.py +++ b/tests/sentry/search/events/builder/test_metrics.py @@ -2106,7 +2106,6 @@ def test_standard_metrics(self): ) assert query.on_demand_metrics_enabled - assert not query._on_demand_spec def test_run_on_demand_query(self): query = AlertMetricsQueryBuilder( From c429213a5253f1e48a7f2a3301bd0411fef4e3ae Mon Sep 17 00:00:00 2001 From: Ogi <86684834+obostjancic@users.noreply.github.com> Date: Thu, 20 Jul 2023 14:40:28 +0200 Subject: [PATCH 18/21] use_on_demand_metrics --- src/sentry/api/endpoints/organization_events_stats.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/sentry/api/endpoints/organization_events_stats.py b/src/sentry/api/endpoints/organization_events_stats.py index 027222d68b7e8..d2938accc2f19 100644 --- a/src/sentry/api/endpoints/organization_events_stats.py +++ b/src/sentry/api/endpoints/organization_events_stats.py @@ -183,6 +183,8 @@ def get(self, request: Request, organization: Organization) -> Response: allow_metric_aggregates = request.GET.get("preventMetricAggregates") != "1" sentry_sdk.set_tag("performance.metrics_enhanced", metrics_enhanced) + use_on_demand_metrics = request.GET.get("useOnDemandMetrics") + def get_event_stats( query_columns: Sequence[str], query: str, @@ -218,9 +220,8 @@ def get_event_stats( allow_metric_aggregates=allow_metric_aggregates, has_metrics=use_metrics, use_metrics_layer=batch_features.get("organizations:use-metrics-layer", False), - on_demand_metrics_enabled=batch_features.get( - "organizations:on-demand-metrics-extraction", False - ), + on_demand_metrics_enabled=use_on_demand_metrics + and batch_features.get("organizations:on-demand-metrics-extraction", False), ) try: From 2ea5a820fbfeab0a3558c545660b00d35e02e608 Mon Sep 17 00:00:00 2001 From: Ogi <86684834+obostjancic@users.noreply.github.com> Date: Thu, 20 Jul 2023 15:36:48 +0200 Subject: [PATCH 19/21] handle project name under quotes --- src/sentry/snuba/metrics/extraction.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/sentry/snuba/metrics/extraction.py b/src/sentry/snuba/metrics/extraction.py index 0714584db19fa..1bce662670d79 100644 --- a/src/sentry/snuba/metrics/extraction.py +++ b/src/sentry/snuba/metrics/extraction.py @@ -313,7 +313,8 @@ def _init__query(self, query: str) -> None: # On-demand metrics are implicitly transaction metrics. Remove the # filters from the query that can't be translated to a RuleCondition. query = re.sub(r"event\.type:transaction\s*", "", query) - query = re.sub(r"project:\w+\s*", "", query) + # extend the following to also support project:"some-project" + query = re.sub(r"project:[\w\"]+\s*", "", query) self._query = query.strip() From 2f5de751afe77abfc103e9f80a11787f3fe2f14a Mon Sep 17 00:00:00 2001 From: Ogi <86684834+obostjancic@users.noreply.github.com> Date: Thu, 20 Jul 2023 15:44:02 +0200 Subject: [PATCH 20/21] remove test --- tests/sentry/search/events/builder/test_metrics.py | 13 ------------- 1 file changed, 13 deletions(-) diff --git a/tests/sentry/search/events/builder/test_metrics.py b/tests/sentry/search/events/builder/test_metrics.py index babd916cf5115..54ae93880d612 100644 --- a/tests/sentry/search/events/builder/test_metrics.py +++ b/tests/sentry/search/events/builder/test_metrics.py @@ -2094,19 +2094,6 @@ def test_query_normal_distribution(self): class AlertMetricsQueryBuilderTest(MetricBuilderBaseTest): - def test_standard_metrics(self): - query = AlertMetricsQueryBuilder( - self.params, - use_metrics_layer=False, - granularity=3600, - query="", - dataset=Dataset.PerformanceMetrics, - selected_columns=["p75(measurements.fp)"], - on_demand_metrics_enabled=True, - ) - - assert query.on_demand_metrics_enabled - def test_run_on_demand_query(self): query = AlertMetricsQueryBuilder( self.params, From 25d8adb1da7d5cfa6cb7a80ed25ed2cdb0eccddc Mon Sep 17 00:00:00 2001 From: Ogi <86684834+obostjancic@users.noreply.github.com> Date: Thu, 20 Jul 2023 16:52:24 +0200 Subject: [PATCH 21/21] remove is_timeseries_query --- src/sentry/search/events/builder/metrics.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/sentry/search/events/builder/metrics.py b/src/sentry/search/events/builder/metrics.py index 769590c3fb8d5..f213ef031819f 100644 --- a/src/sentry/search/events/builder/metrics.py +++ b/src/sentry/search/events/builder/metrics.py @@ -50,7 +50,7 @@ class MetricsQueryBuilder(QueryBuilder): requires_organization_condition = True is_alerts_query = False - is_timeseries_query = False + organization_column: str = "organization_id" def __init__( @@ -119,7 +119,7 @@ def _get_on_demand_metrics_query(self) -> Optional[MetricsQuery]: spec = self._on_demand_spec # TimeseriesQueryBuilder specific parameters - if self.is_timeseries_query: + if isinstance(self, TimeseriesMetricQueryBuilder): limit = None alias = "count" else: @@ -1073,7 +1073,6 @@ def resolve_split_granularity(self) -> Tuple[List[Condition], Optional[Granulari class TimeseriesMetricQueryBuilder(MetricsQueryBuilder): time_alias = "time" - is_timeseries_query = True def __init__( self,