Skip to content

Commit 8dd273b

Browse files
Lms24Lms24
committed
account for custom traces and traces with more than 1 root span
1 parent 974103a commit 8dd273b

File tree

7 files changed

+217
-4
lines changed

7 files changed

+217
-4
lines changed

‎dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/previous-trace-links/custom-trace/subject.js

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
const btn1 = document.getElementById('btn1');
2+
const btn2 = document.getElementById('btn2');
3+
4+
btn1.addEventListener('click', () => {
5+
Sentry.startNewTrace(() => {
6+
Sentry.startSpan({name: 'custom root span 1', op: 'custom'}, () => {});
7+
});
8+
});
9+
10+
11+
btn2.addEventListener('click', () => {
12+
Sentry.startNewTrace(() => {
13+
Sentry.startSpan({name: 'custom root span 2', op: 'custom'}, () => {});
14+
});
15+
});

‎dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/previous-trace-links/custom-trace/template.html

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
<!DOCTYPE html>
2+
<html>
3+
<head>
4+
<meta charset="utf-8" />
5+
</head>
6+
<button id="btn1">
7+
<button id="btn2">
8+
</button>
9+
</html>

‎dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/previous-trace-links/custom-trace/test.ts

Lines changed: 63 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,63 @@
1+
import { expect } from '@playwright/test';
2+
import { SEMANTIC_LINK_ATTRIBUTE_LINK_TYPE } from '@sentry/core';
3+
4+
import { sentryTest } from '../../../../../utils/fixtures';
5+
import { envelopeRequestParser, shouldSkipTracingTest, waitForTransactionRequest } from '../../../../../utils/helpers';
6+
7+
sentryTest('manually started custom traces are linked correctly in the chain', async ({ getLocalTestUrl, page }) => {
8+
if (shouldSkipTracingTest()) {
9+
sentryTest.skip();
10+
}
11+
12+
const url = await getLocalTestUrl({ testDir: __dirname });
13+
14+
const pageloadTraceContext = await sentryTest.step('Initial pageload', async () => {
15+
const pageloadRequestPromise = waitForTransactionRequest(page, evt => evt.contexts?.trace?.op === 'pageload');
16+
await page.goto(url);
17+
const pageloadRequest = envelopeRequestParser(await pageloadRequestPromise);
18+
return pageloadRequest.contexts?.trace;
19+
});
20+
21+
const customTrace1Context = await sentryTest.step('Custom trace', async () => {
22+
const customTrace1RequestPromise = waitForTransactionRequest(page, evt => evt.contexts?.trace?.op === 'custom');
23+
await page.locator('#btn1').click();
24+
const customTrace1Event = envelopeRequestParser(await customTrace1RequestPromise);
25+
26+
const customTraceCtx = customTrace1Event.contexts?.trace;
27+
28+
expect(customTraceCtx?.trace_id).not.toEqual(pageloadTraceContext?.trace_id);
29+
expect(customTraceCtx?.links).toEqual([
30+
{
31+
trace_id: pageloadTraceContext?.trace_id,
32+
span_id: pageloadTraceContext?.span_id,
33+
sampled: true,
34+
attributes: {
35+
[SEMANTIC_LINK_ATTRIBUTE_LINK_TYPE]: 'previous_trace',
36+
},
37+
},
38+
]);
39+
40+
return customTraceCtx;
41+
});
42+
43+
await sentryTest.step('Navigation', async () => {
44+
const navigation1RequestPromise = waitForTransactionRequest(page, evt => evt.contexts?.trace?.op === 'navigation');
45+
await page.goto(`${url}#foo`);
46+
const navigationEvent = envelopeRequestParser(await navigation1RequestPromise);
47+
const navTraceContext = navigationEvent.contexts?.trace;
48+
49+
expect(navTraceContext?.trace_id).not.toEqual(customTrace1Context?.trace_id);
50+
expect(navTraceContext?.trace_id).not.toEqual(pageloadTraceContext?.trace_id);
51+
52+
expect(navTraceContext?.links).toEqual([
53+
{
54+
trace_id: customTrace1Context?.trace_id,
55+
span_id: customTrace1Context?.span_id,
56+
sampled: true,
57+
attributes: {
58+
[SEMANTIC_LINK_ATTRIBUTE_LINK_TYPE]: 'previous_trace',
59+
},
60+
},
61+
]);
62+
});
63+
});

‎dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/previous-trace-links/interaction-spans/init.js

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
import * as Sentry from '@sentry/browser';
2+
3+
window.Sentry = Sentry;
4+
5+
Sentry.init({
6+
dsn: 'https://[email protected]/1337',
7+
tracesSampleRate: 1,
8+
integrations: [Sentry.browserTracingIntegration({_experiments: {enableInteractions: true}})],
9+
});

‎dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/previous-trace-links/interaction-spans/template.html

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
<!DOCTYPE html>
2+
<html>
3+
<head>
4+
<meta charset="utf-8" />
5+
</head>
6+
<button id="btn">
7+
</button>
8+
</html>

‎dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/previous-trace-links/interaction-spans/test.ts

