diff --git a/dev-packages/browser-integration-tests/suites/public-api/debug/test.ts b/dev-packages/browser-integration-tests/suites/public-api/debug/test.ts index e4335f4683ba..1867b9a4d35e 100644 --- a/dev-packages/browser-integration-tests/suites/public-api/debug/test.ts +++ b/dev-packages/browser-integration-tests/suites/public-api/debug/test.ts @@ -17,6 +17,15 @@ sentryTest('logs debug messages correctly', async ({ getLocalTestUrl, page }) => await page.goto(url); + if (hasDebug) { + // The initial session capture is deferred to the browser's idle period, so its + // "Discarded session" warning is emitted asynchronously. Wait for it before + // logging so the console message order stays deterministic. + await expect + .poll(() => consoleMessages) + .toContain('Sentry Logger [warn]: Discarded session because of missing or non-string release'); + } + await page.evaluate(() => console.log('test log')); expect(consoleMessages).toEqual( @@ -34,8 +43,8 @@ sentryTest('logs debug messages correctly', async ({ getLocalTestUrl, page }) => 'Sentry Logger [log]: Integration installed: Dedupe', 'Sentry Logger [log]: Integration installed: HttpContext', 'Sentry Logger [log]: Integration installed: CultureContext', - 'Sentry Logger [warn]: Discarded session because of missing or non-string release', 'Sentry Logger [log]: Integration installed: BrowserSession', + 'Sentry Logger [warn]: Discarded session because of missing or non-string release', 'test log', ] : ['[Sentry] Cannot initialize SDK with `debug` option using a non-debug bundle.', 'test log'], diff --git a/dev-packages/browser-integration-tests/suites/sessions/user/subject.js b/dev-packages/browser-integration-tests/suites/sessions/user/subject.js index 16d9653f0b7b..6a99d1f59877 100644 --- a/dev-packages/browser-integration-tests/suites/sessions/user/subject.js +++ b/dev-packages/browser-integration-tests/suites/sessions/user/subject.js @@ -1,5 +1,7 @@ -Sentry.setUser({ - id: '1337', - email: 'user@name.com', - username: 'user1337', +document.getElementById('set-user').addEventListener('click', () => { + Sentry.setUser({ + id: '1337', + email: 'user@name.com', + username: 'user1337', + }); }); diff --git a/dev-packages/browser-integration-tests/suites/sessions/user/template.html b/dev-packages/browser-integration-tests/suites/sessions/user/template.html index e7c463998c02..baf31b734dee 100644 --- a/dev-packages/browser-integration-tests/suites/sessions/user/template.html +++ b/dev-packages/browser-integration-tests/suites/sessions/user/template.html @@ -5,5 +5,6 @@ Navigate + diff --git a/dev-packages/browser-integration-tests/suites/sessions/user/test.ts b/dev-packages/browser-integration-tests/suites/sessions/user/test.ts index f9ba096356ce..d4022e4c0614 100644 --- a/dev-packages/browser-integration-tests/suites/sessions/user/test.ts +++ b/dev-packages/browser-integration-tests/suites/sessions/user/test.ts @@ -2,15 +2,15 @@ import { expect } from '@playwright/test'; import { sentryTest } from '../../../utils/fixtures'; import { waitForSession } from '../../../utils/helpers'; -sentryTest('updates the session when setting the user', async ({ getLocalTestUrl, page }) => { +sentryTest('updates the session when the user is set after the initial session', async ({ getLocalTestUrl, page }) => { const initialSessionPromise = waitForSession(page, s => !!s.init && s.status === 'ok'); - const updatedSessionPromise = waitForSession(page, s => !s.init && s.status === 'ok'); const url = await getLocalTestUrl({ testDir: __dirname }); await page.goto(url); + // The initial session envelope is deferred (sent once the browser is idle). At this point + // no user has been set yet, so it carries no `did`. const initialSession = await initialSessionPromise; - const updatedSession = await updatedSessionPromise; expect(initialSession).toEqual({ attrs: { @@ -26,6 +26,12 @@ sentryTest('updates the session when setting the user', async ({ getLocalTestUrl timestamp: expect.any(String), }); + // Setting the user _after_ the initial session was sent must still be captured as a + // dedicated session update carrying the `did`. + const updatedSessionPromise = waitForSession(page, s => !s.init && s.status === 'ok'); + await page.locator('#set-user').click(); + const updatedSession = await updatedSessionPromise; + expect(updatedSession).toEqual({ ...initialSession, init: false, @@ -42,6 +48,10 @@ sentryTest('includes the user id in the exited session', async ({ getLocalTestUr const initialSession = await initialSessionPromise; + // Set the user after the initial session was sent, then navigate so the (now exited) + // initial session carries the `did`. + await page.locator('#set-user').click(); + const exitedInitialSessionPromise = waitForSession(page, s => !s.init && s.status === 'exited'); await page.locator('#navigate').click(); @@ -79,6 +89,10 @@ sentryTest('includes the user id in the subsequent session', async ({ getLocalTe timestamp: expect.any(String), }); + // Set the user after the initial session was sent, then navigate so the subsequent + // session inherits the `did`. + await page.locator('#set-user').click(); + const secondSessionPromise = waitForSession(page, s => !!s.init && s.status === 'ok' && s.sid !== initialSession.sid); await page.locator('#navigate').click(); diff --git a/packages/browser-utils/src/index.ts b/packages/browser-utils/src/index.ts index e4822bf05b7a..d73f0263d12e 100644 --- a/packages/browser-utils/src/index.ts +++ b/packages/browser-utils/src/index.ts @@ -25,6 +25,8 @@ export { extractNetworkProtocol } from './metrics/utils'; export { trackClsAsSpan, trackInpAsSpan, trackLcpAsSpan } from './metrics/webVitalSpans'; +export { whenIdleOrHidden } from './metrics/web-vitals/lib/whenIdleOrHidden'; + export { addClickKeypressInstrumentationHandler } from './instrument/dom'; export { addHistoryInstrumentationHandler } from './instrument/history'; diff --git a/packages/browser/src/integrations/browsersession.ts b/packages/browser/src/integrations/browsersession.ts index 50af36ea3283..c386ef1fcb00 100644 --- a/packages/browser/src/integrations/browsersession.ts +++ b/packages/browser/src/integrations/browsersession.ts @@ -1,5 +1,5 @@ import { captureSession, debug, defineIntegration, getIsolationScope, startSession } from '@sentry/core/browser'; -import { addHistoryInstrumentationHandler } from '@sentry/browser-utils'; +import { addHistoryInstrumentationHandler, whenIdleOrHidden } from '@sentry/browser-utils'; import { DEBUG_BUILD } from '../debug-build'; import { WINDOW } from '../helpers'; @@ -41,7 +41,22 @@ export const browserSessionIntegration = defineIntegration((options: BrowserSess // Automatically captured sessions are akin to page views, and thus we // discard their duration. startSession({ ignoreDuration: true }); - captureSession(); + + // Sending the session envelope synchronously in `init()` runs the full send + // pipeline during page load, competing with critical resources for the network and + // adding overhead that measurably hurts LCP. We defer the initial send until the + // browser is idle; `whenIdleOrHidden` flushes it on page-hide so we don't lose short + // (page-view-like) sessions. + let initialSessionSent = false; + whenIdleOrHidden(() => { + // A navigation (in `'route'` lifecycle) may start and send a new session before this + // deferred callback fires. In that case the current session was already sent, so + // re-capturing here would send it a second time - guard against that. + if (!initialSessionSent) { + captureSession(); + initialSessionSent = true; + } + }); // User data can be set at any time, for example async after Sentry.init has run and the initial session // envelope was already sent, but still on the initial page. @@ -58,9 +73,15 @@ export const browserSessionIntegration = defineIntegration((options: BrowserSess const maybeNewUser = scope.getUser(); // sessions only care about user id and ip address, so we only need to capture the session if the user has changed if (previousUser?.id !== maybeNewUser?.id || previousUser?.ip_address !== maybeNewUser?.ip_address) { - // the scope class already writes the user to its session, so we only need to capture the session here - captureSession(); previousUser = maybeNewUser; + // Only emit a dedicated update envelope for user data that arrives _after_ the + // deferred initial session was sent. User data set during page load is already + // reflected in that session (the scope writes it onto the session), so capturing + // here would send a redundant envelope - and do so during page load, which is + // exactly the overhead we're deferring away from. + if (initialSessionSent) { + captureSession(); + } } }); @@ -71,6 +92,9 @@ export const browserSessionIntegration = defineIntegration((options: BrowserSess if (from !== to) { startSession({ ignoreDuration: true }); captureSession(); + // A session has now been sent, so the deferred initial capture (if still pending) + // must not re-send this navigation session. + initialSessionSent = true; } }); } diff --git a/packages/browser/test/integrations/browsersession.test.ts b/packages/browser/test/integrations/browsersession.test.ts new file mode 100644 index 000000000000..7d42fb7e3715 --- /dev/null +++ b/packages/browser/test/integrations/browsersession.test.ts @@ -0,0 +1,190 @@ +/** + * @vitest-environment jsdom + */ + +import type * as BrowserUtils from '@sentry/browser-utils'; +import type { Scope, User } from '@sentry/core/browser'; +import * as SentryCore from '@sentry/core/browser'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import { browserSessionIntegration } from '../../src/integrations/browsersession'; + +const scopeHolder = vi.hoisted(() => ({ current: undefined as unknown as FakeIsolationScope })); + +const historyHandlers = vi.hoisted(() => ({ current: [] as Array<(data: { from?: string; to?: string }) => void> })); + +vi.mock('@sentry/core/browser', async importActual => { + const actual = (await importActual()) as typeof SentryCore; + return { + ...actual, + startSession: vi.fn(), + captureSession: vi.fn(), + getIsolationScope: () => scopeHolder.current, + }; +}); + +// Capture the registered history handler so navigation can be driven deterministically, +// while keeping the real `whenIdleOrHidden` (the tests drive its timers/events directly). +vi.mock('@sentry/browser-utils', async importActual => { + const actual = (await importActual()) as typeof BrowserUtils; + return { + ...actual, + addHistoryInstrumentationHandler: (handler: (data: { from?: string; to?: string }) => void) => { + historyHandlers.current.push(handler); + }, + }; +}); + +function navigate(from: string, to: string): void { + historyHandlers.current.forEach(handler => handler({ from, to })); +} + +interface FakeIsolationScope { + getUser: () => User | undefined; + addScopeListener: (cb: (scope: Scope) => void) => void; + setUser: (user: User | undefined) => void; +} + +/** + * Minimal isolation-scope stand-in so we can drive the integration's scope listener + * deterministically (and in isolation from the global scope) across tests. + */ +function createFakeIsolationScope(initialUser?: User): FakeIsolationScope { + let user = initialUser; + let listener: ((scope: Scope) => void) | undefined; + return { + getUser: () => user, + addScopeListener: cb => { + listener = cb; + }, + setUser: nextUser => { + user = nextUser; + listener?.({ getUser: () => user } as Scope); + }, + }; +} + +function setVisibilityState(state: DocumentVisibilityState): void { + Object.defineProperty(document, 'visibilityState', { value: state, configurable: true }); +} + +function setupBrowserSession(options?: Parameters[0]): void { + const integration = browserSessionIntegration(options); + integration.setupOnce?.(); +} + +describe('browserSessionIntegration', () => { + beforeEach(() => { + vi.clearAllMocks(); + vi.useFakeTimers(); + // `requestIdleCallback` is unavailable in jsdom, so `whenIdleOrHidden` falls back to + // `setTimeout` which we drive via fake timers to simulate the browser going idle. + delete (globalThis as { requestIdleCallback?: unknown }).requestIdleCallback; + setVisibilityState('visible'); + scopeHolder.current = createFakeIsolationScope(); + historyHandlers.current = []; + }); + + afterEach(() => { + vi.useRealTimers(); + }); + + it('starts the session synchronously but defers the initial capture until the browser is idle', () => { + setupBrowserSession({ lifecycle: 'page' }); + + expect(SentryCore.startSession).toHaveBeenCalledTimes(1); + expect(SentryCore.startSession).toHaveBeenCalledWith({ ignoreDuration: true }); + + // The send must not happen synchronously during init. + expect(SentryCore.captureSession).not.toHaveBeenCalled(); + + vi.runAllTimers(); + + expect(SentryCore.captureSession).toHaveBeenCalledTimes(1); + }); + + it('captures the session immediately when the page is already hidden', () => { + setVisibilityState('hidden'); + + setupBrowserSession({ lifecycle: 'page' }); + + expect(SentryCore.captureSession).toHaveBeenCalledTimes(1); + }); + + it('flushes the deferred session when the page is hidden before the browser goes idle', () => { + setupBrowserSession({ lifecycle: 'page' }); + expect(SentryCore.captureSession).not.toHaveBeenCalled(); + + window.dispatchEvent(new Event('pagehide')); + expect(SentryCore.captureSession).toHaveBeenCalledTimes(1); + + // The idle fallback must not send the session a second time. + vi.runAllTimers(); + expect(SentryCore.captureSession).toHaveBeenCalledTimes(1); + }); + + it('does not send a separate envelope for user data set before the initial capture', () => { + setupBrowserSession({ lifecycle: 'page' }); + + // User set during page load (before idle): folded into the deferred initial session, + // so it must not trigger its own send. + scopeHolder.current.setUser({ id: '1337' }); + expect(SentryCore.captureSession).not.toHaveBeenCalled(); + + vi.runAllTimers(); + + // Only the (single) deferred initial session is sent. + expect(SentryCore.captureSession).toHaveBeenCalledTimes(1); + }); + + it('captures an update when user data changes after the initial capture', () => { + setupBrowserSession({ lifecycle: 'page' }); + vi.runAllTimers(); + expect(SentryCore.captureSession).toHaveBeenCalledTimes(1); + + // User set after the initial session was sent: emits a dedicated update envelope. + scopeHolder.current.setUser({ id: '1337' }); + expect(SentryCore.captureSession).toHaveBeenCalledTimes(2); + }); + + it('does not re-send the navigation session when navigation happens before the deferred initial capture', () => { + // Default lifecycle is 'route', which also registers the navigation handler. + setupBrowserSession(); + + // The initial capture is deferred, so nothing is sent synchronously. + expect(SentryCore.captureSession).not.toHaveBeenCalled(); + + // User navigates before the browser goes idle: a new session is started and sent. + navigate('/initial', '/next'); + expect(SentryCore.startSession).toHaveBeenCalledTimes(2); + expect(SentryCore.captureSession).toHaveBeenCalledTimes(1); + + // The deferred idle callback now fires. Since the navigation already sent the current + // session, the deferred capture must not re-send it. + vi.runAllTimers(); + expect(SentryCore.captureSession).toHaveBeenCalledTimes(1); + }); + + it('still captures a session on navigation that happens after the initial capture', () => { + setupBrowserSession(); + + vi.runAllTimers(); + expect(SentryCore.captureSession).toHaveBeenCalledTimes(1); + + navigate('/initial', '/next'); + expect(SentryCore.startSession).toHaveBeenCalledTimes(2); + expect(SentryCore.captureSession).toHaveBeenCalledTimes(2); + }); + + it('does not capture again when the user reference changes but id and ip stay the same', () => { + setupBrowserSession({ lifecycle: 'page' }); + vi.runAllTimers(); + expect(SentryCore.captureSession).toHaveBeenCalledTimes(1); + + scopeHolder.current.setUser({ id: '1337', email: 'a@example.com' }); + expect(SentryCore.captureSession).toHaveBeenCalledTimes(2); + + // Same id and ip_address (only unrelated fields change) -> no extra capture. + scopeHolder.current.setUser({ id: '1337', email: 'b@example.com' }); + expect(SentryCore.captureSession).toHaveBeenCalledTimes(2); + }); +}); diff --git a/packages/nextjs/test/clientSdk.test.ts b/packages/nextjs/test/clientSdk.test.ts index 090ef61fe5cd..1a5aa116ca9a 100644 --- a/packages/nextjs/test/clientSdk.test.ts +++ b/packages/nextjs/test/clientSdk.test.ts @@ -17,12 +17,15 @@ 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 }); +Object.defineProperty(global, 'removeEventListener', { value: () => undefined, writable: true }); const originalGlobalDocument = WINDOW.document; const originalGlobalLocation = WINDOW.location; const originalNavigator = WINDOW.navigator; // eslint-disable-next-line @typescript-eslint/unbound-method const originalGlobalAddEventListener = WINDOW.addEventListener; +// eslint-disable-next-line @typescript-eslint/unbound-method +const originalGlobalRemoveEventListener = WINDOW.removeEventListener; afterAll(() => { // Clean up JSDom @@ -30,6 +33,7 @@ afterAll(() => { Object.defineProperty(WINDOW, 'location', { value: originalGlobalLocation }); Object.defineProperty(WINDOW, 'navigator', { value: originalNavigator, writable: true, configurable: true }); Object.defineProperty(WINDOW, 'addEventListener', { value: originalGlobalAddEventListener }); + Object.defineProperty(WINDOW, 'removeEventListener', { value: originalGlobalRemoveEventListener }); }); function findIntegrationByName(integrations: Integration[] = [], name: string): Integration | undefined { diff --git a/packages/nextjs/test/config/conflictingDebugOptions.test.ts b/packages/nextjs/test/config/conflictingDebugOptions.test.ts index 5e7a46997b2b..6356fcc9bfdc 100644 --- a/packages/nextjs/test/config/conflictingDebugOptions.test.ts +++ b/packages/nextjs/test/config/conflictingDebugOptions.test.ts @@ -19,6 +19,7 @@ describe('debug: true + removeDebugLogging warning', () => { let originalDocument: unknown; let originalLocation: unknown; let originalAddEventListener: unknown; + let originalRemoveEventListener: unknown; beforeAll(async () => { // Pre-warm V8 compilation cache for the large SDK module graphs. @@ -34,16 +35,19 @@ describe('debug: true + removeDebugLogging warning', () => { originalDocument = (globalThis as any).document; originalLocation = (globalThis as any).location; originalAddEventListener = (globalThis as any).addEventListener; + originalRemoveEventListener = (globalThis as any).removeEventListener; Object.defineProperty(globalThis, 'document', { value: dom.window.document, writable: true }); Object.defineProperty(globalThis, 'location', { value: dom.window.location, writable: true }); Object.defineProperty(globalThis, 'addEventListener', { value: () => undefined, writable: true }); + Object.defineProperty(globalThis, 'removeEventListener', { value: () => undefined, writable: true }); }); afterAll(() => { Object.defineProperty(globalThis, 'document', { value: originalDocument, writable: true }); Object.defineProperty(globalThis, 'location', { value: originalLocation, writable: true }); Object.defineProperty(globalThis, 'addEventListener', { value: originalAddEventListener, writable: true }); + Object.defineProperty(globalThis, 'removeEventListener', { value: originalRemoveEventListener, writable: true }); }); afterEach(() => {