feat(supergroups): add issues-with-supergroups endpoint #113563
feat(supergroups): add issues-with-supergroups endpoint #113563
Conversation
|
🚨 Warning: This pull request contains Frontend and Backend changes! It's discouraged to make changes to Sentry's Frontend and Backend in a single pull request. The Frontend and Backend are not atomically deployed. If the changes are interdependent of each other, they must be separated into two pull requests and be made forward or backwards compatible, such that the Backend or Frontend can be safely deployed independently. Have questions? Please ask in the |
8c90cee to
c829675
Compare
8146943 to
20ca2cc
Compare
c829675 to
4c56715
Compare
20ca2cc to
d959eb7
Compare
4c56715 to
2051bce
Compare
d959eb7 to
318688d
Compare
318688d to
9467611
Compare
2051bce to
aad3ceb
Compare
9467611 to
a425799
Compare
a425799 to
c921aaf
Compare
c921aaf to
f449dd8
Compare
f449dd8 to
5a1ede2
Compare
5a1ede2 to
f64cced
Compare
f64cced to
7139650
Compare
7139650 to
b8754be
Compare
yuvmen
left a comment
There was a problem hiding this comment.
looks good overall, I think this works as like a best effort to make this viable for now.
| return Response({"detail": str(exc)}, status=400) | ||
|
|
||
| response = Response(rows) | ||
| # X-Hits is the number of rows we're returning on this page. We skip |
There was a problem hiding this comment.
minor - can add information as a comment on the endpoint so its more visible, this x-hits behaviour that might be kinda hidden
There was a problem hiding this comment.
going to skip for now, agree that it's not great behavior in general. i'm kind of hoping we are able to fix this with a more holistic solution before we have to deal with it seriously
| # Re-run sized to what we actually consumed so its next cursor | ||
| # sits at our page boundary, not past unemitted groups. | ||
| if last_consumed + 1 < len(groups): | ||
| _, cursor_result, _ = search_and_serialize_issues( |
There was a problem hiding this comment.
minor - we technically dont need to serialize here cuz we dont use the issues we get and its kind of a heavy operation, dont think it exists but we could actually do a search_only method just for this purpose, and save the serialization, kinda alleviating some of the pain of this second fetch
| assert "supergroup" not in response.data[0] | ||
|
|
||
| @patch("sentry.seer.supergroups.by_group.make_supergroups_get_by_group_ids_request") | ||
| def test_paginates_without_group_overlap(self, mock_seer: MagicMock) -> None: |
There was a problem hiding this comment.
feel like we should have a parallel test to this where a page does include a supergroup to make sure the cursor gets updated correctly
Subclasses OrganizationGroupIndexEndpoint and post-processes the row list to collapse Seer supergroup members into a single row. Restricted to GET via http_method_names. Behind the organizations:top-issues-ui flag. Co-authored-by: Claude <noreply@anthropic.com>
b8754be to
08d4fd6
Compare
| return Response({"detail": ERR_INVALID_STATS_PERIOD}, status=400) | ||
|
|
||
| try: | ||
| limit = max(0, int(request.GET.get("limit") or 25)) |
There was a problem hiding this comment.
Bug: A user can pass limit=0, causing a second search_issues call to also be made with limit=0. This is an invalid state that can lead to incorrect results.
Severity: LOW
Suggested Fix
Prevent limit=0 at the input validation stage on line 49 by changing max(0, ...) to max(1, ...). Alternatively, ensure the second search_issues call never receives a limit less than or equal to zero.
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/issues/endpoints/organization_issues_with_supergroups.py#L49
Potential issue: The endpoint allows a user to pass `limit=0` as a query parameter. When
this happens, the `_combine_groups_and_supergroups` function returns `last_consumed=-1`.
This state triggers a second call to `search_issues` around line 110, but with `limit`
set to `0`. While the search backend might not crash, other paginators in the codebase
explicitly raise an error for `limit <= 0`, indicating this is an unsupported and
problematic state that can lead to empty or incorrect results.
Also affects:
src/sentry/issues/endpoints/organization_issues_with_supergroups.py:110~122
Did we get this right? 👍 / 👎 to inform future reviews.
Add a new 'issues' endpoint for a blended issues + supergroups view. This works by fetching x2 results, combining issues that are in the same supergroups, and then fetching that exact number to get the correct cursor for the next page. Unfortunately this approach requires 2 search queries over basically the same data. The problem is maintaining the cursor here. Let's say we get a request for 25 results and fetch 50. We blend together to get 25 rows, using 40 of the original groups we fetched. We don't have a cursor to return at the 40th result, since each time we fetch a page, we get the cursor from the end of the page — we only have the cursor at 50. To fix this, we make another request for the number of issues we actually used, then modifying that cursor to work accordingly. Co-authored-by: Claude <noreply@anthropic.com>
Add a new 'issues' endpoint for a blended issues + supergroups view. This works by fetching x2 results, combining issues that are in the same supergroups, and then fetching that exact number to get the correct cursor for the next page.
Unfortunately this approach requires 2 search queries over basically the same data. The problem is maintaining the cursor here. Let's say we get a request for 25 results and fetch 50. We blend together to get 25 rows, using 40 of the original groups we fetched. We don't have a cursor to return at the 40th result, since each time we fetch a page, we get the cursor from the end of the page — we only have the cursor at 50. To fix this, we make another request for the number of issues we actually used, then modifying that cursor to work accordingly.