Browse Source

fix(breadcrumbs): Ensure all data appears on custom renderers (#74208)

There are some special renderers for SQL, HTTP requests, and Exceptions.
This PR fixes a bug where SQL and Exception renderers would not display
their data blobs associated with the breadcrumbs:

Exception:

![image](https://github.com/user-attachments/assets/9567837b-7701-4cb5-bbab-6ff1272ba3bd)

SQL:

![image](https://github.com/user-attachments/assets/ae8a32b3-5ea1-4834-a137-000ce8128ccd)
Leander Rodrigues 8 months ago
parent
commit
f9b7f3fe55

+ 120 - 0
static/app/components/events/breadcrumbs/breadcrumbItemContent.spec.tsx

@@ -0,0 +1,120 @@
+import {render, screen} from 'sentry-test/reactTestingLibrary';
+
+import BreadcrumbItemContent from 'sentry/components/events/breadcrumbs/breadcrumbItemContent';
+import {
+  BreadcrumbLevelType,
+  BreadcrumbMessageFormat,
+  BreadcrumbType,
+  type BreadcrumbTypeDefault,
+  type BreadcrumbTypeHTTP,
+} from 'sentry/types/breadcrumbs';
+
+describe('BreadcrumbItemContent', function () {
+  it('renders default crumbs with all data', function () {
+    const breadcrumb: BreadcrumbTypeDefault = {
+      type: BreadcrumbType.DEBUG,
+      level: BreadcrumbLevelType.INFO,
+      message: 'my message',
+      data: {a: 1, b: 2, c: 3, d: 4, e: 5, f: 6},
+    };
+    render(<BreadcrumbItemContent breadcrumb={breadcrumb} />);
+    expect(screen.getByText(breadcrumb.message as string)).toBeInTheDocument();
+    expect(screen.getByText('6 items')).toBeInTheDocument();
+  });
+
+  it('renders HTTP crumbs with all data', function () {
+    const breadcrumb: BreadcrumbTypeHTTP = {
+      type: BreadcrumbType.HTTP,
+      level: BreadcrumbLevelType.INFO,
+      message: 'my message',
+      data: {
+        method: 'GET',
+        status_code: 500,
+        url: 'https://example.com',
+        someOtherData: 123,
+        responseSize: 15080,
+      },
+    };
+    render(<BreadcrumbItemContent breadcrumb={breadcrumb} />);
+    expect(screen.getByText(breadcrumb.message as string)).toBeInTheDocument();
+    // Link is rendered in a span between method and status code
+    expect(
+      screen.getByText(`${breadcrumb.data?.method}: [${breadcrumb.data?.status_code}]`)
+    ).toBeInTheDocument();
+    expect(screen.getByRole('link', {name: breadcrumb.data?.url})).toBeInTheDocument();
+    expect(screen.getByText('2 items')).toBeInTheDocument();
+  });
+
+  it('renders SQL crumbs with all data', function () {
+    const breadcrumb: BreadcrumbTypeDefault = {
+      type: BreadcrumbType.QUERY,
+      level: BreadcrumbLevelType.INFO,
+      messageFormat: BreadcrumbMessageFormat.SQL,
+      message: "SELECT * from 'table'",
+      data: {a: 1, b: 2, c: 3, d: 4, e: 5, f: 6},
+    };
+    render(<BreadcrumbItemContent breadcrumb={breadcrumb} />);
+    // .token denotes Prism tokens for special formatting
+    expect(
+      screen.getByText(breadcrumb.message as string, {selector: '.token'})
+    ).toBeInTheDocument();
+    expect(screen.getByText('6 items')).toBeInTheDocument();
+  });
+
+  it('renders exception crumbs with all data', function () {
+    const breadcrumb: BreadcrumbTypeDefault = {
+      type: BreadcrumbType.WARNING,
+      level: BreadcrumbLevelType.WARNING,
+      message: 'Consider using more emoji',
+      data: {
+        type: 'EmojiError',
+        value: '🔥🤔',
+        a: 1,
+        b: 2,
+        c: 3,
+        d: 4,
+        e: 5,
+        f: 6,
+      },
+    };
+    const item = render(<BreadcrumbItemContent breadcrumb={breadcrumb} />);
+    expect(screen.getByText(breadcrumb.message as string)).toBeInTheDocument();
+    expect(
+      screen.getByText(`${breadcrumb?.data?.type}: ${breadcrumb?.data?.value}`)
+    ).toBeInTheDocument();
+    expect(screen.getByText('6 items')).toBeInTheDocument();
+    item.unmount();
+
+    const itemWithoutType = render(
+      <BreadcrumbItemContent
+        breadcrumb={{
+          ...breadcrumb,
+          data: {
+            ...breadcrumb.data,
+            type: undefined,
+          },
+        }}
+      />
+    );
+    expect(screen.getByText(breadcrumb.message as string)).toBeInTheDocument();
+    expect(screen.getByText(breadcrumb?.data?.value)).toBeInTheDocument();
+    expect(screen.getByText('6 items')).toBeInTheDocument();
+    itemWithoutType.unmount();
+
+    const itemWithoutValue = render(
+      <BreadcrumbItemContent
+        breadcrumb={{
+          ...breadcrumb,
+          data: {
+            ...breadcrumb.data,
+            value: undefined,
+          },
+        }}
+      />
+    );
+    expect(screen.getByText(breadcrumb.message as string)).toBeInTheDocument();
+    expect(screen.getByText(breadcrumb?.data?.type)).toBeInTheDocument();
+    expect(screen.getByText('6 items')).toBeInTheDocument();
+    itemWithoutValue.unmount();
+  });
+});

+ 58 - 29
static/app/components/events/breadcrumbs/breadcrumbsItemContent.tsx → static/app/components/events/breadcrumbs/breadcrumbItemContent.tsx

@@ -18,12 +18,6 @@ import {defined} from 'sentry/utils';
 import {isUrl} from 'sentry/utils/string/isUrl';
 import {usePrismTokens} from 'sentry/utils/usePrismTokens';
 
-interface BreadcrumbsItemContentProps {
-  breadcrumb: RawCrumb;
-  fullyExpanded?: boolean;
-  meta?: Record<string, any>;
-}
-
 const DEFAULT_STRUCTURED_DATA_PROPS = {
   depth: 0,
   maxDefaultDepth: 2,
@@ -31,14 +25,23 @@ const DEFAULT_STRUCTURED_DATA_PROPS = {
   withOnlyFormattedText: true,
 };
 
-export default function BreadcrumbsItemContent({
+interface BreadcrumbItemContentProps {
+  breadcrumb: RawCrumb;
+  fullyExpanded?: boolean;
+  meta?: Record<string, any>;
+}
+
+export default function BreadcrumbItemContent({
   breadcrumb: bc,
   meta,
   fullyExpanded = false,
-}: BreadcrumbsItemContentProps) {
+}: BreadcrumbItemContentProps) {
   const structuredDataProps = {
     ...DEFAULT_STRUCTURED_DATA_PROPS,
-    maxDefaultDepth: fullyExpanded ? 10000 : 1,
+    forceDefaultExpand: fullyExpanded,
+    maxDefaultDepth: fullyExpanded
+      ? 10000
+      : DEFAULT_STRUCTURED_DATA_PROPS.maxDefaultDepth,
   };
 
   const defaultMessage = defined(bc.message) ? (
@@ -69,11 +72,19 @@ export default function BreadcrumbsItemContent({
     bc?.message &&
     bc?.messageFormat === BreadcrumbMessageFormat.SQL
   ) {
-    return <SQLCrumbContent breadcrumb={bc} />;
+    return <SQLCrumbContent breadcrumb={bc}>{defaultData}</SQLCrumbContent>;
   }
 
   if (bc?.type === BreadcrumbType.WARNING || bc?.type === BreadcrumbType.ERROR) {
-    return <ErrorCrumbContent breadcrumb={bc}>{defaultMessage}</ErrorCrumbContent>;
+    return (
+      <ExceptionCrumbContent
+        breadcrumb={bc}
+        meta={meta}
+        structuredDataProps={structuredDataProps}
+      >
+        {defaultMessage}
+      </ExceptionCrumbContent>
+    );
   }
 
   return (
@@ -103,7 +114,10 @@ function HTTPCrumbContent({
       <Timeline.Text>
         {defined(method) && `${method}: `}
         {isValidUrl ? (
-          <Link onClick={() => openNavigateToExternalLinkModal({linkText: url})}>
+          <Link
+            role="link"
+            onClick={() => openNavigateToExternalLinkModal({linkText: url})}
+          >
             {url}
           </Link>
         ) : (
@@ -122,41 +136,56 @@ function HTTPCrumbContent({
 
 function SQLCrumbContent({
   breadcrumb,
+  children,
 }: {
   breadcrumb: BreadcrumbTypeDefault | BreadcrumbTypeNavigation;
+  children: React.ReactNode;
 }) {
   const tokens = usePrismTokens({code: breadcrumb?.message ?? '', language: 'sql'});
   return (
-    <Timeline.Data>
-      <LightenTextColor className="language-sql">
-        {tokens.map((line, i) => (
-          <div key={i}>
-            {line.map((token, j) => (
-              <span key={j} className={token.className}>
-                {token.children}
-              </span>
-            ))}
-          </div>
-        ))}
-      </LightenTextColor>
-    </Timeline.Data>
+    <Fragment>
+      <Timeline.Data>
+        <LightenTextColor className="language-sql">
+          {tokens.map((line, i) => (
+            <div key={i}>
+              {line.map((token, j) => (
+                <span key={j} className={token.className}>
+                  {token.children}
+                </span>
+              ))}
+            </div>
+          ))}
+        </LightenTextColor>
+      </Timeline.Data>
+      {children}
+    </Fragment>
   );
 }
 
-function ErrorCrumbContent({
+function ExceptionCrumbContent({
   breadcrumb,
+  meta,
   children = null,
+  structuredDataProps,
 }: {
   breadcrumb: BreadcrumbTypeDefault;
-  children?: React.ReactNode;
+  children: React.ReactNode;
+  structuredDataProps: typeof DEFAULT_STRUCTURED_DATA_PROPS;
+  meta?: Record<string, any>;
 }) {
+  const {type, value, ...otherData} = breadcrumb?.data ?? {};
   return (
     <Fragment>
       <Timeline.Text>
-        {breadcrumb?.data?.type && `${breadcrumb?.data?.type}: `}
-        {breadcrumb?.data?.value}
+        {type && type}
+        {type ? value && `: ${value}` : value && value}
       </Timeline.Text>
       {children}
+      {Object.keys(otherData).length > 0 ? (
+        <Timeline.Data>
+          <StructuredData value={otherData} meta={meta} {...structuredDataProps} />
+        </Timeline.Data>
+      ) : null}
     </Fragment>
   );
 }

+ 3 - 3
static/app/components/events/breadcrumbs/breadcrumbsTimeline.tsx

@@ -5,7 +5,7 @@ import moment from 'moment';
 
 import DateTime from 'sentry/components/dateTime';
 import Duration from 'sentry/components/duration';
-import BreadcrumbsItemContent from 'sentry/components/events/breadcrumbs/breadcrumbsItemContent';
+import BreadcrumbItemContent from 'sentry/components/events/breadcrumbs/breadcrumbItemContent';
 import type {EnhancedCrumb} from 'sentry/components/events/breadcrumbs/utils';
 import Timeline from 'sentry/components/timeline';
 import {Tooltip} from 'sentry/components/tooltip';
@@ -100,7 +100,7 @@ export default function BreadcrumbsTimeline({
         data-index={virtualizedRow.index}
       >
         <ContentWrapper isCompact={isCompact}>
-          <BreadcrumbsItemContent
+          <BreadcrumbItemContent
             breadcrumb={breadcrumb}
             meta={meta}
             fullyExpanded={!isCompact}
@@ -140,5 +140,5 @@ const Timestamp = styled('div')`
 `;
 
 const ContentWrapper = styled('div')<{isCompact: boolean}>`
-  padding-bottom: ${p => space(p.isCompact ? 0.5 : 1.5)};
+  padding-bottom: ${p => space(p.isCompact ? 0.5 : 1.0)};
 `;

+ 19 - 15
static/app/types/breadcrumbs.tsx

@@ -77,21 +77,25 @@ export interface BreadcrumbTypeInit extends BreadcrumbTypeBase {
 
 export interface BreadcrumbTypeHTTP extends BreadcrumbTypeBase {
   type: BreadcrumbType.HTTP;
-  data?: null | {
-    method?:
-      | 'POST'
-      | 'PUT'
-      | 'GET'
-      | 'HEAD'
-      | 'DELETE'
-      | 'CONNECT'
-      | 'OPTIONS'
-      | 'TRACE'
-      | 'PATCH';
-    reason?: string;
-    status_code?: number;
-    url?: string;
-  };
+  data?:
+    | null
+    | Record<string, any>
+    // Though this is the expected type, more data can be attached to these crumbs
+    | {
+        method?:
+          | 'POST'
+          | 'PUT'
+          | 'GET'
+          | 'HEAD'
+          | 'DELETE'
+          | 'CONNECT'
+          | 'OPTIONS'
+          | 'TRACE'
+          | 'PATCH';
+        reason?: string;
+        status_code?: number;
+        url?: string;
+      };
 }
 
 export interface BreadcrumbTypeDefault extends BreadcrumbTypeBase {