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

fix(replays): Catch errors that happen inside ReplayReader.factory and show a message (#46348)

Now whenever an exception happens inside our ReplayReader.factory call
(so things like attachments/breadcrumbs/rrweb events are malformed)
we're going to see an error inside the page, instead of everything being
useless.

| Before | After |
| --- | --- |
|
![SCR-20230324-nrkl](https://user-images.githubusercontent.com/187460/227654650-8b81b9da-1ad5-40cf-9cf2-b07760e7f3d6.png)
| <img width="1122" alt="SCR-20230413-myes"
src="https://user-images.githubusercontent.com/187460/231673802-485e301b-bce9-4ff8-9736-619f661accbe.png">
|

Fixes https://github.com/getsentry/sentry/issues/46128



<!-- Describe your PR here. -->
Ryan Albrecht 1 год назад
Родитель
Сommit
5e33e0075d

+ 1 - 0
fixtures/js-stubs/replaySegments.ts

@@ -104,6 +104,7 @@ export function ReplaySegmentNavigation({
   return ReplaySegmentBreadcrumb({
     timestamp,
     payload: {
+      timestamp: timestamp.getTime() / 1000, // sentry data inside rrweb is in seconds
       type: 'default',
       category: 'navigation',
       data: {

+ 22 - 5
static/app/components/replays/walker/splitCrumbs.tsx

@@ -8,23 +8,40 @@ import TextOverflow from 'sentry/components/textOverflow';
 import {Tooltip} from 'sentry/components/tooltip';
 import {tn} from 'sentry/locale';
 import {space} from 'sentry/styles/space';
-import {BreadcrumbTypeNavigation, Crumb} from 'sentry/types/breadcrumbs';
+import {
+  BreadcrumbType,
+  BreadcrumbTypeInit,
+  BreadcrumbTypeNavigation,
+  Crumb,
+} from 'sentry/types/breadcrumbs';
 import useCrumbHandlers from 'sentry/utils/replays/hooks/useCrumbHandlers';
 
 type MaybeOnClickHandler = null | ((crumb: Crumb) => void);
 
+type InitOrNavCrumb = BreadcrumbTypeNavigation | BreadcrumbTypeInit;
+
+function getUrl(crumb: undefined | InitOrNavCrumb) {
+  if (crumb?.type === BreadcrumbType.NAVIGATION) {
+    return crumb.data?.to?.split('?')?.[0];
+  }
+  if (crumb?.type === BreadcrumbType.INIT) {
+    return crumb.data.url;
+  }
+  return undefined;
+}
+
 function splitCrumbs({
   crumbs,
   onClick,
   startTimestampMs,
 }: {
-  crumbs: BreadcrumbTypeNavigation[];
+  crumbs: InitOrNavCrumb[];
   onClick: MaybeOnClickHandler;
   startTimestampMs: number;
 }) {
-  const firstUrl = first(crumbs)?.data?.to?.split('?')?.[0];
+  const firstUrl = getUrl(first(crumbs));
   const summarizedCrumbs = crumbs.slice(1, -1) as Crumb[];
-  const lastUrl = last(crumbs)?.data?.to?.split('?')?.[0];
+  const lastUrl = getUrl(last(crumbs));
 
   if (crumbs.length === 0) {
     // This one shouldn't overflow, but by including the component css stays
@@ -60,7 +77,7 @@ function splitCrumbs({
   return crumbs.map((crumb, i) => (
     <SingleLinkSegment
       key={i}
-      path={crumb.data?.to?.split('?')?.[0]}
+      path={getUrl(crumb)}
       onClick={onClick ? () => onClick(crumb as Crumb) : null}
     />
   ));

+ 2 - 2
static/app/components/replays/walker/urlWalker.tsx

@@ -24,8 +24,8 @@ export const CrumbWalker = memo(function CrumbWalker({crumbs, replayRecord}: Cru
   const startTimestampMs = replayRecord.started_at.getTime();
   const {handleClick} = useCrumbHandlers(startTimestampMs);
 
-  const navCrumbs = crumbs.filter(
-    crumb => crumb.type === BreadcrumbType.NAVIGATION
+  const navCrumbs = crumbs.filter(crumb =>
+    [BreadcrumbType.INIT, BreadcrumbType.NAVIGATION].includes(crumb.type)
   ) as BreadcrumbTypeNavigation[];
 
   return (

+ 9 - 0
static/app/types/breadcrumbs.tsx

@@ -60,6 +60,15 @@ export interface BreadcrumbTypeNavigation extends BreadcrumbTypeBase {
   };
 }
 
+export interface BreadcrumbTypeInit extends BreadcrumbTypeBase {
+  data: {
+    action: 'replay-init';
+    label: string;
+    url: string;
+  };
+  type: BreadcrumbType.INIT;
+}
+
 export interface BreadcrumbTypeHTTP extends BreadcrumbTypeBase {
   type: BreadcrumbType.HTTP;
   data?: null | {

+ 1 - 0
static/app/utils/replays/hooks/useReplayData.spec.tsx

@@ -33,6 +33,7 @@ function getMockReplayRecord(replayRecord?: Partial<ReplayRecord>) {
 
 describe('useReplayData', () => {
   beforeEach(() => {
+    MockedReplayReaderFactory.mockClear();
     MockApiClient.clearMockResponses();
   });
 

+ 5 - 4
static/app/utils/replays/replayDataUtils.tsx

@@ -5,6 +5,7 @@ import {transformCrumbs} from 'sentry/components/events/interfaces/breadcrumbs/u
 import {t} from 'sentry/locale';
 import type {
   BreadcrumbTypeDefault,
+  BreadcrumbTypeInit,
   BreadcrumbTypeNavigation,
   Crumb,
   RawCrumb,
@@ -147,7 +148,7 @@ export function breadcrumbFactory(
 ): Crumb[] {
   const UNWANTED_CRUMB_CATEGORIES = ['ui.focus', 'ui.blur'];
 
-  const initialUrl = replayRecord.tags.url?.join(', ');
+  const initialUrl = replayRecord.urls?.[0] ?? replayRecord.tags.url?.join(', ');
   const initBreadcrumb = {
     type: BreadcrumbType.INIT,
     timestamp: replayRecord.started_at.toISOString(),
@@ -158,7 +159,7 @@ export function breadcrumbFactory(
       label: t('Start recording'),
       url: initialUrl,
     },
-  } as BreadcrumbTypeDefault;
+  } as BreadcrumbTypeInit;
 
   const errorCrumbs: RawCrumb[] = errors.map(error => ({
     type: BreadcrumbType.ERROR,
@@ -246,7 +247,7 @@ export function breadcrumbFactory(
     });
 
   const result = transformCrumbs([
-    ...(!hasPageLoad ? [initBreadcrumb] : []),
+    ...(spans.length && !hasPageLoad ? [initBreadcrumb] : []),
     ...rawCrumbsWithTimestamp,
     ...errorCrumbs,
     ...spanCrumbs,
@@ -266,7 +267,7 @@ export function spansFactory(spans: ReplaySpan[]) {
 }
 
 /**
- * Calculate min/max of an array simultaniously.
+ * Calculate min/max of an array simultaneously.
  * This prevents two things:
  * - Avoid extra allocations and iterations, just loop through once.
  * - Avoid `Maximum call stack size exceeded` when the array is too large

+ 16 - 1
static/app/utils/replays/replayReader.tsx

@@ -1,3 +1,4 @@
+import * as Sentry from '@sentry/react';
 import {duration} from 'moment';
 
 import type {Crumb} from 'sentry/types/breadcrumbs';
@@ -45,7 +46,21 @@ export default class ReplayReader {
       return null;
     }
 
-    return new ReplayReader({attachments, replayRecord, errors});
+    try {
+      return new ReplayReader({attachments, replayRecord, errors});
+    } catch (err) {
+      Sentry.captureException(err);
+
+      // If something happens then we don't really know if it's the attachments
+      // array or errors array to blame (it's probably attachments though).
+      // Either way we can use the replayRecord to show some metadata, and then
+      // put an error message below it.
+      return new ReplayReader({
+        attachments: [],
+        errors: [],
+        replayRecord,
+      });
+    }
   }
 
   private constructor({

+ 7 - 3
static/app/views/replays/detail/page.tsx

@@ -8,7 +8,7 @@ import DetailsPageBreadcrumbs from 'sentry/components/replays/header/detailsPage
 import FeedbackButton from 'sentry/components/replays/header/feedbackButton';
 import HeaderPlaceholder from 'sentry/components/replays/header/headerPlaceholder';
 import ShareButton from 'sentry/components/replays/shareButton';
-import {CrumbWalker} from 'sentry/components/replays/walker/urlWalker';
+import {CrumbWalker, StringWalker} from 'sentry/components/replays/walker/urlWalker';
 import SentryDocumentTitle from 'sentry/components/sentryDocumentTitle';
 import {t} from 'sentry/locale';
 import {space} from 'sentry/styles/space';
@@ -38,7 +38,7 @@ function Page({children, crumbs, orgSlug, replayRecord}: Props) {
         <DeleteButton />
       </ButtonActionsWrapper>
 
-      {replayRecord && crumbs ? (
+      {replayRecord ? (
         <UserBadge
           avatarSize={32}
           displayName={
@@ -56,7 +56,11 @@ function Page({children, crumbs, orgSlug, replayRecord}: Props) {
           // this is the subheading for the avatar, so displayEmail in this case is a misnomer
           displayEmail={
             <Cols>
-              <CrumbWalker replayRecord={replayRecord} crumbs={crumbs} />
+              {crumbs?.length ? (
+                <CrumbWalker replayRecord={replayRecord} crumbs={crumbs} />
+              ) : (
+                <StringWalker urls={replayRecord.urls} />
+              )}
             </Cols>
           }
         />

+ 6 - 2
static/app/views/replays/details.tsx

@@ -89,10 +89,14 @@ function ReplayDetails({params: {replaySlug}}: Props) {
       <Page orgSlug={orgSlug} replayRecord={replayRecord}>
         <DetailedError
           hideSupportLinks
-          heading={t('Expected two or more replay events')}
+          heading={t('Error loading replay')}
           message={
             <Fragment>
-              <p>{t('This Replay may not have captured any user actions.')}</p>
+              <p>
+                {t(
+                  'Expected two or more replay events. This Replay may not have captured any user actions.'
+                )}
+              </p>
               <p>
                 {t(
                   'Or there may be an issue loading the actions from the server, click to try loading the Replay again.'