Browse Source

feat(tracing): reparent ssr spans under pageload txn and under browser request span (#74675)

Implements required changes for
https://github.com/getsentry/rfcs/pull/138
Jonas 7 months ago
parent
commit
26e6464206

+ 144 - 0
static/app/views/performance/newTraceDetails/traceModels/traceTree.spec.tsx

@@ -2740,4 +2740,148 @@ describe('TraceTree', () => {
       expect(mockedResponse3).toHaveBeenCalledTimes(1);
     });
   });
+
+  describe('SSR', () => {
+    it('makes pageload transaction a parent of server handler transaction', () => {
+      const tree: TraceTree = TraceTree.FromTrace(
+        makeTrace({
+          transactions: [
+            makeTransaction({
+              transaction: 'SSR',
+              ['transaction.op']: 'http.server',
+              children: [
+                makeTransaction({
+                  transaction: 'pageload',
+                  ['transaction.op']: 'pageload',
+                }),
+              ],
+            }),
+          ],
+        }),
+        null
+      );
+
+      const root = tree.root.children[0];
+      expect(root?.children?.[0]?.value?.['transaction.op']).toBe('pageload');
+      expect(root?.children?.[0]?.children?.[0]?.value?.['transaction.op']).toBe(
+        'http.server'
+      );
+    });
+
+    it('skips reparenting if server handler has multiple direct transaction children', () => {
+      const tree: TraceTree = TraceTree.FromTrace(
+        makeTrace({
+          transactions: [
+            makeTransaction({
+              transaction: 'SSR',
+              ['transaction.op']: 'http.server',
+              children: [
+                makeTransaction({
+                  transaction: 'first pageload',
+                  ['transaction.op']: 'pageload',
+                }),
+                makeTransaction({
+                  transaction: 'second pageload',
+                  ['transaction.op']: 'pageload',
+                }),
+              ],
+            }),
+          ],
+        }),
+        null
+      );
+
+      const transaction = tree.list[1];
+      assertTransactionNode(transaction);
+      expect(transaction.value.transaction).toBe('SSR');
+
+      const firstPageload = tree.list[2];
+      assertTransactionNode(firstPageload);
+      expect(firstPageload.value.transaction).toBe('first pageload');
+
+      const secondPageload = tree.list[3];
+      assertTransactionNode(secondPageload);
+      expect(secondPageload.value.transaction).toBe('second pageload');
+    });
+    describe('expanded', () => {
+      it('server handler transaction becomes a child of browser request span if present', async () => {
+        const tree: TraceTree = TraceTree.FromTrace(
+          makeTrace({
+            transactions: [
+              makeTransaction({
+                transaction: 'SSR',
+                event_id: 'ssr',
+                project_slug: 'js',
+                ['transaction.op']: 'http.server',
+                children: [
+                  makeTransaction({
+                    transaction: 'pageload',
+                    ['transaction.op']: 'pageload',
+                  }),
+                ],
+              }),
+            ],
+          }),
+          null
+        );
+
+        MockApiClient.addMockResponse({
+          url: '/organizations/org-slug/events/js:ssr/?averageColumn=span.self_time&averageColumn=span.duration',
+          method: 'GET',
+          body: makeEvent({}, [makeSpan({description: 'request', op: 'browser'})]),
+        });
+
+        tree.zoomIn(tree.list[1], true, {
+          api: new MockApiClient(),
+          organization: OrganizationFixture(),
+        });
+
+        await waitFor(() => tree.list.length === 4);
+        const browserRequestSpan = tree.list[1].children[0];
+        const ssrTransaction = browserRequestSpan.children[0];
+
+        assertSpanNode(browserRequestSpan);
+        assertTransactionNode(ssrTransaction);
+        expect(ssrTransaction.value.transaction).toBe('SSR');
+      });
+      it('server handler transaction becomes a direct child if there is no matching browser request span', async () => {
+        const tree: TraceTree = TraceTree.FromTrace(
+          makeTrace({
+            transactions: [
+              makeTransaction({
+                transaction: 'SSR',
+                event_id: 'ssr',
+                project_slug: 'js',
+                ['transaction.op']: 'http.server',
+                children: [
+                  makeTransaction({
+                    transaction: 'pageload',
+                    ['transaction.op']: 'pageload',
+                  }),
+                ],
+              }),
+            ],
+          }),
+          null
+        );
+
+        MockApiClient.addMockResponse({
+          url: '/organizations/org-slug/events/js:ssr/?averageColumn=span.self_time&averageColumn=span.duration',
+          method: 'GET',
+          body: makeEvent({}, [makeSpan({description: 'request', op: 'almost-browser'})]),
+        });
+
+        tree.zoomIn(tree.list[1], true, {
+          api: new MockApiClient(),
+          organization: OrganizationFixture(),
+        });
+
+        await waitFor(() => tree.list.length === 4);
+
+        const transaction = tree.list[tree.list.length - 1];
+        assertTransactionNode(transaction);
+        expect(transaction.value.transaction).toBe('SSR');
+      });
+    });
+  });
 });

+ 93 - 7
static/app/views/performance/newTraceDetails/traceModels/traceTree.tsx

@@ -218,6 +218,41 @@ function isJavascriptSDKTransaction(transaction: TraceTree.Transaction): boolean
   );
 }
 
