Browse Source

Revert "feat(replay-new-trace) Fetching replay trace data in batches. (#72639)"

This reverts commit cd6efeaa61511d64199b2013905d5bba9a804f49.

Co-authored-by: JonasBa <9317857+JonasBa@users.noreply.github.com>
getsentry-bot 9 months ago
parent
commit
ba9b0829d4

+ 53 - 40
static/app/views/performance/newTraceDetails/trace.spec.tsx

@@ -64,7 +64,7 @@ function mockTraceResponse(resp?: Partial<ResponseType>) {
     url: '/organizations/org-slug/events-trace/trace-id/',
     method: 'GET',
     asyncDelay: 1,
-    ...(resp ?? {body: {}}),
+    ...(resp ?? {}),
   });
 }
 
@@ -73,14 +73,7 @@ function mockTraceMetaResponse(resp?: Partial<ResponseType>) {
     url: '/organizations/org-slug/events-trace-meta/trace-id/',
     method: 'GET',
     asyncDelay: 1,
-    ...(resp ?? {
-      body: {
-        errors: 0,
-        performance_issues: 0,
-        projects: 0,
-        transactions: 0,
-      },
-    }),
+    ...(resp ?? {}),
   });
 }
 
@@ -89,7 +82,7 @@ function mockTraceTagsResponse(resp?: Partial<ResponseType>) {
     url: '/organizations/org-slug/events-facets/',
     method: 'GET',
     asyncDelay: 1,
-    ...(resp ?? {body: []}),
+    ...(resp ?? []),
   });
 }
 
@@ -223,25 +216,6 @@ function makeSpan(overrides: Partial<RawSpanType> = {}): TraceTree.Span {
   };
 }
 
