ref(querybuilder): Remove params from snuba modules #76198
Conversation
wmak
commented
Aug 14, 2024
wmak
commented
- Removes all the params from snuba modules for snuba_params instead
- Removes all the params from snuba modules for snuba_params instead
github-actions
bot
added
the
Scope: Backend
❌ 1 Tests Failed:
View the top 1 failed tests by shortest run timetests.sentry.snuba.test_errors.ErrorsArithmeticTest�test_simple To view individual test run time comparison to the main branch, go to the Test Analytics Dashboard |
| @@ -391,26 +392,26 @@ def measure_span(op_tag): | |||
| serialized["options"] = options[project.id] | |||
| return result | |||
|
|
|||
| def get_stats(self, project_ids, query): | |||
| def get_stats(self, projects, query): | |||
There was a problem hiding this comment.
Actual changes here, changed the param to a list of param objects so we don't need to hit the db again
| def __init__(self, organization_id, discover_query): | ||
| self.projects = self.get_projects(organization_id, discover_query) | ||
| self.environments = self.get_environments(organization_id, discover_query) | ||
| def __init__(self, organization, discover_query): |
There was a problem hiding this comment.
Actual changes here, had to change the param from organization_id to the organization object.
And changed get_environments to return a list of objects instead of names
| params={ | ||
| "organization_id": alert_rule.organization.id, | ||
| "project_id": [project.id], | ||
| "granularity": granularity, |
There was a problem hiding this comment.
Granularity never did anything? not sure why it was passed here
| comp_query_params = snuba_params.copy() | ||
| assert comp_query_params.start is not None, "start is required" | ||
| assert comp_query_params.end is not None, "end is required" | ||
| comp_query_params.start -= comparison_delta | ||
| comp_query_params.end -= comparison_delta |
There was a problem hiding this comment.
I sorta hate this block 🤔 after the follow up PR to remove params entirely might add a utility to clean this up
There was a problem hiding this comment.
Okay, like you said, most of the changes were either not passing param anymore or passing that empty param dict to the query builders, so that was straightforward. I also looked at the function signatures you explicitly called out, those looked good to me too.
🚀
…n easier - The plan: - Update getsentry's call to discover.query to use this function instead - Merge #76198 - Update getsentry back to using discover.query and remove this function
|
PR reverted: f4a5282 |
This reverts commit 3e923c3. Co-authored-by: ceorourke <29959063+ceorourke@users.noreply.github.com>
- Removes all the params from snuba modules for snuba_params instead - Reintroduces #76198 which had to be reverted because of https://sentry.sentry.io/issues/5721375935/events/37f53d54aa0741d3becea6f75399f1d6/?project=1&referrer=issue-list&statsPeriod=90d - Which was fixed in fe1d578 and a test added to avoid this again in the future
