Skip to content

Commit 724ec92

Browse files
mydeamydea
authored
fix(browser): Ensure pageload & navigation spans have correct data (#16279)
This PR fixes two things about our idle spans emitted from `browserTracingIntegration`: 1. The navigation name was sometimes incorrect. This is because we look at `window.location.pathname` at the time when the `popstate` event is emitted - but at this point, this may not be updated yet. So a `navigation` transaction would possibly have the pathname of the previous page as transaction name. 2. The request data is also possibly incorrect - this is set by `HttpContext` integration at event processing time. However, at this time the `window.location` data is also usually already of the following navigation, so the pageload would often have wrong request data associated to it. Now, we store this on the current scope at span creation time to ensure it is actually correct.
1 parent 4b039f8 commit 724ec92

File tree

11 files changed

+193
-33
lines changed

11 files changed

+193
-33
lines changed

‎dev-packages/browser-integration-tests/suites/integrations/Breadcrumbs/history/navigation/test.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -29,14 +29,14 @@ sentryTest('should record history changes as navigation breadcrumbs', async ({ g
2929
category: 'navigation',
3030
data: {
3131
from: '/bar?a=1#fragment',
32-
to: '[object Object]',
32+
to: '/[object%20Object]',
3333
},
3434
timestamp: expect.any(Number),
3535
},
3636
{
3737
category: 'navigation',
3838
data: {
39-
from: '[object Object]',
39+
from: '/[object%20Object]',
4040
to: '/bar?a=1#fragment',
4141
},
4242
timestamp: expect.any(Number),

‎dev-packages/browser-integration-tests/suites/replay/multiple-pages/test.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -210,7 +210,7 @@ sentryTest(
210210
expect(replayEvent6).toEqual(
211211
getExpectedReplayEvent({
212212
segment_id: 6,
213-
urls: ['/spa'],
213+
urls: [`${TEST_HOST}/spa`],
214214
request: {
215215
url: `${TEST_HOST}/spa`,
216216
headers: {

‎dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/navigation-aborting-pageload/init.js‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,4 +9,4 @@ Sentry.init({
99
});
1010

1111
// Immediately navigate to a new page to abort the pageload
12-
window.location.href = '#foo';
12+
window.history.pushState({}, '', '/sub-page');

‎dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/navigation-aborting-pageload/test.ts‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,9 @@ sentryTest(
4040
expect(navigationTraceId).toBeDefined();
4141
expect(pageloadTraceId).not.toEqual(navigationTraceId);
4242

43+
expect(pageloadRequest.transaction).toEqual('/index.html');
44+
expect(navigationRequest.transaction).toEqual('/sub-page');
45+
4346
expect(pageloadRequest.contexts?.trace?.data).toMatchObject({
4447
[SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.pageload.browser',
4548
[SEMANTIC_ATTRIBUTE_SENTRY_SAMPLE_RATE]: 1,
@@ -54,5 +57,17 @@ sentryTest(
5457
[SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'navigation',
5558
['sentry.idle_span_finish_reason']: 'idleTimeout',
5659
});
60+
expect(pageloadRequest.request).toEqual({
61+
headers: {
62+
'User-Agent': expect.any(String),
63+
},
64+
url: 'http://sentry-test.io/index.html',
65+
});
66+
expect(navigationRequest.request).toEqual({
67+
headers: {
68+
'User-Agent': expect.any(String),
69+
},
70+
url: 'http://sentry-test.io/sub-page',
71+
});
5772
},
5873
);

‎dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/navigation/test.ts‎

Lines changed: 84 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,12 @@ import {
77
SEMANTIC_ATTRIBUTE_SENTRY_SOURCE,
88
} from '@sentry/core';
99
import { sentryTest } from '../../../../utils/fixtures';
10-
import { getFirstSentryEnvelopeRequest, shouldSkipTracingTest } from '../../../../utils/helpers';
10+
import {
11+
envelopeRequestParser,
12+
getFirstSentryEnvelopeRequest,
13+
shouldSkipTracingTest,
14+
waitForTransactionRequest,
15+
} from '../../../../utils/helpers';
1116

1217
sentryTest('should create a navigation transaction on page navigation', async ({ getLocalTestUrl, page }) => {
1318
if (shouldSkipTracingTest()) {
@@ -31,6 +36,10 @@ sentryTest('should create a navigation transaction on page navigation', async ({
3136
expect(navigationTraceId).toBeDefined();
3237
expect(pageloadTraceId).not.toEqual(navigationTraceId);
3338

39+
expect(pageloadRequest.transaction).toEqual('/index.html');
40+
// Fragment is not in transaction name
41+
expect(navigationRequest.transaction).toEqual('/index.html');
42+
3443
expect(pageloadRequest.contexts?.trace?.data).toMatchObject({
3544
[SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.pageload.browser',
3645
[SEMANTIC_ATTRIBUTE_SENTRY_SAMPLE_RATE]: 1,
@@ -45,6 +54,18 @@ sentryTest('should create a navigation transaction on page navigation', async ({
4554
[SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'navigation',
4655
['sentry.idle_span_finish_reason']: 'idleTimeout',
4756
});
57+
expect(pageloadRequest.request).toEqual({
58+
headers: {
59+
'User-Agent': expect.any(String),
60+
},
61+
url: 'http://sentry-test.io/index.html',
62+
});
63+
expect(navigationRequest.request).toEqual({
64+
headers: {
65+
'User-Agent': expect.any(String),
66+
},
67+
url: 'http://sentry-test.io/index.html#foo',
68+
});
4869

4970
const pageloadSpans = pageloadRequest.spans;
5071
const navigationSpans = navigationRequest.spans;
@@ -69,3 +90,65 @@ sentryTest('should create a navigation transaction on page navigation', async ({
6990

7091
expect(pageloadSpanId).not.toEqual(navigationSpanId);
7192
});
93+
94+
//
95+
sentryTest('should handle pushState with full URL', async ({ getLocalTestUrl, page }) => {
96+
if (shouldSkipTracingTest()) {
97+
sentryTest.skip();
98+
}
99+
100+
const url = await getLocalTestUrl({ testDir: __dirname });
101+
102+
const pageloadRequestPromise = waitForTransactionRequest(page, event => event.contexts?.trace?.op === 'pageload');
103+
const navigationRequestPromise = waitForTransactionRequest(
104+
page,
105+
event => event.contexts?.trace?.op === 'navigation' && event.transaction === '/sub-page',
106+
);
107+
const navigationRequestPromise2 = waitForTransactionRequest(
108+
page,
109+
event => event.contexts?.trace?.op === 'navigation' && event.transaction === '/sub-page-2',
110+
);
111+
112+
await page.goto(url);
113+
await pageloadRequestPromise;
114+
115+
await page.evaluate("window.history.pushState({}, '', `${window.location.origin}/sub-page`);");
116+
117+
const navigationRequest = envelopeRequestParser(await navigationRequestPromise);
118+
119+
expect(navigationRequest.transaction).toEqual('/sub-page');
120+
121+
expect(navigationRequest.contexts?.trace?.data).toMatchObject({
122+
[SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.navigation.browser',
123+
[SEMANTIC_ATTRIBUTE_SENTRY_SAMPLE_RATE]: 1,
124+
[SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'url',
125+
[SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'navigation',
126+
['sentry.idle_span_finish_reason']: 'idleTimeout',
127+
});
128+
expect(navigationRequest.request).toEqual({
129+
headers: {
130+
'User-Agent': expect.any(String),
131+
},
132+
url: 'http://sentry-test.io/sub-page',
133+
});
134+
135+
await page.evaluate("window.history.pushState({}, '', `${window.location.origin}/sub-page-2`);");
136+
137+
const navigationRequest2 = envelopeRequestParser(await navigationRequestPromise2);
138+
139+
expect(navigationRequest2.transaction).toEqual('/sub-page-2');
140+
141+
expect(navigationRequest2.contexts?.trace?.data).toMatchObject({
142+
[SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.navigation.browser',
143+
[SEMANTIC_ATTRIBUTE_SENTRY_SAMPLE_RATE]: 1,
144+
[SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'url',
145+
[SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'navigation',
146+
['sentry.idle_span_finish_reason']: 'idleTimeout',
147+
});
148+
expect(navigationRequest2.request).toEqual({
149+
headers: {
150+
'User-Agent': expect.any(String),
151+
},
152+
url: 'http://sentry-test.io/sub-page-2',
153+
});
154+
});

‎packages/browser-utils/src/instrument/history.ts‎

Lines changed: 18 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -47,9 +47,15 @@ export function instrumentHistory(): void {
4747
return function (this: History, ...args: unknown[]): void {
4848
const url = args.length > 2 ? args[2] : undefined;
4949
if (url) {
50-
// coerce to string (this is what pushState does)
5150
const from = lastHref;
52-
const to = String(url);
51+
52+
// Ensure the URL is absolute
53+
// this can be either a path, then it is relative to the current origin
54+
// or a full URL of the current origin - other origins are not allowed
55+
// See: https://developer.mozilla.org/en-US/docs/Web/API/History/pushState#url
56+
// coerce to string (this is what pushState does)
57+
const to = getAbsoluteUrl(String(url));
58+
5359
// keep track of the current URL state, as we always receive only the updated state
5460
lastHref = to;
5561

@@ -67,3 +73,13 @@ export function instrumentHistory(): void {
6773
fill(WINDOW.history, 'pushState', historyReplacementFunction);
6874
fill(WINDOW.history, 'replaceState', historyReplacementFunction);
6975
}
76+
77+
function getAbsoluteUrl(urlOrPath: string): string {
78+
try {
79+
const url = new URL(urlOrPath, WINDOW.location.origin);
80+
return url.toString();
81+
} catch {
82+
// fallback, just do nothing
83+
return urlOrPath;
84+
}
85+
}

‎packages/browser/src/helpers.ts‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import {
44
addExceptionTypeValue,
55
addNonEnumerableProperty,
66
captureException,
7+
getLocationHref,
78
getOriginalFunction,
89
GLOBAL_OBJ,
910
markFunctionWrapped,
@@ -175,3 +176,24 @@ export function wrap<T extends WrappableFunction, NonFunction>(
175176

176177
return sentryWrapped;
177178
}
179+
180+
/**
181+
* Get HTTP request data from the current page.
182+
*/
183+
export function getHttpRequestData(): { url: string; headers: Record<string, string> } {
184+
// grab as much info as exists and add it to the event
185+
const url = getLocationHref();
186+
const { referrer } = WINDOW.document || {};
187+
const { userAgent } = WINDOW.navigator || {};
188+
189+
const headers = {
190+
...(referrer && { Referer: referrer }),
191+
...(userAgent && { 'User-Agent': userAgent }),
192+
};
193+
const request = {
194+
url,
195+
headers,
196+
};
197+
198+
return request;
199+
}

‎packages/browser/src/integrations/httpcontext.ts‎

Lines changed: 7 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
1-
import { defineIntegration, getLocationHref } from '@sentry/core';
2-
import { WINDOW } from '../helpers';
1+
import { defineIntegration } from '@sentry/core';
2+
import { getHttpRequestData, WINDOW } from '../helpers';
33

44
/**
55
* Collects information about HTTP request headers and
@@ -14,23 +14,17 @@ export const httpContextIntegration = defineIntegration(() => {
1414
return;
1515
}
1616

17-
// grab as much info as exists and add it to the event
18-
const url = event.request?.url || getLocationHref();
19-
const { referrer } = WINDOW.document || {};
20-
const { userAgent } = WINDOW.navigator || {};
21-
17+
const reqData = getHttpRequestData();
2218
const headers = {
19+
...reqData.headers,
2320
...event.request?.headers,
24-
...(referrer && { Referer: referrer }),
25-
...(userAgent && { 'User-Agent': userAgent }),
2621
};
27-
const request = {
22+
23+
event.request = {
24+
...reqData,
2825
...event.request,
29-
...(url && { url }),
3026
headers,
3127
};
32-
33-
event.request = request;
3428
},
3529
};
3630
});

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

Lines changed: 38 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ import {
1212
getLocationHref,
1313
GLOBAL_OBJ,
1414
logger,
15+
parseStringToURLObject,
1516
propagationContextFromHeaders,
1617
registerSpanErrorInstrumentation,
1718
SEMANTIC_ATTRIBUTE_SENTRY_IDLE_SPAN_FINISH_REASON,
@@ -33,7 +34,7 @@ import {
3334
startTrackingWebVitals,
3435
} from '@sentry-internal/browser-utils';
3536
import { DEBUG_BUILD } from '../debug-build';
36-
import { WINDOW } from '../helpers';
37+
import { getHttpRequestData, WINDOW } from '../helpers';
3738
import { registerBackgroundTabDetection } from './backgroundtab';
3839
import { linkTraces } from './linkedTraces';
3940
import { defaultRequestInstrumentationOptions, instrumentOutgoingRequests } from './request';
@@ -399,7 +400,14 @@ export const browserTracingIntegration = ((_options: Partial<BrowserTracingOptio
399400
maybeEndActiveSpan();
400401

401402
getIsolationScope().setPropagationContext({ traceId: generateTraceId(), sampleRand: Math.random() });
402-
getCurrentScope().setPropagationContext({ traceId: generateTraceId(), sampleRand: Math.random() });
403+
404+
const scope = getCurrentScope();
405+
scope.setPropagationContext({ traceId: generateTraceId(), sampleRand: Math.random() });
406+
// We reset this to ensure we do not have lingering incorrect data here
407+
// places that call this hook may set this where appropriate - else, the URL at span sending time is used
408+
scope.setSDKProcessingMetadata({
409+
normalizedRequest: undefined,
410+
});
403411

404412
_createRouteSpan(client, {
405413
op: 'navigation',
@@ -417,7 +425,15 @@ export const browserTracingIntegration = ((_options: Partial<BrowserTracingOptio
417425
const baggage = traceOptions.baggage || getMetaContent('baggage');
418426

419427
const propagationContext = propagationContextFromHeaders(sentryTrace, baggage);
420-
getCurrentScope().setPropagationContext(propagationContext);
428+
429+
const scope = getCurrentScope();
430+
scope.setPropagationContext(propagationContext);
431+
432+
// We store the normalized request data on the scope, so we get the request data at time of span creation
433+
// otherwise, the URL etc. may already be of the following navigation, and we'd report the wrong URL
434+
scope.setSDKProcessingMetadata({
435+
normalizedRequest: getHttpRequestData(),
436+
});
421437

422438
_createRouteSpan(client, {
423439
op: 'pageload',
@@ -459,16 +475,25 @@ export const browserTracingIntegration = ((_options: Partial<BrowserTracingOptio
459475
return;
460476
}
461477

462-
if (from !== to) {
463-
startingUrl = undefined;
464-
startBrowserTracingNavigationSpan(client, {
465-
name: WINDOW.location.pathname,
466-
attributes: {
467-
[SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'url',
468-
[SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.navigation.browser',
469-
},
470-
});
471-
}
478+
startingUrl = undefined;
479+
const parsed = parseStringToURLObject(to);
480+
startBrowserTracingNavigationSpan(client, {
481+
name: parsed?.pathname || WINDOW.location.pathname,
482+
attributes: {
483+
[SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'url',
484+
[SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.navigation.browser',
485+
},
486+
});
487+
488+
// We store the normalized request data on the scope, so we get the request data at time of span creation
489+
// otherwise, the URL etc. may already be of the following navigation, and we'd report the wrong URL
490+
getCurrentScope().setSDKProcessingMetadata({
491+
normalizedRequest: {
492+
...getHttpRequestData(),
493+
// Ensure to set this, so this matches the target route even if the URL has not yet been updated
494+
url: to,
495+
},
496+
});
472497
});
473498
}
474499
}

‎packages/core/src/tracing/sentrySpan.ts‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -336,6 +336,8 @@ export class SentrySpan implements Span {
336336

337337
const { scope: capturedSpanScope, isolationScope: capturedSpanIsolationScope } = getCapturedScopesOnSpan(this);
338338

339+
const normalizedRequest = capturedSpanScope?.getScopeData().sdkProcessingMetadata?.normalizedRequest;
340+
339341
if (this._sampled !== true) {
340342
return undefined;
341343
}
@@ -374,6 +376,7 @@ export class SentrySpan implements Span {
374376
capturedSpanIsolationScope,
375377
dynamicSamplingContext: getDynamicSamplingContextFromSpan(this),
376378
},
379+
request: normalizedRequest,
377380
...(source && {
378381
transaction_info: {
379382
source,

0 commit comments

Comments
 (0)