Skip to content

ref(mep): Some cleanup to the metric query builder #32139

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
wmak merged 4 commits into from
Mar 7, 2022
Merged

ref(mep): Some cleanup to the metric query builder #32139

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
dbf2fec
ref(mep): Some cleanup to the metric query builder
wmak Feb 28, 2022
65eb48f
Merge branch 'master' into wmak/ref/query-builder-cleanup
wmak Mar 2, 2022
28850e7
ref: comment
wmak Mar 2, 2022
9ef5c93
Merge branch 'master' into wmak/ref/query-builder-cleanup
wmak Mar 7, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions src/sentry/search/events/builder.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1451,6 +1451,7 @@ def __init__(self, *args: Any, **kwargs: Any):
self.distributions: List[CurriedFunction] = []
self.sets: List[CurriedFunction] = []
self.counters: List[CurriedFunction] = []
self.metric_ids: List[int] = []
super().__init__(
# Dataset is always Metrics
Dataset.Metrics,
Expand Down Expand Up @@ -1496,6 +1497,15 @@ def resolve_params(self) -> List[WhereType]:
)
return conditions

def resolve_query(self, *args: Any, **kwargs: Any) -> None:
super().resolve_query(*args, **kwargs)
# Optimization to add metric ids to the filter
if len(self.metric_ids) > 0:
self.where.append(
# Metric id is intentionally sorted so we create consistent queries here both for testing & caching
Condition(Column("metric_id"), Op.IN, sorted(self.metric_ids))
)

def resolve_limit(self, limit: Optional[int]) -> Limit:
"""Impose a max limit, since we may need to create a large condition based on the group by values when the query
is run"""
Expand Down
31 changes: 18 additions & 13 deletions src/sentry/search/events/datasets/metrics.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,14 +7,14 @@
from sentry.api.event_search import SearchFilter
from sentry.exceptions import IncompatibleMetricsQuery, InvalidSearchQuery
from sentry.search.events import constants, fields
from sentry.search.events.builder import QueryBuilder
from sentry.search.events.builder import MetricsQueryBuilder
from sentry.search.events.datasets.base import DatasetConfig
from sentry.search.events.types import SelectType, WhereType
from sentry.sentry_metrics import indexer


class MetricsDatasetConfig(DatasetConfig):
def __init__(self, builder: QueryBuilder):
def __init__(self, builder: MetricsQueryBuilder):
self.builder = builder

@property
Expand All @@ -40,10 +40,15 @@ def resolve_metric(self, value: str) -> int:
# TODO: unsure if this should be incompatible or invalid
raise InvalidSearchQuery(f"Metric: {value} could not be resolved")

self.builder.metric_ids.append(metric_id)
return metric_id

