Browse Source

fix(new-trace): Fixing hook rendering mismatch caused by drawer cards (#77269)

Previously we tried to filter out components that returned null from
within:
```
//<Component1/> --> returns a <Card/>
// {null}
// <Component2/> --> returns null
// Gives us back [<Component1/>]
```
This leads to `Rendered more hooks than during the previous render`
error as we try to conditionally render the children to see if they
return null:
[example](https://sentry.sentry.io/issues/5800979093/?project=11276&query=rendered%20more%20hooks&referrer=issue-stream&sort=date&statsPeriod=7d&stream_index=6)

Now we moved the rendering logic to the parent of Component, and just
filter out null values, without having to render the children:
```
//<Component1/> --> returns a <Card/>
// {null}
// { condition ? <Component2/> : null} where Component2 returns a Card
// Gives us back [<Component1/>]
```

---------

Co-authored-by: Abdullah Khan <abdullahkhan@PG9Y57YDXQ.local>
Co-authored-by: edwardgou-sentry <83961295+edwardgou-sentry@users.noreply.github.com>
Abdullah Khan 6 months ago
parent
commit
e30435120f

+ 10 - 25
static/app/components/keyValueData/index.tsx

@@ -1,4 +1,4 @@
-import {Children, isValidElement, type ReactNode, useRef, useState} from 'react';
+import {Children, type ReactNode, useRef, useState} from 'react';
 import React from 'react';
 import {css} from '@emotion/react';
 import styled from '@emotion/styled';
@@ -159,36 +159,21 @@ export function Card({
   );
 }
 
-type ReactFCWithProps = React.FC<KeyValueDataCardProps>;
-
-const isReactComponent = (type): type is ReactFCWithProps => {
-  return (
-    typeof type === 'function' ||
-    (typeof type === 'object' && type !== null && 'render' in type)
-  );
-};
-
-// Returns an array of children where null/undefined children and children returning null are filtered out.
+// Returns an array of children where null/undefined children are filtered out.
 // For example:
 // <Component1/> --> returns a <Card/>
 // {null}
-// <Component2/> --> returns null
-// Gives us back [<Component1/>]
+// <Component2/> --> returns a <Card/>
+// Gives us back [<Component1/>, <Component2/>]
 const filterChildren = (children: ReactNode): ReactNode[] => {
-  return Children.toArray(children).filter((child: React.ReactNode) => {
-    if (isValidElement(child) && isReactComponent(child.type)) {
-      // Render the child and check if it returns null
-      const renderedChild = child.type(child.props);
-      return renderedChild !== null;
-    }
-
-    return child != null;
-  });
+  return Children.toArray(children).filter(
+    (child: ReactNode) => child !== null && child !== undefined
+  );
 };
 
-// Note: When rendered children have hooks, we need to ensure that there are no hook count mismatches between renders.
-// Instead of rendering rendering {condition ? <Component/> : null}, we should render
-// if(!condition) return null inside Component itself, where Component renders a Card.
+// Note: When conditionally rendering children, instead of returning
+// if(!condition) return null inside Component, we should render  {condition ? <Component/> : null}
+// where Component returns a <Card/>. {null} is ignored when distributing cards into columns.
 export function Container({children}: {children: React.ReactNode}) {
   const containerRef = useRef<HTMLDivElement>(null);
   const columnCount = useIssueDetailsColumnCount(containerRef);

+ 12 - 0
static/app/components/metrics/customMetricsEventData.tsx

@@ -21,6 +21,7 @@ import type {
   MetricType,
   MRI,
 } from 'sentry/types/metrics';
+import type {Organization} from 'sentry/types/organization';
 import {defined} from 'sentry/utils';
 import {getDefaultAggregation, getMetricsUrl} from 'sentry/utils/metrics';
 import {hasCustomMetrics} from 'sentry/utils/metrics/features';
@@ -55,6 +56,17 @@ function tagToQuery(tagKey: string, tagValue: string) {
   return `${tagKey}:"${tagValue}"`;
 }
 
+export function eventHasCustomMetrics(
+  organization: Organization,
+  metricsSummary: MetricsSummary | undefined
+) {
+  return (
+    hasCustomMetrics(organization) &&
+    metricsSummary &&
+    flattenMetricsSummary(metricsSummary).length > 0
+  );
+}
+
 const HALF_HOUR_IN_MS = 30 * 60 * 1000;
 
 interface DataRow {

+ 27 - 18
static/app/views/performance/newTraceDetails/traceDrawer/details/span/index.tsx

@@ -4,7 +4,10 @@ import {EventContexts} from 'sentry/components/events/contexts';
 import {SpanProfileDetails} from 'sentry/components/events/interfaces/spans/spanProfileDetails';
 import {getSpanOperation} from 'sentry/components/events/interfaces/spans/utils';
 import ProjectBadge from 'sentry/components/idBadge/projectBadge';
-import {CustomMetricsEventData} from 'sentry/components/metrics/customMetricsEventData';
+import {
+  CustomMetricsEventData,
+  eventHasCustomMetrics,
+} from 'sentry/components/metrics/customMetricsEventData';
 import {Tooltip} from 'sentry/components/tooltip';
 import {t} from 'sentry/locale';
 import type {Organization} from 'sentry/types/organization';
@@ -23,11 +26,11 @@ import {TraceDrawerComponents} from '.././styles';
 import {IssueList} from '../issues/issues';
 
 import Alerts from './sections/alerts';
-import {SpanDescription} from './sections/description';
+import {hasFormattedSpanDescription, SpanDescription} from './sections/description';
 import {GeneralInfo} from './sections/generalInfo';
-import {SpanHTTPInfo} from './sections/http';
-import {SpanKeys} from './sections/keys';
-import {Tags} from './sections/tags';
+import {hasSpanHTTPInfo, SpanHTTPInfo} from './sections/http';
+import {hasSpanKeys, SpanKeys} from './sections/keys';
+import {hasSpanTags, Tags} from './sections/tags';
 
 function SpanNodeDetailHeader({
   node,
@@ -109,25 +112,31 @@ export function SpanNodeDetails({
                   <IssueList organization={organization} issues={issues} node={node} />
                 ) : null}
                 <TraceDrawerComponents.SectionCardGroup>
-                  <SpanDescription
-                    node={node}
-                    organization={organization}
-                    location={location}
-                  />
+                  {hasFormattedSpanDescription(node) ? (
+                    <SpanDescription
+                      node={node}
+                      organization={organization}
+                      location={location}
+                    />
+                  ) : null}
                   <GeneralInfo
                     node={node}
                     organization={organization}
                     location={location}
                     onParentClick={onParentClick}
                   />
-                  <SpanHTTPInfo span={node.value} />
-                  <Tags span={node.value} />
-                  <SpanKeys node={node} />
-                  <CustomMetricsEventData
-                    projectId={project?.id || ''}
-                    metricsSummary={node.value._metrics_summary}
-                    startTimestamp={node.value.start_timestamp}
-                  />
+                  {hasSpanHTTPInfo(node.value) ? (
+                    <SpanHTTPInfo span={node.value} />
+                  ) : null}
+                  {hasSpanTags(node.value) ? <Tags span={node.value} /> : null}
+                  {hasSpanKeys(node) ? <SpanKeys node={node} /> : null}
+                  {eventHasCustomMetrics(organization, node.value._metrics_summary) ? (
+                    <CustomMetricsEventData
+                      projectId={project?.id || ''}
+                      metricsSummary={node.value._metrics_summary}
+                      startTimestamp={node.value.start_timestamp}
+                    />
+                  ) : null}
                 </TraceDrawerComponents.SectionCardGroup>
                 <EventContexts event={node.value.event} />
                 {organization.features.includes('profiling') ? (

+ 19 - 4
static/app/views/performance/newTraceDetails/traceDrawer/details/span/sections/description.tsx

@@ -26,6 +26,24 @@ import {TraceDrawerComponents} from '../../styles';
 
 const formatter = new SQLishFormatter();
 
+export function hasFormattedSpanDescription(node: TraceTreeNode<TraceTree.Span>) {
+  const span = node.value;
+  const resolvedModule: ModuleName = resolveSpanModule(
+    span.sentry_tags?.op,
+    span.sentry_tags?.category
+  );
+
+  const formattedDescription =
+    resolvedModule !== ModuleName.DB
+      ? span.description ?? ''
+      : formatter.toString(span.description ?? '');
+
+  return (
+    !!formattedDescription &&
+    [ModuleName.DB, ModuleName.RESOURCE].includes(resolvedModule)
+  );
+}
+
 export function SpanDescription({
   node,
   organization,
@@ -50,10 +68,7 @@ export function SpanDescription({
     return formatter.toString(span.description ?? '');
   }, [span.description, resolvedModule]);
 
-  if (
-    !formattedDescription ||
-    ![ModuleName.DB, ModuleName.RESOURCE].includes(resolvedModule)
-  ) {
+  if (!hasFormattedSpanDescription(node)) {
     return null;
   }
 

+ 11 - 0
static/app/views/performance/newTraceDetails/traceDrawer/details/span/sections/http.tsx

@@ -6,6 +6,17 @@ import {safeURL} from 'sentry/utils/url/safeURL';
 
 import {type SectionCardKeyValueList, TraceDrawerComponents} from '../../styles';
 
+export function hasSpanHTTPInfo(span: RawSpanType) {
+  if (span.op !== 'http.client' || !span.description) {
+    return false;
+  }
+
+  const [_, url] = span.description.split(' ');
+  const parsedURL = safeURL(url);
+
+  return !!parsedURL;
+}
+
 export function SpanHTTPInfo({span}: {span: RawSpanType}) {
   if (span.op === 'http.client' && span.description) {
     const [method, url] = span.description.split(' ');

+ 47 - 20
static/app/views/performance/newTraceDetails/traceDrawer/details/span/sections/keys.tsx

@@ -67,6 +67,52 @@ function partitionSizes(data: RawSpanType['data']): {
   };
 }
 
+function getSpanAggregateMeasurements(node: TraceTreeNode<TraceTree.Span>) {
+  if (!/^ai\.pipeline($|\.)/.test(node.value.op ?? '')) {
+    return [];
+  }
+
+  let sum = 0;
+  TraceTreeNode.ForEachChild(node, n => {
+    if (
+      isSpanNode(n) &&
+      typeof n?.value?.measurements?.ai_total_tokens_used?.value === 'number'
+    ) {
+      sum += n.value.measurements.ai_total_tokens_used.value;
+    }
+  });
+  return [
+    {
+      key: 'ai.pipeline',
+      subject: 'sum(ai_total_tokens_used)',
+      value: sum,
+    },
+  ];
+}
+
+export function hasSpanKeys(node: TraceTreeNode<TraceTree.Span>) {
+  const span = node.value;
+  const {sizeKeys, nonSizeKeys} = partitionSizes(span?.data ?? {});
+  const allZeroSizes = SIZE_DATA_KEYS.map(key => sizeKeys[key]).every(
+    value => value === 0
+  );
+  const unknownKeys = Object.keys(span).filter(key => {
+    return !isHiddenDataKey(key) && !rawSpanKeys.has(key as any);
+  });
+  const timingKeys = getSpanSubTimings(span) ?? [];
+  const aggregateMeasurements: SectionCardKeyValueList =
+    getSpanAggregateMeasurements(node);
+
+  return (
+    !allZeroSizes ||
+    unknownKeys.length > 0 ||
+    timingKeys.length > 0 ||
+    aggregateMeasurements.length > 0 ||
+    Object.keys(nonSizeKeys).length > 0 ||
+    Object.keys(sizeKeys).length > 0
+  );
+}
+
 export function SpanKeys({node}: {node: TraceTreeNode<TraceTree.Span>}) {
   const span = node.value;
   const {sizeKeys, nonSizeKeys} = partitionSizes(span?.data ?? {});
@@ -81,26 +127,7 @@ export function SpanKeys({node}: {node: TraceTreeNode<TraceTree.Span>}) {
   const items: SectionCardKeyValueList = [];
 
   const aggregateMeasurements: SectionCardKeyValueList = useMemo(() => {
-    if (!/^ai\.pipeline($|\.)/.test(node.value.op ?? '')) {
-      return [];
-    }
-
-    let sum = 0;
-    TraceTreeNode.ForEachChild(node, n => {
-      if (
-        isSpanNode(n) &&
-        typeof n?.value?.measurements?.ai_total_tokens_used?.value === 'number'
-      ) {
-        sum += n.value.measurements.ai_total_tokens_used.value;
-      }
-    });
-    return [
-      {
-        key: 'ai.pipeline',
-        subject: 'sum(ai_total_tokens_used)',
-        value: sum,
-      },
-    ];
+    return getSpanAggregateMeasurements(node);
   }, [node]);
 
   if (allZeroSizes) {

+ 4 - 0
static/app/views/performance/newTraceDetails/traceDrawer/details/span/sections/tags.tsx

@@ -3,6 +3,10 @@ import {t} from 'sentry/locale';
 
 import {type SectionCardKeyValueList, TraceDrawerComponents} from '../../styles';
 
+export function hasSpanTags(span: RawSpanType) {
+  return !!span.tags && Object.keys(span.tags).length > 0;
+}
+
 export function Tags({span}: {span: RawSpanType}) {
   const tags: {[tag_name: string]: string} | undefined = span?.tags;
 

+ 22 - 13
static/app/views/performance/newTraceDetails/traceDrawer/details/transaction/index.tsx

@@ -12,7 +12,10 @@ import type {LazyRenderProps} from 'sentry/components/lazyRender';
 import ExternalLink from 'sentry/components/links/externalLink';
 import LoadingError from 'sentry/components/loadingError';
 import LoadingIndicator from 'sentry/components/loadingIndicator';
-import {CustomMetricsEventData} from 'sentry/components/metrics/customMetricsEventData';
+import {
+  CustomMetricsEventData,
+  eventHasCustomMetrics,
+} from 'sentry/components/metrics/customMetricsEventData';
 import {Tooltip} from 'sentry/components/tooltip';
 import {t, tct} from 'sentry/locale';
 import type {EventTransaction} from 'sentry/types/event';
@@ -37,14 +40,14 @@ import {getCustomInstrumentationLink} from '../../../traceConfigurations';
 import {IssueList} from '../issues/issues';
 import {TraceDrawerComponents} from '../styles';
 
-import {AdditionalData} from './sections/additionalData';
+import {AdditionalData, hasAdditionalData} from './sections/additionalData';
 import {BreadCrumbs} from './sections/breadCrumbs';
 import {Entries} from './sections/entries';
 import GeneralInfo from './sections/generalInfo';
-import {Measurements} from './sections/measurements';
+import {hasMeasurements, Measurements} from './sections/measurements';
 import ReplayPreview from './sections/replayPreview';
 import {Request} from './sections/request';
-import {Sdk} from './sections/sdk';
+import {hasSDKContext, Sdk} from './sections/sdk';
 
 export const LAZY_RENDER_PROPS: Partial<LazyRenderProps> = {
   observerOptions: {rootMargin: '50px'},
@@ -172,16 +175,22 @@ export function TransactionNodeDetails({
           event={event}
           location={location}
         />
-        <AdditionalData event={event} />
-        <Measurements event={event} location={location} organization={organization} />
+        {hasAdditionalData(event) ? <AdditionalData event={event} /> : null}
+        {hasMeasurements(event) ? (
+          <Measurements event={event} location={location} organization={organization} />
+        ) : null}
         {cacheMetrics.length > 0 ? <CacheMetrics cacheMetrics={cacheMetrics} /> : null}
-        <Sdk event={event} />
-        <CustomMetricsEventData
-          metricsSummary={event._metrics_summary}
-          startTimestamp={event.startTimestamp}
-          projectId={event.projectID}
-        />
-        <TraceDrawerComponents.TraceDataSection event={event} />
+        {hasSDKContext(event) ? <Sdk event={event} /> : null}
+        {eventHasCustomMetrics(organization, event._metrics_summary) ? (
+          <CustomMetricsEventData
+            metricsSummary={event._metrics_summary}
+            startTimestamp={event.startTimestamp}
+            projectId={event.projectID}
+          />
+        ) : null}
+        {event.contexts.trace?.data ? (
+          <TraceDrawerComponents.TraceDataSection event={event} />
+        ) : null}
       </TraceDrawerComponents.SectionCardGroup>
 
       <Request event={event} />

+ 4 - 0
static/app/views/performance/newTraceDetails/traceDrawer/details/transaction/sections/additionalData.tsx

@@ -45,6 +45,10 @@ function getEventExtraDataKnownDataDetails({
   }
 }
 
+export function hasAdditionalData(event: EventTransaction) {
+  return !!event.context && !isEmptyObject(event.context);
+}
+
 export function AdditionalData({event}: {event: EventTransaction}) {
   const [raw, setRaw] = useState(false);
 

+ 10 - 0
static/app/views/performance/newTraceDetails/traceDrawer/details/transaction/sections/measurements.tsx

@@ -47,6 +47,16 @@ function generateLinkWithQuery(
   });
 }
 
+export function hasMeasurements(event: EventTransaction) {
+  const measurementNames = Object.keys(event.measurements ?? {})
+    .filter(name => isCustomMeasurement(`measurements.${name}`))
+    .filter(isNotMarkMeasurement)
+    .filter(isNotPerformanceScoreMeasurement)
+    .sort();
+
+  return measurementNames.length > 0;
+}
+
 export function Measurements({event, location, organization}: MeasurementsProps) {
   const measurementNames = Object.keys(event.measurements ?? {})
     .filter(name => isCustomMeasurement(`measurements.${name}`))

Some files were not shown because too many files changed in this diff