diff --git a/dev-packages/bun-integration-tests/node-suites/excludes.ts b/dev-packages/bun-integration-tests/node-suites/excludes.ts index c9a89b180612..0ea4c85af423 100644 --- a/dev-packages/bun-integration-tests/node-suites/excludes.ts +++ b/dev-packages/bun-integration-tests/node-suites/excludes.ts @@ -180,6 +180,7 @@ const FETCH_INTEGRATION_DIFFERS = [ // that sets `emit` back to the `emit` it had before its first request gets no spans after that. const HTTP_SERVER_OPTIONS_IGNORED = [ 'suites/sessions/exited-session-aggregate/test.ts', + 'suites/sessions/server-without-process-session/test.ts', 'suites/tracing/httpIntegration/test.ts', ]; diff --git a/dev-packages/node-integration-tests/suites/sessions/process-session/instrument.mjs b/dev-packages/node-integration-tests/suites/sessions/process-session/instrument.mjs new file mode 100644 index 000000000000..9ffde125d498 --- /dev/null +++ b/dev-packages/node-integration-tests/suites/sessions/process-session/instrument.mjs @@ -0,0 +1,8 @@ +import * as Sentry from '@sentry/node'; +import { loggingTransport } from '@sentry-internal/node-integration-tests'; + +Sentry.init({ + dsn: 'https://public@dsn.ingest.sentry.io/1337', + release: '1.0', + transport: loggingTransport, +}); diff --git a/dev-packages/node-integration-tests/suites/sessions/process-session/scenario.mjs b/dev-packages/node-integration-tests/suites/sessions/process-session/scenario.mjs new file mode 100644 index 000000000000..329cecf7fd16 --- /dev/null +++ b/dev-packages/node-integration-tests/suites/sessions/process-session/scenario.mjs @@ -0,0 +1,3 @@ +import * as Sentry from '@sentry/node'; + +Sentry.captureException(new Error('Scenario error')); diff --git a/dev-packages/node-integration-tests/suites/sessions/process-session/test.ts b/dev-packages/node-integration-tests/suites/sessions/process-session/test.ts new file mode 100644 index 000000000000..a7e9260bd16c --- /dev/null +++ b/dev-packages/node-integration-tests/suites/sessions/process-session/test.ts @@ -0,0 +1,18 @@ +import { afterAll, test } from 'vitest'; +import { join } from 'path'; +import { cleanupChildProcesses, createRunner } from '../../../utils/runner'; + +afterAll(() => { + cleanupChildProcesses(); +}); + +test('sends a process session for a process that does not serve requests', async () => { + await createRunner(__dirname, 'scenario.mjs') + .withInstrument(join(__dirname, 'instrument.mjs')) + .ignore('event') + .unignore('session') + .expect({ session: { init: true, status: 'ok', errors: 1 } }) + .expect({ session: { init: false, status: 'exited', errors: 1 } }) + .start() + .completed(); +}); diff --git a/dev-packages/node-integration-tests/suites/sessions/server-without-process-session/server-with-startup-error.mjs b/dev-packages/node-integration-tests/suites/sessions/server-without-process-session/server-with-startup-error.mjs new file mode 100644 index 000000000000..0eed9d1fe8a5 --- /dev/null +++ b/dev-packages/node-integration-tests/suites/sessions/server-without-process-session/server-with-startup-error.mjs @@ -0,0 +1,20 @@ +import * as Sentry from '@sentry/node'; +import { startExpressServerAndSendPortToRunner } from '@sentry-internal/node-integration-tests'; +import express from 'express'; + +Sentry.captureException(new Error('Startup error')); +// Event processing is async; make sure the error has updated the process session before the first request. +await Sentry.flush(); + +const app = express(); + +app.get('/test/success', (_req, res) => { + res.send('Success!'); +}); + +app.get('/test/error_handled', (_req, res) => { + Sentry.captureException(new Error('Request error')); + res.send('Handled!'); +}); + +startExpressServerAndSendPortToRunner(app); diff --git a/dev-packages/node-integration-tests/suites/sessions/server-without-process-session/test.ts b/dev-packages/node-integration-tests/suites/sessions/server-without-process-session/test.ts new file mode 100644 index 000000000000..f22ea4c47ad5 --- /dev/null +++ b/dev-packages/node-integration-tests/suites/sessions/server-without-process-session/test.ts @@ -0,0 +1,56 @@ +import { afterAll, expect, test } from 'vitest'; +import { join } from 'path'; +import { cleanupChildProcesses, createRunner } from '../../../utils/runner'; +import type { SessionAggregates } from '@sentry/core'; + +afterAll(() => { + cleanupChildProcesses(); +}); + +function sumAggregates(agg: SessionAggregates): { exited: number; errored: number; crashed: number } { + return agg.aggregates.reduce( + (acc, b) => ({ + exited: acc.exited + (b.exited ?? 0), + errored: acc.errored + (b.errored ?? 0), + crashed: acc.crashed + (b.crashed ?? 0), + }), + { exited: 0, errored: 0, crashed: 0 }, + ); +} + +test('does not send a process session once requests are tracked as request sessions', async () => { + const runner = createRunner(__dirname, '..', 'server.mjs') + .withInstrument(join(__dirname, '..', 'instrument.mjs')) + .ignore('transaction', 'event') + .unignore('session', 'sessions') + .expect({ + sessions: agg => { + expect(sumAggregates(agg)).toEqual({ exited: 1, errored: 1, crashed: 1 }); + }, + }) + .start(); + + runner.makeRequest('get', '/test/success'); + runner.makeRequest('get', '/test/error_handled'); + runner.makeRequest('get', '/test/error_unhandled', { expectError: true }); + await runner.completed(); +}); + +test('counts an error before the first request in the process session, and later errors in the aggregate', async () => { + const runner = createRunner(__dirname, 'server-with-startup-error.mjs') + .withInstrument(join(__dirname, '..', 'instrument.mjs')) + .ignore('transaction', 'event') + .unignore('session', 'sessions') + .expect({ session: { init: true, status: 'ok', errors: 1 } }) + .expect({ session: { init: false, status: 'exited', errors: 1 } }) + .expect({ + sessions: agg => { + expect(sumAggregates(agg)).toEqual({ exited: 1, errored: 1, crashed: 0 }); + }, + }) + .start(); + + await runner.makeRequest('get', '/test/success'); + await runner.makeRequest('get', '/test/error_handled'); + await runner.completed(); +}); diff --git a/packages/core/src/client.ts b/packages/core/src/client.ts index 12d02b348bb2..045c61cb259a 100644 --- a/packages/core/src/client.ts +++ b/packages/core/src/client.ts @@ -883,6 +883,12 @@ export abstract class Client { callback: (breadcrumb: Breadcrumb, hint: XhrBreadcrumbHint | FetchBreadcrumbHint) => void, ): () => void; + /** + * A hook that is called when an incoming request starts being tracked as a request session. + * @returns {() => void} A function that, when executed, removes the registered callback. + */ + public on(hook: 'startRequestSession', callback: () => void): () => void; + /** * A hook that is called when the client is flushing * @returns {() => void} A function that, when executed, removes the registered callback. @@ -1194,6 +1200,11 @@ export abstract class Client { hint: XhrBreadcrumbHint | FetchBreadcrumbHint, ): void; + /** + * Emit a hook event when an incoming request starts being tracked as a request session. + */ + public emit(hook: 'startRequestSession'): void; + /** * Emit a hook event for client flush */ diff --git a/packages/core/src/integrations/http/record-request-session.ts b/packages/core/src/integrations/http/record-request-session.ts index cdef0538d3ee..4ab0eccb88dd 100644 --- a/packages/core/src/integrations/http/record-request-session.ts +++ b/packages/core/src/integrations/http/record-request-session.ts @@ -33,9 +33,13 @@ export function recordRequestSession( sessionFlushingDelayMS?: number; }, ): void { + // Errors in this request are counted by the request session aggregate, so they must not also + // update a process session inherited from the parent isolation scope. + requestIsolationScope.setSession(); requestIsolationScope.setSDKProcessingMetadata({ requestSession: { status: 'ok' }, }); + client.emit('startRequestSession'); response.once('close', () => { const requestSession = requestIsolationScope.getScopeData().sdkProcessingMetadata.requestSession; diff --git a/packages/node/src/integrations/processSession.ts b/packages/node/src/integrations/processSession.ts index d19948cbe6f9..4e9c23134c8b 100644 --- a/packages/node/src/integrations/processSession.ts +++ b/packages/node/src/integrations/processSession.ts @@ -1,15 +1,20 @@ -import { defineIntegration, endSession, getIsolationScope, startSession } from '@sentry/core'; +import { closeSession, defineIntegration, endSession, getIsolationScope, startSession } from '@sentry/core'; const INTEGRATION_NAME = 'ProcessSession' as const; /** * Records a Session for the current process to track release health. + * + * Once the process starts serving requests that are tracked as request sessions, release health is + * reported through session aggregates instead, and the process session is discarded. + * This integration exists so that we capture some form of session health data even when the process is not serving requests. */ export const processSessionIntegration = defineIntegration(() => { return { name: INTEGRATION_NAME, - setupOnce() { + setup(client) { startSession(); + const processIsolationScope = getIsolationScope(); // Emitted in the case of healthy sessions, error of `mechanism.handled: true` and unhandledrejections because // The 'beforeExit' event is not emitted for conditions causing explicit termination, @@ -26,6 +31,22 @@ export const processSessionIntegration = defineIntegration(() => { endSession(); } }); + + const unsubscribe = client.on('startRequestSession', () => { + unsubscribe(); + + const session = processIsolationScope.getSession(); + if (!session) { + return; + } + + // A session that was already sent upstream is closed so it does not linger as never-ended. + if (!session.init && session.status === 'ok') { + closeSession(session); + client.captureSession(session); + } + processIsolationScope.setSession(); + }); }, }; }); diff --git a/packages/node/test/integrations/httpServerIntegration.test.ts b/packages/node/test/integrations/httpServerIntegration.test.ts index 0a677246773b..f6b27eb35888 100644 --- a/packages/node/test/integrations/httpServerIntegration.test.ts +++ b/packages/node/test/integrations/httpServerIntegration.test.ts @@ -1,5 +1,5 @@ import type { Client } from '@sentry/core'; -import { createTransport, Scope, withScope } from '@sentry/core'; +import { createTransport, makeSession, Scope, withScope } from '@sentry/core'; import { ServerRuntimeClient } from '@sentry/core/server'; import { EventEmitter } from 'stream'; import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; @@ -125,6 +125,27 @@ describe('recordRequestSession()', () => { ], }); }); + + it('removes a session inherited from the parent isolation scope', () => { + const client = createTestClient(); + const requestIsolationScope = new Scope(); + requestIsolationScope.setSession(makeSession()); + + recordRequestSession(client, { requestIsolationScope, response: new EventEmitter() }); + + expect(requestIsolationScope.getSession()).toBeUndefined(); + }); + + it('emits the "startRequestSession" hook for every recorded request', () => { + const client = createTestClient(); + const onStartRequestSession = vi.fn(); + client.on('startRequestSession', onStartRequestSession); + + simulateRequest(client, 'ok'); + simulateRequest(client, 'ok'); + + expect(onStartRequestSession).toHaveBeenCalledTimes(2); + }); }); function simulateRequest(client: Client, status: 'ok' | 'errored' | 'crashed') { diff --git a/packages/node/test/integrations/processSession.test.ts b/packages/node/test/integrations/processSession.test.ts index 4d511035b8f9..69e8ce19dc68 100644 --- a/packages/node/test/integrations/processSession.test.ts +++ b/packages/node/test/integrations/processSession.test.ts @@ -1,4 +1,4 @@ -import { getIsolationScope, setCurrentClient } from '@sentry/core'; +import { getIsolationScope, makeSession, setCurrentClient } from '@sentry/core'; import { beforeEach, describe, expect, it, vi } from 'vitest'; import { processSessionIntegration } from '../../src/integrations/processSession'; import { NodeClient } from '../../src/sdk/client'; @@ -24,7 +24,7 @@ describe('processSessionIntegration', () => { return process; }) as never); - processSessionIntegration().setupOnce!(); + processSessionIntegration().setup!(client); processOn.mockRestore(); }); @@ -62,6 +62,56 @@ describe('processSessionIntegration', () => { expect(sendSession).not.toHaveBeenCalled(); }); + describe('when requests start being tracked as request sessions', () => { + it('removes a session that was never sent, without sending it', () => { + client.emit('startRequestSession'); + + expect(getIsolationScope().getSession()).toBeUndefined(); + expect(sendSession).not.toHaveBeenCalled(); + }); + + it('closes and removes a session that was already sent', () => { + const session = getIsolationScope().getSession()!; + session.init = false; + session.errors = 1; + + client.emit('startRequestSession'); + + expect(sendSession).toHaveBeenCalledTimes(1); + expect(sendSession).toHaveBeenCalledWith(expect.objectContaining({ status: 'exited', errors: 1 })); + expect(getIsolationScope().getSession()).toBeUndefined(); + }); + + it('removes an already-crashed session without updating it', () => { + const session = getIsolationScope().getSession()!; + session.init = false; + session.status = 'crashed'; + + client.emit('startRequestSession'); + + expect(sendSession).not.toHaveBeenCalled(); + expect(getIsolationScope().getSession()).toBeUndefined(); + }); + + it('only reacts to the first request session', () => { + client.emit('startRequestSession'); + const laterSession = makeSession(); + getIsolationScope().setSession(laterSession); + + client.emit('startRequestSession'); + + expect(getIsolationScope().getSession()).toBe(laterSession); + }); + + it('does not send a session on exit afterwards', () => { + client.emit('startRequestSession'); + + beforeExitHandler(); + + expect(sendSession).not.toHaveBeenCalled(); + }); + }); + it('does nothing when no session is on the scope', () => { getIsolationScope().setSession(undefined); sendSession.mockClear();