@property
def function_converter(self) -> Mapping[str, fields.MetricsFunction]:
"""While the final functions in clickhouse must have their -Merge combinators in order to function, we don't
need to add them here since snuba has a FunctionMapper that will add it for us. Basically it turns expressions
like quantiles(0.9)(value) into quantilesMerge(0.9)(percentiles)
"""
resolve_metric_id = {
"name": "metric_id",
"fn": lambda args: self.resolve_metric(args["column"]),
Expand Down Expand Up @@ -112,7 +117,7 @@ def function_converter(self) -> Mapping[str, fields.MetricsFunction]:
required_args=[fields.FunctionArg("column")],
calculated_args=[resolve_metric_id],
snql_set=lambda args, alias: Function(
"uniqCombinedIf",
"uniqIf",
[
Column("value"),
Function("equals", [Column("metric_id"), args["metric_id"]]),
Expand All @@ -127,9 +132,9 @@ def function_converter(self) -> Mapping[str, fields.MetricsFunction]:
"divide",
[
Function(
"countMergeIf",
"countIf",
[
Column("count"),
Column("value"),
Function(
"equals",
[
Expand All @@ -152,9 +157,9 @@ def function_converter(self) -> Mapping[str, fields.MetricsFunction]:
"divide",
[
Function(
"countMergeIf",
"countIf",
[
Column("count"),
Column("value"),
Function(
"equals",
[
Expand Down Expand Up @@ -183,9 +188,9 @@ def function_converter(self) -> Mapping[str, fields.MetricsFunction]:
[
self._resolve_failure_count(args),
Function(
"countMergeIf",
"countIf",
[
Column("count"),
Column("value"),
Function(
"equals",
[
Expand Down Expand Up @@ -225,9 +230,9 @@ def _resolve_failure_count(
) -> SelectType:
statuses = [indexer.resolve(status) for status in constants.NON_FAILURE_STATUS]
return Function(
"countMergeIf",
"countIf",
[
Column("count"),
Column("value"),
Function(
"and",
[
Expand Down Expand Up @@ -261,9 +266,9 @@ def _resolve_percentile(
"arrayElement",
[
Function(
f"quantilesMergeIf({fixed_percentile})",
f"quantilesIf({fixed_percentile})",
[
Column("percentiles"),
Column("value"),
Function("equals", [Column("metric_id"), args["metric_id"]]),
],
),
Expand Down
60 changes: 53 additions & 7 deletions tests/sentry/search/events/test_builder.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import datetime
import re
from typing import List

import pytest
from django.utils import timezone
Expand Down Expand Up @@ -579,9 +580,9 @@ def _metric_percentile_definition(quantile, field="transaction.duration") -> Fun
"arrayElement",
[
Function(
f"quantilesMergeIf(0.{quantile.rstrip('0')})",
f"quantilesIf(0.{quantile.rstrip('0')})",
[
Column("percentiles"),
Column("value"),
Function(
"equals",
[Column("metric_id"), indexer.resolve(constants.METRICS_MAP[field])],
Expand All @@ -594,6 +595,16 @@ def _metric_percentile_definition(quantile, field="transaction.duration") -> Fun
)


def _metric_conditions(metrics) -> List[Condition]:
return [
Condition(
Column("metric_id"),
Op.IN,
sorted(indexer.resolve(constants.METRICS_MAP[metric]) for metric in metrics),
)
]


class MetricBuilderBaseTest(MetricsEnhancedPerformanceTestCase):
METRIC_STRINGS = [
"foo_transaction",
Expand Down Expand Up @@ -660,7 +671,21 @@ def test_simple_aggregates(self):
"p99(measurements.fid)",
],
)
self.assertCountEqual(query.where, self.default_conditions)
self.assertCountEqual(
query.where,
[
*self.default_conditions,
*_metric_conditions(
[
"transaction.duration",
"measurements.lcp",
"measurements.fcp",
"measurements.cls",
"measurements.fid",
]
),
],
)
self.assertCountEqual(
query.distributions,
[
Expand All @@ -678,7 +703,9 @@ def test_grouping(self):
"",
selected_columns=["transaction", "project", "p95(transaction.duration)"],
)
self.assertCountEqual(query.where, self.default_conditions)
self.assertCountEqual(
query.where, [*self.default_conditions, *_metric_conditions(["transaction.duration"])]
)
transaction_index = indexer.resolve("transaction")
transaction = AliasedExpression(
Column(f"tags[{transaction_index}]"),
Expand Down Expand Up @@ -713,7 +740,12 @@ def test_transaction_filter(self):
transaction_name = indexer.resolve("foo_transaction")
transaction = Column(f"tags[{transaction_index}]")
self.assertCountEqual(
query.where, [*self.default_conditions, Condition(transaction, Op.EQ, transaction_name)]
query.where,
[
*self.default_conditions,
*_metric_conditions(["transaction.duration"]),
Condition(transaction, Op.EQ, transaction_name),
],
)

def test_transaction_in_filter(self):
Expand All @@ -730,6 +762,7 @@ def test_transaction_in_filter(self):
query.where,
[
*self.default_conditions,
*_metric_conditions(["transaction.duration"]),
Condition(transaction, Op.IN, [transaction_name1, transaction_name2]),
],
)
Expand Down Expand Up @@ -764,7 +797,11 @@ def test_project_filter(self):
)
self.assertCountEqual(
query.where,
[*self.default_conditions, Condition(Column("project_id"), Op.EQ, self.project.id)],
[
*self.default_conditions,
*_metric_conditions(["transaction.duration"]),
Condition(Column("project_id"), Op.EQ, self.project.id),
],
)

def test_limit_validation(self):
Expand Down Expand Up @@ -1088,6 +1125,10 @@ def test_get_query(self):
)
snql_query = query.get_snql_query()
assert len(snql_query) == 1
assert snql_query[0].where == [
*self.default_conditions,
*_metric_conditions(["transaction.duration"]),
]
assert snql_query[0].select == [_metric_percentile_definition("50")]
assert snql_query[0].match.name == "metrics_distributions"
assert snql_query[0].granularity.granularity == 60
Expand Down Expand Up @@ -1164,6 +1205,7 @@ def test_transaction_in_filter(self):
query.where,
[
*self.default_conditions,
*_metric_conditions(["transaction.duration"]),
Condition(transaction, Op.IN, [transaction_name1, transaction_name2]),
],
)
Expand Down Expand Up @@ -1201,7 +1243,11 @@ def test_project_filter(self):
)
self.assertCountEqual(
query.where,
[*self.default_conditions, Condition(Column("project_id"), Op.EQ, self.project.id)],
[
*self.default_conditions,
*_metric_conditions(["transaction.duration"]),
Condition(Column("project_id"), Op.EQ, self.project.id),
],
)

def test_meta(self):
Expand Down