feat(weekly-reports): Add data layer for "Top Issues" section - #119174
feat(weekly-reports): Add data layer for "Top Issues" section#119174amy-chen23 wants to merge 9 commits into
Conversation
github-actions
Bot
added
the
Scope: Backend
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
amy-chen23
changed the title
0b9c556 to
f7f7a57
Compare
![sentry-warden[bot]](https://avatars.githubusercontent.com/in/2755838?s=60&v=4){kind=small}
| # Use batched Snuba queries for weekly report key errors instead of per-project queries | ||
| manager.add("organizations:weekly-report-batched-key-errors", OrganizationFeature, FeatureHandlerStrategy.FLAGPOLE, api_expose=False) | ||
| # Use recommended sort instead of frequency sort for weekly report top issues | ||
| manager.add("organizations:weekly-report-recommended-sort", OrganizationFeature, FeatureHandlerStrategy.FLAGPOLE, api_expose=False) |
There was a problem hiding this comment.
i thought we were sticking with frequency for now and moving to recommended sort in the future?
There was a problem hiding this comment.
was thinking about using recommended sort for sentry, but i can remove this flag and use frequency for all
| if features.has("organizations:weekly-report-top-issues", organization): | ||
| self._append_project_key_issues_new(ctx) | ||
| else: | ||
| self._append_project_key_issues_legacy(ctx) |
There was a problem hiding this comment.
tbh it might be fine to just remove the legacy version entirely and roll out the new version to everyone
There was a problem hiding this comment.
yea, the only concern was about performance, but it seems like using search.backend.query() would actually be better for performance
| per_project_key_errors = project_key_errors( | ||
| ctx, project, referrer=Referrer.REPORTS_KEY_ERRORS.value | ||
| try: | ||
| errors_by_project, perf_by_project = org_top_issues( |
There was a problem hiding this comment.
would recommend renaming this here and in the org_top_issues function so it's clear these are issues, not errors, i.e. error_issues_by_project and perf_issues_by_project might be better
| f"Expected a ProjectContext, received {type(project_ctx)}" | ||
| ) | ||
| if project.id in errors_by_project: | ||
| project_ctx.key_errors_by_id = errors_by_project[project.id] |
There was a problem hiding this comment.
as a follow-up can we also rename this in the context and bring it into line with the performance issues name, e.g. ctx.key_error_issues and ctx.key_performance_issues?
There was a problem hiding this comment.
yea i will put renaming into a separate follow-up PR
| op = "weekly_reports.org_top_issues" | ||
| with start_span(op=op, name=op): | ||
| limit = min(3 * len(projects), 1000) | ||
| result = search.backend.query( |
There was a problem hiding this comment.
i don't think it should be necessary to query Postgres first to get the groups themselves, i think we can just issue the below Snuba queries below and group on the group ID
There was a problem hiding this comment.
agreed. purpose of postgres first was to use rec sort, but since we're using sort_by=freq for all, i can just use snuba queries
77f61e9 to
1efa0b2
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want reviews to match your repository better? Bugbot Learning can learn team-specific rules from PR activity. A team admin can enable Learning in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 1efa0b2. Configure here.
| """ | ||
| op = "weekly_reports.org_top_issues" | ||
| with start_span(op=op, name=op): | ||
| limit = min(3 * len(projects), 1000) |
There was a problem hiding this comment.
Bug: The org_top_issues function fetches a globally sorted list of top issues instead of enforcing a per-project limit. High-volume projects can exhaust the global limit, preventing other projects from getting their top issues.
Severity: HIGH
Suggested Fix
Reintroduce a mechanism to enforce a per-project limit within the search query, similar to the previous implementation's use of a LimitBy clause. This will ensure that the top issues are fetched for each project individually, guaranteeing that each project receives its top 5 issues as intended by the function's contract.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: src/sentry/tasks/summaries/utils.py#L149
Potential issue: The `org_top_issues` function is intended to fetch the top 5 issues for
each project. However, it queries for a global list of top issues across all projects,
sorted by frequency, with a limit calculated as `min(3 * len(projects), 1000)`. It then
iterates this list to collect up to 5 issues per project. If one project has a high
volume of frequent issues, it can dominate this global list. This can cause the function
to exhaust the result set before finding the top issues for lower-volume projects,
leading to them receiving fewer than 5 issues or none at all. This breaks the function's
documented behavior, as the previous implementation's `LimitBy` clause, which enforced
per-project limits, was removed.
There was a problem hiding this comment.
setting to min(5 * len(projects), 1000) instead
fe5fc6f to
263b62d
Compare
|
closing b/c the file diff is too big |
2 participants
Resolves backend of ID-1645
Changes
project_key_errors()andproject_key_performance_issues()Snuba queries with a singlesearch.backend.query()call per organizationorganizations:weekly-report-recommended-sortandorganizations:weekly-report-top-issuesFF