+function isPageloadTransactionNode(node: TraceTreeNode<TraceTree.NodeValue>): boolean {
+  return isTransactionNode(node) && node.value['transaction.op'] === 'pageload';
+}
+
+function isServerRequestHandlerTransactionNode(
+  node: TraceTreeNode<TraceTree.NodeValue>
+): boolean {
+  return isTransactionNode(node) && node.value['transaction.op'] === 'http.server';
+}
+
+function isBrowserRequestSpan(value: TraceTree.Span): boolean {
+  return value.op === 'browser' && value.description === 'request';
+}
+
+/**
+ * Swaps the two nodes in the graph.
+ */
+function childParentSwap({
+  parent,
+  child,
+}: {
+  child: TraceTreeNode<TraceTree.NodeValue>;
+  parent: TraceTreeNode<TraceTree.NodeValue>;
+}) {
+  const parentOfParent = parent.parent!;
+
+  const parentIndex = parentOfParent.children.indexOf(parent);
+  parentOfParent.children[parentIndex] = child;
+  child.parent = parentOfParent;
+
+  // We need to remove the portion of the tree that was previously a child, else we will have a circular reference
+  parent.parent = child;
+  child.children.push(parent.filter(parent, n => n !== child));
+}
+
 function measurementToTimestamp(
   start_timestamp: number,
   measurement: number,
@@ -490,7 +525,8 @@ export class TraceTree {
 
     function visit(
       parent: TraceTreeNode<TraceTree.NodeValue | null>,
-      value: TraceTree.Transaction | TraceTree.TraceError
+      value: TraceTree.Transaction | TraceTree.TraceError,
+      childrenCount: number
     ) {
       const node = new TraceTreeNode(parent, value, {
         project_slug:
@@ -552,9 +588,21 @@ export class TraceTree {
         );
       }
 
+      if (
+        childrenCount === 1 &&
+        isPageloadTransactionNode(node) &&
+        isServerRequestHandlerTransactionNode(parent)
+      ) {
+        // The swap can occur at a later point when new transactions are fetched,
+        // which means we need to invalidate the tree and re-render the UI.
+        childParentSwap({parent, child: node});
+        parent.invalidate(parent);
+        node.invalidate(node);
+      }
+
       if (value && 'children' in value) {
         for (const child of value.children) {
-          visit(node, child);
+          visit(node, child, value.children.length);
         }
       }
 
@@ -580,17 +628,17 @@ export class TraceTree {
           typeof orphan.timestamp === 'number' &&
           transaction.start_timestamp <= orphan.timestamp
         ) {
-          visit(traceNode, transaction);
+          visit(traceNode, transaction, transaction.children.length);
           tIdx++;
         } else {
-          visit(traceNode, orphan);
+          visit(traceNode, orphan, 0);
           oIdx++;
         }
       } else if (transaction) {
-        visit(traceNode, transaction);
+        visit(traceNode, transaction, transaction.children.length);
         tIdx++;
       } else if (orphan) {
-        visit(traceNode, orphan);
+        visit(traceNode, orphan, 0);
         oIdx++;
       }
     }
@@ -852,8 +900,10 @@ export class TraceTree {
       TraceTreeNode<TraceTree.Transaction>[]
     >();
 
+    let firstTransaction: TraceTreeNode<TraceTree.Transaction> | null = null;
     for (const child of parent.children) {
       if (isTransactionNode(child)) {
+        firstTransaction = firstTransaction || child;
         // keep track of the transaction nodes that should be reparented under the newly fetched spans.
         const key =
           'parent_span_id' in child.value &&
@@ -871,12 +921,28 @@ export class TraceTree {
     const remappedTransactionParents = new Set<TraceTreeNode<TraceTree.NodeValue>>();
 
     for (const span of spans) {
-      const childTransactions = transactionsToSpanMap.get(span.span_id) ?? [];
+      let childTransactions = transactionsToSpanMap.get(span.span_id) ?? [];
+
       const spanNodeValue: TraceTree.Span = {
         ...span,
         event: data as EventTransaction,
         childTransactions,
       };
+
+      // If we have a browser request span and a server request handler transaction, we want to
+      // reparent the transaction under the span. This is because the server request handler
+      // was the parent of the browser request span which likely served the document.
+      if (
+        firstTransaction &&
+        !childTransactions.length &&
+        isBrowserRequestSpan(spanNodeValue) &&
+        isServerRequestHandlerTransactionNode(firstTransaction)
+      ) {
+        childTransactions = [firstTransaction];
+        spanNodeValue.childTransactions = childTransactions;
+        transactionsToSpanMap.delete(`unreachable-${firstTransaction.value.event_id}`);
+      }
+
       const node: TraceTreeNode<TraceTree.Span> = new TraceTreeNode(null, spanNodeValue, {
         event_id: parent.metadata.event_id,
         project_slug: parent.metadata.project_slug,
@@ -1833,6 +1899,26 @@ export class TraceTreeNode<T extends TraceTree.NodeValue = TraceTree.NodeValue>
     return clone;
   }
 
+  filter(
+    node: TraceTreeNode<TraceTree.NodeValue>,
+    predicate: (node: TraceTreeNode) => boolean
+  ): TraceTreeNode<TraceTree.NodeValue> {
+    const queue = [node];
+
+    while (queue.length) {
+      const next = queue.pop()!;
+      for (let i = 0; i < next.children.length; i++) {
+        if (!predicate(next.children[i])) {
+          next.children.splice(i, 1);
+        } else {
+          queue.push(next.children[i]);
+        }
+      }
+    }
+
+    return node;
+  }
+
   get isOrphaned() {
     return this.parent?.value && 'orphan_errors' in this.parent.value;
   }