From b4b1ae1b6453019e631561d50f387cd740e489f2 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Fri, 13 Sep 2024 13:57:40 +0000 Subject: [PATCH 1/8] ref(nextjs): Improve app router routing instrumentation accuracy --- packages/nextjs/package.json | 1 + .../appRouterRoutingInstrumentation.ts | 128 ++++++++++-------- 2 files changed, 74 insertions(+), 55 deletions(-) diff --git a/packages/nextjs/package.json b/packages/nextjs/package.json index 8e4fd27370da..902ba8a22f40 100644 --- a/packages/nextjs/package.json +++ b/packages/nextjs/package.json @@ -71,6 +71,7 @@ "@opentelemetry/instrumentation-http": "0.53.0", "@opentelemetry/semantic-conventions": "^1.27.0", "@rollup/plugin-commonjs": "26.0.1", + "@sentry-internal/browser-utils": "8.29.0", "@sentry/core": "8.29.0", "@sentry/node": "8.29.0", "@sentry/opentelemetry": "8.29.0", diff --git a/packages/nextjs/src/client/routing/appRouterRoutingInstrumentation.ts b/packages/nextjs/src/client/routing/appRouterRoutingInstrumentation.ts index 25c1496d25b4..87566dd08330 100644 --- a/packages/nextjs/src/client/routing/appRouterRoutingInstrumentation.ts +++ b/packages/nextjs/src/client/routing/appRouterRoutingInstrumentation.ts @@ -4,8 +4,8 @@ import { SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, } from '@sentry/core'; import { WINDOW, startBrowserTracingNavigationSpan, startBrowserTracingPageLoadSpan } from '@sentry/react'; -import type { Client } from '@sentry/types'; -import { addFetchInstrumentationHandler, browserPerformanceTimeOrigin } from '@sentry/utils'; +import type { Client, Span } from '@sentry/types'; +import { GLOBAL_OBJ, browserPerformanceTimeOrigin } from '@sentry/utils'; /** Instruments the Next.js app router for pageloads. */ export function appRouterInstrumentPageLoad(client: Client): void { @@ -21,70 +21,88 @@ export function appRouterInstrumentPageLoad(client: Client): void { }); } +const GLOBAL_OBJ_WITH_NEXT_ROUTER = GLOBAL_OBJ as typeof GLOBAL_OBJ & { + nd?: { + router?: { + back: () => void; + forward: () => void; + push: (target: string) => void; + replace: (target: string) => void; + }; + }; +}; + /** Instruments the Next.js app router for navigation. */ export function appRouterInstrumentNavigation(client: Client): void { - addFetchInstrumentationHandler(handlerData => { - // The instrumentation handler is invoked twice - once for starting a request and once when the req finishes - // We can use the existence of the end-timestamp to filter out "finishing"-events. - if (handlerData.endTimestamp !== undefined) { - return; - } - - // Only GET requests can be navigating RSC requests - if (handlerData.fetchData.method !== 'GET') { - return; - } + let currentNavigationSpan: Span | undefined = undefined; - const parsedNavigatingRscFetchArgs = parseNavigatingRscFetchArgs(handlerData.args); + WINDOW.addEventListener('popstate', () => { + console.log({ currentNavigationSpan, r: currentNavigationSpan?.isRecording() }); - if (parsedNavigatingRscFetchArgs === null) { - return; + if (currentNavigationSpan && currentNavigationSpan.isRecording()) { + currentNavigationSpan.updateName(WINDOW.location.pathname); + } else { + currentNavigationSpan = startBrowserTracingNavigationSpan(client, { + name: WINDOW.location.pathname, + attributes: { + [SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'navigation.popstate', + [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.navigation.nextjs.app_router_instrumentation', + [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'url', + }, + }); } - - const newPathname = parsedNavigatingRscFetchArgs.targetPathname; - - startBrowserTracingNavigationSpan(client, { - name: newPathname, - attributes: { - [SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'navigation', - [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.navigation.nextjs.app_router_instrumentation', - [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'url', - }, - }); }); -} - -function parseNavigatingRscFetchArgs(fetchArgs: unknown[]): null | { - targetPathname: string; -} { - // Make sure the first arg is a URL object - if (!fetchArgs[0] || typeof fetchArgs[0] !== 'object' || (fetchArgs[0] as URL).searchParams === undefined) { - return null; - } - // Make sure the second argument is some kind of fetch config obj that contains headers - if (!fetchArgs[1] || typeof fetchArgs[1] !== 'object' || !('headers' in fetchArgs[1])) { - return null; - } + setTimeout( + () => { + (['back', 'forward', 'push', 'replace'] as const).forEach(routerFunctionName => { + if (GLOBAL_OBJ_WITH_NEXT_ROUTER?.nd?.router?.[routerFunctionName]) { + // @ts-expect-error TODO + GLOBAL_OBJ_WITH_NEXT_ROUTER.nd.router[routerFunctionName] = new Proxy( + GLOBAL_OBJ_WITH_NEXT_ROUTER.nd.router[routerFunctionName], + { + apply(target, thisArg, argArray) { + let targetPathName: string; + const opParts = ['navigation']; - try { - const url = fetchArgs[0] as URL; - const headers = fetchArgs[1].headers as Record; + if (routerFunctionName === 'push') { + targetPathName = transactionNameifyRouterArgument(argArray[0]); + } else if (routerFunctionName === 'replace') { + targetPathName = transactionNameifyRouterArgument(argArray[0]); + } else if (routerFunctionName === 'back') { + targetPathName = 'TOOD'; // TODO - figure out how to filter txns with this name + opParts.push('router', 'back'); + } else if (routerFunctionName === 'forward') { + targetPathName = 'TOOD'; // TODO - figure out how to filter txns with this name + opParts.push('router', 'forward'); + } - // Not an RSC request - if (headers['RSC'] !== '1') { - return null; - } + currentNavigationSpan = startBrowserTracingNavigationSpan(client, { + // eslint-disable-next-line @typescript-eslint/no-non-null-assertion + name: targetPathName!, + attributes: { + [SEMANTIC_ATTRIBUTE_SENTRY_OP]: opParts.join('.'), + [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.navigation.nextjs.app_router_instrumentation', + [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'url', + }, + }); - // Prefetch requests are not navigating RSC requests - if (headers['Next-Router-Prefetch'] === '1') { - return null; - } + return target.apply(thisArg, argArray); + }, + }, + ); + } + }); + }, + // Some arbitrary amount of time that is enough for Next.js to populate `window.next.router` + 300, + ); +} - return { - targetPathname: url.pathname, - }; +function transactionNameifyRouterArgument(target: string): string { + try { + return new URL(target, 'http://some-random-base.com/').pathname; } catch { - return null; + return '/'; } } From 410981db30cfd485b525738d4af78e4409b27568 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Thu, 19 Sep 2024 08:46:18 +0000 Subject: [PATCH 2/8] . --- .../routing/appRouterRoutingInstrumentation.ts | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/packages/nextjs/src/client/routing/appRouterRoutingInstrumentation.ts b/packages/nextjs/src/client/routing/appRouterRoutingInstrumentation.ts index 87566dd08330..a64bde28a446 100644 --- a/packages/nextjs/src/client/routing/appRouterRoutingInstrumentation.ts +++ b/packages/nextjs/src/client/routing/appRouterRoutingInstrumentation.ts @@ -32,13 +32,23 @@ const GLOBAL_OBJ_WITH_NEXT_ROUTER = GLOBAL_OBJ as typeof GLOBAL_OBJ & { }; }; +/** + * The routing instrumentation needs to handle a few cases: + * - Router operations: + * - router.push() (either explicitly called or implicitly through tags) + * - router.replace() (either explicitly called or implicitly through tags) + * - router.back() + * - router.forward() + * - Browser operations: + * - native Browser-back + * - native Browser-forward + */ + /** Instruments the Next.js app router for navigation. */ export function appRouterInstrumentNavigation(client: Client): void { let currentNavigationSpan: Span | undefined = undefined; WINDOW.addEventListener('popstate', () => { - console.log({ currentNavigationSpan, r: currentNavigationSpan?.isRecording() }); - if (currentNavigationSpan && currentNavigationSpan.isRecording()) { currentNavigationSpan.updateName(WINDOW.location.pathname); } else { @@ -95,7 +105,7 @@ export function appRouterInstrumentNavigation(client: Client): void { }); }, // Some arbitrary amount of time that is enough for Next.js to populate `window.next.router` - 300, + 50, ); } From 79d430c3b6fa596ee5ad4215f4d82ca9ae09e007 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Thu, 19 Sep 2024 10:47:23 +0000 Subject: [PATCH 3/8] Make everything a bit more sane --- packages/nextjs/src/client/index.ts | 8 ++ .../appRouterRoutingInstrumentation.ts | 81 ++++++++++--------- 2 files changed, 51 insertions(+), 38 deletions(-) diff --git a/packages/nextjs/src/client/index.ts b/packages/nextjs/src/client/index.ts index a68734a10398..c66f50a293f2 100644 --- a/packages/nextjs/src/client/index.ts +++ b/packages/nextjs/src/client/index.ts @@ -8,6 +8,7 @@ import { devErrorSymbolicationEventProcessor } from '../common/devErrorSymbolica import { getVercelEnv } from '../common/getVercelEnv'; import { browserTracingIntegration } from './browserTracingIntegration'; import { nextjsClientStackFrameNormalizationIntegration } from './clientNormalizationIntegration'; +import { INCOMPLETE_APP_ROUTER_INSTRUMENTATION_TRANSACTION_NAME } from './routing/appRouterRoutingInstrumentation'; import { applyTunnelRouteOption } from './tunnelRoute'; export * from '@sentry/react'; @@ -39,6 +40,13 @@ export function init(options: BrowserOptions): Client | undefined { filterTransactions.id = 'NextClient404Filter'; addEventProcessor(filterTransactions); + const filterIncompleteNavigationTransactions: EventProcessor = event => + event.type === 'transaction' && event.transaction === INCOMPLETE_APP_ROUTER_INSTRUMENTATION_TRANSACTION_NAME + ? null + : event; + filterIncompleteNavigationTransactions.id = 'IncompleteTransactionFilter'; + addEventProcessor(filterIncompleteNavigationTransactions); + if (process.env.NODE_ENV === 'development') { addEventProcessor(devErrorSymbolicationEventProcessor); } diff --git a/packages/nextjs/src/client/routing/appRouterRoutingInstrumentation.ts b/packages/nextjs/src/client/routing/appRouterRoutingInstrumentation.ts index a64bde28a446..083e4ad3cd02 100644 --- a/packages/nextjs/src/client/routing/appRouterRoutingInstrumentation.ts +++ b/packages/nextjs/src/client/routing/appRouterRoutingInstrumentation.ts @@ -7,6 +7,8 @@ import { WINDOW, startBrowserTracingNavigationSpan, startBrowserTracingPageLoadS import type { Client, Span } from '@sentry/types'; import { GLOBAL_OBJ, browserPerformanceTimeOrigin } from '@sentry/utils'; +export const INCOMPLETE_APP_ROUTER_INSTRUMENTATION_TRANSACTION_NAME = 'incomplete-app-router-transaction'; + /** Instruments the Next.js app router for pageloads. */ export function appRouterInstrumentPageLoad(client: Client): void { startBrowserTracingPageLoadSpan(client, { @@ -21,14 +23,20 @@ export function appRouterInstrumentPageLoad(client: Client): void { }); } +interface NextRouter { + back: () => void; + forward: () => void; + push: (target: string) => void; + replace: (target: string) => void; +} // Yes, yes, I know we shouldn't depend on these internals. But that's where we are at. We write the ugly code, so you don't have to. const GLOBAL_OBJ_WITH_NEXT_ROUTER = GLOBAL_OBJ as typeof GLOBAL_OBJ & { + // Available until 13.4.4-canary.3 - https://github.com/vercel/next.js/pull/50210 nd?: { - router?: { - back: () => void; - forward: () => void; - push: (target: string) => void; - replace: (target: string) => void; - }; + router?: NextRouter; + }; + // Avalable from 13.4.4-canary.4 - https://github.com/vercel/next.js/pull/50210 + next?: { + router?: NextRouter; }; }; @@ -40,8 +48,8 @@ const GLOBAL_OBJ_WITH_NEXT_ROUTER = GLOBAL_OBJ as typeof GLOBAL_OBJ & { * - router.back() * - router.forward() * - Browser operations: - * - native Browser-back - * - native Browser-forward + * - native Browser-back / popstate event (implicitly called by router.back()) + * - native Browser-forward / popstate event (implicitly called by router.forward()) */ /** Instruments the Next.js app router for navigation. */ @@ -66,41 +74,38 @@ export function appRouterInstrumentNavigation(client: Client): void { setTimeout( () => { (['back', 'forward', 'push', 'replace'] as const).forEach(routerFunctionName => { - if (GLOBAL_OBJ_WITH_NEXT_ROUTER?.nd?.router?.[routerFunctionName]) { + const router = GLOBAL_OBJ_WITH_NEXT_ROUTER?.next?.router ?? GLOBAL_OBJ_WITH_NEXT_ROUTER?.nd?.router; + + if (router?.[routerFunctionName]) { // @ts-expect-error TODO - GLOBAL_OBJ_WITH_NEXT_ROUTER.nd.router[routerFunctionName] = new Proxy( - GLOBAL_OBJ_WITH_NEXT_ROUTER.nd.router[routerFunctionName], - { - apply(target, thisArg, argArray) { - let targetPathName: string; - const opParts = ['navigation']; + router[routerFunctionName] = new Proxy(router[routerFunctionName], { + apply(target, thisArg, argArray) { + const span = startBrowserTracingNavigationSpan(client, { + name: INCOMPLETE_APP_ROUTER_INSTRUMENTATION_TRANSACTION_NAME, + attributes: { + [SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'navigation', + [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.navigation.nextjs.app_router_instrumentation', + [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'url', + }, + }); - if (routerFunctionName === 'push') { - targetPathName = transactionNameifyRouterArgument(argArray[0]); - } else if (routerFunctionName === 'replace') { - targetPathName = transactionNameifyRouterArgument(argArray[0]); - } else if (routerFunctionName === 'back') { - targetPathName = 'TOOD'; // TODO - figure out how to filter txns with this name - opParts.push('router', 'back'); - } else if (routerFunctionName === 'forward') { - targetPathName = 'TOOD'; // TODO - figure out how to filter txns with this name - opParts.push('router', 'forward'); - } + currentNavigationSpan = span; - currentNavigationSpan = startBrowserTracingNavigationSpan(client, { - // eslint-disable-next-line @typescript-eslint/no-non-null-assertion - name: targetPathName!, - attributes: { - [SEMANTIC_ATTRIBUTE_SENTRY_OP]: opParts.join('.'), - [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.navigation.nextjs.app_router_instrumentation', - [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'url', - }, - }); + if (routerFunctionName === 'push') { + span?.updateName(transactionNameifyRouterArgument(argArray[0])); + span?.setAttribute('navigation.type', 'navigation.router.push'); + } else if (routerFunctionName === 'replace') { + span?.updateName(transactionNameifyRouterArgument(argArray[0])); + span?.setAttribute('navigation.type', 'navigation.router.replace'); + } else if (routerFunctionName === 'back') { + span?.setAttribute('navigation.type', 'navigation.router.back'); + } else if (routerFunctionName === 'forward') { + span?.setAttribute('navigation.type', 'navigation.router.forward'); + } - return target.apply(thisArg, argArray); - }, + return target.apply(thisArg, argArray); }, - ); + }); } }); }, From b54192e5c4a5b44adc0d72355ed1a95764ff3988 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Thu, 19 Sep 2024 10:48:47 +0000 Subject: [PATCH 4/8] comment formatting --- .../src/client/routing/appRouterRoutingInstrumentation.ts | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/packages/nextjs/src/client/routing/appRouterRoutingInstrumentation.ts b/packages/nextjs/src/client/routing/appRouterRoutingInstrumentation.ts index 083e4ad3cd02..ea7a8b50e503 100644 --- a/packages/nextjs/src/client/routing/appRouterRoutingInstrumentation.ts +++ b/packages/nextjs/src/client/routing/appRouterRoutingInstrumentation.ts @@ -28,7 +28,9 @@ interface NextRouter { forward: () => void; push: (target: string) => void; replace: (target: string) => void; -} // Yes, yes, I know we shouldn't depend on these internals. But that's where we are at. We write the ugly code, so you don't have to. +} + +// Yes, yes, I know we shouldn't depend on these internals. But that's where we are at. We write the ugly code, so you don't have to. const GLOBAL_OBJ_WITH_NEXT_ROUTER = GLOBAL_OBJ as typeof GLOBAL_OBJ & { // Available until 13.4.4-canary.3 - https://github.com/vercel/next.js/pull/50210 nd?: { @@ -40,7 +42,7 @@ const GLOBAL_OBJ_WITH_NEXT_ROUTER = GLOBAL_OBJ as typeof GLOBAL_OBJ & { }; }; -/** +/* * The routing instrumentation needs to handle a few cases: * - Router operations: * - router.push() (either explicitly called or implicitly through tags) From dedc5812e3b5c4273c2d9c853983b8c7702b282d Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Thu, 19 Sep 2024 12:59:04 +0000 Subject: [PATCH 5/8] Add tests --- .../navigation/[param]/browser-back/page.tsx | 7 + .../navigation/[param]/link-replace/page.tsx | 7 + .../app/navigation/[param]/link/page.tsx | 7 + .../navigation/[param]/router-back/page.tsx | 7 + .../navigation/[param]/router-push/page.tsx | 5 + .../[param]/router-replace/page.tsx | 5 + .../nextjs-app-dir/app/navigation/page.tsx | 57 ++++++++ ...client-app-routing-instrumentation.test.ts | 136 ++++++++++++++++++ package.json | 2 +- .../appRouterRoutingInstrumentation.ts | 11 +- 10 files changed, 238 insertions(+), 6 deletions(-) create mode 100644 dev-packages/e2e-tests/test-applications/nextjs-app-dir/app/navigation/[param]/browser-back/page.tsx create mode 100644 dev-packages/e2e-tests/test-applications/nextjs-app-dir/app/navigation/[param]/link-replace/page.tsx create mode 100644 dev-packages/e2e-tests/test-applications/nextjs-app-dir/app/navigation/[param]/link/page.tsx create mode 100644 dev-packages/e2e-tests/test-applications/nextjs-app-dir/app/navigation/[param]/router-back/page.tsx create mode 100644 dev-packages/e2e-tests/test-applications/nextjs-app-dir/app/navigation/[param]/router-push/page.tsx create mode 100644 dev-packages/e2e-tests/test-applications/nextjs-app-dir/app/navigation/[param]/router-replace/page.tsx create mode 100644 dev-packages/e2e-tests/test-applications/nextjs-app-dir/app/navigation/page.tsx diff --git a/dev-packages/e2e-tests/test-applications/nextjs-app-dir/app/navigation/[param]/browser-back/page.tsx b/dev-packages/e2e-tests/test-applications/nextjs-app-dir/app/navigation/[param]/browser-back/page.tsx new file mode 100644 index 000000000000..9e32c27abce2 --- /dev/null +++ b/dev-packages/e2e-tests/test-applications/nextjs-app-dir/app/navigation/[param]/browser-back/page.tsx @@ -0,0 +1,7 @@ +import Link from 'next/link'; + +export const dynamic = 'force-dynamic'; + +export default function Page() { + return Go back home; +} diff --git a/dev-packages/e2e-tests/test-applications/nextjs-app-dir/app/navigation/[param]/link-replace/page.tsx b/dev-packages/e2e-tests/test-applications/nextjs-app-dir/app/navigation/[param]/link-replace/page.tsx new file mode 100644 index 000000000000..9e32c27abce2 --- /dev/null +++ b/dev-packages/e2e-tests/test-applications/nextjs-app-dir/app/navigation/[param]/link-replace/page.tsx @@ -0,0 +1,7 @@ +import Link from 'next/link'; + +export const dynamic = 'force-dynamic'; + +export default function Page() { + return Go back home; +} diff --git a/dev-packages/e2e-tests/test-applications/nextjs-app-dir/app/navigation/[param]/link/page.tsx b/dev-packages/e2e-tests/test-applications/nextjs-app-dir/app/navigation/[param]/link/page.tsx new file mode 100644 index 000000000000..9e32c27abce2 --- /dev/null +++ b/dev-packages/e2e-tests/test-applications/nextjs-app-dir/app/navigation/[param]/link/page.tsx @@ -0,0 +1,7 @@ +import Link from 'next/link'; + +export const dynamic = 'force-dynamic'; + +export default function Page() { + return Go back home; +} diff --git a/dev-packages/e2e-tests/test-applications/nextjs-app-dir/app/navigation/[param]/router-back/page.tsx b/dev-packages/e2e-tests/test-applications/nextjs-app-dir/app/navigation/[param]/router-back/page.tsx new file mode 100644 index 000000000000..9e32c27abce2 --- /dev/null +++ b/dev-packages/e2e-tests/test-applications/nextjs-app-dir/app/navigation/[param]/router-back/page.tsx @@ -0,0 +1,7 @@ +import Link from 'next/link'; + +export const dynamic = 'force-dynamic'; + +export default function Page() { + return Go back home; +} diff --git a/dev-packages/e2e-tests/test-applications/nextjs-app-dir/app/navigation/[param]/router-push/page.tsx b/dev-packages/e2e-tests/test-applications/nextjs-app-dir/app/navigation/[param]/router-push/page.tsx new file mode 100644 index 000000000000..de789f9af524 --- /dev/null +++ b/dev-packages/e2e-tests/test-applications/nextjs-app-dir/app/navigation/[param]/router-push/page.tsx @@ -0,0 +1,5 @@ +export const dynamic = 'force-dynamic'; + +export default function Page() { + return

hello world

; +} diff --git a/dev-packages/e2e-tests/test-applications/nextjs-app-dir/app/navigation/[param]/router-replace/page.tsx b/dev-packages/e2e-tests/test-applications/nextjs-app-dir/app/navigation/[param]/router-replace/page.tsx new file mode 100644 index 000000000000..de789f9af524 --- /dev/null +++ b/dev-packages/e2e-tests/test-applications/nextjs-app-dir/app/navigation/[param]/router-replace/page.tsx @@ -0,0 +1,5 @@ +export const dynamic = 'force-dynamic'; + +export default function Page() { + return

hello world

; +} diff --git a/dev-packages/e2e-tests/test-applications/nextjs-app-dir/app/navigation/page.tsx b/dev-packages/e2e-tests/test-applications/nextjs-app-dir/app/navigation/page.tsx new file mode 100644 index 000000000000..4f03a59d71cf --- /dev/null +++ b/dev-packages/e2e-tests/test-applications/nextjs-app-dir/app/navigation/page.tsx @@ -0,0 +1,57 @@ +'use client'; + +import Link from 'next/link'; +import { useRouter } from 'next/navigation'; + +export default function Page() { + const router = useRouter(); + + return ( +
    +
  • + +
    • +
  • + +
    • +
  • + +
    • +
  • + +
    • +
  • + Normal Link +
    • +
  • + + Link Replace + +
    • +
+ ); +} diff --git a/dev-packages/e2e-tests/test-applications/nextjs-app-dir/tests/client-app-routing-instrumentation.test.ts b/dev-packages/e2e-tests/test-applications/nextjs-app-dir/tests/client-app-routing-instrumentation.test.ts index 9143bd0b2f90..d2d39cb82a8f 100644 --- a/dev-packages/e2e-tests/test-applications/nextjs-app-dir/tests/client-app-routing-instrumentation.test.ts +++ b/dev-packages/e2e-tests/test-applications/nextjs-app-dir/tests/client-app-routing-instrumentation.test.ts @@ -53,3 +53,139 @@ test('Creates a navigation transaction for app router routes', async ({ page }) expect(await clientNavigationTransactionPromise).toBeDefined(); expect(await serverComponentTransactionPromise).toBeDefined(); }); + +test('Creates a navigation transaction for `router.push()`', async ({ page }) => { + const navigationTransactionPromise = waitForTransaction('nextjs-app-dir', transactionEvent => { + return ( + transactionEvent?.transaction === `/navigation/42/router-push` && + transactionEvent.contexts?.trace?.op === 'navigation' && + transactionEvent.contexts.trace.data?.['navigation.type'] === 'router.push' + ); + }); + + await page.goto('/navigation'); + await page.getByText('router.push()').click(); + + expect(await navigationTransactionPromise).toBeDefined(); +}); + +test('Creates a navigation transaction for `router.replace()`', async ({ page }) => { + const navigationTransactionPromise = waitForTransaction('nextjs-app-dir', transactionEvent => { + return ( + transactionEvent?.transaction === `/navigation/42/router-replace` && + transactionEvent.contexts?.trace?.op === 'navigation' && + transactionEvent.contexts.trace.data?.['navigation.type'] === 'router.replace' + ); + }); + + await page.goto('/navigation'); + await page.getByText('router.replace()').click(); + + expect(await navigationTransactionPromise).toBeDefined(); +}); + +test('Creates a navigation transaction for `router.back()`', async ({ page }) => { + const navigationTransactionPromise = waitForTransaction('nextjs-app-dir', transactionEvent => { + return ( + transactionEvent?.transaction === `/navigation/1337/router-back` && + transactionEvent.contexts?.trace?.op === 'navigation' + ); + }); + + await page.goto('/navigation/1337/router-back'); + await page.getByText('Go back home').click(); + await page.getByText('router.back()').click(); + + expect(await navigationTransactionPromise).toMatchObject({ + contexts: { + trace: { + data: { + 'navigation.type': 'router.back', + }, + }, + }, + }); +}); + +test('Creates a navigation transaction for `router.forward()`', async ({ page }) => { + const navigationTransactionPromise = waitForTransaction('nextjs-app-dir', transactionEvent => { + return ( + transactionEvent?.transaction === `/navigation/42/router-push` && + transactionEvent.contexts?.trace?.op === 'navigation' && + transactionEvent.contexts.trace.data?.['navigation.type'] === 'router.forward' + ); + }); + + await page.goto('/navigation'); + await page.getByText('router.push()').click(); + await page.goBack(); + await page.getByText('router.forward()').click(); + + expect(await navigationTransactionPromise).toBeDefined(); +}); + +test('Creates a navigation transaction for ``', async ({ page }) => { + const navigationTransactionPromise = waitForTransaction('nextjs-app-dir', transactionEvent => { + return ( + transactionEvent?.transaction === `/navigation/42/link` && + transactionEvent.contexts?.trace?.op === 'navigation' && + transactionEvent.contexts.trace.data?.['navigation.type'] === 'router.push' + ); + }); + + await page.goto('/navigation'); + await page.getByText('Normal Link').click(); + + expect(await navigationTransactionPromise).toBeDefined(); +}); + +test('Creates a navigation transaction for ``', async ({ page }) => { + const navigationTransactionPromise = waitForTransaction('nextjs-app-dir', transactionEvent => { + return ( + transactionEvent?.transaction === `/navigation/42/link-replace` && + transactionEvent.contexts?.trace?.op === 'navigation' && + transactionEvent.contexts.trace.data?.['navigation.type'] === 'router.replace' + ); + }); + + await page.goto('/navigation'); + await page.getByText('Link Replace').click(); + + expect(await navigationTransactionPromise).toBeDefined(); +}); + +test('Creates a navigation transaction for browser-back', async ({ page }) => { + const navigationTransactionPromise = waitForTransaction('nextjs-app-dir', transactionEvent => { + return ( + transactionEvent?.transaction === `/navigation/42/browser-back` && + transactionEvent.contexts?.trace?.op === 'navigation' && + transactionEvent.contexts.trace.data?.['navigation.type'] === 'browser.popstate' + ); + }); + + await page.goto('/navigation/42/browser-back'); + await page.getByText('Go back home').click(); + await page.waitForTimeout(3000); + await page.goBack(); + + expect(await navigationTransactionPromise).toBeDefined(); +}); + +test('Creates a navigation transaction for browser-forward', async ({ page }) => { + const navigationTransactionPromise = waitForTransaction('nextjs-app-dir', transactionEvent => { + return ( + transactionEvent?.transaction === `/navigation/42/router-push` && + transactionEvent.contexts?.trace?.op === 'navigation' && + transactionEvent.contexts.trace.data?.['navigation.type'] === 'browser.popstate' + ); + }); + + await page.goto('/navigation'); + await page.getByText('router.push()').click(); + await page.waitForTimeout(3000); + await page.goBack(); + await page.waitForTimeout(3000); + await page.goForward(); + + expect(await navigationTransactionPromise).toBeDefined(); +}); diff --git a/package.json b/package.json index 4b9ad0383c02..365e1eb13922 100644 --- a/package.json +++ b/package.json @@ -17,7 +17,7 @@ "clean:build": "lerna run clean", "clean:caches": "yarn rimraf eslintcache .nxcache && yarn jest --clearCache", "clean:deps": "lerna clean --yes && rm -rf node_modules && yarn", - "clean:tarballs": "rimraf **/*.tgz", + "clean:tarballs": "rimraf -g **/*.tgz", "clean:all": "run-s clean:build clean:tarballs clean:caches clean:deps", "fix": "run-s fix:biome fix:prettier fix:lerna", "fix:lerna": "lerna run fix", diff --git a/packages/nextjs/src/client/routing/appRouterRoutingInstrumentation.ts b/packages/nextjs/src/client/routing/appRouterRoutingInstrumentation.ts index ea7a8b50e503..346130ec388c 100644 --- a/packages/nextjs/src/client/routing/appRouterRoutingInstrumentation.ts +++ b/packages/nextjs/src/client/routing/appRouterRoutingInstrumentation.ts @@ -65,9 +65,10 @@ export function appRouterInstrumentNavigation(client: Client): void { currentNavigationSpan = startBrowserTracingNavigationSpan(client, { name: WINDOW.location.pathname, attributes: { - [SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'navigation.popstate', + [SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'navigation', [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.navigation.nextjs.app_router_instrumentation', [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'url', + 'navigation.type': 'browser.popstate', }, }); } @@ -95,14 +96,14 @@ export function appRouterInstrumentNavigation(client: Client): void { if (routerFunctionName === 'push') { span?.updateName(transactionNameifyRouterArgument(argArray[0])); - span?.setAttribute('navigation.type', 'navigation.router.push'); + span?.setAttribute('navigation.type', 'router.push'); } else if (routerFunctionName === 'replace') { span?.updateName(transactionNameifyRouterArgument(argArray[0])); - span?.setAttribute('navigation.type', 'navigation.router.replace'); + span?.setAttribute('navigation.type', 'router.replace'); } else if (routerFunctionName === 'back') { - span?.setAttribute('navigation.type', 'navigation.router.back'); + span?.setAttribute('navigation.type', 'router.back'); } else if (routerFunctionName === 'forward') { - span?.setAttribute('navigation.type', 'navigation.router.forward'); + span?.setAttribute('navigation.type', 'router.forward'); } return target.apply(thisArg, argArray); From 7c337e20fe42a40528cfefc93008a55281680f13 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Thu, 19 Sep 2024 14:26:52 +0000 Subject: [PATCH 6/8] Timing issues in dev mode? --- ...client-app-routing-instrumentation.test.ts | 9 +++++++ .../appRouterRoutingInstrumentation.ts | 24 ++++++++++++------- 2 files changed, 25 insertions(+), 8 deletions(-) diff --git a/dev-packages/e2e-tests/test-applications/nextjs-app-dir/tests/client-app-routing-instrumentation.test.ts b/dev-packages/e2e-tests/test-applications/nextjs-app-dir/tests/client-app-routing-instrumentation.test.ts index d2d39cb82a8f..35984640bcf6 100644 --- a/dev-packages/e2e-tests/test-applications/nextjs-app-dir/tests/client-app-routing-instrumentation.test.ts +++ b/dev-packages/e2e-tests/test-applications/nextjs-app-dir/tests/client-app-routing-instrumentation.test.ts @@ -64,6 +64,7 @@ test('Creates a navigation transaction for `router.push()`', async ({ page }) => }); await page.goto('/navigation'); + await page.waitForTimeout(3000); await page.getByText('router.push()').click(); expect(await navigationTransactionPromise).toBeDefined(); @@ -79,6 +80,7 @@ test('Creates a navigation transaction for `router.replace()`', async ({ page }) }); await page.goto('/navigation'); + await page.waitForTimeout(3000); await page.getByText('router.replace()').click(); expect(await navigationTransactionPromise).toBeDefined(); @@ -93,7 +95,9 @@ test('Creates a navigation transaction for `router.back()`', async ({ page }) => }); await page.goto('/navigation/1337/router-back'); + await page.waitForTimeout(3000); await page.getByText('Go back home').click(); + await page.waitForTimeout(3000); await page.getByText('router.back()').click(); expect(await navigationTransactionPromise).toMatchObject({ @@ -117,8 +121,11 @@ test('Creates a navigation transaction for `router.forward()`', async ({ page }) }); await page.goto('/navigation'); + await page.waitForTimeout(3000); await page.getByText('router.push()').click(); + await page.waitForTimeout(3000); await page.goBack(); + await page.waitForTimeout(3000); await page.getByText('router.forward()').click(); expect(await navigationTransactionPromise).toBeDefined(); @@ -149,6 +156,7 @@ test('Creates a navigation transaction for ``', async ({ page }) }); await page.goto('/navigation'); + await page.waitForTimeout(3000); await page.getByText('Link Replace').click(); expect(await navigationTransactionPromise).toBeDefined(); @@ -164,6 +172,7 @@ test('Creates a navigation transaction for browser-back', async ({ page }) => { }); await page.goto('/navigation/42/browser-back'); + await page.waitForTimeout(3000); await page.getByText('Go back home').click(); await page.waitForTimeout(3000); await page.goBack(); diff --git a/packages/nextjs/src/client/routing/appRouterRoutingInstrumentation.ts b/packages/nextjs/src/client/routing/appRouterRoutingInstrumentation.ts index 346130ec388c..89f8cafaaa76 100644 --- a/packages/nextjs/src/client/routing/appRouterRoutingInstrumentation.ts +++ b/packages/nextjs/src/client/routing/appRouterRoutingInstrumentation.ts @@ -1,3 +1,4 @@ +import { setInterval } from 'timers'; import { SEMANTIC_ATTRIBUTE_SENTRY_OP, SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN, @@ -74,11 +75,20 @@ export function appRouterInstrumentNavigation(client: Client): void { } }); - setTimeout( - () => { - (['back', 'forward', 'push', 'replace'] as const).forEach(routerFunctionName => { - const router = GLOBAL_OBJ_WITH_NEXT_ROUTER?.next?.router ?? GLOBAL_OBJ_WITH_NEXT_ROUTER?.nd?.router; + let routerPatched = false; + let triesToFindRouter = 0; + const MAX_TRIES_TO_FIND_ROUTER = 500; + const ROUTER_AVAILABILITY_CHECK_INTERVAL_MS = 20; + const checkForRouterAvailabilityInterval = setInterval(() => { + triesToFindRouter++; + const router = GLOBAL_OBJ_WITH_NEXT_ROUTER?.next?.router ?? GLOBAL_OBJ_WITH_NEXT_ROUTER?.nd?.router; + if (routerPatched || triesToFindRouter > MAX_TRIES_TO_FIND_ROUTER) { + clearInterval(checkForRouterAvailabilityInterval); + } else if (router) { + clearInterval(checkForRouterAvailabilityInterval); + routerPatched = true; + (['back', 'forward', 'push', 'replace'] as const).forEach(routerFunctionName => { if (router?.[routerFunctionName]) { // @ts-expect-error TODO router[routerFunctionName] = new Proxy(router[routerFunctionName], { @@ -111,10 +121,8 @@ export function appRouterInstrumentNavigation(client: Client): void { }); } }); - }, - // Some arbitrary amount of time that is enough for Next.js to populate `window.next.router` - 50, - ); + } + }, ROUTER_AVAILABILITY_CHECK_INTERVAL_MS); } function transactionNameifyRouterArgument(target: string): string { From 35ba1f13358d995d53694cfeccd06f29f60c781f Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Thu, 19 Sep 2024 15:36:17 +0000 Subject: [PATCH 7/8] Fix tests --- .../appRouterRoutingInstrumentation.ts | 1 - packages/nextjs/test/clientSdk.test.ts | 5 + .../appRouterInstrumentation.test.ts | 174 ------------------ 3 files changed, 5 insertions(+), 175 deletions(-) delete mode 100644 packages/nextjs/test/performance/appRouterInstrumentation.test.ts diff --git a/packages/nextjs/src/client/routing/appRouterRoutingInstrumentation.ts b/packages/nextjs/src/client/routing/appRouterRoutingInstrumentation.ts index 89f8cafaaa76..9d59d41af2e9 100644 --- a/packages/nextjs/src/client/routing/appRouterRoutingInstrumentation.ts +++ b/packages/nextjs/src/client/routing/appRouterRoutingInstrumentation.ts @@ -1,4 +1,3 @@ -import { setInterval } from 'timers'; import { SEMANTIC_ATTRIBUTE_SENTRY_OP, SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN, diff --git a/packages/nextjs/test/clientSdk.test.ts b/packages/nextjs/test/clientSdk.test.ts index ac159564410b..f136b29e6887 100644 --- a/packages/nextjs/test/clientSdk.test.ts +++ b/packages/nextjs/test/clientSdk.test.ts @@ -16,13 +16,18 @@ const loggerLogSpy = jest.spyOn(logger, 'log'); const dom = new JSDOM(undefined, { url: 'https://example.com/' }); Object.defineProperty(global, 'document', { value: dom.window.document, writable: true }); Object.defineProperty(global, 'location', { value: dom.window.document.location, writable: true }); +Object.defineProperty(global, 'addEventListener', { value: () => undefined, writable: true }); const originalGlobalDocument = WINDOW.document; const originalGlobalLocation = WINDOW.location; +// eslint-disable-next-line @typescript-eslint/unbound-method +const originalGlobalAddEventListener = WINDOW.addEventListener; + afterAll(() => { // Clean up JSDom Object.defineProperty(WINDOW, 'document', { value: originalGlobalDocument }); Object.defineProperty(WINDOW, 'location', { value: originalGlobalLocation }); + Object.defineProperty(WINDOW, 'addEventListener', { value: originalGlobalAddEventListener }); }); function findIntegrationByName(integrations: Integration[] = [], name: string): Integration | undefined { diff --git a/packages/nextjs/test/performance/appRouterInstrumentation.test.ts b/packages/nextjs/test/performance/appRouterInstrumentation.test.ts deleted file mode 100644 index 16992a498f83..000000000000 --- a/packages/nextjs/test/performance/appRouterInstrumentation.test.ts +++ /dev/null @@ -1,174 +0,0 @@ -import { WINDOW } from '@sentry/react'; -import type { Client, HandlerDataFetch } from '@sentry/types'; -import * as sentryUtils from '@sentry/utils'; -import { JSDOM } from 'jsdom'; - -import { - appRouterInstrumentNavigation, - appRouterInstrumentPageLoad, -} from '../../src/client/routing/appRouterRoutingInstrumentation'; - -const addFetchInstrumentationHandlerSpy = jest.spyOn(sentryUtils, 'addFetchInstrumentationHandler'); - -function setUpPage(url: string) { - const dom = new JSDOM('

nothingness

', { url }); - - // The Next.js routing instrumentations requires a few things to be present on pageload: - // 1. Access to window.document API for `window.document.getElementById` - // 2. Access to window.location API for `window.location.pathname` - Object.defineProperty(WINDOW, 'document', { value: dom.window.document, writable: true }); - Object.defineProperty(WINDOW, 'location', { value: dom.window.document.location, writable: true }); -} - -describe('appRouterInstrumentPageLoad', () => { - const originalGlobalDocument = WINDOW.document; - const originalGlobalLocation = WINDOW.location; - - afterEach(() => { - // Clean up JSDom - Object.defineProperty(WINDOW, 'document', { value: originalGlobalDocument }); - Object.defineProperty(WINDOW, 'location', { value: originalGlobalLocation }); - }); - - it('should create a pageload transactions with the current location name', () => { - setUpPage('https://example.com/some/page?someParam=foobar'); - - const emit = jest.fn(); - const client = { - emit, - } as unknown as Client; - - appRouterInstrumentPageLoad(client); - - expect(emit).toHaveBeenCalledTimes(1); - expect(emit).toHaveBeenCalledWith( - 'startPageLoadSpan', - expect.objectContaining({ - name: '/some/page', - attributes: { - 'sentry.op': 'pageload', - 'sentry.origin': 'auto.pageload.nextjs.app_router_instrumentation', - 'sentry.source': 'url', - }, - }), - undefined, - ); - }); -}); - -describe('appRouterInstrumentNavigation', () => { - const originalGlobalDocument = WINDOW.document; - const originalGlobalLocation = WINDOW.location; - - afterEach(() => { - // Clean up JSDom - Object.defineProperty(WINDOW, 'document', { value: originalGlobalDocument }); - Object.defineProperty(WINDOW, 'location', { value: originalGlobalLocation }); - }); - - it('should create a navigation transactions when a navigation RSC request is sent', () => { - setUpPage('https://example.com/some/page?someParam=foobar'); - let fetchInstrumentationHandlerCallback: (arg: HandlerDataFetch) => void; - - addFetchInstrumentationHandlerSpy.mockImplementationOnce(callback => { - fetchInstrumentationHandlerCallback = callback; - }); - - const emit = jest.fn(); - const client = { - emit, - } as unknown as Client; - - appRouterInstrumentNavigation(client); - - fetchInstrumentationHandlerCallback!({ - args: [ - new URL('https://example.com/some/server/component/page?_rsc=2rs8t'), - { - headers: { - RSC: '1', - }, - }, - ], - fetchData: { method: 'GET', url: 'https://example.com/some/server/component/page?_rsc=2rs8t' }, - startTimestamp: 1337, - }); - - expect(emit).toHaveBeenCalledTimes(1); - expect(emit).toHaveBeenCalledWith('startNavigationSpan', { - name: '/some/server/component/page', - attributes: { - 'sentry.op': 'navigation', - 'sentry.origin': 'auto.navigation.nextjs.app_router_instrumentation', - 'sentry.source': 'url', - }, - }); - }); - - it.each([ - [ - 'no RSC header', - { - args: [ - new URL('https://example.com/some/server/component/page?_rsc=2rs8t'), - { - headers: {}, - }, - ], - fetchData: { method: 'GET', url: 'https://example.com/some/server/component/page?_rsc=2rs8t' }, - startTimestamp: 1337, - }, - ], - [ - 'no GET request', - { - args: [ - new URL('https://example.com/some/server/component/page?_rsc=2rs8t'), - { - headers: { - RSC: '1', - }, - }, - ], - fetchData: { method: 'POST', url: 'https://example.com/some/server/component/page?_rsc=2rs8t' }, - startTimestamp: 1337, - }, - ], - [ - 'prefetch request', - { - args: [ - new URL('https://example.com/some/server/component/page?_rsc=2rs8t'), - { - headers: { - RSC: '1', - 'Next-Router-Prefetch': '1', - }, - }, - ], - fetchData: { method: 'GET', url: 'https://example.com/some/server/component/page?_rsc=2rs8t' }, - startTimestamp: 1337, - }, - ], - ])( - 'should not create navigation transactions for fetch requests that are not navigating RSC requests (%s)', - (_, fetchCallbackData) => { - setUpPage('https://example.com/some/page?someParam=foobar'); - let fetchInstrumentationHandlerCallback: (arg: HandlerDataFetch) => void; - - addFetchInstrumentationHandlerSpy.mockImplementationOnce(callback => { - fetchInstrumentationHandlerCallback = callback; - }); - - const emit = jest.fn(); - const client = { - emit, - } as unknown as Client; - - appRouterInstrumentNavigation(client); - fetchInstrumentationHandlerCallback!(fetchCallbackData); - - expect(emit).toHaveBeenCalledTimes(0); - }, - ); -}); From c0652853b6417472d724349557ce50b49fff403f Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Thu, 19 Sep 2024 15:45:07 +0000 Subject: [PATCH 8/8] Update todo comment --- .../src/client/routing/appRouterRoutingInstrumentation.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/nextjs/src/client/routing/appRouterRoutingInstrumentation.ts b/packages/nextjs/src/client/routing/appRouterRoutingInstrumentation.ts index 9d59d41af2e9..741849c481ab 100644 --- a/packages/nextjs/src/client/routing/appRouterRoutingInstrumentation.ts +++ b/packages/nextjs/src/client/routing/appRouterRoutingInstrumentation.ts @@ -89,7 +89,7 @@ export function appRouterInstrumentNavigation(client: Client): void { routerPatched = true; (['back', 'forward', 'push', 'replace'] as const).forEach(routerFunctionName => { if (router?.[routerFunctionName]) { - // @ts-expect-error TODO + // @ts-expect-error Weird type error related to not knowing how to associate return values with the individual functions - we can just ignore router[routerFunctionName] = new Proxy(router[routerFunctionName], { apply(target, thisArg, argArray) { const span = startBrowserTracingNavigationSpan(client, {