feat(discover): Use SnQL for some of event-stats #29471
Conversation
- This enables the use of a new TimeseriesQueryBuilder on event-stats
queries that don't include comparsion, and aren't top events queries
- Had to refactor a lot of the tests to use the same `do_request`
pattern from eventsv2 to make testing easier
- Needed to add a 4th list to resolve_equation_list so we could
differentiate easily between equations that are on functions or not
- Didn't go with a class or namedtuple in the response since we'll
likely be removing some of this once we're done migrating
- Only adding project_threshold_config when auto_fields are on, which
means that the table response is unchanged, but now we no longer need
to do the averaging for graphing threshold based functions
| @@ -71,7 +71,7 @@ def validate(self, data): | |||
|
|
|||
| if equations is not None: | |||
| try: | |||
| resolved_equations, _, _ = resolve_equation_list(equations, fields) | |||
| resolved_equations, _, _, _ = resolve_equation_list(equations, fields) | |||
There was a problem hiding this comment.
Not sure of a better suggestion but destructuring a 4-tuple is getting a little unwieldy. Perhaps consider returning an object with named attributes?
There was a problem hiding this comment.
I considered that, but decided since this is only temporary (first two go away once we no longer need to return JSON) to leave this as-is for now
| select=self.select, | ||
| where=self.where, | ||
| having=self.having, | ||
| # This is a timeseries, the groupby will always be time |
There was a problem hiding this comment.
This assumption breaks with top 5 charts right? Did you already have an idea for that?
There was a problem hiding this comment.
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
src/sentry/search/events/fields.py
Outdated
| ): | ||
| stripped_columns.append(PROJECT_THRESHOLD_CONFIG_ALIAS) | ||
| break | ||
| if self.auto_fields: |
There was a problem hiding this comment.
This auto_fields condition did not exist previously, what is the motivation behind this?
There was a problem hiding this comment.
Ah hm leftover from earlier iteration where i slammed all of self.select into the query and not just the aggregates. I'll undo this change 👍
| if isinstance(obj["time"], str): | ||
| obj["time"] = int(to_timestamp(parse_datetime(obj["time"]))) |
There was a problem hiding this comment.
Why is this needed? Does snql return a str now instead of a int like it did previously?
There was a problem hiding this comment.
so the query has always returned the date as a str, but there was a reverse processor here
Which we're no longer using.
- Adding a comment explaining why we need to parse the times now - Removing the auto fields that we no longer need - Fixing tests that broke cause I removed the select property
src/sentry/search/events/fields.py
Outdated
| for index, (equation, is_function) in enumerate( | ||
| zip(parsed_equations, contains_function) | ||
| ): |
There was a problem hiding this comment.
This is now dependent on the fact that parsed_equations and contains_function are the same lengths. Does this need a comment in resolve_equation_list to make it clear that this must be the case? Or even add a test?
There was a problem hiding this comment.
dangit this comment convinced me, going to have resolve_equation_list return a list of objects instead of two lists for the snql syntax, which should mean after the migration we only need the single list in the response.
queries that don't include comparsion, and aren't top events queries
do_requestpattern from eventsv2 to make testing easier
differentiate easily between equations that are on functions or not
likely be removing some of this once we're done migrating
means that the table response is unchanged, but now we no longer need
to do the averaging for graphing threshold based functions