Просмотр исходного кода

bug(replays): Use the new `/replays-count/?returnIds=true` query to list replays related to issues (#45271)

This follows the backend work done in
https://github.com/getsentry/sentry/pull/44774

Basically we're asking now for a list of replays, that are related to
this issue, and where the replays are guaranteed to exist.

Before we were asking for issues with replay_id attached, but the replay
itself might be missing (there are many reasons it could be gone. ie:
deleted).

For this example issue it was common to get back a list of 3 replays,
even though the title suggested that there were 13.
After we see all 13:
<img width="1277" alt="SCR-20230301-j4r"
src="https://user-images.githubusercontent.com/187460/222273029-aae790d0-b04b-4309-942a-6d1f38ebc0c8.png">

I also checked against an example issue that has over 50 replays in the
count, we do still see 50 in the table. As before there is no
pagination, so 50 is the maximum that people will see.

50 replays in the table means 50 examples of the issue. It should be
enough to start a debugging session.

Fixes #44267
Ryan Albrecht 2 лет назад
Родитель
Сommit
59eae7eb2e

+ 38 - 121
static/app/views/issueDetails/groupReplays/groupReplays.spec.tsx

@@ -9,7 +9,7 @@ jest.mock('sentry/utils/useMedia', () => ({
   default: jest.fn(() => true),
 }));
 
-const mockEventsUrl = '/organizations/org-slug/events/';
+const mockReplayCountUrl = '/organizations/org-slug/replay-count/';
 const mockReplayUrl = '/organizations/org-slug/replays/';
 
 type InitializeOrgProps = {
@@ -18,6 +18,9 @@ type InitializeOrgProps = {
   };
 };
 
+const REPLAY_ID_1 = '346789a703f6454384f1de473b8b9fcc';
+const REPLAY_ID_2 = 'b05dae9b6be54d21a4d5ad9f8f02b780';
+
 function init({
   organizationProps = {features: ['session-replay-ui']},
 }: InitializeOrgProps) {
@@ -75,16 +78,13 @@ describe('GroupReplays', () => {
   describe('Replay Feature Enabled', () => {
     const {router, organization, routerContext} = init({});
 
-    it('should query the events endpoint with the fetched replayIds', async () => {
+    it('should query the replay-count endpoint with the fetched replayIds', async () => {
       const mockGroup = TestStubs.Group();
 
-      const mockEventsApi = MockApiClient.addMockResponse({
-        url: mockEventsUrl,
+      const mockReplayCountApi = MockApiClient.addMockResponse({
+        url: mockReplayCountUrl,
         body: {
-          data: [
-            {replayId: '346789a703f6454384f1de473b8b9fcc', 'count()': 1},
-            {replayId: 'b05dae9b6be54d21a4d5ad9f8f02b780', 'count()': 1},
-          ],
+          [mockGroup.id]: [REPLAY_ID_1, REPLAY_ID_2],
         },
       });
 
@@ -102,16 +102,14 @@ describe('GroupReplays', () => {
       });
 
       await waitFor(() => {
-        expect(mockEventsApi).toHaveBeenCalledWith(
-          mockEventsUrl,
+        expect(mockReplayCountApi).toHaveBeenCalledWith(
+          mockReplayCountUrl,
           expect.objectContaining({
             query: {
-              environment: [],
-              field: ['replayId', 'count()'],
-              per_page: 50,
-              project: ['2'],
-              query: `issue.id:${mockGroup.id} !replayId:""`,
+              returnIds: true,
+              query: `issue.id:[${mockGroup.id}]`,
               statsPeriod: '14d',
+              project: '2',
             },
           })
         );
@@ -134,8 +132,7 @@ describe('GroupReplays', () => {
               ],
               per_page: 50,
               project: ['2'],
-              query:
-                'id:[346789a703f6454384f1de473b8b9fcc,b05dae9b6be54d21a4d5ad9f8f02b780]',
+              query: `id:[${REPLAY_ID_1},${REPLAY_ID_2}]`,
               sort: '-started_at',
               statsPeriod: '14d',
             }),
@@ -147,13 +144,10 @@ describe('GroupReplays', () => {
     it('should show empty message when no replays are found', async () => {
       const mockGroup = TestStubs.Group();
 
-      const mockEventsApi = MockApiClient.addMockResponse({
-        url: mockEventsUrl,
+      const mockReplayCountApi = MockApiClient.addMockResponse({
+        url: mockReplayCountUrl,
         body: {
-          data: [
-            {replayId: '346789a703f6454384f1de473b8b9fcc', 'count()': 1},
-            {replayId: 'b05dae9b6be54d21a4d5ad9f8f02b780', 'count()': 1},
-          ],
+          [mockGroup.id]: [REPLAY_ID_1, REPLAY_ID_2],
         },
       });
 
@@ -173,7 +167,7 @@ describe('GroupReplays', () => {
       expect(
         await screen.findByText('There are no items to display')
       ).toBeInTheDocument();
-      expect(mockEventsApi).toHaveBeenCalledTimes(1);
+      expect(mockReplayCountApi).toHaveBeenCalledTimes(1);
       expect(mockReplayApi).toHaveBeenCalledTimes(1);
       expect(container).toSnapshot();
     });
@@ -181,13 +175,10 @@ describe('GroupReplays', () => {
     it('should display error message when api call fails', async () => {
       const mockGroup = TestStubs.Group();
 
-      const mockEventsApi = MockApiClient.addMockResponse({
-        url: mockEventsUrl,
+      const mockReplayCountApi = MockApiClient.addMockResponse({
+        url: mockReplayCountUrl,
         body: {
-          data: [
-            {replayId: '346789a703f6454384f1de473b8b9fcc', 'count()': 1},
-            {replayId: 'b05dae9b6be54d21a4d5ad9f8f02b780', 'count()': 1},
-          ],
+          [mockGroup.id]: [REPLAY_ID_1, REPLAY_ID_2],
         },
       });
 
@@ -206,7 +197,7 @@ describe('GroupReplays', () => {
       });
 
       await waitFor(() => {
-        expect(mockEventsApi).toHaveBeenCalledTimes(1);
+        expect(mockReplayCountApi).toHaveBeenCalledTimes(1);
         expect(mockReplayApi).toHaveBeenCalledTimes(1);
         expect(
           screen.getByText('Invalid number: asdf. Expected number.')
@@ -217,13 +208,10 @@ describe('GroupReplays', () => {
     it('should display default error message when api call fails without a body', async () => {
       const mockGroup = TestStubs.Group();
 
-      const mockEventsApi = MockApiClient.addMockResponse({
-        url: mockEventsUrl,
+      const mockReplayCountApi = MockApiClient.addMockResponse({
+        url: mockReplayCountUrl,
         body: {
-          data: [
-            {replayId: '346789a703f6454384f1de473b8b9fcc', 'count()': 1},
-            {replayId: 'b05dae9b6be54d21a4d5ad9f8f02b780', 'count()': 1},
-          ],
+          [mockGroup.id]: [REPLAY_ID_1, REPLAY_ID_2],
         },
       });
 
@@ -240,7 +228,7 @@ describe('GroupReplays', () => {
       });
 
       await waitFor(() => {
-        expect(mockEventsApi).toHaveBeenCalledTimes(1);
+        expect(mockReplayCountApi).toHaveBeenCalledTimes(1);
         expect(mockReplayApi).toHaveBeenCalledTimes(1);
         expect(
           screen.getByText(
@@ -253,13 +241,10 @@ describe('GroupReplays', () => {
     it('should show loading indicator when loading replays', async () => {
       const mockGroup = TestStubs.Group();
 
-      const mockEventsApi = MockApiClient.addMockResponse({
-        url: mockEventsUrl,
+      const mockReplayCountApi = MockApiClient.addMockResponse({
+        url: mockReplayCountUrl,
         body: {
-          data: [
-            {replayId: '346789a703f6454384f1de473b8b9fcc', 'count()': 1},
-            {replayId: 'b05dae9b6be54d21a4d5ad9f8f02b780', 'count()': 1},
-          ],
+          [mockGroup.id]: [REPLAY_ID_1, REPLAY_ID_2],
         },
       });
 
@@ -279,7 +264,7 @@ describe('GroupReplays', () => {
 
       expect(screen.getByTestId('loading-indicator')).toBeInTheDocument();
       await waitFor(() => {
-        expect(mockEventsApi).toHaveBeenCalledTimes(1);
+        expect(mockReplayCountApi).toHaveBeenCalledTimes(1);
         expect(mockReplayApi).toHaveBeenCalledTimes(1);
       });
     });
@@ -287,13 +272,10 @@ describe('GroupReplays', () => {
     it('should show a list of replays and have the correct values', async () => {
       const mockGroup = TestStubs.Group();
 
-      const mockEventsApi = MockApiClient.addMockResponse({
-        url: mockEventsUrl,
+      const mockReplayCountApi = MockApiClient.addMockResponse({
+        url: mockReplayCountUrl,
         body: {
-          data: [
-            {replayId: '346789a703f6454384f1de473b8b9fcc', 'count()': 1},
-            {replayId: 'b05dae9b6be54d21a4d5ad9f8f02b780', 'count()': 1},
-          ],
+          [mockGroup.id]: [REPLAY_ID_1, REPLAY_ID_2],
         },
       });
 
@@ -306,7 +288,7 @@ describe('GroupReplays', () => {
               count_errors: 1,
               duration: 52346,
               finished_at: '2022-09-15T06:54:00+00:00',
-              id: '346789a703f6454384f1de473b8b9fcc',
+              id: REPLAY_ID_1,
               project_id: '2',
               started_at: '2022-09-15T06:50:03+00:00',
               urls: [
@@ -325,7 +307,7 @@ describe('GroupReplays', () => {
               count_errors: 4,
               duration: 400,
               finished_at: '2022-09-21T21:40:38+00:00',
-              id: 'b05dae9b6be54d21a4d5ad9f8f02b780',
+              id: REPLAY_ID_2,
               project_id: '2',
               started_at: '2022-09-21T21:30:44+00:00',
               urls: [
@@ -355,7 +337,7 @@ describe('GroupReplays', () => {
       });
 
       await waitFor(() => {
-        expect(mockEventsApi).toHaveBeenCalledTimes(1);
+        expect(mockReplayCountApi).toHaveBeenCalledTimes(1);
         expect(mockReplayApi).toHaveBeenCalledTimes(1);
       });
 
@@ -368,13 +350,13 @@ describe('GroupReplays', () => {
       // Expect the first row to have the correct href
       expect(screen.getAllByRole('link', {name: 'testDisplayName'})[0]).toHaveAttribute(
         'href',
-        `/organizations/org-slug/replays/project-slug:346789a703f6454384f1de473b8b9fcc/?${expectedQuery}`
+        `/organizations/org-slug/replays/project-slug:${REPLAY_ID_1}/?${expectedQuery}`
       );
 
       // Expect the second row to have the correct href
       expect(screen.getAllByRole('link', {name: 'testDisplayName'})[1]).toHaveAttribute(
         'href',
-        `/organizations/org-slug/replays/project-slug:b05dae9b6be54d21a4d5ad9f8f02b780/?${expectedQuery}`
+        `/organizations/org-slug/replays/project-slug:${REPLAY_ID_2}/?${expectedQuery}`
       );
 
       // Expect the first row to have the correct duration
@@ -400,69 +382,4 @@ describe('GroupReplays', () => {
       expect(screen.getByText('7 days ago')).toBeInTheDocument();
     });
   });
-  describe('sorting', () => {
-    let mockEventsApi;
-    let mockReplayApi;
-
-    beforeEach(() => {
-      mockEventsApi = MockApiClient.addMockResponse({
-        url: mockEventsUrl,
-        body: {
-          data: [
-            {replayId: '346789a703f6454384f1de473b8b9fcc', 'count()': 1},
-            {replayId: 'b05dae9b6be54d21a4d5ad9f8f02b780', 'count()': 1},
-          ],
-        },
-      });
-
-      mockReplayApi = MockApiClient.addMockResponse({
-        url: mockReplayUrl,
-        body: {
-          data: [],
-        },
-        statusCode: 200,
-      });
-    });
-
-    it('should not call the events api again when sorting the visible rows', async () => {
-      const mockGroup = TestStubs.Group();
-
-      const {router, organization, routerContext} = init({});
-      const {rerender} = render(<GroupReplays group={mockGroup} />, {
-        context: routerContext,
-        organization,
-        router,
-      });
-
-      await waitFor(() => {
-        expect(mockEventsApi).toHaveBeenCalledTimes(1);
-        expect(mockReplayApi).toHaveBeenCalledTimes(1);
-        expect(mockReplayApi).toHaveBeenLastCalledWith(
-          mockReplayUrl,
-          expect.objectContaining({
-            query: expect.objectContaining({
-              sort: '-started_at',
-            }),
-          })
-        );
-      });
-
-      // Change the sort order then tell react to re-render
-      router.location.query.sort = 'duration';
-      rerender(<GroupReplays group={mockGroup} />);
-
-      await waitFor(() => {
-        expect(mockEventsApi).toHaveBeenCalledTimes(1);
-        expect(mockReplayApi).toHaveBeenCalledTimes(2);
-        expect(mockReplayApi).toHaveBeenLastCalledWith(
-          mockReplayUrl,
-          expect.objectContaining({
-            query: expect.objectContaining({
-              sort: 'duration',
-            }),
-          })
-        );
-      });
-    });
-  });
 });

+ 21 - 36
static/app/views/issueDetails/groupReplays/useReplaysFromIssue.tsx

@@ -3,14 +3,11 @@ import * as Sentry from '@sentry/react';
 import {Location} from 'history';
 
 import type {Group, Organization} from 'sentry/types';
-import {TableData} from 'sentry/utils/discover/discoverQuery';
 import EventView from 'sentry/utils/discover/eventView';
-import {doDiscoverQuery} from 'sentry/utils/discover/genericDiscoverQuery';
 import {decodeScalar} from 'sentry/utils/queryString';
 import {DEFAULT_SORT, REPLAY_LIST_FIELDS} from 'sentry/utils/replays/fetchReplayList';
 import useApi from 'sentry/utils/useApi';
 import useCleanQueryParamsOnRouteLeave from 'sentry/utils/useCleanQueryParamsOnRouteLeave';
-import type {ReplayListLocationQuery} from 'sentry/views/replays/types';
 
 function useReplayFromIssue({
   group,
@@ -23,45 +20,32 @@ function useReplayFromIssue({
 }) {
   const api = useApi();
 
-  const [response, setResponse] = useState<{
-    pageLinks: null | string;
-    replayIds: undefined | string[];
-  }>({pageLinks: null, replayIds: undefined});
+  const [replayIds, setReplayIds] = useState<string[]>([]);
 
   const [fetchError, setFetchError] = useState();
 
-  const {cursor} = location.query;
   const fetchReplayIds = useCallback(async () => {
-    const eventView = EventView.fromSavedQuery({
-      id: '',
-      name: `Errors within replay`,
-      version: 2,
-      fields: ['replayId', 'count()'],
-      query: `issue.id:${group.id} !replayId:""`,
-      projects: [Number(group.project.id)],
-    });
-
     try {
-      const [{data}, _textStatus, resp] = await doDiscoverQuery<TableData>(
-        api,
-        `/organizations/${organization.slug}/events/`,
-        eventView.getEventsAPIPayload({
-          query: {cursor},
-        } as Location<ReplayListLocationQuery>)
+      const response = await api.requestPromise(
+        `/organizations/${organization.slug}/replay-count/`,
+        {
+          query: {
+            returnIds: true,
+            query: `issue.id:[${group.id}]`,
+            statsPeriod: '14d',
+            project: group.project.id,
+          },
+        }
       );
-
-      setResponse({
-        pageLinks: resp?.getResponseHeader('Link') ?? '',
-        replayIds: data.map(record => String(record.replayId)),
-      });
-    } catch (err) {
-      Sentry.captureException(err);
-      setFetchError(err);
+      setReplayIds(response[group.id] || []);
+    } catch (error) {
+      Sentry.captureException(error);
+      setFetchError(error);
     }
-  }, [api, cursor, organization.slug, group.id, group.project.id]);
+  }, [api, organization.slug, group.id, group.project.id]);
 
   const eventView = useMemo(() => {
-    if (!response.replayIds) {
+    if (!replayIds.length) {
       return null;
     }
     return EventView.fromSavedQuery({
@@ -70,10 +54,11 @@ function useReplayFromIssue({
       version: 2,
       fields: REPLAY_LIST_FIELDS,
       projects: [Number(group.project.id)],
-      query: `id:[${String(response.replayIds)}]`,
+      query: `id:[${String(replayIds)}]`,
+      range: '14d',
       orderby: decodeScalar(location.query.sort, DEFAULT_SORT),
     });
-  }, [location.query.sort, group.project.id, response.replayIds]);
+  }, [location.query.sort, group.project.id, replayIds]);
 
   useCleanQueryParamsOnRouteLeave({fieldsToClean: ['cursor']});
   useEffect(() => {
@@ -83,7 +68,7 @@ function useReplayFromIssue({
   return {
     eventView,
     fetchError,
-    pageLinks: response.pageLinks,
+    pageLinks: null,
   };
 }