Skip to content

Commit 45bc790

Browse files
Lms24Lms24
committed
use spanStart client hook instead; adjust tests
1 parent 7ee4f2c commit 45bc790

File tree

8 files changed

+148
-117
lines changed

8 files changed

+148
-117
lines changed

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

Lines changed: 43 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -1,13 +1,8 @@
11
import { expect } from '@playwright/test';
2-
import { SEMANTIC_LINK_ATTRIBUTE_LINK_TYPE, type Event } from '@sentry/core';
2+
import { SEMANTIC_LINK_ATTRIBUTE_LINK_TYPE } from '@sentry/core';
33

44
import { sentryTest } from '../../../../../utils/fixtures';
5-
import {
6-
envelopeRequestParser,
7-
getFirstSentryEnvelopeRequest,
8-
shouldSkipTracingTest,
9-
waitForTransactionRequest,
10-
} from '../../../../../utils/helpers';
5+
import { envelopeRequestParser, shouldSkipTracingTest, waitForTransactionRequest } from '../../../../../utils/helpers';
116

127
sentryTest("navigation spans link back to previous trace's root span", async ({ getLocalTestUrl, page }) => {
138
if (shouldSkipTracingTest()) {
@@ -16,24 +11,34 @@ sentryTest("navigation spans link back to previous trace's root span", async ({
1611

1712
const url = await getLocalTestUrl({ testDir: __dirname });
1813

19-
const pageloadRequest = await getFirstSentryEnvelopeRequest<Event>(page, url);
20-
const navigationRequest = await getFirstSentryEnvelopeRequest<Event>(page, `${url}#foo`);
21-
const navigation2Request = await getFirstSentryEnvelopeRequest<Event>(page, `${url}#bar`);
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+
});
2220

23-
const pageloadTraceContext = pageloadRequest.contexts?.trace;
24-
const navigationTraceContext = navigationRequest.contexts?.trace;
25-
const navigation2TraceContext = navigation2Request.contexts?.trace;
21+
const navigation1TraceContext = await sentryTest.step('First navigation', async () => {
22+
const navigation1RequestPromise = waitForTransactionRequest(page, evt => evt.contexts?.trace?.op === 'navigation');
23+
await page.goto(`${url}#foo`);
24+
const navigation1Request = envelopeRequestParser(await navigation1RequestPromise);
25+
return navigation1Request.contexts?.trace;
26+
});
27+
28+
const navigation2TraceContext = await sentryTest.step('Second navigation', async () => {
29+
const navigation2RequestPromise = waitForTransactionRequest(page, evt => evt.contexts?.trace?.op === 'navigation');
30+
await page.goto(`${url}#bar`);
31+
const navigation2Request = envelopeRequestParser(await navigation2RequestPromise);
32+
return navigation2Request.contexts?.trace;
33+
});
2634

2735
const pageloadTraceId = pageloadTraceContext?.trace_id;
28-
const navigationTraceId = navigationTraceContext?.trace_id;
36+
const navigation1TraceId = navigation1TraceContext?.trace_id;
2937
const navigation2TraceId = navigation2TraceContext?.trace_id;
3038

31-
expect(pageloadTraceContext?.op).toBe('pageload');
32-
expect(navigationTraceContext?.op).toBe('navigation');
33-
expect(navigation2TraceContext?.op).toBe('navigation');
34-
3539
expect(pageloadTraceContext?.links).toBeUndefined();
36-
expect(navigationTraceContext?.links).toEqual([
40+
41+
expect(navigation1TraceContext?.links).toEqual([
3742
{
3843
trace_id: pageloadTraceId,
3944
span_id: pageloadTraceContext?.span_id,
@@ -46,17 +51,17 @@ sentryTest("navigation spans link back to previous trace's root span", async ({
4651

4752
expect(navigation2TraceContext?.links).toEqual([
4853
{
49-
trace_id: navigationTraceId,
50-
span_id: navigationTraceContext?.span_id,
54+
trace_id: navigation1TraceId,
55+
span_id: navigation1TraceContext?.span_id,
5156
sampled: true,
5257
attributes: {
5358
[SEMANTIC_LINK_ATTRIBUTE_LINK_TYPE]: 'previous_trace',
5459
},
5560
},
5661
]);
5762

58-
expect(pageloadTraceId).not.toEqual(navigationTraceId);
59-
expect(navigationTraceId).not.toEqual(navigation2TraceId);
63+
expect(pageloadTraceId).not.toEqual(navigation1TraceId);
64+
expect(navigation1TraceId).not.toEqual(navigation2TraceId);
6065
expect(pageloadTraceId).not.toEqual(navigation2TraceId);
6166
});
6267

@@ -67,14 +72,21 @@ sentryTest("doesn't link between hard page reloads by default", async ({ getLoca
6772

6873
const url = await getLocalTestUrl({ testDir: __dirname });
6974

70-
const pageload1Event = await getFirstSentryEnvelopeRequest<Event>(page, url);
75+
await sentryTest.step('First pageload', async () => {
76+
const pageloadRequestPromise = waitForTransactionRequest(page, evt => evt.contexts?.trace?.op === 'pageload');
77+
await page.goto(url);
78+
const pageload1Event = envelopeRequestParser(await pageloadRequestPromise);
79+
80+
expect(pageload1Event.contexts?.trace).toBeDefined();
81+
expect(pageload1Event.contexts?.trace?.links).toBeUndefined();
82+
});
7183

72-
const pageload2RequestPromise = waitForTransactionRequest(page, evt => evt.contexts?.trace?.op === 'pageload');
73-
await page.reload();
74-
const pageload2Event = envelopeRequestParser(await pageload2RequestPromise);
84+
await sentryTest.step('Second pageload', async () => {
85+
const pageload2RequestPromise = waitForTransactionRequest(page, evt => evt.contexts?.trace?.op === 'pageload');
86+
await page.reload();
87+
const pageload2Event = envelopeRequestParser(await pageload2RequestPromise);
7588

76-
expect(pageload1Event.contexts?.trace).toBeDefined();
77-
expect(pageload2Event.contexts?.trace).toBeDefined();
78-
expect(pageload1Event.contexts?.trace?.links).toBeUndefined();
79-
expect(pageload2Event.contexts?.trace?.links).toBeUndefined();
89+
expect(pageload2Event.contexts?.trace).toBeDefined();
90+
expect(pageload2Event.contexts?.trace?.links).toBeUndefined();
91+
});
8092
});

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,4 +6,5 @@ Sentry.init({
66
dsn: 'https://[email protected]/1337',
77
integrations: [Sentry.browserTracingIntegration()],
88
tracesSampleRate: 1,
9+
debug: true,
910
});

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

Lines changed: 22 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,8 @@
11
import { expect } from '@playwright/test';
2-
import { SEMANTIC_LINK_ATTRIBUTE_LINK_TYPE, type Event } from '@sentry/core';
2+
import { SEMANTIC_LINK_ATTRIBUTE_LINK_TYPE } from '@sentry/core';
33

44
import { sentryTest } from '../../../../../utils/fixtures';
5-
import { getFirstSentryEnvelopeRequest, shouldSkipTracingTest } from '../../../../../utils/helpers';
5+
import { envelopeRequestParser, shouldSkipTracingTest, waitForTransactionRequest } from '../../../../../utils/helpers';
66

77
sentryTest(
88
"links back to previous trace's local root span if continued from meta tags",
@@ -13,22 +13,31 @@ sentryTest(
1313

1414
const url = await getLocalTestUrl({ testDir: __dirname });
1515

16-
const pageloadRequest = await getFirstSentryEnvelopeRequest<Event>(page, url);
17-
const navigationRequest = await getFirstSentryEnvelopeRequest<Event>(page, `${url}#foo`);
16+
const metaTagTraceId = '12345678901234567890123456789012';
1817

19-
const pageloadTraceContext = pageloadRequest.contexts?.trace;
20-
const navigationTraceContext = navigationRequest.contexts?.trace;
18+
const pageloadTraceContext = await sentryTest.step('Initial pageload', async () => {
19+
const pageloadRequestPromise = waitForTransactionRequest(page, evt => evt.contexts?.trace?.op === 'pageload');
20+
await page.goto(url);
21+
const pageloadRequest = envelopeRequestParser(await pageloadRequestPromise);
2122

22-
const metaTagTraceId = '12345678901234567890123456789012';
23+
const traceContext = pageloadRequest.contexts?.trace;
2324

24-
const navigationTraceId = navigationTraceContext?.trace_id;
25+
// sanity check
26+
expect(traceContext?.trace_id).toBe(metaTagTraceId);
2527

26-
expect(pageloadTraceContext?.op).toBe('pageload');
27-
expect(navigationTraceContext?.op).toBe('navigation');
28+
expect(traceContext?.links).toBeUndefined();
2829

29-
// sanity check
30-
expect(pageloadTraceContext?.trace_id).toBe(metaTagTraceId);
31-
expect(pageloadTraceContext?.links).toBeUndefined();
30+
return traceContext;
31+
});
32+
33+
const navigationTraceContext = await sentryTest.step('Navigation', async () => {
34+
const navigationRequestPromise = waitForTransactionRequest(page, evt => evt.contexts?.trace?.op === 'navigation');
35+
await page.goto(`${url}#foo`);
36+
const navigationRequest = envelopeRequestParser(await navigationRequestPromise);
37+
return navigationRequest.contexts?.trace;
38+
});
39+
40+
const navigationTraceId = navigationTraceContext?.trace_id;
3241

3342
expect(navigationTraceContext?.links).toEqual([
3443
{

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

Lines changed: 25 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,8 @@
11
import { expect } from '@playwright/test';
2-
import { SEMANTIC_LINK_ATTRIBUTE_LINK_TYPE, type Event } from '@sentry/core';
2+
import { SEMANTIC_LINK_ATTRIBUTE_LINK_TYPE } from '@sentry/core';
33

44
import { sentryTest } from '../../../../../utils/fixtures';
5-
import { getFirstSentryEnvelopeRequest, shouldSkipTracingTest } from '../../../../../utils/helpers';
5+
import { envelopeRequestParser, shouldSkipTracingTest, waitForTransactionRequest } from '../../../../../utils/helpers';
66

77
sentryTest('includes a span link to a previously negatively sampled span', async ({ getLocalTestUrl, page }) => {
88
if (shouldSkipTracingTest()) {
@@ -11,26 +11,29 @@ sentryTest('includes a span link to a previously negatively sampled span', async
1111

1212
const url = await getLocalTestUrl({ testDir: __dirname });
1313

14-
await page.goto(url);
15-
16-
const navigationRequest = await getFirstSentryEnvelopeRequest<Event>(page, `${url}#foo`);
17-
18-
const navigationTraceContext = navigationRequest.contexts?.trace;
19-
20-
const navigationTraceId = navigationTraceContext?.trace_id;
21-
22-
expect(navigationTraceContext?.op).toBe('navigation');
23-
24-
expect(navigationTraceContext?.links).toEqual([
25-
{
26-
trace_id: expect.stringMatching(/[a-f0-9]{32}/),
27-
span_id: expect.stringMatching(/[a-f0-9]{16}/),
28-
sampled: false,
29-
attributes: {
30-
[SEMANTIC_LINK_ATTRIBUTE_LINK_TYPE]: 'previous_trace',
14+
await sentryTest.step('Initial pageload', async () => {
15+
// No event to check for an event here because this pageload span is sampled negatively!
16+
await page.goto(url);
17+
});
18+
19+
await sentryTest.step('Navigation', async () => {
20+
const navigationRequestPromise = waitForTransactionRequest(page, evt => evt.contexts?.trace?.op === 'navigation');
21+
await page.goto(`${url}#foo`);
22+
const navigationEvent = envelopeRequestParser(await navigationRequestPromise);
23+
const navigationTraceContext = navigationEvent.contexts?.trace;
24+
25+
expect(navigationTraceContext?.op).toBe('navigation');
26+
expect(navigationTraceContext?.links).toEqual([
27+
{
28+
trace_id: expect.stringMatching(/[a-f0-9]{32}/),
29+
span_id: expect.stringMatching(/[a-f0-9]{16}/),
30+
sampled: false,
31+
attributes: {
32+
[SEMANTIC_LINK_ATTRIBUTE_LINK_TYPE]: 'previous_trace',
33+
},
3134
},
32-
},
33-
]);
35+
]);
3436

35-
expect(navigationTraceId).not.toEqual(navigationTraceContext?.links![0].trace_id);
37+
expect(navigationTraceContext?.trace_id).not.toEqual(navigationTraceContext?.links![0].trace_id);
38+
});
3639
});

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

Lines changed: 30 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -1,42 +1,40 @@
11
import { expect } from '@playwright/test';
2-
import { SEMANTIC_LINK_ATTRIBUTE_LINK_TYPE, type Event } from '@sentry/core';
2+
import { SEMANTIC_LINK_ATTRIBUTE_LINK_TYPE } from '@sentry/core';
33

44
import { sentryTest } from '../../../../../utils/fixtures';
5-
import {
6-
envelopeRequestParser,
7-
getFirstSentryEnvelopeRequest,
8-
shouldSkipTracingTest,
9-
waitForTransactionRequest,
10-
} from '../../../../../utils/helpers';
5+
import { envelopeRequestParser, shouldSkipTracingTest, waitForTransactionRequest } from '../../../../../utils/helpers';
116

12-
sentryTest(
13-
'adds link between hard page reload traces when opting into sessionStorage',
14-
async ({ getLocalTestUrl, page }) => {
15-
if (shouldSkipTracingTest()) {
16-
sentryTest.skip();
17-
}
7+
sentryTest('adds link between hard page reloads when opting into sessionStorage', async ({ getLocalTestUrl, page }) => {
8+
if (shouldSkipTracingTest()) {
9+
sentryTest.skip();
10+
}
1811

19-
const url = await getLocalTestUrl({ testDir: __dirname });
20-
21-
const pageload1Event = await getFirstSentryEnvelopeRequest<Event>(page, url);
22-
23-
const pageload2RequestPromise = waitForTransactionRequest(page, evt => evt.contexts?.trace?.op === 'pageload');
24-
await page.reload();
25-
const pageload2Event = envelopeRequestParser(await pageload2RequestPromise);
12+
const url = await getLocalTestUrl({ testDir: __dirname });
2613

14+
const pageload1TraceContext = await sentryTest.step('First pageload', async () => {
15+
const pageloadRequestPromise = waitForTransactionRequest(page, evt => evt.contexts?.trace?.op === 'pageload');
16+
await page.goto(url);
17+
const pageload1Event = envelopeRequestParser(await pageloadRequestPromise);
2718
const pageload1TraceContext = pageload1Event.contexts?.trace;
2819
expect(pageload1TraceContext).toBeDefined();
2920
expect(pageload1TraceContext?.links).toBeUndefined();
21+
return pageload1TraceContext;
22+
});
3023

31-
expect(pageload2Event.contexts?.trace?.links).toEqual([
32-
{
33-
trace_id: pageload1TraceContext?.trace_id,
34-
span_id: pageload1TraceContext?.span_id,
35-
sampled: true,
36-
attributes: { [SEMANTIC_LINK_ATTRIBUTE_LINK_TYPE]: 'previous_trace' },
37-
},
38-
]);
39-
40-
expect(pageload1TraceContext?.trace_id).not.toEqual(pageload2Event.contexts?.trace?.trace_id);
41-
},
42-
);
24+
const pageload2Event = await sentryTest.step('Hard page reload', async () => {
25+
const pageload2RequestPromise = waitForTransactionRequest(page, evt => evt.contexts?.trace?.op === 'pageload');
26+
await page.reload();
27+
return envelopeRequestParser(await pageload2RequestPromise);
28+
});
29+
30+
expect(pageload2Event.contexts?.trace?.links).toEqual([
31+
{
32+
trace_id: pageload1TraceContext?.trace_id,
33+
span_id: pageload1TraceContext?.span_id,
34+
sampled: true,
35+
attributes: { [SEMANTIC_LINK_ATTRIBUTE_LINK_TYPE]: 'previous_trace' },
36+
},
37+
]);
38+
39+
expect(pageload1TraceContext?.trace_id).not.toEqual(pageload2Event.contexts?.trace?.trace_id);
40+
});

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

Lines changed: 16 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -385,6 +385,22 @@ export const browserTracingIntegration = ((_options: Partial<BrowserTracingOptio
385385
});
386386
});
387387

388+
if (enablePreviousTrace) {
389+
let previousTraceInfo = persistPreviousTrace ? getPreviousTraceFromSessionStorage() : undefined;
390+
391+
client.on('spanStart', span => {
392+
if (getRootSpan(span) !== span) {
393+
return;
394+
}
395+
396+
previousTraceInfo = addPreviousTraceSpanLink(previousTraceInfo, span);
397+
398+
if (persistPreviousTrace) {
399+
storePreviousTraceInSessionStorage(previousTraceInfo);
400+
}
401+
});
402+
}
403+
388404
if (WINDOW.location) {
389405
if (instrumentPageLoad) {
390406
const origin = browserPerformanceTimeOrigin();
@@ -449,22 +465,6 @@ export const browserTracingIntegration = ((_options: Partial<BrowserTracingOptio
449465
shouldCreateSpanForRequest,
450466
enableHTTPTimings,
451467
});
452-
453-
if (enablePreviousTrace) {
454-
let previousTraceInfo = persistPreviousTrace ? getPreviousTraceFromSessionStorage() : undefined;
455-
456-
client.on('spanStart', span => {
457-
if (getRootSpan(span) !== span) {
458-
return;
459-
}
460-
461-
previousTraceInfo = addPreviousTraceSpanLink(previousTraceInfo, span);
462-
463-
if (persistPreviousTrace) {
464-
storePreviousTraceInSessionStorage(previousTraceInfo);
465-
}
466-
});
467-
}
468468
},
469469
};
470470
}) satisfies IntegrationFn;

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

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,15 @@ export function addPreviousTraceSpanLink(
2929
previousTraceInfo: PreviousTraceInfo | undefined,
3030
span: Span,
3131
): PreviousTraceInfo {
32+
const spanJson = spanToJSON(span);
33+
3234
if (previousTraceInfo && Date.now() / 1000 - previousTraceInfo.startTimestamp <= PREVIOUS_TRACE_MAX_DURATION) {
35+
if (DEBUG_BUILD) {
36+
logger.info(
37+
`Adding previous_trace ${previousTraceInfo.spanContext} to span ${{ op: spanJson.op, ...span.spanContext() }}`,
38+
);
39+
}
40+
3341
span.addLink({
3442
context: previousTraceInfo.spanContext,
3543
attributes: {
@@ -61,7 +69,7 @@ export function storePreviousTraceInSessionStorage(previousTraceInfo: PreviousTr
6169
*/
6270
export function getPreviousTraceFromSessionStorage(): PreviousTraceInfo | undefined {
6371
try {
64-
const previousTraceInfo = WINDOW.sessionStorage.getItem(PREVIOUS_TRACE_KEY);
72+
const previousTraceInfo = WINDOW.sessionStorage?.getItem(PREVIOUS_TRACE_KEY);
6573
// @ts-expect-error - intentionally risking JSON.parse throwing when previousTraceInfo is null to save bundle size
6674
return JSON.parse(previousTraceInfo);
6775
} catch (e) {

‎packages/core/src/client.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -768,8 +768,8 @@ export abstract class Client<O extends ClientOptions = ClientOptions> {
768768
*/
769769
public emit(hook: string, ...rest: unknown[]): void {
770770
const callbacks = this._hooks[hook];
771-
if (callbacks) {
772-
callbacks.forEach(callback => callback(...rest));
771+
for (const callback of callbacks || []) {
772+
callback(...rest);
773773
}
774774
}
775775

0 commit comments

Comments
 (0)