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 1/9] 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 c287561d2d0a8b..a4a6846ba54438 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 c747391311d3ec..c4497241f96e60 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 2/9] 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 a4a6846ba54438..a4000f520d11bc 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 3/9] 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 a4000f520d11bc..aa06f0eadc700f 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 4/9] :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 c4497241f96e60..4691ad6dfbdf64 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 5/9] 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 aa06f0eadc700f..cd5fbf7b86229d 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 6/9] 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 4691ad6dfbdf64..5a6cf70697d845 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 7/9] 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 cd5fbf7b86229d..4fa03ff8672c30 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 8/9] 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 a2a9593a9b50b2..36f2572a41a0e8 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 917e4e6a245803..675dfbf56e111f 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 9/9] 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 5a6cf70697d845..eab5be778f6b1f 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 36f2572a41a0e8..af22e19c6a96a9 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)"],