Skip to content

feat(trace-tree-node): BaseNode implementation changes to achieve feature parity #101873

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 4 commits into
base: abdk/trace-tree-basenode-usage
Choose a base branch
from
Open

feat(trace-tree-node): BaseNode implementation changes to achieve feature parity #101873

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
536f806
feat(trace-tree-node): BaseNode implementation changes to achieve fea…
Oct 21, 2025
2d36bd3
:hammer_and_wrench: apply pre-commit fixes
getsantry[bot] Oct 21, 2025
35d845e
feat(trace-tree-node): Minor cleanup
Oct 21, 2025
1689827
feat(trace-tree-node): Removing stale comments
Nov 8, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
160 changes: 128 additions & 32 deletions static/app/views/performance/newTraceDetails/traceModels/traceTreeNode/baseNode.spec.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ import {render, screen} from 'sentry-test/reactTestingLibrary';

import type {TraceTreeNodeDetailsProps} from 'sentry/views/performance/newTraceDetails/traceDrawer/tabs/traceTreeNodeDetails';
import type {TraceTree} from 'sentry/views/performance/newTraceDetails/traceModels/traceTree';
import type {TraceTreeNode} from 'sentry/views/performance/newTraceDetails/traceModels/traceTreeNode';
import {
makeEAPOccurrence,
makeTraceError,
Expand Down Expand Up @@ -59,7 +58,7 @@ class TestNode extends BaseNode {
return <div data-test-id="waterfall-row">Waterfall Row</div>;
}

renderDetails<T extends TraceTreeNode<TraceTree.NodeValue>>(
renderDetails<T extends BaseNode>(
_props: TraceTreeNodeDetailsProps<T>
): React.ReactNode {
return <div data-test-id="node-details">Details</div>;
Expand Down Expand Up @@ -1226,36 +1225,6 @@ describe('BaseNode', () => {
expect(child1.isLastChild()).toBe(false);
expect(child2.isLastChild()).toBe(true);
});

it('should consider nested visible children when determining last child', () => {
const extra = createMockExtra();
const grandparent = new TestNode(
null,
createMockValue({event_id: 'grandparent'}),
extra
);
const parent1 = new TestNode(
grandparent,
createMockValue({event_id: 'parent1'}),
extra
);
const parent2 = new TestNode(
grandparent,
createMockValue({event_id: 'parent2'}),
extra
);
const child = new TestNode(parent2, createMockValue({event_id: 'child'}), extra);

grandparent.children = [parent1, parent2];
parent2.children = [child];
grandparent.expanded = true;
parent2.expanded = true;

// child should be the last visible child of grandparent's visible children
expect(child.isLastChild()).toBe(true);
expect(parent2.isLastChild()).toBe(false);
expect(parent1.isLastChild()).toBe(false);
});
});

describe('isRootNodeChild', () => {
Expand Down Expand Up @@ -1593,4 +1562,131 @@ describe('BaseNode', () => {
expect(node.connectors).toBeUndefined();
});
});

describe('findParentTransaction', () => {
it('should return null when no transaction parent exists', () => {
const extra = createMockExtra();
const node = new TestNode(null, createMockValue({event_id: 'test'}), extra);

expect(node.findParentTransaction()).toBeNull();
});

it('should find parent transaction node', () => {
const extra = createMockExtra();
const mockTransactionParent = {
type: 'txn',
id: 'transaction-parent',
};

const child = new TestNode(null, createMockValue({event_id: 'child'}), extra);
jest.spyOn(child, 'findParent').mockReturnValue(mockTransactionParent as any);

const result = child.findParentTransaction();
expect(result).toBe(mockTransactionParent);
expect(child.findParent).toHaveBeenCalledWith(expect.any(Function));
});
});

describe('findParentEapTransaction', () => {
it('should return null when no EAP transaction parent exists', () => {
const extra = createMockExtra();
const node = new TestNode(null, createMockValue({event_id: 'test'}), extra);

expect(node.findParentEapTransaction()).toBeNull();
});

it('should find parent EAP transaction node', () => {
const extra = createMockExtra();
const mockEapTransactionParent = {
type: 'eap_span',
id: 'eap-transaction-parent',
value: {is_transaction: true},
};

const child = new TestNode(null, createMockValue({event_id: 'child'}), extra);
jest.spyOn(child, 'findParent').mockReturnValue(mockEapTransactionParent as any);

const result = child.findParentEapTransaction();
expect(result).toBe(mockEapTransactionParent);
expect(child.findParent).toHaveBeenCalledWith(expect.any(Function));
});
});

describe('hasErrors getter', () => {
it('should return false when node has no errors', () => {
const extra = createMockExtra();
const node = new TestNode(null, createMockValue({event_id: 'test'}), extra);

expect(node.hasErrors).toBe(false);
});

it('should return true when node has errors', () => {
const extra = createMockExtra();
const node = new TestNode(null, createMockValue({event_id: 'test'}), extra);

node.errors.add(
makeTraceError({
issue_id: 1,
event_id: 'error-1',
})
);

expect(node.hasErrors).toBe(true);
});
});

describe('hasOccurrences getter', () => {
it('should return false when node has no occurrences', () => {
const extra = createMockExtra();
const node = new TestNode(null, createMockValue({event_id: 'test'}), extra);

expect(node.hasOccurrences).toBe(false);
});

it('should return true when node has occurrences', () => {
const extra = createMockExtra();
const node = new TestNode(null, createMockValue({event_id: 'test'}), extra);

node.occurrences.add(
makeEAPOccurrence({
issue_id: 1,
event_id: 'occurrence-1',
})
);

expect(node.hasOccurrences).toBe(true);
});
});

describe('transactionId getter', () => {
it('should return null when no parent transaction or EAP transaction exists', () => {
const extra = createMockExtra();
const node = new TestNode(null, createMockValue({event_id: 'test'}), extra);

expect(node.transactionId).toBeNull();
});

it('should return transaction ID from parent transaction node', () => {
const extra = createMockExtra();
const child = new TestNode(null, createMockValue({event_id: 'child'}), extra);

jest.spyOn(child, 'findParentTransaction').mockReturnValue({
transactionId: 'txn-123',
} as any);

expect(child.transactionId).toBe('txn-123');
});

it('should return transaction ID from parent EAP transaction when no regular transaction parent exists', () => {
const extra = createMockExtra();
const child = new TestNode(null, createMockValue({event_id: 'child'}), extra);

jest.spyOn(child, 'findParentTransaction').mockReturnValue(null);
jest.spyOn(child, 'findParentEapTransaction').mockReturnValue({
transactionId: 'eap-txn-456',
} as any);

expect(child.transactionId).toBe('eap-txn-456');
});
});
});
74 changes: 62 additions & 12 deletions static/app/views/performance/newTraceDetails/traceModels/traceTreeNode/baseNode.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,16 @@ import {pickBarColor} from 'sentry/components/performance/waterfall/utils';
import type {Organization} from 'sentry/types/organization';
import type {TraceMetaQueryResults} from 'sentry/views/performance/newTraceDetails/traceApi/useTraceMeta';
import type {TraceTreeNodeDetailsProps} from 'sentry/views/performance/newTraceDetails/traceDrawer/tabs/traceTreeNodeDetails';
import {
isEAPTransactionNode,
isTransactionNode,
} from 'sentry/views/performance/newTraceDetails/traceGuards';
import type {TraceTree} from 'sentry/views/performance/newTraceDetails/traceModels/traceTree';
import type {TraceTreeNode} from 'sentry/views/performance/newTraceDetails/traceModels/traceTreeNode';
import type {TraceRowProps} from 'sentry/views/performance/newTraceDetails/traceRow/traceRow';

