Skip to content

Commit

Permalink
ref(trace) split expanding from scroll position (#78232)
Browse files Browse the repository at this point in the history 
Split scroll from expanding - this is going to enable a change to
autogrouping logic later on that we need in order to support
enabling/disabling autogrouping
  • Loading branch information
@JonasBa
JonasBa authored Sep 26, 2024
1 parent 60674f0 commit fa5facd
Show file tree
Hide file tree
Showing 4 changed files with 119 additions and 100 deletions.
42 changes: 25 additions & 17 deletions static/app/views/performance/newTraceDetails/index.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -734,28 +734,32 @@ export function TraceViewWaterfall(props: TraceViewWaterfallProps) {
}).then(maybeNode => {
if (maybeNode) {
previouslyFocusedNodeRef.current = null;
scrollRowIntoView(maybeNode.node, maybeNode.index, 'center if outside', true);
const index = tree.list.findIndex(n => n === maybeNode);
if (index === -1) {
Sentry.captureMessage('Trace tree node is not visible in tree');
return;
}
scrollRowIntoView(maybeNode, index, 'center if outside', true);
traceDispatch({
type: 'set roving index',
node: maybeNode.node,
index: maybeNode.index,
node: maybeNode,
index: index,
action_source: 'click',
});
setRowAsFocused(
maybeNode.node,
maybeNode,
null,
traceStateRef.current.search.resultsLookup,
null,
0
);

if (traceStateRef.current.search.resultsLookup.has(maybeNode.node)) {
if (traceStateRef.current.search.resultsLookup.has(maybeNode)) {
traceDispatch({
type: 'set search iterator index',
resultIndex: maybeNode.index,
resultIteratorIndex: traceStateRef.current.search.resultsLookup.get(
maybeNode.node
)!,
resultIndex: index,
resultIteratorIndex:
traceStateRef.current.search.resultsLookup.get(maybeNode)!,
});
} else if (traceStateRef.current.search.resultIteratorIndex !== null) {
traceDispatch({type: 'clear search iterator index'});
Expand Down Expand Up @@ -784,21 +788,25 @@ export function TraceViewWaterfall(props: TraceViewWaterfallProps) {
}).then(maybeNode => {
if (maybeNode) {
previouslyFocusedNodeRef.current = null;
scrollRowIntoView(maybeNode.node, maybeNode.index, 'center if outside', true);
const index = tree.list.findIndex(n => n === maybeNode);
if (index === -1) {
Sentry.captureMessage('Trace tree node is not visible in tree');
return;
}
scrollRowIntoView(maybeNode, index, 'center if outside', true);
traceDispatch({
type: 'set roving index',
node: maybeNode.node,
index: maybeNode.index,
node: maybeNode,
index: index,
action_source: 'click',
});

if (traceStateRef.current.search.resultsLookup.has(maybeNode.node)) {
if (traceStateRef.current.search.resultsLookup.has(maybeNode)) {
traceDispatch({
type: 'set search iterator index',
resultIndex: maybeNode.index,
resultIteratorIndex: traceStateRef.current.search.resultsLookup.get(
maybeNode.node
)!,
resultIndex: index,
resultIteratorIndex:
traceStateRef.current.search.resultsLookup.get(maybeNode)!,
});
} else if (traceStateRef.current.search.resultIteratorIndex !== null) {
traceDispatch({type: 'clear search iterator index'});
Expand Down
46 changes: 43 additions & 3 deletions static/app/views/performance/newTraceDetails/trace.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -290,13 +290,53 @@ export function Trace({
: Promise.resolve(null);

promise
.then(maybeNode => {
onTraceLoad(trace, maybeNode?.node ?? null, maybeNode?.index ?? null);
.then(async node => {
if (!scrollQueueRef.current?.path && !scrollQueueRef.current?.eventId) {
return;
}

if (!maybeNode) {
if (!node) {
Sentry.captureMessage('Failed to find and scroll to node in tree');
return;
}

// When users are coming off an eventID link, we want to fetch the children
// of the node that the eventID points to. This is because the eventID link
// only points to the transaction, but we want to fetch the children of the
// transaction to show the user the list of spans in that transaction
if (scrollQueueRef.current.eventId && node?.canFetch) {
await trace.zoomIn(node, true, {api, organization}).catch(_e => {
Sentry.captureMessage('Failed to fetch children of eventId on mount');
});
}

let index = trace.list.indexOf(node);
// We have found the node, yet it is somehow not in the visible tree.
// This means that the path we were given did not match the current tree.
// This sometimes happens when we receive external links like span-x, txn-y
// however the resulting tree looks like span-x, autogroup, txn-y. In this case,
// we should expand the autogroup node and try to find the node again.
if (node && index === -1) {
let parent_node = node.parent;
while (parent_node) {
// Transactions break autogrouping chains, so we can stop here
if (isTransactionNode(parent_node)) {
break;
}
if (isAutogroupedNode(parent_node)) {
trace.expand(parent_node, true);
// This is very wasteful as it performs O(n^2) search each time we expand a node...
// In most cases though, we should be operating on a tree with sub 10k elements and hopefully
// a low autogrouped node count.
index = node ? trace.list.findIndex(n => n === node) : -1;
if (index !== -1) {
break;
}
}
parent_node = parent_node.parent;
}
}
onTraceLoad(trace, node, index === -1 ? null : index);
})
.finally(() => {
// Important to set scrollQueueRef.current to null and trigger a rerender
Expand Down
120 changes: 46 additions & 74 deletions static/app/views/performance/newTraceDetails/traceModels/traceTree.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -835,6 +835,45 @@ export class TraceTree {
}
}

findByEventId(
start: TraceTreeNode<TraceTree.NodeValue>,
eventId: string
): TraceTreeNode<TraceTree.NodeValue> | null {
return TraceTreeNode.Find(start, node => {
if (isTransactionNode(node)) {
return node.value.event_id === eventId;
}
if (isSpanNode(node)) {
return node.value.span_id === eventId;
}
if (isTraceErrorNode(node)) {
return node.value.event_id === eventId;
}
return hasEventWithEventId(node, eventId);
});
}

findByPath(
start: TraceTreeNode<TraceTree.NodeValue>,
path: TraceTree.NodePath[]
): TraceTreeNode<TraceTree.NodeValue> | null {
const queue = [...path];
let segment = start;
let node: TraceTreeNode<TraceTree.NodeValue> | null = null;

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

node = findInTreeFromSegment(segment, current);
if (!node) {
return null;
}
segment = node;
}

return node;
}

get shape(): TraceType {
const trace = this.root.children[0];
if (!trace) {
Expand Down Expand Up @@ -1420,37 +1459,22 @@ export class TraceTree {
tree: TraceTree,
rerender: () => void,
options: ViewManagerScrollToOptions
): Promise<{index: number; node: TraceTreeNode<TraceTree.NodeValue>} | null | null> {
const node = findInTreeByEventId(tree.root, eventId);
): Promise<TraceTreeNode<TraceTree.NodeValue> | null> {
const node = tree.findByEventId(tree.root, eventId);

if (!node) {
return Promise.resolve(null);
}

return TraceTree.ExpandToPath(tree, node.path, rerender, options).then(
async result => {
// When users are coming off an eventID link, we want to fetch the children
// of the node that the eventID points to. This is because the eventID link
// only points to the transaction, but we want to fetch the children of the
// transaction to show the user the list of spans in that transaction
if (result?.node?.canFetch) {
await tree.zoomIn(result.node, true, options).catch(_e => {
Sentry.captureMessage('Failed to fetch children of eventId on mount');
});
return result;
}

return result;
}
);
return TraceTree.ExpandToPath(tree, node.path, rerender, options);
}

static ExpandToPath(
tree: TraceTree,
scrollQueue: TraceTree.NodePath[],
rerender: () => void,
options: ViewManagerScrollToOptions
): Promise<{index: number; node: TraceTreeNode<TraceTree.NodeValue>} | null | null> {
): Promise<TraceTreeNode<TraceTree.NodeValue> | null> {
const segments = [...scrollQueue];
const list = tree.list;

Expand All @@ -1460,17 +1484,14 @@ export class TraceTree {

if (segments.length === 1 && segments[0] === 'trace-root') {
rerender();
return Promise.resolve({index: 0, node: tree.root.children[0]});
return Promise.resolve(tree.root.children[0]);
}

// Keep parent reference as we traverse the tree so that we can only
// perform searching in the current level and not the entire tree
let parent: TraceTreeNode<TraceTree.NodeValue> = tree.root;

const recurseToRow = async (): Promise<{
index: number;
node: TraceTreeNode<TraceTree.NodeValue>;
} | null | null> => {
const recurseToRow = async (): Promise<TraceTreeNode<TraceTree.NodeValue> | null> => {
const path = segments.pop();
let current = findInTreeFromSegment(parent, path!);

Expand Down Expand Up @@ -1524,42 +1545,8 @@ export class TraceTree {
return recurseToRow();
}

// We are at the last path segment (the node that the user clicked on)
// and we should scroll the view to this node.
let index = current ? tree.list.findIndex(node => node === current) : -1;

// We have found the node, yet it is somehow not in the visible tree.
// This means that the path we were given did not match the current tree.
// This sometimes happens when we receive external links like span-x, txn-y
// however the resulting tree looks like span-x, autogroup, txn-y. In this case,
// we should expand the autogroup node and try to find the node again.
if (current && index === -1) {
let parent_node = current.parent;
while (parent_node) {
// Transactions break autogrouping chains, so we can stop here
if (isTransactionNode(parent_node)) {
break;
}
if (isAutogroupedNode(parent_node)) {
tree.expand(parent_node, true);
index = current ? tree.list.findIndex(node => node === current) : -1;
// This is very wasteful as it performs O(n^2) search each time we expand a node...
// In most cases though, we should be operating on a tree with sub 10k elements and hopefully
// a low autogrouped node count.
if (index !== -1) {
break;
}
}
parent_node = parent_node.parent;
}
}

if (index === -1) {
throw new Error(`Couldn't find node in list ${scrollQueue.join(',')}`);
}

rerender();
return {index, node: current};
return current;
};

return recurseToRow();
Expand Down Expand Up @@ -2596,21 +2583,6 @@ function hasEventWithEventId(
return false;
}

function findInTreeByEventId(start: TraceTreeNode<TraceTree.NodeValue>, eventId: string) {
return TraceTreeNode.Find(start, node => {
if (isTransactionNode(node)) {
return node.value.event_id === eventId;
}
if (isSpanNode(node)) {
return node.value.span_id === eventId;
}
if (isTraceErrorNode(node)) {
return node.value.event_id === eventId;
}
return hasEventWithEventId(node, eventId);
});
}

function findInTreeFromSegment(
start: TraceTreeNode<TraceTree.NodeValue>,
segment: TraceTree.NodePath
Expand Down
11 changes: 5 additions & 6 deletions static/app/views/performance/newTraceDetails/traceRenderers/virtualizedViewManager.spec.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -329,13 +329,12 @@ describe('VirtualizedViewManger', () => {
);

manager.list = makeList();

const result = await TraceTree.ExpandToPath(tree, tree.list[0].path, () => void 0, {
api: api,
organization,
});

expect(result?.node).toBe(tree.list[0]);
expect(result).toBe(tree.list[0]);
});

it('scrolls to transaction', async () => {
Expand All @@ -360,7 +359,7 @@ describe('VirtualizedViewManger', () => {
organization,
});

expect(result?.node).toBe(tree.list[2]);
expect(result).toBe(tree.list[2]);
});

it('scrolls to nested transaction', async () => {
Expand Down Expand Up @@ -404,7 +403,7 @@ describe('VirtualizedViewManger', () => {
}
);

expect(result?.node).toBe(tree.list[tree.list.length - 1]);
expect(result).toBe(tree.list[tree.list.length - 1]);
});

it('scrolls to spans of expanded transaction', async () => {
Expand Down Expand Up @@ -441,7 +440,7 @@ describe('VirtualizedViewManger', () => {
);

expect(tree.list[1].zoomedIn).toBe(true);
expect(result?.node).toBe(tree.list[2]);
expect(result).toBe(tree.list[2]);
});

it('scrolls to span -> transaction -> span -> transaction', async () => {
Expand Down Expand Up @@ -678,7 +677,7 @@ describe('VirtualizedViewManger', () => {
organization,
});

expect(result?.node).toBe(tree.list[2]);
expect(result).toBe(tree.list[2]);
});

describe('error handling', () => {
Expand Down

0 comments on commit fa5facd

Please sign in to comment.