From 7674495dfff4f44ca27419c964cc006c38c1ec3d Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Thu, 11 Jul 2024 09:30:00 +0200 Subject: [PATCH] fix(inp): Ensure INP spans have correct transaction --- .../create-remix-app/app/entry.client.tsx | 1 + .../create-remix-app/app/routes/user.$id.tsx | 15 +- .../create-remix-app/tests/client-inp.test.ts | 194 ++++++++++++++++++ packages/browser-utils/src/metrics/inp.ts | 39 ++-- .../src/tracing/browserTracingIntegration.ts | 2 +- .../src/client/browserTracingIntegration.ts | 20 +- packages/remix/src/client/performance.tsx | 10 +- 7 files changed, 239 insertions(+), 42 deletions(-) create mode 100644 dev-packages/e2e-tests/test-applications/create-remix-app/tests/client-inp.test.ts diff --git a/dev-packages/e2e-tests/test-applications/create-remix-app/app/entry.client.tsx b/dev-packages/e2e-tests/test-applications/create-remix-app/app/entry.client.tsx index d0c95287e0c9..6a1a4b4c427e 100644 --- a/dev-packages/e2e-tests/test-applications/create-remix-app/app/entry.client.tsx +++ b/dev-packages/e2e-tests/test-applications/create-remix-app/app/entry.client.tsx @@ -19,6 +19,7 @@ Sentry.init({ 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. replaysOnErrorSampleRate: 1.0, // If you're not already sampling the entire session, change the sample rate to 100% when sampling sessions where errors occur. tunnel: 'http://localhost:3031/', // proxy server + release: 'e2e-test', }); startTransition(() => { diff --git a/dev-packages/e2e-tests/test-applications/create-remix-app/app/routes/user.$id.tsx b/dev-packages/e2e-tests/test-applications/create-remix-app/app/routes/user.$id.tsx index 13b2e0a34d1e..ecbeb440219e 100644 --- a/dev-packages/e2e-tests/test-applications/create-remix-app/app/routes/user.$id.tsx +++ b/dev-packages/e2e-tests/test-applications/create-remix-app/app/routes/user.$id.tsx @@ -1,3 +1,16 @@ export default function User() { - return
I am a blank page
; + return ( +
+
I am a blank page
+ +
+ ); } diff --git a/dev-packages/e2e-tests/test-applications/create-remix-app/tests/client-inp.test.ts b/dev-packages/e2e-tests/test-applications/create-remix-app/tests/client-inp.test.ts new file mode 100644 index 000000000000..9469a4462563 --- /dev/null +++ b/dev-packages/e2e-tests/test-applications/create-remix-app/tests/client-inp.test.ts @@ -0,0 +1,194 @@ +import { expect, test } from '@playwright/test'; +import { waitForEnvelopeItem, waitForTransaction } from '@sentry-internal/test-utils'; + +test('sends an INP span during pageload', async ({ page }) => { + const inpSpanPromise = waitForEnvelopeItem('create-remix-app', item => { + return item[0].type === 'span'; + }); + + await page.goto(`/`); + + await page.click('#exception-button'); + + await page.waitForTimeout(500); + + // Page hide to trigger INP + await page.evaluate(() => { + window.dispatchEvent(new Event('pagehide')); + }); + + const inpSpan = await inpSpanPromise; + + expect(inpSpan[1]).toEqual({ + data: { + 'sentry.origin': 'auto.http.browser.inp', + 'sentry.op': 'ui.interaction.click', + release: 'e2e-test', + environment: 'qa', + transaction: 'routes/_index', + 'sentry.exclusive_time': expect.any(Number), + 'sentry.sample_rate': 1, + 'sentry.source': 'custom', + replay_id: expect.any(String), + }, + description: 'body > div > input#exception-button[type="button"]', + op: 'ui.interaction.click', + parent_span_id: expect.any(String), + is_segment: true, + span_id: expect.any(String), + start_timestamp: expect.any(Number), + timestamp: expect.any(Number), + trace_id: expect.any(String), + origin: 'auto.http.browser.inp', + exclusive_time: expect.any(Number), + measurements: { inp: { unit: 'millisecond', value: expect.any(Number) } }, + segment_id: expect.any(String), + }); +}); + +test('sends an INP span after pageload', async ({ page }) => { + const transactionPromise = waitForTransaction('create-remix-app', transactionEvent => { + return transactionEvent.contexts?.trace?.op === 'pageload' && transactionEvent.transaction === 'routes/_index'; + }); + + await page.goto(`/`); + + await transactionPromise; + + const inpSpanPromise1 = waitForEnvelopeItem('create-remix-app', item => { + return item[0].type === 'span'; + }); + + await page.click('#exception-button'); + + await page.waitForTimeout(500); + + // Page hide to trigger INP + await page.evaluate(() => { + window.dispatchEvent(new Event('pagehide')); + }); + + const inpSpan1 = await inpSpanPromise1; + + expect(inpSpan1[1]).toEqual({ + data: { + 'sentry.origin': 'auto.http.browser.inp', + 'sentry.op': 'ui.interaction.click', + release: 'e2e-test', + environment: 'qa', + transaction: 'routes/_index', + 'sentry.exclusive_time': expect.any(Number), + 'sentry.sample_rate': 1, + 'sentry.source': 'custom', + replay_id: expect.any(String), + }, + description: 'body > div > input#exception-button[type="button"]', + op: 'ui.interaction.click', + parent_span_id: expect.any(String), + is_segment: true, + span_id: expect.any(String), + start_timestamp: expect.any(Number), + timestamp: expect.any(Number), + trace_id: expect.any(String), + origin: 'auto.http.browser.inp', + exclusive_time: expect.any(Number), + measurements: { inp: { unit: 'millisecond', value: expect.any(Number) } }, + segment_id: expect.any(String), + }); +}); + +test('sends an INP span during navigation', async ({ page }) => { + page.on('console', msg => console.log(msg.text())); + const inpSpanPromise = waitForEnvelopeItem('create-remix-app', item => { + return item[0].type === 'span'; + }); + + await page.goto(`/`); + + await page.click('#navigation'); + + await page.waitForTimeout(500); + + // Page hide to trigger INP + await page.evaluate(() => { + window.dispatchEvent(new Event('pagehide')); + }); + + const inpSpan = await inpSpanPromise; + + expect(inpSpan[1]).toEqual({ + data: { + 'sentry.origin': 'auto.http.browser.inp', + 'sentry.op': 'ui.interaction.click', + release: 'e2e-test', + environment: 'qa', + transaction: 'routes/user.$id', + 'sentry.exclusive_time': expect.any(Number), + replay_id: expect.any(String), + }, + description: '', + op: 'ui.interaction.click', + parent_span_id: expect.any(String), + span_id: expect.any(String), + start_timestamp: expect.any(Number), + timestamp: expect.any(Number), + trace_id: expect.any(String), + origin: 'auto.http.browser.inp', + exclusive_time: expect.any(Number), + measurements: { inp: { unit: 'millisecond', value: expect.any(Number) } }, + segment_id: expect.any(String), + }); +}); + +test('sends an INP span after navigation', async ({ page }) => { + page.on('console', msg => console.log(msg.text())); + const transactionPromise = waitForTransaction('create-remix-app', transactionEvent => { + return transactionEvent.contexts?.trace?.op === 'navigation' && transactionEvent.transaction === 'routes/user.$id'; + }); + + await page.goto(`/`); + + await page.click('#navigation'); + + await transactionPromise; + + const inpSpanPromise = waitForEnvelopeItem('create-remix-app', item => { + return item[0].type === 'span'; + }); + + await page.click('#button'); + + await page.waitForTimeout(500); + + // Page hide to trigger INP + await page.evaluate(() => { + window.dispatchEvent(new Event('pagehide')); + }); + + const inpSpan = await inpSpanPromise; + + expect(inpSpan[1]).toEqual({ + data: { + 'sentry.origin': 'auto.http.browser.inp', + 'sentry.op': 'ui.interaction.click', + release: 'e2e-test', + environment: 'qa', + transaction: 'routes/user.$id', + 'sentry.exclusive_time': expect.any(Number), + replay_id: expect.any(String), + 'sentry.sample_rate': 1, + 'sentry.source': 'custom', + }, + description: '', + op: 'ui.interaction.click', + is_segment: true, + span_id: expect.any(String), + start_timestamp: expect.any(Number), + timestamp: expect.any(Number), + trace_id: expect.any(String), + origin: 'auto.http.browser.inp', + exclusive_time: expect.any(Number), + measurements: { inp: { unit: 'millisecond', value: expect.any(Number) } }, + segment_id: expect.any(String), + }); +}); diff --git a/packages/browser-utils/src/metrics/inp.ts b/packages/browser-utils/src/metrics/inp.ts index 00e524c048b6..1055635bc32f 100644 --- a/packages/browser-utils/src/metrics/inp.ts +++ b/packages/browser-utils/src/metrics/inp.ts @@ -10,7 +10,7 @@ import { spanToJSON, startInactiveSpan, } from '@sentry/core'; -import type { Integration, SpanAttributes } from '@sentry/types'; +import type { Integration, Span, SpanAttributes } from '@sentry/types'; import { browserPerformanceTimeOrigin, dropUndefinedKeys, htmlTreeAsString } from '@sentry/utils'; import { addInpInstrumentationHandler, @@ -19,13 +19,8 @@ import { } from './instrument'; import { getBrowserPerformanceAPI, msToSec } from './utils'; -// We only care about name here -interface PartialRouteInfo { - name: string | undefined; -} - const LAST_INTERACTIONS: number[] = []; -const INTERACTIONS_ROUTE_MAP = new Map(); +const INTERACTIONS_SPAN_MAP = new Map(); /** * Start tracking INP webvital events. @@ -97,14 +92,15 @@ function _trackINP(): () => void { const activeSpan = getActiveSpan(); const rootSpan = activeSpan ? getRootSpan(activeSpan) : undefined; - // We first try to lookup the route name from our INTERACTIONS_ROUTE_MAP, + // We first try to lookup the span from our INTERACTIONS_SPAN_MAP, // where we cache the route per interactionId - const cachedRouteName = interactionId != null ? INTERACTIONS_ROUTE_MAP.get(interactionId) : undefined; + const cachedSpan = interactionId != null ? INTERACTIONS_SPAN_MAP.get(interactionId) : undefined; + + const spanToUse = cachedSpan || rootSpan; // Else, we try to use the active span. // Finally, we fall back to look at the transactionName on the scope - const routeName = - cachedRouteName || (rootSpan ? spanToJSON(rootSpan).description : scope.getScopeData().transactionName); + const routeName = spanToUse ? spanToJSON(spanToUse).description : scope.getScopeData().transactionName; const user = scope.getUser(); @@ -154,11 +150,17 @@ function _trackINP(): () => void { }); } -/** Register a listener to cache route information for INP interactions. */ -export function registerInpInteractionListener(latestRoute: PartialRouteInfo): void { +/** + * Register a listener to cache route information for INP interactions. + * TODO(v9): `latestRoute` no longer needs to be passed in and will be removed in v9. + */ +export function registerInpInteractionListener(_latestRoute?: unknown): void { const handleEntries = ({ entries }: { entries: PerformanceEntry[] }): void => { + const activeSpan = getActiveSpan(); + const activeRootSpan = activeSpan && getRootSpan(activeSpan); + entries.forEach(entry => { - if (!isPerformanceEventTiming(entry) || !latestRoute.name) { + if (!isPerformanceEventTiming(entry) || !activeRootSpan) { return; } @@ -168,21 +170,20 @@ export function registerInpInteractionListener(latestRoute: PartialRouteInfo): v } // If the interaction was already recorded before, nothing more to do - if (INTERACTIONS_ROUTE_MAP.has(interactionId)) { + if (INTERACTIONS_SPAN_MAP.has(interactionId)) { return; } // We keep max. 10 interactions in the list, then remove the oldest one & clean up if (LAST_INTERACTIONS.length > 10) { const last = LAST_INTERACTIONS.shift() as number; - INTERACTIONS_ROUTE_MAP.delete(last); + INTERACTIONS_SPAN_MAP.delete(last); } // We add the interaction to the list of recorded interactions - // and store the route information for this interaction - // (we clone the object because it is mutated when it changes) + // and store the span for this interaction LAST_INTERACTIONS.push(interactionId); - INTERACTIONS_ROUTE_MAP.set(interactionId, latestRoute.name); + INTERACTIONS_SPAN_MAP.set(interactionId, activeRootSpan); }); }; diff --git a/packages/browser/src/tracing/browserTracingIntegration.ts b/packages/browser/src/tracing/browserTracingIntegration.ts index 0423831219e2..3deaa195abe3 100644 --- a/packages/browser/src/tracing/browserTracingIntegration.ts +++ b/packages/browser/src/tracing/browserTracingIntegration.ts @@ -395,7 +395,7 @@ export const browserTracingIntegration = ((_options: Partial(); - - if (!client) { - return; - } - startBrowserTracingPageLoadSpan(client, spanContext); }