import type {EapSpanNode} from './eapSpanNode';
import type {TransactionNode} from './transactionNode';

export interface TraceTreeNodeExtra {
organization: Organization;
meta?: TraceMetaQueryResults['data'] | null;
Expand Down Expand Up @@ -254,8 +260,16 @@ export abstract class BaseNode<T extends TraceTree.NodeValue = TraceTree.NodeVal
return [...this.uniqueErrorIssues, ...this.uniqueOccurrenceIssues];
}

get hasErrors(): boolean {
return this.errors.size > 0;
}

get hasOccurrences(): boolean {
return this.occurrences.size > 0;
}

get hasIssues(): boolean {
return this.errors.size > 0 || this.occurrences.size > 0;
return this.hasErrors || this.hasOccurrences;
}

get visibleChildren(): BaseNode[] {
Expand Down Expand Up @@ -310,6 +324,22 @@ export abstract class BaseNode<T extends TraceTree.NodeValue = TraceTree.NodeVal
return `${this.type}-${this.id}`;
}

get transactionId(): string | null {
const parentTransaction = this.findParentTransaction();

if (parentTransaction) {
return parentTransaction.transactionId;
}

const parentEapTransaction = this.findParentEapTransaction();

if (parentEapTransaction) {
return parentEapTransaction.transactionId;
}

return null;
}

isRootNodeChild(): boolean {
return this.parent?.value === null;
}
Expand All @@ -319,7 +349,7 @@ export abstract class BaseNode<T extends TraceTree.NodeValue = TraceTree.NodeVal
return false;
}

const visibleChildren = this.parent.visibleChildren;
const visibleChildren = this.parent.directVisibleChildren;
return visibleChildren[visibleChildren.length - 1] === this;
}

Expand All @@ -334,7 +364,13 @@ export abstract class BaseNode<T extends TraceTree.NodeValue = TraceTree.NodeVal
const hasMatchingOccurrences = Array.from(this.occurrences).some(
occurrence => occurrence.event_id === id
);
return this.id === id || hasMatchingErrors || hasMatchingOccurrences;

return (
this.id === id ||
this.transactionId === id ||
hasMatchingErrors ||
hasMatchingOccurrences
);
Copy link
Contributor

@cursor cursor bot Nov 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: Tree Traversal: A Performance Bottleneck

The matchById method calls this.transactionId, which triggers parent tree traversal via findParentTransaction() and findParentEapTransaction() on every invocation. When matchById is called repeatedly during tree searches, this creates expensive redundant parent traversals for nodes that don't override matchById, significantly impacting performance on large trace trees.

