Skip to content
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

fix(insights): Opportunity score forward browser name filter #83078

Merged
merged 1 commit into from
Jan 8, 2025
Merged

fix(insights): Opportunity score forward browser name filter #83078

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
19 changes: 18 additions & 1 deletion src/sentry/search/events/datasets/metrics.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
from snuba_sdk import Column, Condition, Function, Op, OrderBy

from sentry import features
from sentry.api.event_search import SearchFilter
from sentry.api.event_search import ParenExpression, SearchFilter, parse_search_query
from sentry.exceptions import IncompatibleMetricsQuery, InvalidSearchQuery
from sentry.search.events import constants, fields
from sentry.search.events.builder import metrics
Expand Down Expand Up @@ -1796,11 +1796,28 @@ def _resolve_total_score_weights_function(self, column: str, alias: str | None)
if column in self.total_score_weights and self.total_score_weights[column] is not None:
return Function("toFloat64", [self.total_score_weights[column]], alias)

# Pull out browser.name filters from the query
parsed_terms = parse_search_query(self.builder.query)
query = " ".join(
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if theres a cleaner way to do this, or if theres some function that already exists.
This code is to pull out any browser.name filters that exist on the original query, including those that are wrapped by parenthesis.

term.to_query_string()
for term in parsed_terms
if (isinstance(term, SearchFilter) and term.key.name == "browser.name")
or (
isinstance(term, ParenExpression)
and all( # type: ignore[unreachable]
(isinstance(child_term, SearchFilter) and child_term.key.name == "browser.name")
or child_term == "OR"
for child_term in term.children
)
)
)

total_query = metrics.MetricsQueryBuilder(
dataset=self.builder.dataset,
params={},
snuba_params=self.builder.params,
selected_columns=[f"sum({column})"],
query=query,
)

total_query.columns += self.builder.resolve_groupby()
Expand Down
34 changes: 23 additions & 11 deletions tests/snuba/api/endpoints/test_organization_events_mep.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3025,61 +3025,73 @@ def test_opportunity_score_with_fixed_weights_and_missing_vitals(self):
self.store_transaction_metric(
0.5,
metric="measurements.score.inp",
tags={"transaction": "foo_transaction"},
tags={"transaction": "foo_transaction", "browser.name": "Safari"},
timestamp=self.min_ago,
)
self.store_transaction_metric(
1.0,
metric="measurements.score.weight.inp",
tags={"transaction": "foo_transaction"},
tags={"transaction": "foo_transaction", "browser.name": "Safari"},
timestamp=self.min_ago,
)
self.store_transaction_metric(
0.2,
metric="measurements.score.inp",
tags={"transaction": "foo_transaction"},
tags={"transaction": "foo_transaction", "browser.name": "Safari"},
timestamp=self.min_ago,
)
self.store_transaction_metric(
1.0,
metric="measurements.score.weight.inp",
tags={"transaction": "foo_transaction"},
tags={"transaction": "foo_transaction", "browser.name": "Safari"},
timestamp=self.min_ago,
)
self.store_transaction_metric(
0.2,
metric="measurements.score.inp",
tags={"transaction": "foo_transaction"},
tags={"transaction": "foo_transaction", "browser.name": "Safari"},
timestamp=self.min_ago,
)
self.store_transaction_metric(
0.5,
metric="measurements.score.weight.inp",
tags={"transaction": "foo_transaction"},
tags={"transaction": "foo_transaction", "browser.name": "Safari"},
timestamp=self.min_ago,
)
self.store_transaction_metric(
0.1,
metric="measurements.score.lcp",
tags={"transaction": "foo_transaction"},
tags={"transaction": "foo_transaction", "browser.name": "Firefox"},
timestamp=self.min_ago,
)
self.store_transaction_metric(
0.3,
metric="measurements.score.weight.lcp",
tags={"transaction": "foo_transaction"},
tags={"transaction": "foo_transaction", "browser.name": "Firefox"},
timestamp=self.min_ago,
)
self.store_transaction_metric(
0.2,
metric="measurements.score.inp",
tags={"transaction": "bar_transaction"},
tags={"transaction": "bar_transaction", "browser.name": "Safari"},
timestamp=self.min_ago,
)
self.store_transaction_metric(
0.5,
metric="measurements.score.weight.inp",
tags={"transaction": "bar_transaction"},
tags={"transaction": "bar_transaction", "browser.name": "Safari"},
timestamp=self.min_ago,
)
self.store_transaction_metric(
0.5,
metric="measurements.score.total",
tags={"transaction": "foo_transaction", "browser.name": "Safari"},
timestamp=self.min_ago,
)
self.store_transaction_metric(
0.5,
metric="measurements.score.total",
tags={"transaction": "bar_transaction", "browser.name": "Safari"},
timestamp=self.min_ago,
)

Expand All @@ -3090,7 +3102,7 @@ def test_opportunity_score_with_fixed_weights_and_missing_vitals(self):
"transaction",
"total_opportunity_score()",
],
"query": "event.type:transaction",
"query": 'event.type:transaction transaction.op:[pageload,""] (browser.name:Safari OR browser.name:Firefox) avg(measurements.score.total):>0',
"orderby": "transaction",
"dataset": "metrics",
"per_page": 50,
Expand Down
Loading