diff --git a/static/app/views/performance/newTraceDetails/traceModels/__snapshots__/traceTree.spec.tsx.snap b/static/app/views/performance/newTraceDetails/traceModels/__snapshots__/traceTree.spec.tsx.snap index 29de4bdcb242f5..f068d5d11a6ec1 100644 --- a/static/app/views/performance/newTraceDetails/traceModels/__snapshots__/traceTree.spec.tsx.snap +++ b/static/app/views/performance/newTraceDetails/traceModels/__snapshots__/traceTree.spec.tsx.snap @@ -98,8 +98,8 @@ exports[`TraceTree eap trace correctly renders eap-transactions toggle state 1`] " eap trace root span.op - span.description (eap-transaction) - span.op - span.description (eap-transaction) span.op - span.description (eap-transaction) + span.op - span.description (eap-transaction) " `; @@ -109,7 +109,7 @@ eap trace root span.op - span.description (eap-transaction) span.op - span.description span.op - span.description (eap-transaction) - span.op - span.description (eap-transaction) + span.op - span.description (eap-transaction) " `; @@ -117,8 +117,8 @@ exports[`TraceTree eap trace correctly renders eap-transactions toggle state 3`] " eap trace root span.op - span.description (eap-transaction) - span.op - span.description (eap-transaction) span.op - span.description (eap-transaction) + span.op - span.description (eap-transaction) " `; diff --git a/static/app/views/performance/newTraceDetails/traceModels/traceTree.spec.tsx b/static/app/views/performance/newTraceDetails/traceModels/traceTree.spec.tsx index 7558ce83ed437f..73d488bb6b9568 100644 --- a/static/app/views/performance/newTraceDetails/traceModels/traceTree.spec.tsx +++ b/static/app/views/performance/newTraceDetails/traceModels/traceTree.spec.tsx @@ -776,6 +776,94 @@ describe('TraceTree', () => { expect(tree.build().serialize()).toMatchSnapshot(); }); + it('preserves actual parents while summarizing collapsed EAP transactions', () => { + const tree = TraceTree.FromTrace( + makeEAPTrace([ + makeEAPSpan({ + event_id: 'root-transaction', + is_transaction: true, + start_timestamp: start, + end_timestamp: start + 5, + children: [ + makeEAPSpan({ + event_id: 'span-a', + is_transaction: false, + parent_span_id: 'root-transaction', + start_timestamp: start + 1, + end_timestamp: start + 2, + children: [ + makeEAPSpan({ + event_id: 'child-transaction-a', + is_transaction: true, + parent_span_id: 'span-a', + start_timestamp: start + 1.25, + end_timestamp: start + 2, + children: [], + }), + ], + }), + makeEAPSpan({ + event_id: 'span-b', + is_transaction: false, + parent_span_id: 'root-transaction', + start_timestamp: start + 3, + end_timestamp: start + 4, + children: [ + makeEAPSpan({ + event_id: 'child-transaction-b', + is_transaction: true, + parent_span_id: 'span-b', + start_timestamp: start + 3.25, + end_timestamp: start + 4, + children: [], + }), + ], + }), + ], + }), + ]), + traceOptions + ).build(); + + const rootTransaction = tree.root.findChild(n => n.id === 'root-transaction'); + const spanA = tree.root.findChild(n => n.id === 'span-a'); + const spanB = tree.root.findChild(n => n.id === 'span-b'); + const childTransactionA = tree.root.findChild(n => n.id === 'child-transaction-a'); + const childTransactionB = tree.root.findChild(n => n.id === 'child-transaction-b'); + + assertEAPSpanNode(rootTransaction); + assertEAPSpanNode(spanA); + assertEAPSpanNode(spanB); + assertEAPSpanNode(childTransactionA); + assertEAPSpanNode(childTransactionB); + + expect(rootTransaction.children).toEqual([spanA, spanB]); + expect(childTransactionA.parent).toBe(spanA); + expect(childTransactionB.parent).toBe(spanB); + expect(rootTransaction.directVisibleChildren).toEqual([ + childTransactionA, + childTransactionB, + ]); + const rootTransactionIndex = tree.list.indexOf(rootTransaction); + expect(tree.list.slice(rootTransactionIndex, rootTransactionIndex + 3)).toEqual([ + rootTransaction, + childTransactionA, + childTransactionB, + ]); + + rootTransaction.expand(true, tree); + + expect(childTransactionA.parent).toBe(spanA); + expect(childTransactionB.parent).toBe(spanB); + expect(tree.list.slice(rootTransactionIndex, rootTransactionIndex + 5)).toEqual([ + rootTransaction, + spanA, + childTransactionA, + spanB, + childTransactionB, + ]); + }); + it('collects measurements', () => { const tree = TraceTree.FromTrace( makeEAPTrace([ diff --git a/static/app/views/performance/newTraceDetails/traceModels/traceTreeNode/eapSpanNode.spec.tsx b/static/app/views/performance/newTraceDetails/traceModels/traceTreeNode/eapSpanNode.spec.tsx index f383a064db161a..05c60b6fdfc4d9 100644 --- a/static/app/views/performance/newTraceDetails/traceModels/traceTreeNode/eapSpanNode.spec.tsx +++ b/static/app/views/performance/newTraceDetails/traceModels/traceTreeNode/eapSpanNode.spec.tsx @@ -1,13 +1,14 @@ import {OrganizationFixture} from 'sentry-fixture/organization'; import {ThemeFixture} from 'sentry-fixture/theme'; +import {TraceTree} from 'sentry/views/performance/newTraceDetails/traceModels/traceTree'; import { makeEAPError, makeEAPOccurrence, makeEAPSpan, } from 'sentry/views/performance/newTraceDetails/traceModels/traceTreeTestUtils'; -import type {TraceTreeNodeExtra} from './baseNode'; +import type {BaseNode, TraceTreeNodeExtra} from './baseNode'; import {EapSpanNode} from './eapSpanNode'; const createMockExtra = ( @@ -79,7 +80,7 @@ describe('EapSpanNode', () => { expect(node.opsBreakdown).toEqual([]); }); - it('should reparent transaction under closest EAP transaction parent', () => { + it('should preserve the actual parent for transaction spans', () => { const extra = createMockExtra(); const rootTransactionValue = makeEAPSpan({ event_id: 'root-transaction', @@ -100,11 +101,11 @@ describe('EapSpanNode', () => { const rootTransaction = new EapSpanNode(null, rootTransactionValue, extra); const span = new EapSpanNode(rootTransaction, spanValue, extra); - // Child transaction should be reparented under root transaction, not the span const childTransaction = new EapSpanNode(span, childTransactionValue, extra); - expect(childTransaction.parent).toBe(rootTransaction); - expect(rootTransaction.children).toContain(childTransaction); + expect(childTransaction.parent).toBe(span); + expect(span.children).toContain(childTransaction); + expect(rootTransaction.children).toEqual([span]); }); it('should add itself to parent children and sort chronologically', () => { @@ -407,6 +408,74 @@ describe('EapSpanNode', () => { expect(transaction.directVisibleChildren).toEqual([childTransaction]); }); + it('should surface nested transactions for collapsed EAP transactions', () => { + const extra = createMockExtra(); + const transactionValue = makeEAPSpan({ + event_id: 'transaction', + is_transaction: true, + }); + const childSpanValue = makeEAPSpan({ + event_id: 'child-span', + is_transaction: false, + }); + const nestedTransactionValue = makeEAPSpan({ + event_id: 'nested-transaction', + is_transaction: true, + parent_span_id: 'child-span', + }); + + const transaction = new EapSpanNode(null, transactionValue, extra); + const childSpan = new EapSpanNode(transaction, childSpanValue, extra); + const nestedTransaction = new EapSpanNode(childSpan, nestedTransactionValue, extra); + + transaction.expanded = false; + + expect(transaction.directVisibleChildren).toEqual([nestedTransaction]); + expect(nestedTransaction.parent).toBe(childSpan); + }); + + it('should surface nested transactions beneath parent autogroups', () => { + const extra = createMockExtra(); + const transactionValue = makeEAPSpan({ + event_id: 'transaction', + is_transaction: true, + }); + const groupHeadValue = makeEAPSpan({ + event_id: 'group-head', + is_transaction: false, + op: 'db.query', + }); + const groupTailValue = makeEAPSpan({ + event_id: 'group-tail', + is_transaction: false, + op: 'db.query', + }); + const childSpanValue = makeEAPSpan({ + event_id: 'child-span', + is_transaction: false, + op: 'http.client', + }); + const nestedTransactionValue = makeEAPSpan({ + event_id: 'nested-transaction', + is_transaction: true, + parent_span_id: 'child-span', + op: 'rpc.call', + }); + + const transaction = new EapSpanNode(null, transactionValue, extra); + const groupHead = new EapSpanNode(transaction, groupHeadValue, extra); + const groupTail = new EapSpanNode(groupHead, groupTailValue, extra); + const childSpan = new EapSpanNode(groupTail, childSpanValue, extra); + const nestedTransaction = new EapSpanNode(childSpan, nestedTransactionValue, extra); + + expect(TraceTree.AutogroupDirectChildrenSpanNodes(transaction)).toBe(1); + + transaction.expanded = false; + + expect(transaction.directVisibleChildren).toEqual([nestedTransaction]); + expect(nestedTransaction.parent).toBe(childSpan); + }); + it('should return all children for expanded EAP transactions', () => { const extra = createMockExtra(); const transactionValue = makeEAPSpan({ @@ -505,7 +574,7 @@ describe('EapSpanNode', () => { describe('expand method', () => { const createMockTraceTree = () => ({ - list: [] as EapSpanNode[], + list: [] as BaseNode[], }); it('should handle expanding transaction with reparenting', () => { @@ -516,30 +585,58 @@ describe('EapSpanNode', () => { op: 'http.server', }); const spanValue = makeEAPSpan({ - event_id: 'span', + event_id: 'span-a', is_transaction: false, op: 'db.query', + start_timestamp: 1, }); const childTransactionValue = makeEAPSpan({ - event_id: 'child-transaction', + event_id: 'child-transaction-a', + is_transaction: true, + parent_span_id: 'span-a', + start_timestamp: 2, + }); + const siblingSpanValue = makeEAPSpan({ + event_id: 'span-b', + is_transaction: false, + op: 'db.query', + start_timestamp: 3, + }); + const siblingTransactionValue = makeEAPSpan({ + event_id: 'child-transaction-b', is_transaction: true, - parent_span_id: 'span', + parent_span_id: 'span-b', + start_timestamp: 4, }); const transaction = new EapSpanNode(null, transactionValue, extra); const span = new EapSpanNode(transaction, spanValue, extra); - const childTransaction = new EapSpanNode(transaction, childTransactionValue, extra); + const childTransaction = new EapSpanNode(span, childTransactionValue, extra); + const siblingSpan = new EapSpanNode(transaction, siblingSpanValue, extra); + const siblingTransaction = new EapSpanNode( + siblingSpan, + siblingTransactionValue, + extra + ); - transaction.children = [span, childTransaction]; transaction.expanded = false; const tree = createMockTraceTree(); - tree.list = [transaction]; + tree.list = [transaction, ...transaction.visibleChildren]; + + expect(transaction.directVisibleChildren).toEqual([ + childTransaction, + siblingTransaction, + ]); + expect(childTransaction.parent).toBe(span); + expect(siblingTransaction.parent).toBe(siblingSpan); const result = transaction.expand(true, tree as any); expect(result).toBe(true); expect(transaction.expanded).toBe(true); + expect(childTransaction.parent).toBe(span); + expect(siblingTransaction.parent).toBe(siblingSpan); // Test that tree.list is updated to include visible children after expansion expect(tree.list).toContain(transaction); @@ -554,6 +651,19 @@ describe('EapSpanNode', () => { transactionIndex + 1 + transaction.visibleChildren.length ) ).toEqual(transaction.visibleChildren); + + expect(tree.list).toEqual([ + transaction, + span, + childTransaction, + siblingSpan, + siblingTransaction, + ]); + + transaction.expand(false, tree as any); + expect(childTransaction.parent).toBe(span); + expect(siblingTransaction.parent).toBe(siblingSpan); + expect(tree.list).toEqual([transaction, childTransaction, siblingTransaction]); }); it('should handle collapsing transaction with reparenting', () => { diff --git a/static/app/views/performance/newTraceDetails/traceModels/traceTreeNode/eapSpanNode.tsx b/static/app/views/performance/newTraceDetails/traceModels/traceTreeNode/eapSpanNode.tsx index c6de2ac93fe1cb..75c9ed82b00734 100644 --- a/static/app/views/performance/newTraceDetails/traceModels/traceTreeNode/eapSpanNode.tsx +++ b/static/app/views/performance/newTraceDetails/traceModels/traceTreeNode/eapSpanNode.tsx @@ -19,7 +19,6 @@ export class EapSpanNode extends BaseNode { id: string; type: TraceTree.NodeType; - reparentedEAPTransactions = new Set(); /** * The breakdown of the node's children's operations by count. */ @@ -30,13 +29,7 @@ export class EapSpanNode extends BaseNode { value: TraceTree.EAPSpan, extra: TraceTreeNodeExtra ) { - // For eap transactions, on load we only display the embedded transactions children. - // Mimics the behavior of non-eap traces, enabling a less noisy/summarized view of the trace - const parentNode = value.is_transaction - ? (parent?.findParent(p => isEAPSpanNode(p) && p.value.is_transaction) ?? parent) - : parent; - - super(parentNode, value, extra); + super(parent, value, extra); this.id = value.event_id; this.type = 'span'; @@ -65,8 +58,8 @@ export class EapSpanNode extends BaseNode { this._updateAncestorOpsBreakdown(this, value.op); - parentNode?.children.push(this); - parentNode?.children.sort(traceChronologicalSort); + parent?.children.push(this); + parent?.children.sort(traceChronologicalSort); } private _updateAncestorOpsBreakdown(node: EapSpanNode, op: string) { @@ -84,6 +77,54 @@ export class EapSpanNode extends BaseNode { } } + /** + * Build the summarized child list for a collapsed EAP transaction. + * + * In the collapsed state we do not render the full span tree. Instead, we surface the + * first descendant EAP transaction reachable on each branch so the user still sees + * embedded transactions without expanding all intermediary spans. + * + * This traversal intentionally preserves real parentage. Nested transactions stay attached + * to their actual span parents and are only surfaced as visible descendants in the collapsed + * summary; we do not reparent them under the collapsed transaction node. + * + * We also have to traverse through non-EAP wrapper nodes via `getNextTraversalNodes()`. + * After `ApplyPreferences` runs, wrappers like `ParentAutogroupNode` can sit between this + * transaction and a nested descendant transaction. If we stopped at `!isEAPSpanNode(node)`, + * those descendant transactions would disappear from the collapsed summary. + */ + private getCollapsedTransactionChildren(): BaseNode[] { + const queue: BaseNode[] = []; + const collapsedChildren: BaseNode[] = []; + const visited = new Set(); + visited.add(this); + + for (let i = this.children.length - 1; i >= 0; i--) { + queue.push(this.children[i]!); + } + + while (queue.length > 0) { + const node = queue.pop()!; + + if (visited.has(node)) { + continue; + } + visited.add(node); + + if (isEAPSpanNode(node) && node.value.is_transaction) { + collapsedChildren.push(node); + continue; + } + + const nextNodes = node.getNextTraversalNodes(); + for (let i = nextNodes.length - 1; i >= 0; i--) { + queue.push(nextNodes[i]!); + } + } + + return collapsedChildren.sort(traceChronologicalSort); + } + get drawerTabsTitle(): string { return this.op + (this.description ? ' - ' + this.description : ''); } @@ -148,11 +189,9 @@ export class EapSpanNode extends BaseNode { get directVisibleChildren(): Array> { if (this.value.is_transaction && !this.expanded) { - // For collapsed eap-transactions we still render the embedded eap-transactions as visible children. - // Mimics the behavior of non-eap traces, enabling a less noisy/summarized view of the trace - return this.children.filter( - child => isEAPSpanNode(child) && child.value.is_transaction - ); + // For collapsed eap-transactions we render the first nested transaction on each + // descendant branch while preserving the actual tree parentage. + return this.getCollapsedTransactionChildren(); } return super.directVisibleChildren; @@ -185,80 +224,20 @@ export class EapSpanNode extends BaseNode { // Flip expanded so that we can collect visible children this.expanded = expanding; - // When eap-transaction nodes are expanded, we need to reparent the transactions under - // the eap-spans (by their parent_span_id) that were previously hidden. Note that this only impacts the - // direct eap-transaction children of the targetted eap-transaction node. if (this.value.is_transaction) { - const eapTransactions = this.children.filter( - c => isEAPSpanNode(c) && c.value.is_transaction - ) as EapSpanNode[]; - - for (const txn of eapTransactions) { - // Find the eap-span that is the parent of the transaction - const newParent = this.findChild(n => { - if (isEAPSpanNode(n)) { - return n.value.event_id === txn.value.parent_span_id; - } - return false; - }); - - // If the transaction already has the correct parent, we can continue - if (newParent === txn.parent) { - continue; - } - - this.reparentedEAPTransactions.add(txn); - - // If we have found a new parent to reparent the transaction under, - // remove it from its current parent's children and add it to the new parent - if (newParent) { - if (txn.parent) { - txn.parent.children = txn.parent.children.filter(c => c !== txn); - } - newParent.children.push(txn); - txn.parent = newParent; - txn.parent.children.sort(traceChronologicalSort); - } - } - const browserRequestSpan = this.children.find(c => isBrowserRequestNode(c)); if (browserRequestSpan) { this._reparentSSRUnderBrowserRequestSpan(browserRequestSpan); } } - // Flip expanded so that we can collect visible children + // Insert expanded children into the flat list tree.list.splice(index + 1, 0, ...this.visibleChildren); } else { tree.list.splice(index + 1, this.visibleChildren.length); this.expanded = expanding; - // When eap-transaction nodes are collapsed, they still render transactions as visible children. - // Reparent the transactions from under the eap-spans in the expanded state, to under the closest eap-transaction - // in the collapsed state. This only targets the embedded transactions that are to be direct children of the node upon collapse. - if (this.value.is_transaction) { - for (const txn of this.reparentedEAPTransactions) { - const newParent = this; - - // If the transaction already has the correct parent, we can continue - if (newParent === txn.parent) { - continue; - } - - // If we have found a new parent to reparent the transaction under, - // remove it from its current parent's children and add it to the new parent - if (newParent) { - if (txn.parent) { - txn.parent.children = txn.parent.children.filter(c => c !== txn); - } - newParent.children.push(txn); - txn.parent = newParent; - txn.parent.children.sort(traceChronologicalSort); - } - } - } - // When transaction nodes are collapsed, they still render child transactions if (this.value.is_transaction) { tree.list.splice(index + 1, 0, ...this.visibleChildren);