-
-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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.
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
feat(discover): Use SnQL for some of event-stats #29471
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,12 +6,13 @@ | |
from typing import Dict, Optional, Sequence | ||
|
||
import sentry_sdk | ||
from dateutil.parser import parse as parse_datetime | ||
|
||
from sentry import options | ||
from sentry.discover.arithmetic import categorize_columns, resolve_equation_list | ||
from sentry.models import Group | ||
from sentry.models.transaction_threshold import ProjectTransactionThreshold | ||
from sentry.search.events.builder import QueryBuilder | ||
from sentry.search.events.builder import QueryBuilder, TimeseriesQueryBuilder | ||
from sentry.search.events.constants import CONFIGURABLE_AGGREGATES, DEFAULT_PROJECT_THRESHOLD | ||
from sentry.search.events.fields import ( | ||
FIELD_ALIASES, | ||
|
@@ -24,6 +25,7 @@ | |
from sentry.search.events.filter import get_filter | ||
from sentry.tagstore.base import TOP_VALUES_DEFAULT_LIMIT | ||
from sentry.utils.compat import filter | ||
from sentry.utils.dates import to_timestamp | ||
from sentry.utils.math import mean, nice_int | ||
from sentry.utils.snuba import ( | ||
SNUBA_AND, | ||
|
@@ -106,6 +108,9 @@ def zerofill(data, start, end, rollup, orderby): | |
data_by_time = {} | ||
|
||
for obj in data: | ||
# This is needed for SnQL, and was originally done in utils.snuba.get_snuba_translators | ||
if isinstance(obj["time"], str): | ||
obj["time"] = int(to_timestamp(parse_datetime(obj["time"]))) | ||
Comment on lines
+112
to
+113
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is this needed? Does There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. so the query has always returned the date as a str, but there was a reverse processor here |
||
if obj["time"] in data_by_time: | ||
data_by_time[obj["time"]].append(obj) | ||
else: | ||
|
@@ -467,6 +472,7 @@ def timeseries_query( | |
referrer: Optional[str] = None, | ||
zerofill_results: bool = True, | ||
comparison_delta: Optional[timedelta] = None, | ||
use_snql: Optional[bool] = False, | ||
): | ||
""" | ||
High-level API for doing arbitrary user timeseries queries against events. | ||
|
@@ -490,6 +496,30 @@ def timeseries_query( | |
query time-shifted back by comparison_delta, and compare the results to get the % change for each | ||
time bucket. Requires that we only pass | ||
""" | ||
sentry_sdk.set_tag("discover.use_snql", use_snql and comparison_delta is None) | ||
if use_snql and comparison_delta is None: | ||
# temporarily add snql to referrer | ||
referrer = f"{referrer}.wip-snql" | ||
equations, columns = categorize_columns(selected_columns) | ||
builder = TimeseriesQueryBuilder( | ||
Dataset.Discover, | ||
params, | ||
rollup, | ||
query=query, | ||
selected_columns=columns, | ||
equations=equations, | ||
) | ||
snql_query = builder.get_snql_query() | ||
|
||
query_results = raw_snql_query(snql_query, referrer) | ||
|
||
result = ( | ||
zerofill(query_results["data"], params["start"], params["end"], rollup, "time") | ||
if zerofill_results | ||
else query_results["data"] | ||
) | ||
return SnubaTSResult({"data": result}, params["start"], params["end"], rollup) | ||
|
||
with sentry_sdk.start_span( | ||
op="discover.discover", description="timeseries.filter_transform" | ||
) as span: | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This assumption breaks with top 5 charts right? Did you already have an idea for that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dang I considered commenting this but thought it was overly verbose for this PR
Yeah I think for top events we should have just introduce a different timeseries builder that inherits from this one and changes the groupby etc. as needed