Skip to content

Commit ab4ac07

Browse files
mydeamydea
authored
fix(inp): Ensure INP spans have correct transaction (#12871)
Fixes #12855 Previously, we stored the route name of the pageload in a map for INP interactions. However, in some frameworks - e.g. remix, but also others, we update the pageload span name later (since we rely on e.g. react hooks for this etc). Since we store the name of the pageload span at the time the first interaction is recorded, it can thus happen that these run out of sync. This PR changes this so that instead of the routename itself, we store the pageload span in a map, and pick the last name of this when generating the INP span. I added tests in a remix e2e tests that show the now correct behavior, these used to fail (because `transaction` on the pageload INP was `/`).
1 parent 4852dc7 commit ab4ac07

File tree

7 files changed

+239
-42
lines changed

7 files changed

+239
-42
lines changed

‎dev-packages/e2e-tests/test-applications/create-remix-app/app/entry.client.tsx

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ Sentry.init({
1919
replaysSessionSampleRate: 0.1, // This sets the sample rate at 10%. You may want to change it to 100% while in development and then sample at a lower rate in production.
2020
replaysOnErrorSampleRate: 1.0, // If you're not already sampling the entire session, change the sample rate to 100% when sampling sessions where errors occur.
2121
tunnel: 'http://localhost:3031/', // proxy server
22+
release: 'e2e-test',
2223
});
2324

2425
startTransition(() => {

‎dev-packages/e2e-tests/test-applications/create-remix-app/app/routes/user.$id.tsx

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,16 @@
11
export default function User() {
2-
return <div>I am a blank page</div>;
2+
return (
3+
<div>
4+
<div>I am a blank page</div>
5+
<button
6+
type="button"
7+
id="button"
8+
onClick={() => {
9+
console.log('Button clicked');
10+
}}
11+
>
12+
Click button
13+
</button>
14+
</div>
15+
);
316
}

‎dev-packages/e2e-tests/test-applications/create-remix-app/tests/client-inp.test.ts

Lines changed: 194 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,194 @@
1+
import { expect, test } from '@playwright/test';
2+
import { waitForEnvelopeItem, waitForTransaction } from '@sentry-internal/test-utils';
3+
4+
test('sends an INP span during pageload', async ({ page }) => {
5+
const inpSpanPromise = waitForEnvelopeItem('create-remix-app', item => {
6+
return item[0].type === 'span';
7+
});
8+
9+
await page.goto(`/`);
10+
11+
await page.click('#exception-button');
12+
13+
await page.waitForTimeout(500);
14+
15+
// Page hide to trigger INP
16+
await page.evaluate(() => {
17+
window.dispatchEvent(new Event('pagehide'));
18+
});
19+
20+
const inpSpan = await inpSpanPromise;
21+
22+
expect(inpSpan[1]).toEqual({
23+
data: {
24+
'sentry.origin': 'auto.http.browser.inp',
25+
'sentry.op': 'ui.interaction.click',
26+
release: 'e2e-test',
27+
environment: 'qa',
28+
transaction: 'routes/_index',
29+
'sentry.exclusive_time': expect.any(Number),
30+
'sentry.sample_rate': 1,
31+
'sentry.source': 'custom',
32+
replay_id: expect.any(String),
33+
},
34+
description: 'body > div > input#exception-button[type="button"]',
35+
op: 'ui.interaction.click',
36+
parent_span_id: expect.any(String),
37+
is_segment: true,
38+
span_id: expect.any(String),
39+
start_timestamp: expect.any(Number),
40+
timestamp: expect.any(Number),
41+
trace_id: expect.any(String),
42+
origin: 'auto.http.browser.inp',
43+
exclusive_time: expect.any(Number),
44+
measurements: { inp: { unit: 'millisecond', value: expect.any(Number) } },
45+
segment_id: expect.any(String),
46+
});
47+
});
48+
49+
test('sends an INP span after pageload', async ({ page }) => {
50+
const transactionPromise = waitForTransaction('create-remix-app', transactionEvent => {
51+
return transactionEvent.contexts?.trace?.op === 'pageload' && transactionEvent.transaction === 'routes/_index';
52+
});
53+
54+
await page.goto(`/`);
55+
56+
await transactionPromise;
57+
58+
const inpSpanPromise1 = waitForEnvelopeItem('create-remix-app', item => {
59+
return item[0].type === 'span';
60+
});
61+
62+
await page.click('#exception-button');
63+
64+
await page.waitForTimeout(500);
65+
66+
// Page hide to trigger INP
67+
await page.evaluate(() => {
68+
window.dispatchEvent(new Event('pagehide'));
69+
});
70+
71+
const inpSpan1 = await inpSpanPromise1;
72+
73+
expect(inpSpan1[1]).toEqual({
74+
data: {
75+
'sentry.origin': 'auto.http.browser.inp',
76+
'sentry.op': 'ui.interaction.click',
77+
release: 'e2e-test',
78+
environment: 'qa',
79+
transaction: 'routes/_index',
80+
'sentry.exclusive_time': expect.any(Number),
81+
'sentry.sample_rate': 1,
82+
'sentry.source': 'custom',
83+
replay_id: expect.any(String),
84+
},
85+
description: 'body > div > input#exception-button[type="button"]',
86+
op: 'ui.interaction.click',
87+
parent_span_id: expect.any(String),
88+
is_segment: true,
89+
span_id: expect.any(String),
90+
start_timestamp: expect.any(Number),
91+
timestamp: expect.any(Number),
92+
trace_id: expect.any(String),
93+
origin: 'auto.http.browser.inp',
94+
exclusive_time: expect.any(Number),
95+
measurements: { inp: { unit: 'millisecond', value: expect.any(Number) } },
96+
segment_id: expect.any(String),
97+
});
98+
});
99+
100+
test('sends an INP span during navigation', async ({ page }) => {
101+
page.on('console', msg => console.log(msg.text()));
102+
const inpSpanPromise = waitForEnvelopeItem('create-remix-app', item => {
103+
return item[0].type === 'span';
104+
});
105+
106+
await page.goto(`/`);
107+
108+
await page.click('#navigation');
109+
110+
await page.waitForTimeout(500);
111+
112+
// Page hide to trigger INP
113+
await page.evaluate(() => {
114+
window.dispatchEvent(new Event('pagehide'));
115+
});
116+
117+
const inpSpan = await inpSpanPromise;
118+
119+
expect(inpSpan[1]).toEqual({
120+
data: {
121+
'sentry.origin': 'auto.http.browser.inp',
122+
'sentry.op': 'ui.interaction.click',
123+
release: 'e2e-test',
124+
environment: 'qa',
125+
transaction: 'routes/user.$id',
126+
'sentry.exclusive_time': expect.any(Number),
127+
replay_id: expect.any(String),
128+
},
129+
description: '<unknown>',
130+
op: 'ui.interaction.click',
131+
parent_span_id: expect.any(String),
132+
span_id: expect.any(String),
133+
start_timestamp: expect.any(Number),
134+
timestamp: expect.any(Number),
135+
trace_id: expect.any(String),
136+
origin: 'auto.http.browser.inp',
137+
exclusive_time: expect.any(Number),
138+
measurements: { inp: { unit: 'millisecond', value: expect.any(Number) } },
139+
segment_id: expect.any(String),
140+
});
141+
});
142+
143+
test('sends an INP span after navigation', async ({ page }) => {
144+
page.on('console', msg => console.log(msg.text()));
145+
const transactionPromise = waitForTransaction('create-remix-app', transactionEvent => {
146+
return transactionEvent.contexts?.trace?.op === 'navigation' && transactionEvent.transaction === 'routes/user.$id';
147+
});
148+
149+
await page.goto(`/`);
150+
151+
await page.click('#navigation');
152+
153+
await transactionPromise;
154+
155+
const inpSpanPromise = waitForEnvelopeItem('create-remix-app', item => {
156+
return item[0].type === 'span';
157+
});
158+
159+
await page.click('#button');
160+
161+
await page.waitForTimeout(500);
162+
163+
// Page hide to trigger INP
164+
await page.evaluate(() => {
165+
window.dispatchEvent(new Event('pagehide'));
166+
});
167+
168+
const inpSpan = await inpSpanPromise;
169+
170+
expect(inpSpan[1]).toEqual({
171+
data: {
172+
'sentry.origin': 'auto.http.browser.inp',
173+
'sentry.op': 'ui.interaction.click',
174+
release: 'e2e-test',
175+
environment: 'qa',
176+
transaction: 'routes/user.$id',
177+
'sentry.exclusive_time': expect.any(Number),
178+
replay_id: expect.any(String),
179+
'sentry.sample_rate': 1,
180+
'sentry.source': 'custom',
181+
},
182+
description: '<unknown>',
183+
op: 'ui.interaction.click',
184+
is_segment: true,
185+
span_id: expect.any(String),
186+
start_timestamp: expect.any(Number),
187+
timestamp: expect.any(Number),
188+
trace_id: expect.any(String),
189+
origin: 'auto.http.browser.inp',
190+
exclusive_time: expect.any(Number),
191+
measurements: { inp: { unit: 'millisecond', value: expect.any(Number) } },
192+
segment_id: expect.any(String),
193+
});
194+
});

‎packages/browser-utils/src/metrics/inp.ts

Lines changed: 20 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@ import {
1010
spanToJSON,
1111
startInactiveSpan,
1212
} from '@sentry/core';
13-
import type { Integration, SpanAttributes } from '@sentry/types';
13+
import type { Integration, Span, SpanAttributes } from '@sentry/types';
1414
import { browserPerformanceTimeOrigin, dropUndefinedKeys, htmlTreeAsString } from '@sentry/utils';
1515
import {
1616
addInpInstrumentationHandler,
@@ -19,13 +19,8 @@ import {
1919
} from './instrument';
2020
import { getBrowserPerformanceAPI, msToSec } from './utils';
2121

22-
// We only care about name here
23-
interface PartialRouteInfo {
24-
name: string | undefined;
25-
}
26-
2722
const LAST_INTERACTIONS: number[] = [];
28-
const INTERACTIONS_ROUTE_MAP = new Map<number, string>();
23+
const INTERACTIONS_SPAN_MAP = new Map<number, Span>();
2924

3025
/**
3126
* Start tracking INP webvital events.
@@ -97,14 +92,15 @@ function _trackINP(): () => void {
9792
const activeSpan = getActiveSpan();
9893
const rootSpan = activeSpan ? getRootSpan(activeSpan) : undefined;
9994

100-
// We first try to lookup the route name from our INTERACTIONS_ROUTE_MAP,
95+
// We first try to lookup the span from our INTERACTIONS_SPAN_MAP,
10196
// where we cache the route per interactionId
102-
const cachedRouteName = interactionId != null ? INTERACTIONS_ROUTE_MAP.get(interactionId) : undefined;
97+
const cachedSpan = interactionId != null ? INTERACTIONS_SPAN_MAP.get(interactionId) : undefined;
98+
99+
const spanToUse = cachedSpan || rootSpan;
103100

104101
// Else, we try to use the active span.
105102
// Finally, we fall back to look at the transactionName on the scope
106-
const routeName =
107-
cachedRouteName || (rootSpan ? spanToJSON(rootSpan).description : scope.getScopeData().transactionName);
103+
const routeName = spanToUse ? spanToJSON(spanToUse).description : scope.getScopeData().transactionName;
108104

109105
const user = scope.getUser();
110106

@@ -154,11 +150,17 @@ function _trackINP(): () => void {
154150
});
155151
}
156152

157-
/** Register a listener to cache route information for INP interactions. */
158-
export function registerInpInteractionListener(latestRoute: PartialRouteInfo): void {
153+
/**
154+
* Register a listener to cache route information for INP interactions.
155+
* TODO(v9): `latestRoute` no longer needs to be passed in and will be removed in v9.
156+
*/
157+
export function registerInpInteractionListener(_latestRoute?: unknown): void {
159158
const handleEntries = ({ entries }: { entries: PerformanceEntry[] }): void => {
159+
const activeSpan = getActiveSpan();
160+
const activeRootSpan = activeSpan && getRootSpan(activeSpan);
161+
160162
entries.forEach(entry => {
161-
if (!isPerformanceEventTiming(entry) || !latestRoute.name) {
163+
if (!isPerformanceEventTiming(entry) || !activeRootSpan) {
162164
return;
163165
}
164166

@@ -168,21 +170,20 @@ export function registerInpInteractionListener(latestRoute: PartialRouteInfo): v
168170
}
169171

170172
// If the interaction was already recorded before, nothing more to do
171-
if (INTERACTIONS_ROUTE_MAP.has(interactionId)) {
173+
if (INTERACTIONS_SPAN_MAP.has(interactionId)) {
172174
return;
173175
}
174176

175177
// We keep max. 10 interactions in the list, then remove the oldest one & clean up
176178
if (LAST_INTERACTIONS.length > 10) {
177179
const last = LAST_INTERACTIONS.shift() as number;
178-
INTERACTIONS_ROUTE_MAP.delete(last);
180+
INTERACTIONS_SPAN_MAP.delete(last);
179181
}
180182

181183
// We add the interaction to the list of recorded interactions
182-
// and store the route information for this interaction
183-
// (we clone the object because it is mutated when it changes)
184+
// and store the span for this interaction
184185
LAST_INTERACTIONS.push(interactionId);
185-
INTERACTIONS_ROUTE_MAP.set(interactionId, latestRoute.name);
186+
INTERACTIONS_SPAN_MAP.set(interactionId, activeRootSpan);
186187
});
187188
};
188189

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -395,7 +395,7 @@ export const browserTracingIntegration = ((_options: Partial<BrowserTracingOptio
395395
}
396396

397397
if (enableInp) {
398-
registerInpInteractionListener(latestRoute);
398+
registerInpInteractionListener();
399399
}
400400

401401
instrumentOutgoingRequests({

‎packages/remix/src/client/browserTracingIntegration.ts

Lines changed: 7 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -7,19 +7,13 @@ import type { RemixBrowserTracingIntegrationOptions } from './performance';
77
* This integration will create pageload and navigation spans.
88
*/
99
export function browserTracingIntegration(options: RemixBrowserTracingIntegrationOptions): Integration {
10-
if (options.instrumentPageLoad === undefined) {
11-
options.instrumentPageLoad = true;
12-
}
13-
14-
if (options.instrumentNavigation === undefined) {
15-
options.instrumentNavigation = true;
16-
}
10+
const { instrumentPageLoad = true, instrumentNavigation = true, useEffect, useLocation, useMatches } = options;
1711

1812
setGlobals({
19-
useEffect: options.useEffect,
20-
useLocation: options.useLocation,
21-
useMatches: options.useMatches,
22-
instrumentNavigation: options.instrumentNavigation,
13+
useEffect,
14+
useLocation,
15+
useMatches,
16+
instrumentNavigation,
2317
});
2418

2519
const browserTracingIntegrationInstance = originalBrowserTracingIntegration({
@@ -33,8 +27,8 @@ export function browserTracingIntegration(options: RemixBrowserTracingIntegratio
3327
afterAllSetup(client) {
3428
browserTracingIntegrationInstance.afterAllSetup(client);
3529

36-
if (options.instrumentPageLoad) {
37-
startPageloadSpan();
30+
if (instrumentPageLoad) {
31+
startPageloadSpan(client);
3832
}
3933
},
4034
};

‎packages/remix/src/client/performance.tsx

Lines changed: 2 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@ import {
1414
startBrowserTracingPageLoadSpan,
1515
withErrorBoundary,
1616
} from '@sentry/react';
17-
import type { StartSpanOptions } from '@sentry/types';
17+
import type { Client, StartSpanOptions } from '@sentry/types';
1818
import { isNodeEnv, logger } from '@sentry/utils';
1919
import * as React from 'react';
2020

@@ -67,7 +67,7 @@ function isRemixV2(remixVersion: number | undefined): boolean {
6767
return remixVersion === 2 || getFutureFlagsBrowser()?.v2_errorBoundary || false;
6868
}
6969

70-
export function startPageloadSpan(): void {
70+
export function startPageloadSpan(client: Client): void {
7171
const initPathName = getInitPathName();
7272

7373
if (!initPathName) {
@@ -83,12 +83,6 @@ export function startPageloadSpan(): void {
8383
},
8484
};
8585

86-
const client = getClient<BrowserClient>();
87-
88-
if (!client) {
89-
return;
90-
}
91-
9286
startBrowserTracingPageLoadSpan(client, spanContext);
9387
}
9488

0 commit comments

Comments
 (0)