Skip to content

Commit

Permalink
fix(tracing): do not reparent under span if it was never a parent (#7…
Browse files Browse the repository at this point in the history
…4842)

If a txn wasnt the parent of pageload, it should not be reparented. We
can remove the logic once span streaming becomes a thing or if we can
ever load entire traces through a single request.
  • Loading branch information
JonasBa authored and Christinarlong committed Jul 26, 2024
1 parent 972ff5e commit 8ced13d
Show file tree
Hide file tree
Showing 2 changed files with 47 additions and 2 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -2803,6 +2803,44 @@ describe('TraceTree', () => {
assertTransactionNode(secondPageload);
expect(secondPageload.value.transaction).toBe('second pageload');
});
it('doesnt reparent http.server child txn under browser request span if it was not reparented', async () => {
const tree: TraceTree = TraceTree.FromTrace(
makeTrace({
transactions: [
makeTransaction({
transaction: 'pageload',
['transaction.op']: 'pageload',
event_id: 'pageload',
project_slug: 'js',
children: [
makeTransaction({
transaction: 'http.server',
['transaction.op']: 'http.server',
}),
],
}),
],
}),
null
);
MockApiClient.addMockResponse({
url: '/organizations/org-slug/events/js:pageload/?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);
tree.print();

const pageloadTransaction = tree.list[1];
const serverHandlerTransaction = tree.list[3];
expect(serverHandlerTransaction.parent).toBe(pageloadTransaction);
});
describe('expanded', () => {
it('server handler transaction becomes a child of browser request span if present', async () => {
const tree: TraceTree = TraceTree.FromTrace(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -238,9 +238,11 @@ function isBrowserRequestSpan(value: TraceTree.Span): boolean {
function childParentSwap({
parent,
child,
reason,
}: {
child: TraceTreeNode<TraceTree.NodeValue>;
parent: TraceTreeNode<TraceTree.NodeValue>;
reason: TraceTreeNode['reparent_reason'];
}) {
const parentOfParent = parent.parent!;

Expand All @@ -251,6 +253,9 @@ function childParentSwap({
// 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));

child.reparent_reason = reason;
parent.reparent_reason = reason;
}

function measurementToTimestamp(
Expand Down Expand Up @@ -595,7 +600,7 @@ export class TraceTree {
) {
// 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});
childParentSwap({parent, child: node, reason: 'pageload server handler'});
parent.invalidate(parent);
node.invalidate(node);
}
Expand Down Expand Up @@ -903,7 +908,7 @@ export class TraceTree {
let firstTransaction: TraceTreeNode<TraceTree.Transaction> | null = null;
for (const child of parent.children) {
if (isTransactionNode(child)) {
firstTransaction = firstTransaction || 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 &&
Expand Down Expand Up @@ -934,6 +939,7 @@ export class TraceTree {
// was the parent of the browser request span which likely served the document.
if (
firstTransaction &&
firstTransaction.reparent_reason === 'pageload server handler' &&
!childTransactions.length &&
isBrowserRequestSpan(spanNodeValue) &&
isServerRequestHandlerTransactionNode(firstTransaction)
Expand Down Expand Up @@ -1734,6 +1740,7 @@ export class TraceTreeNode<T extends TraceTree.NodeValue = TraceTree.NodeValue>
canFetch: boolean = false;
fetchStatus: 'resolved' | 'error' | 'idle' | 'loading' = 'idle';
parent: TraceTreeNode | null = null;
reparent_reason: 'pageload server handler' | null = null;
value: T;
expanded: boolean = false;
zoomedIn: boolean = false;
Expand Down

0 comments on commit 8ced13d

Please sign in to comment.