feat(discover): Update timeseries to support comparison #29492
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- 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
- This introduces a bulk_snql_query function to run both queries
together
- Mostly copy paste of the comparison code
- Did add an additional check that comparison_delta was set just for
safety 🤷
wedamija
reviewed
Oct 21, 2021
Comment on lines
+818
to
+826
| response = self.do_request( | ||
| data={ | ||
| "start": iso_format(self.day_ago), | ||
| "end": iso_format(self.day_ago + timedelta(hours=2)), | ||
| "interval": "1h", | ||
| "comparisonDelta": "17h", | ||
| }, | ||
| ) | ||
| assert response.status_code == 400, response.content |
Member
There was a problem hiding this comment.
Confused about why we'd switch away from using get_error_response/get_success_response here and elsewhere? It results in more lines/duplication with the status code checking
Member
Author
There was a problem hiding this comment.
Agreed that those are the better test functions, but the reason I switched to the do_request method here is to make having the duplicate snql tests that OrganizationEventsStatsEndpointTestWithSnql provide easier to write
wedamija
approved these changes
Oct 21, 2021
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
together
safety 🤷