feat(trace-tree-node): Mitigating error node type guards usage (#104501)

Co-authored-by: Abdullah Khan <abdullahkhan@PG9Y57YDXQ.local>
Co-authored-by: getsantry[bot] <66042841+getsantry[bot]@users.noreply.github.com>

Author: 3 people

static/app/components/events/interfaces/performance/anrRootCause.tsx

@@ -25,7 +25,6 @@ import {SectionKey} from 'sentry/views/issueDetails/streamline/context';
 import {InterimSection} from 'sentry/views/issueDetails/streamline/interimSection';
 import {useIssuesTraceTree} from 'sentry/views/performance/newTraceDetails/traceApi/useIssuesTraceTree';
 import {useTrace} from 'sentry/views/performance/newTraceDetails/traceApi/useTrace';
-import {isEAPTraceOccurrence} from 'sentry/views/performance/newTraceDetails/traceGuards';
 import useTraceStateAnalytics from 'sentry/views/performance/newTraceDetails/useTraceStateAnalytics';
 
 enum AnrRootCauseAllowlist {
@@ -143,12 +142,9 @@ export function AnrRootCause({event, organization}: Props) {
     >
       {potentialAnrRootCause?.map(occurence => {
         const project = projects.find(p => p.id === occurence.project_id.toString());
-        const title = isEAPTraceOccurrence(occurence)
-          ? occurence.description
-          : occurence.title;
-        const shortId = isEAPTraceOccurrence(occurence)
-          ? occurence.short_id
-          : occurence.issue_short_id;
+        const isEAPOccurence = 'description' in occurence;
+        const title = isEAPOccurence ? occurence.description : occurence.title;
+        const shortId = isEAPOccurence ? occurence.short_id : occurence.issue_short_id;
         return (
           <IssueSummary key={occurence.issue_id}>
             <Title>

static/app/views/performance/newTraceDetails/traceApi/useTrace.tsx

@@ -10,7 +10,6 @@ import {decodeScalar} from 'sentry/utils/queryString';
 import type RequestError from 'sentry/utils/requestError/requestError';
 import useOrganization from 'sentry/utils/useOrganization';
 import usePageFilters from 'sentry/utils/usePageFilters';
-import {isValidEventUUID} from 'sentry/views/performance/newTraceDetails/traceApi/utils';
 import type {TraceTree} from 'sentry/views/performance/newTraceDetails/traceModels/traceTree';
 import {useIsEAPTraceEnabled} from 'sentry/views/performance/newTraceDetails/useIsEAPTraceEnabled';
 
@@ -291,3 +290,9 @@ export function useTrace(
 
   return isDemoMode ? demoTrace : isEAPEnabled ? eapTraceQuery : traceQuery;
 }
+
+const isValidEventUUID = (id: string): boolean => {
+  const uuidRegex =
+    /^[0-9a-f]{8}[0-9a-f]{4}[1-5][0-9a-f]{3}[89ab][0-9a-f]{3}[0-9a-f]{12}$/i;
+  return uuidRegex.test(id);
+};

static/app/views/performance/newTraceDetails/traceApi/useTraceRootEvent.tsx

@@ -11,13 +11,7 @@ import {
   type OurLogsResponseItem,
 } from 'sentry/views/explore/logs/types';
 import {TraceItemDataset} from 'sentry/views/explore/types';
-import {getRepresentativeTraceEvent} from 'sentry/views/performance/newTraceDetails/traceApi/utils';
-import {
-  isEAPError,
-  isTraceError,
-} from 'sentry/views/performance/newTraceDetails/traceGuards';
 import type {TraceTree} from 'sentry/views/performance/newTraceDetails/traceModels/traceTree';
-import {useIsEAPTraceEnabled} from 'sentry/views/performance/newTraceDetails/useIsEAPTraceEnabled';
 
 type Params = {
   logs: OurLogsResponseItem[] | undefined;
@@ -34,28 +28,20 @@ export function useTraceRootEvent({
   logs,
   traceId,
 }: Params): TraceRootEventQueryResults {
-  const rep = getRepresentativeTraceEvent(tree, logs);
+  const rep = tree.findRepresentativeTraceNode({logs});
   const organization = useOrganization();
 
-  // TODO: This is a bit of a mess, we won't need all of this once we switch to EAP only
   const treeIsLoading = tree.type === 'loading';
-  const hasOnlyLogs = !!(tree.type === 'empty' && logs && logs.length > 0);
-  const enabledBase =
-    !treeIsLoading && (tree.type === 'trace' || hasOnlyLogs) && !!rep?.event && !!traceId;
 
-  const isRepEventError =
-    rep.event && OurLogKnownFieldKey.PROJECT_ID in rep.event
-      ? false
-      : isTraceError(rep.event) || isEAPError(rep.event);
+  const enabledBase = !treeIsLoading && !!rep?.event;
 
-  const isEAPTraceEnabled = useIsEAPTraceEnabled();
-  const isEAPQueryEnabled =
-    !isRepEventError && // Errors are not supported in EAP yet
-    (isEAPTraceEnabled || (!treeIsLoading && hasOnlyLogs));
+  const isRepLog = rep?.dataset === TraceItemDataset.LOGS;
+  const isEAPQueryEnabled = !!(isRepLog || rep?.event?.isEAPEvent);
 
+  const projectSlug = rep?.event?.projectSlug;
   const legacyRootEvent = useApiQuery<EventTransaction>(
     [
-      `/organizations/${organization.slug}/events/${rep?.event?.project_slug}:${rep?.event?.event_id}/`,
+      `/organizations/${organization.slug}/events/${projectSlug}:${rep?.event?.id}/`,
       {
         query: {
           referrer: 'trace-details-summary',
@@ -65,32 +51,27 @@ export function useTraceRootEvent({
     {
       // 10 minutes
       staleTime: 1000 * 60 * 10,
-      enabled: enabledBase && !isEAPQueryEnabled,
+      enabled: enabledBase && !isEAPQueryEnabled && !!projectSlug && !!rep?.event?.id,
     }
   );
 
-  const projectId = rep.event
+  const projectId = rep?.event
     ? OurLogKnownFieldKey.PROJECT_ID in rep.event
       ? rep.event[OurLogKnownFieldKey.PROJECT_ID]
-      : rep.event.project_id
+      : rep.event.projectId
     : '';
-  const eventId = rep.event
-    ? OurLogKnownFieldKey.ID in rep.event
+  const eventId = rep?.event
+    ? OurLogKnownFieldKey.PROJECT_ID in rep.event
       ? rep.event[OurLogKnownFieldKey.ID]
-      : rep.event.event_id
+      : rep.event.id
     : '';
-
-  const itemTypes = {
-    log: TraceItemDataset.LOGS,
-    span: TraceItemDataset.SPANS,
-    uptime_check: TraceItemDataset.UPTIME_RESULTS,
-  };
+  const dataset = rep?.dataset ?? TraceItemDataset.SPANS;
 
   const rootEvent = useTraceItemDetails({
     traceItemId: String(eventId),
     projectId: String(projectId),
     traceId,
-    traceItemType: itemTypes[rep.type],
+    traceItemType: dataset,
     referrer: 'api.explore.log-item-details',
     enabled: enabledBase && isEAPQueryEnabled,
   });

static/app/views/performance/newTraceDetails/traceApi/utils.tsx

@@ -1,14 +1,22 @@
 import type {TraceItemDetailsResponse} from 'sentry/views/explore/hooks/useTraceItemDetails';
-import type {OurLogsResponseItem} from 'sentry/views/explore/logs/types';
+import type {TraceSplitResults} from 'sentry/views/performance/newTraceDetails/traceApi/types';
 import type {TraceRootEventQueryResults} from 'sentry/views/performance/newTraceDetails/traceApi/useTraceRootEvent';
-import {
-  isEAPTraceNode,
-  isRootEvent,
-  isTraceNode,
-  isTraceSplitResult,
-  isUptimeCheckNode,
-} from 'sentry/views/performance/newTraceDetails/traceGuards';
 import type {TraceTree} from 'sentry/views/performance/newTraceDetails/traceModels/traceTree';
+import type {BaseNode} from 'sentry/views/performance/newTraceDetails/traceModels/traceTreeNode/baseNode';
+
+export function isBrowserRequestNode(node: BaseNode): boolean {
+  return (
+    // Adjust for SDK changes in https://github.com/getsentry/sentry-javascript/pull/13527
+    node.op === 'browser.request' ||
+    (node.op === 'browser' && node.description === 'request')
+  );
+}
+
+export function isTraceSplitResult(
+  result: TraceTree.Trace
+): result is TraceSplitResults<TraceTree.Transaction> {
+  return 'transactions' in result && 'orphan_errors' in result;
+}
 
 export function isEmptyTrace(trace: TraceTree.Trace): boolean {
   if (isTraceSplitResult(trace)) {
@@ -18,110 +26,8 @@ export function isEmptyTrace(trace: TraceTree.Trace): boolean {
   return trace.length === 0;
 }
 
-const CANDIDATE_TRACE_TITLE_OPS = ['pageload', 'navigation', 'ui.load'];
-
-export type RepresentativeTraceEvent = {
-  event: TraceTree.TraceEvent | OurLogsResponseItem | null;
-  type: 'span' | 'log' | 'uptime_check';
-};
-
-export const getRepresentativeTraceEvent = (
-  tree: TraceTree,
-  logs: OurLogsResponseItem[] | undefined
-): RepresentativeTraceEvent => {
-  const hasLogs = logs && logs.length > 0;
-  if (tree.type === 'empty' && hasLogs) {
-    return {
-      event: logs[0]!,
-      type: 'log',
-    };
-  }
-
-  const traceNode = tree.root.children[0];
-
-  if (!traceNode) {
-    return {
-      event: null,
-      type: 'span',
-    };
-  }
-
-  if (!isTraceNode(traceNode) && !isEAPTraceNode(traceNode)) {
-    throw new TypeError('Not trace node');
-  }
-
-  const traceChild = traceNode.children[0];
-
-  if (traceChild && isUptimeCheckNode(traceChild)) {
-    return {type: 'uptime_check', event: traceChild.value};
-  }
-
-  let preferredRootEvent: TraceTree.TraceEvent | null = null;
-  let firstRootEvent: TraceTree.TraceEvent | null = null;
-  let candidateEvent: TraceTree.TraceEvent | null = null;
-  let firstEvent: TraceTree.TraceEvent | null = null;
-
-  const isEAP = isEAPTraceNode(traceNode);
-  const events = isEAP
-    ? traceNode.value
-    : [...traceNode.value.transactions, ...traceNode.value.orphan_errors];
-  for (const event of events) {
-    if (isRootEvent(event)) {
-      if (!firstRootEvent) {
-        firstRootEvent = event;
-      }
-
-      if (hasPreferredOp(event)) {
-        preferredRootEvent = event;
-        break;
-      }
-      // Otherwise we keep looking for a root eap transaction. If we don't find one, we use other roots, like standalone spans.
-      continue;
-    } else if (
-      // If we haven't found a root transaction, but we found a candidate transaction
-      // with an op that we care about, we can use it for the title. We keep looking for
-      // a root.
-      !candidateEvent &&
-      hasPreferredOp(event)
-    ) {
-      candidateEvent = event;
-      continue;
-    } else if (!firstEvent) {
-      // If we haven't found a root or candidate transaction, we can use the first transaction
-      // in the trace for the title.
-      firstEvent = event;
-    }
-  }
-
-  return {
-    event: preferredRootEvent ?? firstRootEvent ?? candidateEvent ?? firstEvent,
-    type: 'span',
-  };
-};
-
 export const isTraceItemDetailsResponse = (
   data: TraceRootEventQueryResults['data']
 ): data is TraceItemDetailsResponse => {
   return data !== undefined && 'attributes' in data;
 };
-
-export const isValidEventUUID = (id: string): boolean => {
-  const uuidRegex =
-    /^[0-9a-f]{8}[0-9a-f]{4}[1-5][0-9a-f]{3}[89ab][0-9a-f]{3}[0-9a-f]{12}$/i;
-  return uuidRegex.test(id);
-};
-
-/**
- * Prefer "special" root events over generic root events when generating a title
- * for the waterfall view. Picking these improves contextual navigation for linked
- * traces, resulting in more meaningful waterfall titles.
- */
-function hasPreferredOp(event: TraceTree.TraceEvent): boolean {
-  const op =
-    'op' in event
-      ? event.op
-      : 'transaction.op' in event
-        ? event['transaction.op']
-        : undefined;
-  return !!op && CANDIDATE_TRACE_TITLE_OPS.includes(op);
-}

static/app/views/performance/newTraceDetails/traceContextVitals.tsx

@@ -24,7 +24,6 @@ import {
 import {SectionDivider} from 'sentry/views/issueDetails/streamline/foldSection';
 import type {TraceRootEventQueryResults} from 'sentry/views/performance/newTraceDetails/traceApi/useTraceRootEvent';
 import {isTraceItemDetailsResponse} from 'sentry/views/performance/newTraceDetails/traceApi/utils';
-import {isEAPTraceNode} from 'sentry/views/performance/newTraceDetails/traceGuards';
 import type {TraceTree} from 'sentry/views/performance/newTraceDetails/traceModels/traceTree';
 import {
   TRACE_VIEW_MOBILE_VITALS,
@@ -52,9 +51,8 @@ export function TraceContextVitals({rootEventResults, tree, containerWidth}: Pro
     ? TRACE_VIEW_WEB_VITALS
     : TRACE_VIEW_MOBILE_VITALS;
 
-  const isEAPTrace = isEAPTraceNode(traceNode);
   const collectedVitals =
-    isEAPTrace && tree.vital_types.has('mobile')
+    traceNode.isEAPEvent && tree.vital_types.has('mobile')
       ? getMobileVitalsFromRootEventResults(rootEventResults.data)
       : Array.from(tree.vitals.values()).flat();
 

static/app/views/performance/newTraceDetails/traceDrawer/details/autogroup/index.spec.tsx

@@ -0,0 +1,74 @@
+import {OrganizationFixture} from 'sentry-fixture/organization';
+
+import {render, screen} from 'sentry-test/reactTestingLibrary';
+
+import type {TraceTreeNodeExtra} from 'sentry/views/performance/newTraceDetails/traceModels/traceTreeNode/baseNode';
+import {EapSpanNode} from 'sentry/views/performance/newTraceDetails/traceModels/traceTreeNode/eapSpanNode';
+import {SiblingAutogroupNode} from 'sentry/views/performance/newTraceDetails/traceModels/traceTreeNode/siblingAutogroupNode';
+import {
+  makeEAPSpan,
+  makeSiblingAutogroup,
+} from 'sentry/views/performance/newTraceDetails/traceModels/traceTreeTestUtils';
+import {DEFAULT_TRACE_VIEW_PREFERENCES} from 'sentry/views/performance/newTraceDetails/traceState/tracePreferences';
+import {TraceStateProvider} from 'sentry/views/performance/newTraceDetails/traceState/traceStateProvider';
+
+import {AutogroupNodeDetails} from './index';
+
+const createMockExtra = (
+  overrides: Partial<TraceTreeNodeExtra> = {}
+): TraceTreeNodeExtra => ({
+  organization: OrganizationFixture(),
+  ...overrides,
+});
+
+describe('AutogroupNodeDetails', () => {
+  it('renders autogroup details with title and description', () => {
+    const organization = OrganizationFixture();
+    const extra = createMockExtra({organization});
+    const autogroupValue = makeSiblingAutogroup({
+      span_id: 'test-span-id',
+      autogrouped_by: {
+        op: 'db.query',
+        description: 'SELECT * FROM users',
+      },
+    });
+
+    const childSpanValue = makeEAPSpan({event_id: 'child-span-1'});
+    const childNode = new EapSpanNode(null, childSpanValue, extra);
+    const node = new SiblingAutogroupNode(null, autogroupValue, extra);
+    node.children = [childNode];
+
+    render(
+      <TraceStateProvider initialPreferences={DEFAULT_TRACE_VIEW_PREFERENCES}>
+        <AutogroupNodeDetails
+          node={node as any}
+          organization={organization}
+          onTabScrollToNode={jest.fn()}
+          onParentClick={jest.fn()}
+          manager={null}
+          replay={null}
+          traceId="test-trace-id"
+        />
+      </TraceStateProvider>
+    );
+
+    expect(screen.getByText('Autogroup')).toBeInTheDocument();
+
+    expect(screen.getByText(/ID: test-span-id/)).toBeInTheDocument();
+
+    expect(
+      screen.getByText(/This block represents autogrouped spans/)
+    ).toBeInTheDocument();
+
+    expect(
+      screen.getByText('5 or more siblings with the same operation and description')
+    ).toBeInTheDocument();
+    expect(
+      screen.getByText('2 or more descendants with the same operation')
+    ).toBeInTheDocument();
+
+    expect(
+      screen.getByText(/You can either open this autogroup using the chevron/)
+    ).toBeInTheDocument();
+  });
+});