Skip to content

feat(trace-tree-node): Consolidating node methods and overrides #101174

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 1 commit into
base: abdk/trace-from-spans
Choose a base branch
from
Open

feat(trace-tree-node): Consolidating node methods and overrides #101174

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
8 changes: 5 additions & 3 deletions static/app/views/performance/newTraceDetails/traceModels/traceTree.spec.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1383,7 +1383,7 @@ describe('TraceTree', () => {
describe('DirectVisibleChildren', () => {
it('returns children for transaction', () => {
const tree = TraceTree.FromTrace(trace, traceOptions);
expect(tree.root.children[0]!.directChildren).toEqual(
expect(tree.root.children[0]!.directVisibleChildren).toEqual(
tree.root.children[0]!.children
);
});
Expand All @@ -1404,7 +1404,9 @@ describe('TraceTree', () => {
) as ParentAutogroupNode;

expect(parentAutogroup).not.toBeNull();
expect(parentAutogroup.directChildren[0]).toBe(parentAutogroup.tail.children[0]);
expect(parentAutogroup.directVisibleChildren[0]).toBe(
parentAutogroup.tail.children[0]
);
});
it('returns head for expanded parent autogroup', async () => {
const tree = TraceTree.FromTrace(trace, traceOptions);
Expand All @@ -1423,7 +1425,7 @@ describe('TraceTree', () => {

parentAutogroup.expand(true, tree);

expect(parentAutogroup.directChildren[0]).toBe(parentAutogroup.head);
expect(parentAutogroup.directVisibleChildren[0]).toBe(parentAutogroup.head);
});
});

Expand Down
5 changes: 3 additions & 2 deletions static/app/views/performance/newTraceDetails/traceModels/traceTree.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -528,7 +528,8 @@ export class TraceTree extends TraceTreeEventDispatcher {
c.parent &&
c.op === 'pageload' &&
c.parent.op === 'http.server' &&
c.parent.directChildren.filter(child => child.op === 'pageload').length === 1
c.parent.directVisibleChildren.filter(child => child.op === 'pageload').length ===
1
) {
// // 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.
Expand Down Expand Up @@ -841,7 +842,7 @@ export class TraceTree extends TraceTreeEventDispatcher {
throw new Error('Parent node is missing, this should be unreachable code');
}

const children = node.parent.directChildren;
const children = node.parent.directVisibleChildren;
const index = children.indexOf(node);
if (index === -1) {
throw new Error('Node is not a child of its parent');
Expand Down
7 changes: 5 additions & 2 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 @@ -16,6 +16,10 @@ import type {TraceRowProps} from 'sentry/views/performance/newTraceDetails/trace
import {BaseNode, type TraceTreeNodeExtra} from './baseNode';

class TestNode extends BaseNode {
get id(): string {
return (this.value as any).event_id;
}

get type(): TraceTree.NodeType {
return 'test' as TraceTree.NodeType;
}
Expand Down Expand Up @@ -571,7 +575,7 @@ describe('BaseNode', () => {

parent.children = [child1, child2];

expect(parent.directChildren).toEqual([child1, child2]);
expect(parent.directVisibleChildren).toEqual([child1, child2]);
});

it('should return visible children when expanded', () => {
Expand Down Expand Up @@ -710,7 +714,6 @@ describe('BaseNode', () => {

const result = await node.fetchChildren(false, {} as TraceTree, {
api: {} as any,
preferences: {} as any,
});

expect(result).toBeNull();
Expand Down
26 changes: 14 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 @@ -182,10 +182,6 @@ export abstract class BaseNode<T extends TraceTree.NodeValue = TraceTree.NodeVal
}
}

get id(): string | undefined {
return this.value && 'event_id' in this.value ? this.value.event_id : undefined;
}

get op(): string | undefined {
return this.value && 'op' in this.value ? this.value.op : undefined;
}
Comment on lines 182 to 187
Copy link
Contributor

Choose a reason for hiding this comment

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

Potential bug: Call to non-existent static method causes runtime TypeError

  • Description: A call to a non-existent static method TraceTree.DirectVisibleChildren will cause a runtime TypeError. The refactoring introduced an instance property node.directVisibleChildren but failed to update this specific call site. This will cause a crash when the trace waterfall attempts to display the children count for span rows.

  • Suggested fix: Replace the static method call TraceTree.DirectVisibleChildren(node).length with the instance property node.directVisibleChildren.length.
    severity: 3.0, confidence: 5.0

Did we get this right? 👍 / 👎 to inform future reviews.

Expand Down Expand Up @@ -261,9 +257,9 @@ export abstract class BaseNode<T extends TraceTree.NodeValue = TraceTree.NodeVal
get visibleChildren(): BaseNode[] {
const queue: BaseNode[] = [];
const visibleChildren: BaseNode[] = [];
if (this.expanded) {
for (let i = this.directChildren.length - 1; i >= 0; i--) {
queue.push(this.directChildren[i]!);
if (this.directVisibleChildren.length > 0) {
for (let i = this.directVisibleChildren.length - 1; i >= 0; i--) {
queue.push(this.directVisibleChildren[i]!);
}
}

Expand All @@ -273,17 +269,21 @@ export abstract class BaseNode<T extends TraceTree.NodeValue = TraceTree.NodeVal
visibleChildren.push(node);

// iterate in reverse to ensure nodes are processed in order
if (node.expanded || node.visibleChildren.length > 0) {
for (let i = node.directChildren.length - 1; i >= 0; i--) {
queue.push(node.directChildren[i]!);
if (node.directVisibleChildren.length > 0) {
for (let i = node.directVisibleChildren.length - 1; i >= 0; i--) {
queue.push(node.directVisibleChildren[i]!);
}
}
}

return visibleChildren;
}

get directChildren(): BaseNode[] {
get directVisibleChildren(): BaseNode[] {
if (!this.expanded) {
return [];
}

return this.children;
}

Expand Down Expand Up @@ -481,9 +481,11 @@ export abstract class BaseNode<T extends TraceTree.NodeValue = TraceTree.NodeVal
if (!id) {
return false;
}
return this.matchById(id);
return this.id === id;
Copy link
Contributor

Choose a reason for hiding this comment

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

Bug: Navigation Broken by ID Matching Restriction

The matchByPath method now only matches against a node's primary ID, removing the ability to find nodes by associated error or occurrence event IDs. This breaks navigation for nodes previously discoverable through those secondary IDs.

Fix in Cursor Fix in Web

}

abstract get id(): string;

abstract get type(): TraceTree.NodeType;

abstract get drawerTabsTitle(): string;
Expand Down
8 changes: 4 additions & 4 deletions static/app/views/performance/newTraceDetails/traceModels/traceTreeNode/eapSpanNode.spec.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -406,7 +406,7 @@ describe('EapSpanNode', () => {
const child = new EapSpanNode(parent, childValue, extra);
parent.children = [child];

expect(parent.directChildren).toEqual([child]);
expect(parent.directVisibleChildren).toEqual([child]);
});

it('should filter directChildren for collapsed EAP transactions', () => {
Expand All @@ -432,7 +432,7 @@ describe('EapSpanNode', () => {
transaction.expanded = false;

// Should only return child transactions when collapsed
expect(transaction.directChildren).toEqual([childTransaction]);
expect(transaction.directVisibleChildren).toEqual([childTransaction]);
});

it('should return all children for expanded EAP transactions', () => {
Expand All @@ -457,7 +457,7 @@ describe('EapSpanNode', () => {
transaction.children = [childTransaction, childSpan];
transaction.expanded = true;

expect(transaction.directChildren).toEqual([childTransaction, childSpan]);
expect(transaction.directVisibleChildren).toEqual([childTransaction, childSpan]);
});
});

Expand Down Expand Up @@ -894,7 +894,7 @@ describe('EapSpanNode', () => {
const node = new EapSpanNode(null, value, extra);

expect(node.children).toEqual([]);
expect(node.directChildren).toEqual([]);
expect(node.directVisibleChildren).toEqual([]);
expect(node.visibleChildren).toEqual([]);
});
});
Expand Down
36 changes: 5 additions & 31 deletions static/app/views/performance/newTraceDetails/traceModels/traceTreeNode/eapSpanNode.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,10 @@ export class EapSpanNode extends BaseNode<TraceTree.EAPSpan> {
}
}

get id(): string {
return this.value.event_id;
}

get type(): TraceTree.NodeType {
return 'span';
}
Expand All @@ -106,7 +110,7 @@ export class EapSpanNode extends BaseNode<TraceTree.EAPSpan> {
};
}

get directChildren(): Array<BaseNode<TraceTree.NodeValue>> {
get directVisibleChildren(): Array<BaseNode<TraceTree.NodeValue>> {
if (isEAPTransaction(this.value) && !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
Expand All @@ -116,36 +120,6 @@ export class EapSpanNode extends BaseNode<TraceTree.EAPSpan> {
return this.children;
}

get visibleChildren(): Array<BaseNode<TraceTree.NodeValue>> {
const queue: BaseNode[] = [];
const visibleChildren: BaseNode[] = [];

if (this.expanded || isEAPTransaction(this.value)) {
const children = this.directChildren;

for (let i = children.length - 1; i >= 0; i--) {
queue.push(children[i]!);
}
}

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

visibleChildren.push(node);

// iterate in reverse to ensure nodes are processed in order
if (node.expanded || isEAPTransaction(node.value)) {
const children = node.directChildren;

for (let i = children.length - 1; i >= 0; i--) {
queue.push(children[i]!);
}
}
}

return visibleChildren;
}

private _reparentSSRUnderBrowserRequestSpan(node: BaseNode) {
const serverRequestHandler = node.parent?.children.find(n => n.op === 'http.server');

Expand Down
4 changes: 4 additions & 0 deletions static/app/views/performance/newTraceDetails/traceModels/traceTreeNode/errorNode.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,10 @@ export class ErrorNode extends BaseNode<TraceTree.TraceErrorIssue> {
this.parent?.children.push(this);
}

get id(): string {
return this.value.event_id;
}

get type(): TraceTree.NodeType {
return 'error';
}
Expand Down
18 changes: 2 additions & 16 deletions ...iews/performance/newTraceDetails/traceModels/traceTreeNode/noInstrumentationNode.spec.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -195,7 +195,6 @@ describe('NoInstrumentationNode', () => {

it('should include transaction ID in path when closest transaction parent found', () => {
const extra = createMockExtra();
const mockFn = jest.fn();
const transactionValue = makeTransaction({
event_id: 'transaction-id',
'transaction.op': 'navigation',
Expand All @@ -208,13 +207,7 @@ describe('NoInstrumentationNode', () => {
const nextSpanValue = makeSpan({span_id: 'next'});
const missingInstrValue = createMissingInstrumentationSpan();

const transactionNode = new TransactionNode(
null,
transactionValue,
extra,
mockFn,
mockFn
);
const transactionNode = new TransactionNode(null, transactionValue, extra);
const spanNode = new SpanNode(transactionNode, spanValue, extra);
const previousNode = new SpanNode(spanNode, previousSpanValue, extra);
const nextNode = new SpanNode(spanNode, nextSpanValue, extra);
Expand Down Expand Up @@ -243,18 +236,11 @@ describe('NoInstrumentationNode', () => {
const span2Value = makeSpan({span_id: 'span2', op: 'http.request'});
const span3Value = makeSpan({span_id: 'span3', op: 'cache.get'});

const mockFn = jest.fn();
const previousSpanValue = makeSpan({span_id: 'previous'});
const nextSpanValue = makeSpan({span_id: 'next'});
const missingInstrValue = createMissingInstrumentationSpan();

const transactionNode = new TransactionNode(
null,
transactionValue,
extra,
mockFn,
mockFn
);
const transactionNode = new TransactionNode(null, transactionValue, extra);
const span1Node = new SpanNode(transactionNode, span1Value, extra);
const span2Node = new SpanNode(span1Node, span2Value, extra);
const span3Node = new SpanNode(span2Node, span3Value, extra);
Expand Down
33 changes: 3 additions & 30 deletions .../views/performance/newTraceDetails/traceModels/traceTreeNode/parentAutogroupNode.spec.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,25 +155,6 @@ describe('ParentAutogroupNode', () => {
expect(node.id).toBe('tail-span-id');
});

it('should return undefined when both head and tail ids are undefined', () => {
const extra = createMockExtra();
const autogroupValue = makeParentAutogroup({});
const headSpanValue = makeEAPSpan({event_id: undefined});
const tailSpanValue = makeEAPSpan({event_id: undefined});

const headNode = new EapSpanNode(null, headSpanValue, extra);
const tailNode = new EapSpanNode(null, tailSpanValue, extra);
const node = new ParentAutogroupNode(
null,
autogroupValue,
extra,
headNode,
tailNode
);

expect(node.id).toBeUndefined();
});

it('should return correct drawerTabsTitle', () => {
const extra = createMockExtra();
const autogroupValue = makeParentAutogroup({
Expand Down Expand Up @@ -283,7 +264,7 @@ describe('ParentAutogroupNode', () => {

node.expanded = true;

expect(node.directChildren).toEqual([headNode]);
expect(node.directVisibleChildren).toEqual([headNode]);
});

it('should return tail children as directChildren when collapsed', () => {
Expand All @@ -308,7 +289,7 @@ describe('ParentAutogroupNode', () => {

node.expanded = false;

expect(node.directChildren).toEqual([childNode]);
expect(node.directVisibleChildren).toEqual([childNode]);
});

it('should compute autogroupedSegments correctly with node chain', () => {
Expand Down Expand Up @@ -621,14 +602,7 @@ describe('ParentAutogroupNode', () => {
const headSpanValue = makeSpan({span_id: 'head-span-id'});
const tailSpanValue = makeSpan({span_id: 'tail-span-id'});

const mockFn = jest.fn();
const transactionNode = new TransactionNode(
null,
transactionValue,
extra,
mockFn,
mockFn
);
const transactionNode = new TransactionNode(null, transactionValue, extra);
const headNode = new SpanNode(transactionNode, headSpanValue, extra);
const tailNode = new SpanNode(transactionNode, tailSpanValue, extra);
const node = new ParentAutogroupNode(
Expand Down Expand Up @@ -704,7 +678,6 @@ describe('ParentAutogroupNode', () => {
);

expect(node.matchByPath('ag-headSpanId')).toBe(true);
expect(node.matchByPath('ag-tailSpanId')).toBe(true);
expect(node.matchByPath('ag-differentId')).toBe(false);
});

Expand Down
Loading
Loading