Lines changed: 90 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,90 @@
1+
import { expect } from '@playwright/test';
2+
import { SEMANTIC_LINK_ATTRIBUTE_LINK_TYPE } from '@sentry/core';
3+
4+
import { sentryTest } from '../../../../../utils/fixtures';
5+
import { envelopeRequestParser, shouldSkipTracingTest, waitForTransactionRequest } from '../../../../../utils/helpers';
6+
7+
/*
8+
This is quite peculiar behavior but it's a result of the route-based trace lifetime.
9+
Once we shortened trace lifetime, this whole scenario will change as the interaction
10+
spans will be their own trace. So most likely, we can replace this test with a new one
11+
that covers the new default behavior.
12+
*/
13+
sentryTest(
14+
'only the first root spans in the trace link back to the previous trace',
15+
async ({ getLocalTestUrl, page }) => {
16+
if (shouldSkipTracingTest()) {
17+
sentryTest.skip();
18+
}
19+
20+
const url = await getLocalTestUrl({ testDir: __dirname });
21+
22+
const pageloadTraceContext = await sentryTest.step('Initial pageload', async () => {
23+
const pageloadRequestPromise = waitForTransactionRequest(page, evt => evt.contexts?.trace?.op === 'pageload');
24+
await page.goto(url);
25+
26+
const pageloadEvent = envelopeRequestParser(await pageloadRequestPromise);
27+
const traceContext = pageloadEvent.contexts?.trace;
28+
29+
expect(traceContext).toBeDefined();
30+
expect(traceContext?.links).toBeUndefined();
31+
32+
return traceContext;
33+
});
34+
35+
await sentryTest.step('Click Before navigation', async () => {
36+
const interactionRequestPromise = waitForTransactionRequest(page, evt => {
37+
return evt.contexts?.trace?.op === 'ui.action.click';
38+
});
39+
await page.click('#btn');
40+
41+
const interactionEvent = envelopeRequestParser(await interactionRequestPromise);
42+
const interactionTraceContext = interactionEvent.contexts?.trace;
43+
44+
// sanity check: route-based trace lifetime means the trace_id should be the same
45+
expect(interactionTraceContext?.trace_id).toBe(pageloadTraceContext?.trace_id);
46+
47+
// no links yet as previous root span belonged to same trace
48+
expect(interactionTraceContext?.links).toBeUndefined();
49+
});
50+
51+
const navigationTraceContext = await sentryTest.step('Navigation', async () => {
52+
const navigationRequestPromise = waitForTransactionRequest(page, evt => evt.contexts?.trace?.op === 'navigation');
53+
await page.goto(`${url}#foo`);
54+
const navigationEvent = envelopeRequestParser(await navigationRequestPromise);
55+
56+
const traceContext = navigationEvent.contexts?.trace;
57+
58+
expect(traceContext?.op).toBe('navigation');
59+
expect(traceContext?.links).toEqual([
60+
{
61+
trace_id: pageloadTraceContext?.trace_id,
62+
span_id: pageloadTraceContext?.span_id,
63+
sampled: true,
64+
attributes: {
65+
[SEMANTIC_LINK_ATTRIBUTE_LINK_TYPE]: 'previous_trace',
66+
},
67+
},
68+
]);
69+
70+
expect(traceContext?.trace_id).not.toEqual(traceContext?.links![0].trace_id);
71+
return traceContext;
72+
});
73+
74+
await sentryTest.step('Click After navigation', async () => {
75+
const interactionRequestPromise = waitForTransactionRequest(page, evt => {
76+
return evt.contexts?.trace?.op === 'ui.action.click';
77+
});
78+
await page.click('#btn');
79+
const interactionEvent = envelopeRequestParser(await interactionRequestPromise);
80+
81+
const interactionTraceContext = interactionEvent.contexts?.trace;
82+
83+
// sanity check: route-based trace lifetime means the trace_id should be the same
84+
expect(interactionTraceContext?.trace_id).toBe(navigationTraceContext?.trace_id);
85+
86+
// since this is the second root span in the trace, it doesn't link back to the previous trace
87+
expect(interactionTraceContext?.links).toBeUndefined();
88+
});
89+
},
90+
);

‎packages/browser/src/tracing/previousTrace.ts

Lines changed: 23 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -22,19 +22,38 @@ export const PREVIOUS_TRACE_MAX_DURATION = 216_000;
2222
const PREVIOUS_TRACE_KEY = 'sentry_previous_trace';
2323

2424
/**
25-
* Adds a previous_trace span link to @param startSpanOptions if the previous trace from @param previousTraceInfo is still valid.
26-
* Returns @param previousTraceInfo if the previous trace is still valid, otherwise returns undefined.
25+
* Adds a previous_trace span link to the passed span if the passed
26+
* previousTraceInfo is still valid.
27+
*
28+
* @returns the updated previous trace info (based on the current span/trace) or `undefined` the
2729
*/
2830
export function addPreviousTraceSpanLink(
2931
previousTraceInfo: PreviousTraceInfo | undefined,
3032
span: Span,
3133
): PreviousTraceInfo {
3234
const spanJson = spanToJSON(span);
3335

34-
if (previousTraceInfo && Date.now() / 1000 - previousTraceInfo.startTimestamp <= PREVIOUS_TRACE_MAX_DURATION) {
36+
if (!previousTraceInfo) {
37+
return {
38+
spanContext: span.spanContext(),
39+
startTimestamp: spanJson.start_timestamp,
40+
};
41+
}
42+
43+
if (previousTraceInfo.spanContext.traceId === spanJson.trace_id) {
44+
// This means, we're still in the same trace so let's not update the previous trace info
45+
// or add a link to the current span.
46+
// Once we move away from the long-lived, route-based trace model, we can remove this cases
47+
return previousTraceInfo;
48+
}
49+
50+
if (Date.now() / 1000 - previousTraceInfo.startTimestamp <= PREVIOUS_TRACE_MAX_DURATION) {
3551
if (DEBUG_BUILD) {
3652
logger.info(
37-
`Adding previous_trace ${previousTraceInfo.spanContext} to span ${{ op: spanJson.op, ...span.spanContext() }}`,
53+
`Adding previous_trace ${previousTraceInfo.spanContext} link to span ${{
54+
op: spanJson.op,
55+
...span.spanContext(),
56+
}}`,
3857
);
3958
}
4059

0 commit comments

Comments
 (0)