Fix in Cursor Fix in Web

}

invalidate() {
Expand All @@ -346,14 +382,16 @@ export abstract class BaseNode<T extends TraceTree.NodeValue = TraceTree.NodeVal
return this.children;
}

findChild(predicate: (child: BaseNode) => boolean): BaseNode | null {
findChild<ChildType extends BaseNode = BaseNode>(
predicate: (child: BaseNode) => boolean
): ChildType | null {
const queue: BaseNode[] = [...this.getNextTraversalNodes()];

while (queue.length > 0) {
const next = queue.pop()!;

if (predicate(next)) {
return next;
return next as ChildType;
}

const children = next.getNextTraversalNodes();
Expand All @@ -365,15 +403,17 @@ export abstract class BaseNode<T extends TraceTree.NodeValue = TraceTree.NodeVal
return null;
}

findAllChildren(predicate: (child: BaseNode) => boolean): BaseNode[] {
findAllChildren<ChildType extends BaseNode = BaseNode>(
predicate: (child: BaseNode) => boolean
): ChildType[] {
const queue: BaseNode[] = [...this.getNextTraversalNodes()];
const results: BaseNode[] = [];
const results: ChildType[] = [];

while (queue.length > 0) {
const next = queue.pop()!;

if (predicate(next)) {
results.push(next);
results.push(next as ChildType);
}

const children = next.getNextTraversalNodes();
Expand All @@ -400,17 +440,27 @@ export abstract class BaseNode<T extends TraceTree.NodeValue = TraceTree.NodeVal
}
}

findParent(predicate: (parent: BaseNode) => boolean): BaseNode | null {
findParent<ChildType extends BaseNode = BaseNode>(
predicate: (parent: BaseNode) => boolean
): ChildType | null {
let current = this.parent;
while (current) {
if (predicate(current)) {
return current;
return current as ChildType;
}
current = current.parent;
}
return null;
}

findParentTransaction(): TransactionNode | null {
return this.findParent<TransactionNode>(p => isTransactionNode(p));
}

findParentEapTransaction(): EapSpanNode | null {
return this.findParent<EapSpanNode>(p => isEAPTransactionNode(p));
}

Comment on lines +456 to +463
Copy link
Member

@gggritso gggritso Oct 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this possible to do without type guards?

Copy link
Contributor Author

@Abdkhan14 Abdkhan14 Nov 8, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gggritso, I think that this is a good use case for a type guard. We are looking for a specific node in the base node class. The problem initially was that such type guards were scattered all over the place, we mitigated that quite a lot.

We can add is_transaction, is_eap_transaction, is_span... attrs to the baseNode classe but I don't really think that is any cleaner 🤔

We need methods like findParentTransaction if we are looking for a specific property that only exists in a TransactionNode type, like transaction id. The usage of the guard is now embedded in the baseNode instance so we can just do node.findParentTransaction instead of node.find(p => isTransactionNode(p)) which is an improvement.

Open to suggestions, lmk!

expand(expanding: boolean, tree: TraceTree): boolean {
const index = tree.list.indexOf(this);

Expand Down Expand Up @@ -506,7 +556,7 @@ export abstract class BaseNode<T extends TraceTree.NodeValue = TraceTree.NodeVal
props: TraceRowProps<NodeType>
): React.ReactNode;

abstract renderDetails<NodeType extends TraceTreeNode<TraceTree.NodeValue>>(
abstract renderDetails<NodeType extends BaseNode>(
props: TraceTreeNodeDetailsProps<NodeType>
): React.ReactNode;

Expand Down
5 changes: 2 additions & 3 deletions static/app/views/performance/newTraceDetails/traceModels/traceTreeNode/collapsedNode.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ import {uuid4} from '@sentry/core';

import type {TraceTreeNodeDetailsProps} from 'sentry/views/performance/newTraceDetails/traceDrawer/tabs/traceTreeNodeDetails';
import type {TraceTree} from 'sentry/views/performance/newTraceDetails/traceModels/traceTree';
import type {TraceTreeNode} from 'sentry/views/performance/newTraceDetails/traceModels/traceTreeNode';
import {TraceCollapsedRow} from 'sentry/views/performance/newTraceDetails/traceRow/traceCollapsedRow';
import type {TraceRowProps} from 'sentry/views/performance/newTraceDetails/traceRow/traceRow';

Expand Down Expand Up @@ -45,10 +44,10 @@ export class CollapsedNode extends BaseNode<TraceTree.CollapsedNode> {
renderWaterfallRow<NodeType extends TraceTree.Node = TraceTree.Node>(
props: TraceRowProps<NodeType>
): React.ReactNode {
return <TraceCollapsedRow {...props} node={props.node} />;
return <TraceCollapsedRow {...props} node={this} />;
}

renderDetails<NodeType extends TraceTreeNode<TraceTree.NodeValue>>(
renderDetails<NodeType extends BaseNode>(
_props: TraceTreeNodeDetailsProps<NodeType>
): React.ReactNode {
return null;
Expand Down
Loading
Loading