Skip to content
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

Jump to bottom

feat(tracing): reparent ssr spans under pageload txn and under browser request span #74675

Merged
merged 4 commits into from
Jul 23, 2024
Merged

feat(tracing): reparent ssr spans under pageload txn and under browser request span #74675

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
a981b18
feat(tracing): reparent ssr spans under pageload txn and under browse…
JonasBa Jul 22, 2024
28ba972
ref: invalidate outside of the swap call and add comment
JonasBa Jul 23, 2024
0704907
ref: skip reparenting if there are multiple children on the txn value
JonasBa Jul 23, 2024
bcc567f
ref: rename to check for node type
JonasBa Jul 23, 2024
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
144 changes: 144 additions & 0 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 @@ -2740,4 +2740,148 @@ describe('TraceTree', () => {
expect(mockedResponse3).toHaveBeenCalledTimes(1);
});
});

describe('SSR', () => {
it('makes pageload transaction a parent of server handler transaction', () => {
const tree: TraceTree = TraceTree.FromTrace(
makeTrace({
transactions: [
makeTransaction({
transaction: 'SSR',
['transaction.op']: 'http.server',
children: [
makeTransaction({
transaction: 'pageload',
['transaction.op']: 'pageload',
}),
],
}),
],
}),
null
);

const root = tree.root.children[0];
expect(root?.children?.[0]?.value?.['transaction.op']).toBe('pageload');
expect(root?.children?.[0]?.children?.[0]?.value?.['transaction.op']).toBe(
'http.server'
);
});

it('skips reparenting if server handler has multiple direct transaction children', () => {
const tree: TraceTree = TraceTree.FromTrace(
makeTrace({
transactions: [
makeTransaction({
transaction: 'SSR',
['transaction.op']: 'http.server',
children: [
makeTransaction({
transaction: 'first pageload',
['transaction.op']: 'pageload',
}),
makeTransaction({
transaction: 'second pageload',
['transaction.op']: 'pageload',
}),
],
}),
],
}),
null
);

const transaction = tree.list[1];
assertTransactionNode(transaction);
expect(transaction.value.transaction).toBe('SSR');

const firstPageload = tree.list[2];
assertTransactionNode(firstPageload);
expect(firstPageload.value.transaction).toBe('first pageload');

const secondPageload = tree.list[3];
assertTransactionNode(secondPageload);
expect(secondPageload.value.transaction).toBe('second pageload');
});
describe('expanded', () => {
it('server handler transaction becomes a child of browser request span if present', async () => {
const tree: TraceTree = TraceTree.FromTrace(
makeTrace({
transactions: [
makeTransaction({
transaction: 'SSR',
event_id: 'ssr',
project_slug: 'js',
['transaction.op']: 'http.server',
children: [
makeTransaction({
transaction: 'pageload',
['transaction.op']: 'pageload',
}),
],
}),
],
}),
null
);

MockApiClient.addMockResponse({
url: '/organizations/org-slug/events/js:ssr/?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);
const browserRequestSpan = tree.list[1].children[0];
const ssrTransaction = browserRequestSpan.children[0];

assertSpanNode(browserRequestSpan);
assertTransactionNode(ssrTransaction);
expect(ssrTransaction.value.transaction).toBe('SSR');
});
it('server handler transaction becomes a direct child if there is no matching browser request span', async () => {
const tree: TraceTree = TraceTree.FromTrace(
makeTrace({
transactions: [
makeTransaction({
transaction: 'SSR',
event_id: 'ssr',
project_slug: 'js',
['transaction.op']: 'http.server',
children: [
makeTransaction({
transaction: 'pageload',
['transaction.op']: 'pageload',
}),
],
}),
],
}),
null
);

MockApiClient.addMockResponse({
url: '/organizations/org-slug/events/js:ssr/?averageColumn=span.self_time&averageColumn=span.duration',
method: 'GET',
body: makeEvent({}, [makeSpan({description: 'request', op: 'almost-browser'})]),
});

tree.zoomIn(tree.list[1], true, {
api: new MockApiClient(),
organization: OrganizationFixture(),
});

await waitFor(() => tree.list.length === 4);

const transaction = tree.list[tree.list.length - 1];
assertTransactionNode(transaction);
expect(transaction.value.transaction).toBe('SSR');
});
});
});
});
100 changes: 93 additions & 7 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 @@ -218,6 +218,41 @@ function isJavascriptSDKTransaction(transaction: TraceTree.Transaction): boolean
);
}

function isPageloadTransactionNode(node: TraceTreeNode<TraceTree.NodeValue>): boolean {
return isTransactionNode(node) && node.value['transaction.op'] === 'pageload';
}

function isServerRequestHandlerTransactionNode(
node: TraceTreeNode<TraceTree.NodeValue>
): boolean {
return isTransactionNode(node) && node.value['transaction.op'] === 'http.server';
}

