Browse Source

fix(trace): adjust timeline for ns precision (#71075)

Spans have ns precision while transactions only include ms precision.
This means that it is possible for spans to render outside of their
"container" if their duration is sub ms and the txn duration is rounded
down.
Jonas 9 months ago
parent
commit
0a989a85f7

+ 48 - 0
static/app/views/performance/newTraceDetails/index.tsx

@@ -72,6 +72,18 @@ import {TraceType} from './traceType';
 import {TraceUXChangeAlert} from './traceUXChangeBanner';
 import {useTraceQueryParamStateSync} from './useTraceQueryParamStateSync';
 
+function decodeScrollQueue(maybePath: unknown): TraceTree.NodePath[] | null {
+  if (Array.isArray(maybePath)) {
+    return maybePath;
+  }
+
+  if (typeof maybePath === 'string') {
+    return [maybePath as TraceTree.NodePath];
+  }
+
+  return null;
+}
+
 function logTraceType(type: TraceType, organization: Organization) {
   switch (type) {
     case TraceType.BROKEN_SUBTRACES:
@@ -224,6 +236,8 @@ function TraceViewContent(props: TraceViewContentProps) {
   const rootEvent = useTraceRootEvent(props.trace);
   const loadingTraceRef = useRef<TraceTree | null>(null);
   const [forceRender, rerender] = useReducer(x => x + (1 % 2), 0);
+
+  const initializedRef = useRef(false);
   const scrollQueueRef = useRef<{eventId?: string; path?: TraceTree.NodePath[]} | null>(
     null
   );
@@ -374,6 +388,18 @@ function TraceViewContent(props: TraceViewContentProps) {
     // eslint-disable-next-line react-hooks/exhaustive-deps
   }, [tree]);
 
+  useLayoutEffect(() => {
+    const queryParams = qs.parse(location.search);
+    const maybeQueue = decodeScrollQueue(queryParams.node);
+
+    if (maybeQueue || queryParams.eventId) {
+      scrollQueueRef.current = {
+        eventId: queryParams.eventId as string,
+        path: maybeQueue as TraceTreeNode<TraceTree.NodeValue>['path'],
+      };
+    }
+  }, [tree]);
+
   const searchingRaf = useRef<{id: number | null} | null>(null);
   const onTraceSearch = useCallback(
     (
@@ -652,6 +678,7 @@ function TraceViewContent(props: TraceViewContentProps) {
       nodeToScrollTo: TraceTreeNode<TraceTree.NodeValue> | null,
       indexOfNodeToScrollTo: number | null
     ) => {
+      scrollQueueRef.current = null;
       const query = qs.parse(location.search);
 
       if (query.fov && typeof query.fov === 'string') {
@@ -791,6 +818,26 @@ function TraceViewContent(props: TraceViewContentProps) {
     logTraceType(shape, organization);
   }, [tree, shape, organization]);
 
+  useLayoutEffect(() => {
+    if (!tree.root?.space || tree.type !== 'trace') {
+      return undefined;
+    }
+
+    initializedRef.current = true;
+    viewManager.initializeTraceSpace([tree.root.space[0], 0, tree.root.space[1], 1]);
+
+    // Whenever the timeline changes, update the trace space
+    const onTraceTimelineChange = (s: [number, number]) => {
+      viewManager.updateTraceSpace(s[0], s[1]);
+    };
+
+    tree.on('trace timeline change', onTraceTimelineChange);
+
+    return () => {
+      tree.off('trace timeline change', onTraceTimelineChange);
+    };
+  }, [viewManager, tree]);
+
   return (
     <TraceExternalLayout>
       <TraceUXChangeAlert />
@@ -819,6 +866,7 @@ function TraceViewContent(props: TraceViewContentProps) {
             trace_state={traceState}
             trace_dispatch={traceDispatch}
             scrollQueueRef={scrollQueueRef}
+            initializedRef={initializedRef}
             onRowClick={onRowClick}
             onTraceLoad={onTraceLoad}
             onTraceSearch={onTraceSearch}

+ 5 - 33
static/app/views/performance/newTraceDetails/trace.tsx

@@ -4,7 +4,6 @@ import {type Theme, useTheme} from '@emotion/react';
 import styled from '@emotion/styled';
 import * as Sentry from '@sentry/react';
 import {PlatformIcon} from 'platformicons';
-import * as qs from 'query-string';
 
 import LoadingIndicator from 'sentry/components/loadingIndicator';
 import Placeholder from 'sentry/components/placeholder';
@@ -53,18 +52,6 @@ import {
 } from './guards';
 import {TraceIcons} from './icons';
 
-function decodeScrollQueue(maybePath: unknown): TraceTree.NodePath[] | null {
-  if (Array.isArray(maybePath)) {
-    return maybePath;
-  }
-
-  if (typeof maybePath === 'string') {
-    return [maybePath as TraceTree.NodePath];
-  }
-
-  return null;
-}
-
 const COUNT_FORMATTER = Intl.NumberFormat(undefined, {notation: 'compact'});
 const NO_ERRORS = new Set<TraceError>();
 const NO_PERFORMANCE_ISSUES = new Set<TracePerformanceIssue>();
@@ -144,6 +131,7 @@ function maybeFocusRow(
 
 interface TraceProps {
   forceRerender: number;
+  initializedRef: React.MutableRefObject<boolean>;
   manager: VirtualizedViewManager;
   onRowClick: (
     node: TraceTreeNode<TraceTree.NodeValue>,
@@ -183,6 +171,7 @@ export function Trace({
   onTraceLoad,
   rerender,
   trace_state,
+  initializedRef,
   trace_dispatch,
   forceRerender,
 }: TraceProps) {
@@ -207,33 +196,15 @@ export function Trace({
   const traceStateRef = useRef<TraceReducerState>(trace_state);
   traceStateRef.current = trace_state;
 
-  if (
-    trace.root.space &&
-    (trace.root.space[0] !== manager.to_origin ||
-      trace.root.space[1] !== manager.trace_space.width)
-  ) {
-    manager.initializeTraceSpace([trace.root.space[0], 0, trace.root.space[1], 1]);
-    const queryParams = qs.parse(location.search);
-    const maybeQueue = decodeScrollQueue(queryParams.node);
-
-    if (maybeQueue || queryParams.eventId) {
-      scrollQueueRef.current = {
-        eventId: queryParams.eventId as string,
-        path: maybeQueue as TraceTreeNode<TraceTree.NodeValue>['path'],
-      };
-    }
-  }
-
-  const loadedRef = useRef(false);
   useLayoutEffect(() => {
-    if (loadedRef.current) {
+    if (initializedRef.current) {
       return;
     }
     if (trace.type !== 'trace' || !manager) {
       return;
     }
 
-    loadedRef.current = true;
+    initializedRef.current = true;
 
     if (!scrollQueueRef.current) {
       onTraceLoad(trace, null, null);
@@ -282,6 +253,7 @@ export function Trace({
     onTraceLoad,
     trace_dispatch,
     scrollQueueRef,
+    initializedRef,
     organization,
   ]);
 

+ 30 - 5
static/app/views/performance/newTraceDetails/traceModels/traceTree.spec.tsx

@@ -603,6 +603,31 @@ describe('TreeNode', () => {
       });
     });
 
+    it('adjusts trace space when spans exceed the bounds of a trace', () => {
+      const adjusted = TraceTree.FromTrace(
+        makeTrace({
+          transactions: [
+            makeTransaction({
+              start_timestamp: 1,
+              timestamp: 3,
+            }),
+          ],
+          orphan_errors: [],
+        })
+      );
+
+      expect(adjusted.root.space).toEqual([1000, 2000]);
+
+      const [_, adjusted_space] = TraceTree.FromSpans(
+        adjusted.root,
+        makeEvent(),
+        [makeSpan({start_timestamp: 0.5, timestamp: 3.5})],
+        {sdk: undefined}
+      );
+
+      expect(adjusted_space).toEqual([0.5, 3.5]);
+    });
+
     it('inserts no data node when txn has no span children', async () => {
       const tree = TraceTree.FromTrace(
         makeTrace({
@@ -977,7 +1002,7 @@ describe('TraceTree', () => {
       {project_slug: '', event_id: ''}
     );
 
-    const node = TraceTree.FromSpans(
+    const [node] = TraceTree.FromSpans(
       root,
       makeEvent(),
       [
@@ -1029,7 +1054,7 @@ describe('TraceTree', () => {
       )
     );
 
-    const node = TraceTree.FromSpans(
+    const [node] = TraceTree.FromSpans(
       root,
       makeEvent(),
       [
@@ -1079,7 +1104,7 @@ describe('TraceTree', () => {
       start = node;
     }
 
-    const node = TraceTree.FromSpans(
+    const [node] = TraceTree.FromSpans(
       root,
       makeEvent(),
       [
@@ -1109,7 +1134,7 @@ describe('TraceTree', () => {
     );
 
     const date = new Date().getTime();
-    const node = TraceTree.FromSpans(
+    const [node] = TraceTree.FromSpans(
       root,
       makeEvent(),
       [
@@ -1149,7 +1174,7 @@ describe('TraceTree', () => {
     );
 
     const date = new Date().getTime();
-    const node = TraceTree.FromSpans(
+    const [node] = TraceTree.FromSpans(
       root,
       makeEvent(),
       [

+ 90 - 5
static/app/views/performance/newTraceDetails/traceModels/traceTree.tsx

@@ -109,6 +109,8 @@ import {TraceType} from '../traceType';
  * - instead of storing span children separately, we should have meta tree nodes that handle pointing to the correct children
  */
 
+type ArgumentTypes<F> = F extends (...args: infer A) => any ? A : never;
+
 export declare namespace TraceTree {
   type Transaction = TraceFullDetailed;
   interface Span extends RawSpanType {
@@ -172,6 +174,12 @@ export declare namespace TraceTree {
   };
 
   type CollectedVital = {key: string; measurement: Measurement};
+
+  interface TraceTreeEvents {
+    ['trace timeline change']: (view: [number, number]) => void;
+  }
+
+  type EventStore = {[K in keyof TraceTreeEvents]: Set<TraceTreeEvents[K]>};
 }
 
 export type ViewManagerScrollToOptions = {
@@ -508,6 +516,8 @@ export class TraceTree {
     const tLen = transactionQueue.length;
     const oLen = orphanErrorsQueue.length;
 
+    // Items in each queue are sorted by timestamp, so we just take
+    // from the queue with the earliest timestamp which means the final list will be ordered.
     while (tIdx < tLen || oIdx < oLen) {
       const transaction = transactionQueue[tIdx];
       const orphan = orphanErrorsQueue[oIdx];
@@ -612,10 +622,13 @@ export class TraceTree {
     data: Event,
     spans: RawSpanType[],
     options: {sdk: string | undefined} | undefined
-  ): TraceTreeNode<TraceTree.NodeValue> {
+  ): [TraceTreeNode<TraceTree.NodeValue>, [number, number] | null] {
     parent.invalidate(parent);
     const platformHasMissingSpans = shouldAddMissingInstrumentationSpan(options?.sdk);
 
+    let min_span_start = Number.POSITIVE_INFINITY;
+    let min_span_end = Number.NEGATIVE_INFINITY;
+
     const parentIsSpan = isSpanNode(parent);
     const lookuptable: Record<
       RawSpanType['span_id'],
@@ -625,14 +638,14 @@ export class TraceTree {
     // If we've already fetched children, the tree is already assembled
     if (parent.spanChildren.length > 0) {
       parent.zoomedIn = true;
-      return parent;
+      return [parent, null];
     }
 
     // If we have no spans, insert an empty node to indicate that there is no data
     if (!spans.length && !parent.children.length) {
       parent.zoomedIn = true;
       parent.spanChildren.push(new NoDataNode(parent));
-      return parent;
+      return [parent, null];
     }
 
     if (parentIsSpan) {
@@ -676,6 +689,16 @@ export class TraceTree {
         project_slug: undefined,
       });
 
+      if (
+        typeof span.start_timestamp === 'number' &&
+        span.start_timestamp < min_span_start
+      ) {
+        min_span_start = span.start_timestamp;
+      }
+      if (typeof span.timestamp === 'number' && span.timestamp > min_span_end) {
+        min_span_end = span.timestamp;
+      }
+
       for (const error of getRelatedSpanErrorsFromTransaction(span, parent)) {
         node.errors.add(error);
       }
@@ -748,7 +771,8 @@ export class TraceTree {
     parent.zoomedIn = true;
     TraceTree.AutogroupSiblingSpanNodes(parent);
     TraceTree.AutogroupDirectChildrenSpanNodes(parent);
-    return parent;
+
+    return [parent, [min_span_start, min_span_end]];
   }
 
   static AutogroupDirectChildrenSpanNodes(
@@ -1326,7 +1350,36 @@ export class TraceTree {
         // Api response is not sorted
         spans.data.sort((a, b) => a.start_timestamp - b.start_timestamp);
 
-        TraceTree.FromSpans(node, data, spans.data, {sdk: data.sdk?.name});
+        const [_, view] = TraceTree.FromSpans(node, data, spans.data, {
+          sdk: data.sdk?.name,
+        });
+
+        // Spans contain millisecond precision, which means that it is possible for the
+        // children spans of a transaction to extend beyond the start and end of the transaction
+        // through ns precision. To account for this, we need to adjust the space of the transaction node and the space
+        // of our trace so that all of the span children are visible and can be rendered inside the view.
+        if (
+          view &&
+          Number.isFinite(view[0]) &&
+          Number.isFinite(view[1]) &&
+          this.root.space
+        ) {
+          const prev_start = this.root.space[0];
+          const prev_end = this.root.space[1];
+          const new_start = view[0];
+          const new_end = view[1];
+
+          // Update the space of the tree and the trace root node
+          this.root.space = [
+            Math.min(new_start * node.multiplier, this.root.space[0]),
+            Math.max(new_end * node.multiplier - prev_start, this.root.space[1]),
+          ];
+          this.root.children[0].space = [...this.root.space];
+
+          if (prev_start !== this.root.space[0] || prev_end !== this.root.space[1]) {
+            this.dispatch('trace timeline change', this.root.space);
+          }
+        }
 
         const spanChildren = node.getVisibleChildren();
         this._list.splice(index + 1, 0, ...spanChildren);
@@ -1366,6 +1419,38 @@ export class TraceTree {
     return this._list;
   }
 
+  listeners: TraceTree.EventStore = {
+    'trace timeline change': new Set(),
+  };
+
+  on<K extends keyof TraceTree.TraceTreeEvents>(
+    event: K,
+    cb: TraceTree.TraceTreeEvents[K]
+  ): void {
+    this.listeners[event].add(cb);
+  }
+
+  off<K extends keyof TraceTree.TraceTreeEvents>(
+    event: K,
+    cb: TraceTree.TraceTreeEvents[K]
+  ): void {
+    this.listeners[event].delete(cb);
+  }
+
+  dispatch<K extends keyof TraceTree.TraceTreeEvents>(
+    event: K,
+    ...args: ArgumentTypes<TraceTree.TraceTreeEvents[K]>
+  ): void {
+    if (!this.listeners[event]) {
+      return;
+    }
+
+    for (const handler of this.listeners[event]) {
+      // @ts-expect-error
+      handler(...args);
+    }
+  }
+
   /**
    * Prints the tree in a human readable format, useful for debugging and testing
    */

+ 19 - 0
static/app/views/performance/newTraceDetails/traceRenderers/virtualizedViewManager.tsx

@@ -216,6 +216,25 @@ export class VirtualizedViewManager {
     }
   }
 
+  updateTraceSpace(start: number, width: number) {
+    if (this.trace_space.width === width && this.to_origin === start) {
+      return;
+    }
+
+    this.to_origin = start;
+
+    // If view is scaled all the way out, then lets update it to match the new space, else
+    // we are implicitly zooming in, which may be even more confusing.
+    const preventImplicitZoom = this.trace_view.width === this.trace_space.width;
+    this.trace_space = new TraceView(0, 0, width, 1);
+    if (preventImplicitZoom) {
+      this.setTraceView({x: 0, width});
+    }
+
+    this.recomputeTimelineIntervals();
+    this.recomputeSpanToPxMatrix();
+  }
+
   initializeTraceSpace(space: [x: number, y: number, width: number, height: number]) {
     this.to_origin = space[0];