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 a967ebd7eaef..4d4c89316408 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 @@ -81,10 +81,7 @@ test('Creates a navigation span for `router.replace()`', async ({ page }) => { expect(await navigationSpanPromise).toBeDefined(); }); -// Skipped rather than relaxed to `browser.popstate`: under span streaming these navigations lose the -// back/forward distinction, which looks like a regression rather than intended behaviour. -// See https://github.com/getsentry/sentry-javascript/issues/23909 -test.skip('Creates a navigation span for `router.back()`', async ({ page }) => { +test('Creates a navigation span for `router.back()`', async ({ page }) => { const navigationSpanPromise = waitForStreamedSpan('nextjs-app-dir', span => { return span.name === `/navigation/:param/router-back` && getSpanOp(span) === 'navigation'; }); @@ -101,10 +98,7 @@ test.skip('Creates a navigation span for `router.back()`', async ({ page }) => { expect(navigationSpan.attributes['navigation.type']?.value).toMatch(/router\.(back|traverse)/); }); -// Skipped rather than relaxed to `browser.popstate`: under span streaming these navigations lose the -// back/forward distinction, which looks like a regression rather than intended behaviour. -// See https://github.com/getsentry/sentry-javascript/issues/23909 -test.skip('Creates a navigation span for `router.forward()`', async ({ page }) => { +test('Creates a navigation span for `router.forward()`', async ({ page }) => { const navigationSpanPromise = waitForStreamedSpan('nextjs-app-dir', span => { return ( span.name === `/navigation/:param/router-push` && diff --git a/packages/nextjs/src/client/index.ts b/packages/nextjs/src/client/index.ts index 5c5d3ffc2c85..3c8c091d2b91 100644 --- a/packages/nextjs/src/client/index.ts +++ b/packages/nextjs/src/client/index.ts @@ -11,7 +11,6 @@ import { getVercelEnv } from '../common/getVercelEnv'; import { isRedirectNavigationError } from '../common/nextNavigationErrorUtils'; import { browserTracingIntegration } from './browserTracingIntegration'; import { nextjsClientStackFrameNormalizationIntegration } from './clientNormalizationIntegration'; -import { INCOMPLETE_APP_ROUTER_INSTRUMENTATION_TRANSACTION_NAME } from './routing/appRouterRoutingInstrumentation'; import { removeIsrSsgTraceMetaTags } from './routing/isrRoutingTracing'; import { applyTunnelRouteOption } from './tunnelRoute'; @@ -74,12 +73,8 @@ export function init(options: BrowserOptions): Client | undefined { opts.ignoreSpans = [ ...(opts.ignoreSpans || []), - // we filter out segment spans for /404 pages + // we filter out segment spans for /404 pages (exact match, so a string match isn't safe) /^\/404$/, - // segment spans where we didn't get a reasonable transaction name - // in this case, constructing a dynamic RegExp is fine because the variable is a constant - // we need to ensure to exact-match, so a string match isn't safe (same for /404 above) - new RegExp(`^${INCOMPLETE_APP_ROUTER_INSTRUMENTATION_TRANSACTION_NAME}$`), ]; const client = reactInit(opts); diff --git a/packages/nextjs/src/client/routing/appRouterRoutingInstrumentation.ts b/packages/nextjs/src/client/routing/appRouterRoutingInstrumentation.ts index aff86d2c2e37..eb3edbf3eced 100644 --- a/packages/nextjs/src/client/routing/appRouterRoutingInstrumentation.ts +++ b/packages/nextjs/src/client/routing/appRouterRoutingInstrumentation.ts @@ -6,6 +6,7 @@ import { PAGELOAD_SPAN_NAME_FALLBACK, SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN, filterCollectedUrl, + timestampInSeconds, } from '@sentry/core'; import { startBrowserTracingNavigationSpan, @@ -38,7 +39,44 @@ function setNavigationSpanUrlAttributes(span: Span, urlPath: string, urlOrPath: }); } -export const INCOMPLETE_APP_ROUTER_INSTRUMENTATION_TRANSACTION_NAME = 'incomplete-app-router-transaction'; +/** + * `router.back()` and `router.forward()` carry no destination, so their navigation span can only be + * started once the resulting `popstate` event tells us where we ended up. Until then, this remembers + * which router method triggered the traversal and when, so the span still gets the router's + * navigation type and starts at the router call rather than at the `popstate`. + */ +interface PendingHistoryTraversal { + navigationType: 'router.back' | 'router.forward'; + startTime: number; +} + +let pendingHistoryTraversal: PendingHistoryTraversal | undefined; +let pendingHistoryTraversalTimeout: ReturnType | undefined; + +/** + * A `back()`/`forward()` without a matching history entry never fires `popstate`. Without an expiry, + * a later unrelated `popstate` (e.g. the browser's back button) would be attributed to that stale + * router call. Browsers dispatch the `popstate` of a same-document traversal within a few + * milliseconds, so anything older than this is not the traversal we are waiting for. A timer rather + * than a timestamp comparison keeps this tolerant of a blocked main thread, which delays the + * `popstate` and the timer alike. + */ +const PENDING_HISTORY_TRAVERSAL_TIMEOUT_MS = 1000; + +function setPendingHistoryTraversal(navigationType: PendingHistoryTraversal['navigationType']): void { + clearTimeout(pendingHistoryTraversalTimeout); + pendingHistoryTraversal = { navigationType, startTime: timestampInSeconds() }; + pendingHistoryTraversalTimeout = setTimeout(() => { + pendingHistoryTraversal = undefined; + }, PENDING_HISTORY_TRAVERSAL_TIMEOUT_MS); +} + +function takePendingHistoryTraversal(): PendingHistoryTraversal | undefined { + clearTimeout(pendingHistoryTraversalTimeout); + const traversal = pendingHistoryTraversal; + pendingHistoryTraversal = undefined; + return traversal; +} /** * This mutable keeps track of what router navigation instrumentation mechanism we are using. @@ -164,7 +202,10 @@ export function appRouterInstrumentNavigation(client: Client): void { // With span streaming, span names have to be low cardinality, so we can't fall back to the URL. const spanName = parameterizedPathname ?? (hasSpanStreamingEnabled(client) ? NAVIGATION_SPAN_NAME_FALLBACK : pathname); - if (currentRouterPatchingNavigationSpanRef.current?.isRecording()) { + const traversal = takePendingHistoryTraversal(); + // A traversal triggered through the router always gets its own span: an open router-patch span + // here would be a `push()`/`replace()` that the user navigated away from again. + if (!traversal && currentRouterPatchingNavigationSpanRef.current?.isRecording()) { currentRouterPatchingNavigationSpanRef.current.updateName(spanName); currentRouterPatchingNavigationSpanRef.current.setAttribute( SENTRY_SEGMENT_NAME_SOURCE, @@ -179,14 +220,17 @@ export function appRouterInstrumentNavigation(client: Client): void { client, { name: spanName, + startTime: traversal?.startTime, attributes: { [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.navigation.nextjs.app_router_instrumentation', [SENTRY_SEGMENT_NAME_SOURCE]: parameterizedPathname ? 'route' : 'url', - 'navigation.type': 'browser.popstate', + 'navigation.type': traversal?.navigationType ?? 'browser.popstate', ...(parameterizedPathname && { [URL_TEMPLATE]: parameterizedPathname }), }, }, - { url: getAbsoluteUrl(pathname) }, + // The full location rather than just the pathname, so the span's `url.full` keeps the + // (filtered) query string like the update path above does. + { url: WINDOW.location.href }, ); } }); @@ -252,56 +296,34 @@ function patchRouter(client: Client, router: NextRouter, currentNavigationSpanRe return target.apply(thisArg, argArray); } - let transactionName = INCOMPLETE_APP_ROUTER_INSTRUMENTATION_TRANSACTION_NAME; - const transactionAttributes: Record = { - [SENTRY_OP]: NAVIGATION, - [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.navigation.nextjs.app_router_instrumentation', - [SENTRY_SEGMENT_NAME_SOURCE]: 'url', - }; + if (routerFunctionName === 'back' || routerFunctionName === 'forward') { + setPendingHistoryTraversal(`router.${routerFunctionName}`); + return target.apply(thisArg, argArray); + } const href = argArray[0]; const basePath = process.env._sentryBasePath ?? globalWithInjectedBasePath._sentryBasePath; const normalizedHref = basePath && typeof href === 'string' && !href.startsWith(basePath) ? `${basePath}${href}` : href; - if (routerFunctionName === 'push') { - transactionName = stripTrailingSlash(transactionNameifyRouterArgument(normalizedHref)); - transactionAttributes['navigation.type'] = 'router.push'; - } else if (routerFunctionName === 'replace') { - transactionName = stripTrailingSlash(transactionNameifyRouterArgument(normalizedHref)); - transactionAttributes['navigation.type'] = 'router.replace'; - } else if (routerFunctionName === 'back') { - transactionAttributes['navigation.type'] = 'router.back'; - } else if (routerFunctionName === 'forward') { - transactionAttributes['navigation.type'] = 'router.forward'; - } - + const transactionName = stripTrailingSlash(transactionNameifyRouterArgument(normalizedHref)); const parameterizedPathname = maybeParameterizeRoute(transactionName); - const navigationUrl = - routerFunctionName === 'back' || routerFunctionName === 'forward' - ? undefined - : getAbsoluteUrl(normalizedHref); - - // The incomplete-instrumentation placeholder is a static name, so it is low cardinality - // already, and keeping it is what makes the `ignoreSpans` entry filtering those spans match. - const isPlaceholderName = transactionName === INCOMPLETE_APP_ROUTER_INSTRUMENTATION_TRANSACTION_NAME; - currentNavigationSpanRef.current = startBrowserTracingNavigationSpan( client, { // With span streaming, span names have to be low cardinality, so we can't fall back to the URL. name: parameterizedPathname ?? - (isPlaceholderName || !hasSpanStreamingEnabled(client) - ? transactionName - : NAVIGATION_SPAN_NAME_FALLBACK), + (hasSpanStreamingEnabled(client) ? NAVIGATION_SPAN_NAME_FALLBACK : transactionName), attributes: { - ...transactionAttributes, + [SENTRY_OP]: NAVIGATION, + [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.navigation.nextjs.app_router_instrumentation', [SENTRY_SEGMENT_NAME_SOURCE]: parameterizedPathname ? 'route' : 'url', + 'navigation.type': `router.${routerFunctionName}`, ...(parameterizedPathname && { [URL_TEMPLATE]: parameterizedPathname }), }, }, - navigationUrl ? { url: navigationUrl } : undefined, + { url: getAbsoluteUrl(normalizedHref) }, ); return target.apply(thisArg, argArray); diff --git a/packages/nextjs/test/client/appRouterRoutingInstrumentation.test.ts b/packages/nextjs/test/client/appRouterRoutingInstrumentation.test.ts new file mode 100644 index 000000000000..098f54581b58 --- /dev/null +++ b/packages/nextjs/test/client/appRouterRoutingInstrumentation.test.ts @@ -0,0 +1,202 @@ +// @vitest-environment jsdom +import type { Client } from '@sentry/core'; +import type * as SentryCore from '@sentry/core'; +import type * as SentryReact from '@sentry/react'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import type * as AppRouterInstrumentation from '../../src/client/routing/appRouterRoutingInstrumentation'; +import type { RouteManifest } from '../../src/config/manifest/types'; + +type Core = typeof SentryCore; +type React = typeof SentryReact; +type Instrumentation = typeof AppRouterInstrumentation; + +interface NextRouter { + back: () => void; + forward: () => void; + push: (target: string) => void; + replace: (target: string) => void; +} + +const globalWithNext = globalThis as typeof globalThis & { + next?: { router?: NextRouter }; + _sentryRouteManifest?: string; +}; + +const manifest: RouteManifest = { + staticRoutes: [{ path: '/navigation' }], + dynamicRoutes: [ + { + path: '/navigation/:param/router-back', + regex: '^/navigation/([^/]+)/router-back$', + paramNames: ['param'], + hasOptionalPrefix: false, + }, + ], + isrRoutes: [], +}; + +function sleep(ms: number): Promise { + return new Promise(resolve => setTimeout(resolve, ms)); +} + +/** + * The instrumentation module keeps its routing state (patched routers, the current navigation span, + * the popstate listener) at module level, so every test gets fresh copies of it and of the SDK + * packages it imports. The `popstate` listeners of earlier tests stay registered on `window`, but + * they only reach clients that are no longer current, whose navigation handlers bail out early. + */ +async function setup(traceLifecycle: 'stream' | 'static'): Promise<{ + core: Core; + router: NextRouter; + client: Client; +}> { + vi.resetModules(); + const core: Core = await import('@sentry/core'); + const react: React = await import('@sentry/react'); + const instrumentation: Instrumentation = await import('../../src/client/routing/appRouterRoutingInstrumentation'); + + const client = new react.BrowserClient({ + dsn: 'http://examplePublicKey@localhost/0', + transport: () => core.createTransport({ recordDroppedEvent: () => undefined }, () => core.resolvedSyncPromise({})), + stackParser: () => [], + tracesSampleRate: 1, + traceLifecycle, + integrations: [react.browserTracingIntegration({ instrumentPageLoad: false, instrumentNavigation: false })], + }); + core.setCurrentClient(client); + client.init(); + + const router: NextRouter = { back: vi.fn(), forward: vi.fn(), push: vi.fn(), replace: vi.fn() }; + const originalBack = router.back; + globalWithNext.next = { router }; + + instrumentation.appRouterInstrumentNavigation(client); + await vi.waitFor(() => expect(router.back).not.toBe(originalBack)); + + return { core, router, client }; +} + +describe('appRouterInstrumentNavigation (router-patch mode)', () => { + beforeEach(() => { + globalWithNext._sentryRouteManifest = JSON.stringify(manifest); + window.history.replaceState({}, '', '/navigation'); + }); + + afterEach(() => { + vi.useRealTimers(); + delete globalWithNext.next; + delete globalWithNext._sentryRouteManifest; + }); + + describe.each(['stream', 'static'] as const)('with traceLifecycle %s', traceLifecycle => { + it('tags the navigation span of `router.back()` with `router.back` and starts it at the call', async () => { + const { core, router } = await setup(traceLifecycle); + + const beforeCall = core.timestampInSeconds(); + router.back(); + const afterCall = core.timestampInSeconds(); + + await sleep(30); + window.history.replaceState({}, '', '/navigation/1337/router-back?foo=bar'); + window.dispatchEvent(new PopStateEvent('popstate')); + + const span = core.getActiveSpan(); + expect(span).toBeDefined(); + const spanJson = core.spanToJSON(span!); + expect(spanJson.name).toBe('/navigation/:param/router-back'); + expect(spanJson.attributes).toEqual( + expect.objectContaining({ + 'sentry.op': 'navigation', + 'navigation.type': 'router.back', + 'url.template': '/navigation/:param/router-back', + 'url.path': '/navigation/1337/router-back', + 'url.full': 'http://localhost:3000/navigation/1337/router-back?foo=bar', + }), + ); + expect(spanJson.start_timestamp).toBeGreaterThanOrEqual(beforeCall); + expect(spanJson.start_timestamp).toBeLessThanOrEqual(afterCall); + }); + + it('tags the navigation span of `router.forward()` with `router.forward`', async () => { + const { core, router } = await setup(traceLifecycle); + + router.forward(); + window.history.replaceState({}, '', '/navigation/1337/router-back'); + window.dispatchEvent(new PopStateEvent('popstate')); + + const span = core.getActiveSpan(); + expect(span).toBeDefined(); + expect(core.spanToJSON(span!).attributes).toEqual( + expect.objectContaining({ 'navigation.type': 'router.forward' }), + ); + }); + + it('keeps the router call when the main thread is blocked until the popstate', async () => { + const { core, router } = await setup(traceLifecycle); + + router.back(); + const blockedUntil = Date.now() + 1100; + while (Date.now() < blockedUntil) { + // busy-wait + } + window.history.replaceState({}, '', '/navigation/1337/router-back'); + window.dispatchEvent(new PopStateEvent('popstate')); + + const span = core.getActiveSpan(); + expect(span).toBeDefined(); + expect(core.spanToJSON(span!).attributes).toEqual(expect.objectContaining({ 'navigation.type': 'router.back' })); + }); + + it('starts a new span for `router.back()` while a `router.push()` span is still open', async () => { + const { core, router } = await setup(traceLifecycle); + + router.push('/navigation'); + const pushSpan = core.getActiveSpan(); + expect(pushSpan).toBeDefined(); + + router.back(); + window.history.replaceState({}, '', '/navigation/1337/router-back'); + window.dispatchEvent(new PopStateEvent('popstate')); + + const span = core.getActiveSpan(); + expect(span).toBeDefined(); + expect(span).not.toBe(pushSpan); + expect(core.spanToJSON(span!).attributes).toEqual(expect.objectContaining({ 'navigation.type': 'router.back' })); + expect(core.spanToJSON(pushSpan!).attributes).toEqual( + expect.objectContaining({ 'navigation.type': 'router.push' }), + ); + expect(core.spanToJSON(pushSpan!).end_timestamp).toBeDefined(); + }); + + it('tags a popstate without a preceding router call with `browser.popstate`', async () => { + const { core } = await setup(traceLifecycle); + + window.history.replaceState({}, '', '/navigation/1337/router-back'); + window.dispatchEvent(new PopStateEvent('popstate')); + + const span = core.getActiveSpan(); + expect(span).toBeDefined(); + const spanJson = core.spanToJSON(span!); + expect(spanJson.name).toBe('/navigation/:param/router-back'); + expect(spanJson.attributes).toEqual(expect.objectContaining({ 'navigation.type': 'browser.popstate' })); + }); + + it('does not carry a router call over to a later, unrelated popstate', async () => { + const { core, router } = await setup(traceLifecycle); + + vi.useFakeTimers({ toFake: ['setTimeout', 'clearTimeout'] }); + router.forward(); + // A `forward()` without a forward history entry never fires `popstate`. + vi.advanceTimersByTime(1000); + + window.history.replaceState({}, '', '/navigation/1337/router-back'); + window.dispatchEvent(new PopStateEvent('popstate')); + + const span = core.getActiveSpan(); + expect(span).toBeDefined(); + expect(core.spanToJSON(span!).attributes).toEqual( + expect.objectContaining({ 'navigation.type': 'browser.popstate' }), + ); + }); + }); +}); diff --git a/packages/nextjs/test/clientSdk.test.ts b/packages/nextjs/test/clientSdk.test.ts index 3aa69f92f8b3..fc59741506d4 100644 --- a/packages/nextjs/test/clientSdk.test.ts +++ b/packages/nextjs/test/clientSdk.test.ts @@ -5,7 +5,6 @@ import { getClient, WINDOW } from '@sentry/react'; import { JSDOM } from 'jsdom'; import { afterAll, afterEach, describe, expect, it, vi } from 'vitest'; import { breadcrumbsIntegration, browserTracingIntegration, init } from '../src/client'; -import { INCOMPLETE_APP_ROUTER_INSTRUMENTATION_TRANSACTION_NAME } from '../src/client/routing/appRouterRoutingInstrumentation'; const reactInit = vi.spyOn(SentryReact, 'init'); const debugLogSpy = vi.spyOn(debug, 'log'); @@ -101,19 +100,6 @@ describe('Client init()', () => { expect(debugLogSpy).toHaveBeenCalledWith(expect.stringContaining('matches `ignoreSpans`')); }); - it('drops incomplete navigation transactions', () => { - init({ dsn: TEST_DSN_404, tracesSampleRate: 1.0 }); - const transportSend = vi.spyOn(getClient()!.getTransport()!, 'send'); - - // Ensure we have no current span, so our next span is a transaction - SentryReact.withActiveSpan(null, () => { - SentryReact.startInactiveSpan({ name: INCOMPLETE_APP_ROUTER_INSTRUMENTATION_TRANSACTION_NAME })?.end(); - }); - - expect(transportSend).not.toHaveBeenCalled(); - expect(debugLogSpy).toHaveBeenCalledWith(expect.stringContaining('matches `ignoreSpans`')); - }); - describe('span streaming', () => { it('drops /404 segment spans', () => { init({ dsn: TEST_DSN_404, tracesSampleRate: 1.0, traceLifecycle: 'stream' }); @@ -125,18 +111,6 @@ describe('Client init()', () => { expect(debugLogSpy).toHaveBeenCalledWith(expect.stringContaining('matches `ignoreSpans`')); }); - it('drops incomplete navigation segment spans', () => { - init({ dsn: TEST_DSN_404, tracesSampleRate: 1.0, traceLifecycle: 'stream' }); - - // Ensure we have no current span, so our next span is a segment span - const span = SentryReact.withActiveSpan(null, () => - SentryReact.startInactiveSpan({ name: INCOMPLETE_APP_ROUTER_INSTRUMENTATION_TRANSACTION_NAME }), - ); - - expect(span).toBeInstanceOf(SentryNonRecordingSpan); - expect(debugLogSpy).toHaveBeenCalledWith(expect.stringContaining('matches `ignoreSpans`')); - }); - it('drops /404 non-segment spans', () => { init({ dsn: TEST_DSN_404, tracesSampleRate: 1.0, traceLifecycle: 'stream' });