Skip to content

Commit

Permalink
fix(inp): Ensure INP spans have correct transaction
Browse files Browse the repository at this point in the history
  • Loading branch information
mydea committed Jul 11, 2024
1 parent 0e6d802 commit 7674495
Show file tree
Hide file tree
Showing 7 changed files with 239 additions and 42 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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(() => {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,3 +1,16 @@
export default function User() {
return <div>I am a blank page</div>;
return (
<div>
<div>I am a blank page</div>
<button
type="button"
id="button"
onClick={() => {
console.log('Button clicked');
}}
>
Click button
</button>
</div>
);
}
Original file line number Diff line number Diff line change
@@ -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: '<unknown>',
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: '<unknown>',
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),
});
});
39 changes: 20 additions & 19 deletions packages/browser-utils/src/metrics/inp.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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<number, string>();
const INTERACTIONS_SPAN_MAP = new Map<number, Span>();

/**
* Start tracking INP webvital events.
Expand Down Expand Up @@ -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();

Expand Down Expand Up @@ -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;
}

Expand All @@ -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);
});
};

Expand Down
2 changes: 1 addition & 1 deletion packages/browser/src/tracing/browserTracingIntegration.ts
Original file line number Diff line number Diff line change
Expand Up @@ -395,7 +395,7 @@ export const browserTracingIntegration = ((_options: Partial<BrowserTracingOptio
}

if (enableInp) {
registerInpInteractionListener(latestRoute);
registerInpInteractionListener();
}

instrumentOutgoingRequests({
Expand Down
20 changes: 7 additions & 13 deletions packages/remix/src/client/browserTracingIntegration.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,19 +7,13 @@ import type { RemixBrowserTracingIntegrationOptions } from './performance';
* This integration will create pageload and navigation spans.
*/
export function browserTracingIntegration(options: RemixBrowserTracingIntegrationOptions): Integration {
if (options.instrumentPageLoad === undefined) {
options.instrumentPageLoad = true;
}

if (options.instrumentNavigation === undefined) {
options.instrumentNavigation = true;
}
const { instrumentPageLoad = true, instrumentNavigation = true, useEffect, useLocation, useMatches } = options;

setGlobals({
useEffect: options.useEffect,
useLocation: options.useLocation,
useMatches: options.useMatches,
instrumentNavigation: options.instrumentNavigation,
useEffect,
useLocation,
useMatches,
instrumentNavigation,
});

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

if (options.instrumentPageLoad) {
startPageloadSpan();
if (instrumentPageLoad) {
startPageloadSpan(client);
}
},
};
Expand Down
10 changes: 2 additions & 8 deletions packages/remix/src/client/performance.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ import {
startBrowserTracingPageLoadSpan,
withErrorBoundary,
} from '@sentry/react';
import type { StartSpanOptions } from '@sentry/types';
import type { Client, StartSpanOptions } from '@sentry/types';
import { isNodeEnv, logger } from '@sentry/utils';
import * as React from 'react';

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

export function startPageloadSpan(): void {
export function startPageloadSpan(client: Client): void {
const initPathName = getInitPathName();

if (!initPathName) {
Expand All @@ -83,12 +83,6 @@ export function startPageloadSpan(): void {
},
};

const client = getClient<BrowserClient>();

if (!client) {
return;
}

startBrowserTracingPageLoadSpan(client, spanContext);
}

Expand Down

0 comments on commit 7674495

Please sign in to comment.