function isBrowserRequestSpan(value: TraceTree.Span): boolean {
return value.op === 'browser' && value.description === 'request';
}

/**
* Swaps the two nodes in the graph.
*/
function childParentSwap({
Copy link
Member Author

Choose a reason for hiding this comment

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

This is a bit more annoying than I would like. Tldr, but because the child nodes keep a reference to the parent, it means we need to filter out the children part of the tree so that we dont end up with circular references.

parent,
child,
}: {
child: TraceTreeNode<TraceTree.NodeValue>;
parent: TraceTreeNode<TraceTree.NodeValue>;
}) {
const parentOfParent = parent.parent!;

const parentIndex = parentOfParent.children.indexOf(parent);
parentOfParent.children[parentIndex] = child;
child.parent = parentOfParent;

// 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));
}

function measurementToTimestamp(
start_timestamp: number,
measurement: number,
Expand Down Expand Up @@ -490,7 +525,8 @@ export class TraceTree {

function visit(
parent: TraceTreeNode<TraceTree.NodeValue | null>,
value: TraceTree.Transaction | TraceTree.TraceError
value: TraceTree.Transaction | TraceTree.TraceError,
childrenCount: number
) {
const node = new TraceTreeNode(parent, value, {
project_slug:
Expand Down Expand Up @@ -552,9 +588,21 @@ export class TraceTree {
);
}

if (
childrenCount === 1 &&
isPageloadTransactionNode(node) &&
isServerRequestHandlerTransactionNode(parent)
) {
// 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});
parent.invalidate(parent);
node.invalidate(node);
}

if (value && 'children' in value) {
for (const child of value.children) {
visit(node, child);
visit(node, child, value.children.length);
}
}

Expand All @@ -580,17 +628,17 @@ export class TraceTree {
typeof orphan.timestamp === 'number' &&
transaction.start_timestamp <= orphan.timestamp
) {
visit(traceNode, transaction);
visit(traceNode, transaction, transaction.children.length);
tIdx++;
} else {
visit(traceNode, orphan);
visit(traceNode, orphan, 0);
oIdx++;
}
} else if (transaction) {
visit(traceNode, transaction);
visit(traceNode, transaction, transaction.children.length);
tIdx++;
} else if (orphan) {
visit(traceNode, orphan);
visit(traceNode, orphan, 0);
oIdx++;
}
}
Expand Down Expand Up @@ -852,8 +900,10 @@ export class TraceTree {
TraceTreeNode<TraceTree.Transaction>[]
>();

let firstTransaction: TraceTreeNode<TraceTree.Transaction> | null = null;
for (const child of parent.children) {
if (isTransactionNode(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 All @@ -871,12 +921,28 @@ export class TraceTree {
const remappedTransactionParents = new Set<TraceTreeNode<TraceTree.NodeValue>>();

for (const span of spans) {
const childTransactions = transactionsToSpanMap.get(span.span_id) ?? [];
let childTransactions = transactionsToSpanMap.get(span.span_id) ?? [];

const spanNodeValue: TraceTree.Span = {
...span,
event: data as EventTransaction,
childTransactions,
};

// If we have a browser request span and a server request handler transaction, we want to
// reparent the transaction under the span. This is because the server request handler
// was the parent of the browser request span which likely served the document.
if (
firstTransaction &&
!childTransactions.length &&
isBrowserRequestSpan(spanNodeValue) &&
isServerRequestHandlerTransactionNode(firstTransaction)
) {
childTransactions = [firstTransaction];
spanNodeValue.childTransactions = childTransactions;
transactionsToSpanMap.delete(`unreachable-${firstTransaction.value.event_id}`);
}

const node: TraceTreeNode<TraceTree.Span> = new TraceTreeNode(null, spanNodeValue, {
event_id: parent.metadata.event_id,
project_slug: parent.metadata.project_slug,
Expand Down Expand Up @@ -1833,6 +1899,26 @@ export class TraceTreeNode<T extends TraceTree.NodeValue = TraceTree.NodeValue>
return clone;
}

filter(
node: TraceTreeNode<TraceTree.NodeValue>,
JonasBa marked this conversation as resolved.
Show resolved Hide resolved
predicate: (node: TraceTreeNode) => boolean
): TraceTreeNode<TraceTree.NodeValue> {
const queue = [node];

while (queue.length) {
const next = queue.pop()!;
for (let i = 0; i < next.children.length; i++) {
if (!predicate(next.children[i])) {
next.children.splice(i, 1);
} else {
queue.push(next.children[i]);
}
}
}

return node;
}

get isOrphaned() {
return this.parent?.value && 'orphan_errors' in this.parent.value;
}
Expand Down
Loading