-function getVirtualizedContainer(): HTMLElement {
-  const virtualizedContainer = screen.queryByTestId('trace-virtualized-list');
-  if (!virtualizedContainer) {
-    throw new Error('Virtualized container not found');
-  }
-  return virtualizedContainer;
-}
-
-function getVirtualizedScrollContainer(): HTMLElement {
-  const virtualizedScrollContainer = screen.queryByTestId(
-    'trace-virtualized-list-scroll-container'
-  );
-
-  if (!virtualizedScrollContainer) {
-    throw new Error('Virtualized scroll container not found');
-  }
-  return virtualizedScrollContainer;
-}
-
 async function keyboardNavigationTestSetup() {
   const keyboard_navigation_transactions: TraceFullDetailed[] = [];
   for (let i = 0; i < 1e4; i++) {
@@ -268,8 +242,18 @@ async function keyboardNavigationTestSetup() {
   mockMetricsResponse();
 
   const value = render(<TraceView />, {router});
-  const virtualizedContainer = getVirtualizedContainer();
-  const virtualizedScrollContainer = getVirtualizedScrollContainer();
+  const virtualizedContainer = screen.queryByTestId('trace-virtualized-list');
+  const virtualizedScrollContainer = screen.queryByTestId(
+    'trace-virtualized-list-scroll-container'
+  );
+
+  if (!virtualizedContainer) {
+    throw new Error('Virtualized container not found');
+  }
+
+  if (!virtualizedScrollContainer) {
+    throw new Error('Virtualized scroll container not found');
+  }
 
   // Awaits for the placeholder rendering rows to be removed
   expect(await findByText(value.container, /transaction-op-0/i)).toBeInTheDocument();
@@ -302,8 +286,18 @@ async function pageloadTestSetup() {
   mockMetricsResponse();
 
   const value = render(<TraceView />, {router});
-  const virtualizedContainer = getVirtualizedContainer();
-  const virtualizedScrollContainer = getVirtualizedScrollContainer();
+  const virtualizedContainer = screen.queryByTestId('trace-virtualized-list');
+  const virtualizedScrollContainer = screen.queryByTestId(
+    'trace-virtualized-list-scroll-container'
+  );
+
+  if (!virtualizedContainer) {
+    throw new Error('Virtualized container not found');
+  }
+
+  if (!virtualizedScrollContainer) {
+    throw new Error('Virtualized scroll container not found');
+  }
 
   // Awaits for the placeholder rendering rows to be removed
   expect((await screen.findAllByText(/transaction-op-/i)).length).toBeGreaterThan(0);
@@ -336,8 +330,18 @@ async function searchTestSetup() {
   mockMetricsResponse();
 
   const value = render(<TraceView />, {router});
-  const virtualizedContainer = getVirtualizedContainer();
-  const virtualizedScrollContainer = getVirtualizedScrollContainer();
+  const virtualizedContainer = screen.queryByTestId('trace-virtualized-list');
+  const virtualizedScrollContainer = screen.queryByTestId(
+    'trace-virtualized-list-scroll-container'
+  );
+
+  if (!virtualizedContainer) {
+    throw new Error('Virtualized container not found');
+  }
+
+  if (!virtualizedScrollContainer) {
+    throw new Error('Virtualized scroll container not found');
+  }
 
   // Awaits for the placeholder rendering rows to be removed
   expect(await findByText(value.container, /transaction-op-0/i)).toBeInTheDocument();
@@ -376,10 +380,15 @@ async function simpleTestSetup() {
   mockMetricsResponse();
 
   const value = render(<TraceView />, {router});
-  const virtualizedContainer = getVirtualizedContainer();
+  const virtualizedContainer = screen.queryByTestId('trace-virtualized-list');
   const virtualizedScrollContainer = screen.queryByTestId(
     'trace-virtualized-list-scroll-container'
   );
+
+  if (!virtualizedContainer) {
+    throw new Error('Virtualized container not found');
+  }
+
   if (!virtualizedScrollContainer) {
     throw new Error('Virtualized scroll container not found');
   }
@@ -398,12 +407,16 @@ const DRAWER_TABS_CONTAINER_TEST_ID = 'trace-drawer-tabs';
 const VISIBLE_TRACE_ROW_SELECTOR = '.TraceRow:not(.Hidden)';
 const ACTIVE_SEARCH_HIGHLIGHT_ROW = '.TraceRow.SearchResult.Highlight:not(.Hidden)';
 const wait = (ms: number) => new Promise(resolve => setTimeout(resolve, ms));
-const searchToUpdate = async (): Promise<void> => {
-  await wait(500);
+const searchToUpdate = (): Promise<void> => {
+  return act(async () => {
+    await wait(500);
+  });
 };
 
-const scrollToEnd = async (): Promise<void> => {
-  await wait(1000);
+const scrollToEnd = (): Promise<void> => {
+  return act(async () => {
+    await wait(1000);
+  });
 };
 
 // @ts-expect-error ignore this line

+ 12 - 5
static/app/views/performance/newTraceDetails/traceApi/useReplayTraceMeta.tsx

@@ -42,9 +42,12 @@ export function useReplayTraceMeta(
   const start = getUtcDateString(replayRecord?.started_at.getTime());
   const end = getUtcDateString(replayRecord?.finished_at.getTime());
 
-  const {data: eventsData, isLoading: eventsIsLoading} = useApiQuery<{
-    data: TableDataRow[];
-  }>(
+  const {
+    data: eventsData,
+    isLoading: eventsIsLoading,
+    isRefetching: eventsIsRefetching,
+    refetch: eventsRefetch,
+  } = useApiQuery<{data: TableDataRow[]}>(
     [
       `/organizations/${organization.slug}/events/`,
       {
@@ -76,9 +79,13 @@ export function useReplayTraceMeta(
     return {
       data: meta.data,
       isLoading: eventsIsLoading || meta.isLoading,
-      errors: meta.errors,
+      isRefetching: meta.isRefetching || eventsIsRefetching,
+      refetch: () => {
+        meta.refetch();
+        eventsRefetch();
+      },
     };
-  }, [meta, eventsIsLoading]);
+  }, [meta, eventsIsLoading, eventsIsRefetching, eventsRefetch]);
 
   return metaResults;
 }

+ 79 - 76
static/app/views/performance/newTraceDetails/traceApi/useTraceMeta.tsx

@@ -1,4 +1,4 @@
-import {useCallback, useLayoutEffect, useMemo, useState} from 'react';
+import {useMemo} from 'react';
 import type {Location} from 'history';
 import * as qs from 'query-string';
 
@@ -6,8 +6,8 @@ import {normalizeDateTimeParams} from 'sentry/components/organizations/pageFilte
 import {DEFAULT_STATS_PERIOD} from 'sentry/constants';
 import type {PageFilters} from 'sentry/types/core';
 import type {TraceMeta} from 'sentry/utils/performance/quickTrace/types';
+import {type ApiQueryKey, useApiQueries} from 'sentry/utils/queryClient';
 import {decodeScalar} from 'sentry/utils/queryString';
-import useApi from 'sentry/utils/useApi';
 import useOrganization from 'sentry/utils/useOrganization';
 import usePageFilters from 'sentry/utils/usePageFilters';
 
@@ -42,25 +42,41 @@ function getMetaQueryParams(
   };
 }
 
+function getTraceMetaQueryKey(
+  traceSlug: string,
+  orgSlug: string,
+  queryParams:
+    | {
+        // demo has the format ${projectSlug}:${eventId}
+        // used to query a demo transaction event from the backend.
+        demo: string | undefined;
+        statsPeriod: string;
+      }
+    | {
+        demo: string | undefined;
+        timestamp: string;
+      }
+): ApiQueryKey {
+  return [
+    `/organizations/${orgSlug}/events-trace-meta/${traceSlug}/`,
+    {query: queryParams},
+  ];
+}
+
 export type TraceMetaQueryResults = {
   data: TraceMeta;
-  errors: Error[];
   isLoading: boolean;
+  isRefetching: boolean;
+  refetch: () => void;
 };
 
-export function useTraceMeta(traceSlugs: string[]): TraceMetaQueryResults {
+export function useTraceMeta(traceSlugs: string[]): {
+  data: TraceMeta;
+  isLoading: boolean;
+  isRefetching: boolean;
+  refetch: () => void;
+} {
   const filters = usePageFilters();
-  const api = useApi();
-  const [metaResults, setMetaResults] = useState<TraceMetaQueryResults>({
-    data: {
-      errors: 0,
-      performance_issues: 0,
-      projects: 0,
-      transactions: 0,
-    },
-    isLoading: true,
-    errors: [],
-  });
   const organization = useOrganization();
 
   const queryParams = useMemo(() => {
@@ -71,67 +87,53 @@ export function useTraceMeta(traceSlugs: string[]): TraceMetaQueryResults {
 
   const mode = queryParams.demo ? 'demo' : undefined;
 
-  const fetchSingleTraceMeta = useCallback(
-    async (traceSlug: string): Promise<TraceMeta> => {
-      const data = await api.requestPromise(
-        `/organizations/${organization.slug}/events-trace-meta/${traceSlug}/`,
-        {
-          method: 'GET',
-          data: queryParams,
-        }
+  const queryKeys = useMemo(() => {
+    return traceSlugs.map(traceSlug =>
+      getTraceMetaQueryKey(traceSlug, organization.slug, queryParams)
+    );
+  }, [traceSlugs, organization, queryParams]);
+
+  const results = useApiQueries<TraceMeta>(queryKeys, {
+    enabled: traceSlugs.length > 0 && !!organization.slug && mode !== 'demo',
+    staleTime: Infinity,
+  });
+
+  const mergedTraceMeta = useMemo(() => {
+    const mergedResult: {
+      data: TraceMeta;
+      isLoading: boolean;
+      isRefetching: boolean;
+      refetch: () => void;
+    } = {
+      data: {
+        errors: 0,
+        performance_issues: 0,
+        projects: 0,
+        transactions: 0,
+      },
+      isLoading: false,
+      isRefetching: false,
+      refetch: () => {
+        results.forEach(result => result.refetch());
+      },
+    };
+
+    for (const metaResult of results) {
+      mergedResult.isLoading ||= metaResult.isLoading;
+      mergedResult.isRefetching ||= metaResult.isRefetching;
+      mergedResult.data.errors += metaResult.data?.errors ?? 0;
+      mergedResult.data.performance_issues += metaResult.data?.performance_issues ?? 0;
+      // TODO: We want the count of unique projects, not the sum of all projects. When there are multiple traces, taking the best guess by using the max
+      // project count accross all traces for now. We don't use the project count for anything yet, so this is fine for now.
+      mergedResult.data.projects += Math.max(
+        metaResult.data?.projects ?? 0,
+        mergedResult.data.projects
       );
-      return data;
-    },
-    [api, organization.slug, queryParams]
-  );
-
-  const fetchTraceMetasInBatches = useCallback(
-    async (traceIds: string[]) => {
-      const clonedTraceIds = [...traceIds];
-
-      while (clonedTraceIds.length > 0) {
-        const batch = clonedTraceIds.splice(0, 3);
-        try {
-          const results = await Promise.allSettled(batch.map(fetchSingleTraceMeta));
-          setMetaResults(prev => {
-            const updatedData = results.reduce(
-              (acc, result) => {
-                if (result.status === 'fulfilled') {
-                  const {errors, performance_issues, projects, transactions} =
-                    result.value;
-                  acc.errors += errors;
-                  acc.performance_issues += performance_issues;
-                  acc.projects = Math.max(acc.projects, projects);
-                  acc.transactions += transactions;
-                }
-                return acc;
-              },
-              {...prev.data}
-            );
-
-            return {
-              ...prev,
-              isLoading: clonedTraceIds.length > 0,
-              data: updatedData,
-            };
-          });
-        } catch (error) {
-          setMetaResults(prev => {
-            return {
-              ...prev,
-              isLoading: false,
-              errors: prev.errors.concat(error),
-            };
-          });
-        }
-      }
-    },
-    [fetchSingleTraceMeta]
-  );
+      mergedResult.data.transactions += metaResult.data?.transactions ?? 0;
+    }
 
-  useLayoutEffect(() => {
-    fetchTraceMetasInBatches(traceSlugs);
-  }, [traceSlugs, fetchTraceMetasInBatches]);
+    return mergedResult;
+  }, [results]);
 
   // When projects don't have performance set up, we allow them to view a sample transaction.
   // The backend creates the sample transaction, however the trace is created async, so when the
@@ -148,9 +150,10 @@ export function useTraceMeta(traceSlugs: string[]): TraceMetaQueryResults {
         transactions: 1,
       },
       isLoading: false,
-      errors: [],
+      isRefetching: false,
+      refetch: () => {},
     };
   }
 
-  return metaResults;
+  return mergedTraceMeta;
 }

+ 37 - 60
static/app/views/replays/detail/trace/replayTransactionContext.tsx

@@ -10,7 +10,7 @@ import {
 import type {Location} from 'history';
 import sortBy from 'lodash/sortBy';
 
-import {getTimeStampFromTableDateField, getUtcDateString} from 'sentry/utils/dates';
+import {getUtcDateString} from 'sentry/utils/dates';
 import type {TableData} from 'sentry/utils/discover/discoverQuery';
 import EventView from 'sentry/utils/discover/eventView';
 import {doDiscoverQuery} from 'sentry/utils/discover/genericDiscoverQuery';
@@ -109,28 +109,27 @@ function ReplayTransactionContext({children, replayRecord}: Options) {
     });
   }, [replayRecord]);
 
+  const singleTracePayload = useMemo(() => {
+    const start = getUtcDateString(replayRecord?.started_at.getTime());
+    const end = getUtcDateString(replayRecord?.finished_at.getTime());
+    const eventView = makeEventView({start, end});
+
+    if (organization.features.includes('replay-trace-view-v1')) {
+      return {
+        ...getTraceRequestPayload({eventView, location: {} as Location}),
+        useSpans: 1,
+      };
+    }
+
+    return getTraceRequestPayload({eventView, location: {} as Location});
+  }, [replayRecord, organization.features]);
+
   const fetchSingleTraceData = useCallback(
-    async dataRow => {
+    async traceId => {
       try {
-        const {trace: traceId, timestamp} = dataRow;
-        const start = getUtcDateString(replayRecord?.started_at.getTime());
-        const end = getUtcDateString(replayRecord?.finished_at.getTime());
-        const eventView = makeEventView({start, end});
-        let payload;
-
-        if (organization.features.includes('replay-trace-view-v1')) {
-          payload = {
-            limit: 10000,
-            useSpans: 1,
-            timestamp,
-          };
-        } else {
-          payload = getTraceRequestPayload({eventView, location: {} as Location});
-        }
-
         const [trace, _traceResp] = await doDiscoverQuery<
           TraceSplitResults<TraceFullDetailed> | TraceFullDetailed[]
-        >(api, `/organizations/${orgSlug}/events-trace/${traceId}/`, payload);
+        >(api, `/organizations/${orgSlug}/events-trace/${traceId}/`, singleTracePayload);
 
         const {transactions, orphanErrors} = getTraceSplitResults<TraceFullDetailed>(
           trace,
@@ -157,39 +156,7 @@ function ReplayTransactionContext({children, replayRecord}: Options) {
         }));
       }
     },
-    [api, orgSlug, organization, replayRecord]
-  );
-
-  const fetchTracesInBatches = useCallback(
-    async data => {
-      const clonedData = [...data];
-
-      while (clonedData.length > 0) {
-        const batch = clonedData.splice(0, 3);
-
-        // Update state for the current batch request
-        setState(
-          prev =>
-            ({
-              ...prev,
-              detailsRequests: prev.detailsRequests + batch.length,
-            }) as InternalState
-        );
-
-        // Wait for the current batch to finish
-        await Promise.allSettled(batch.map(fetchSingleTraceData));
-
-        // Update state for the current batch response
-        setState(
-          prev =>
-            ({
-              ...prev,
-              detailsResponses: prev.detailsResponses + batch.length,
-            }) as InternalState
-        );
-      }
-    },
-    [fetchSingleTraceData]
+    [api, orgSlug, singleTracePayload, organization]
   );
 
   const fetchTransactionData = useCallback(async () => {
@@ -233,15 +200,25 @@ function ReplayTransactionContext({children, replayRecord}: Options) {
           payload
         );
 
-        const parsedData = data
-          .filter(row => row.trace) // Filter out items where trace is not truthy
-          .map(row => ({
-            trace: row.trace,
-            timestamp: getTimeStampFromTableDateField(row['min(timestamp)']),
-          }));
+        const traceIds = data.map(({trace}) => String(trace)).filter(Boolean);
 
+        // Do not await results here. Do the fetches async and let the loop continue
         (async function () {
-          await fetchTracesInBatches(parsedData);
+          setState(
+            prev =>
+              ({
+                ...prev,
+                detailsRequests: prev.detailsRequests + traceIds.length,
+              }) as InternalState
+          );
+          await Promise.allSettled(traceIds.map(fetchSingleTraceData));
+          setState(
+            prev =>
+              ({
+                ...prev,
+                detailsResponses: prev.detailsResponses + traceIds.length,
+              }) as InternalState
+          );
         })();
 
         const pageLinks = listResp?.getResponseHeader('Link') ?? null;
@@ -253,7 +230,7 @@ function ReplayTransactionContext({children, replayRecord}: Options) {
         cursor = {cursor: '', results: false, href: ''} as ParsedHeader;
       }
     }
-  }, [api, listEventView, orgSlug, replayRecord, fetchTracesInBatches]);
+  }, [api, fetchSingleTraceData, listEventView, orgSlug, replayRecord]);
 
   const externalState = useMemo(() => internalToExternalState(state), [state]);
 

+ 0 - 1
static/app/views/replays/detail/trace/trace.tsx

@@ -109,7 +109,6 @@ function Trace({replayRecord}: Props) {
   } = useTransactionData();
 
   const metaResults = useReplayTraceMeta(replayRecord);
-
   const preferences = useMemo(
     () =>
       loadTraceViewPreferences('replay-trace-view-preferences') ||