diff --git a/dev-packages/node-integration-tests/suites/client-reports/drop-reasons/before-send/scenario.ts b/dev-packages/node-integration-tests/suites/client-reports/drop-reasons/before-send/scenario.ts new file mode 100644 index 000000000000..ab22aa289892 --- /dev/null +++ b/dev-packages/node-integration-tests/suites/client-reports/drop-reasons/before-send/scenario.ts @@ -0,0 +1,23 @@ +import { loggingTransport } from '@sentry-internal/node-integration-tests'; +import * as Sentry from '@sentry/node'; + +// eslint-disable-next-line @typescript-eslint/no-floating-promises +(async () => { + Sentry.init({ + dsn: 'https://public@dsn.ingest.sentry.io/1337', + transport: loggingTransport, + beforeSend(event) { + return !event.type ? null : event; + }, + }); + + Sentry.captureException(new Error('this should get dropped by the event processor')); + + await Sentry.flush(); + + Sentry.captureException(new Error('this should get dropped by the event processor')); + Sentry.captureException(new Error('this should get dropped by the event processor')); + + // eslint-disable-next-line @typescript-eslint/no-floating-promises + Sentry.flush(); +})(); diff --git a/dev-packages/node-integration-tests/suites/client-reports/drop-reasons/before-send/test.ts b/dev-packages/node-integration-tests/suites/client-reports/drop-reasons/before-send/test.ts new file mode 100644 index 000000000000..363b8f268cd2 --- /dev/null +++ b/dev-packages/node-integration-tests/suites/client-reports/drop-reasons/before-send/test.ts @@ -0,0 +1,32 @@ +import { cleanupChildProcesses, createRunner } from '../../../../utils/runner'; + +afterAll(() => { + cleanupChildProcesses(); +}); + +test('should record client report for beforeSend', done => { + createRunner(__dirname, 'scenario.ts') + .expect({ + client_report: { + discarded_events: [ + { + category: 'error', + quantity: 1, + reason: 'before_send', + }, + ], + }, + }) + .expect({ + client_report: { + discarded_events: [ + { + category: 'error', + quantity: 2, + reason: 'before_send', + }, + ], + }, + }) + .start(done); +}); diff --git a/dev-packages/node-integration-tests/suites/client-reports/drop-reasons/event-processors/scenario.ts b/dev-packages/node-integration-tests/suites/client-reports/drop-reasons/event-processors/scenario.ts new file mode 100644 index 000000000000..2b188f8d71f3 --- /dev/null +++ b/dev-packages/node-integration-tests/suites/client-reports/drop-reasons/event-processors/scenario.ts @@ -0,0 +1,24 @@ +import { loggingTransport } from '@sentry-internal/node-integration-tests'; +import * as Sentry from '@sentry/node'; + +// eslint-disable-next-line @typescript-eslint/no-floating-promises +(async () => { + Sentry.init({ + dsn: 'https://public@dsn.ingest.sentry.io/1337', + transport: loggingTransport, + }); + + Sentry.addEventProcessor(event => { + return !event.type ? null : event; + }); + + Sentry.captureException(new Error('this should get dropped by the event processor')); + + await Sentry.flush(); + + Sentry.captureException(new Error('this should get dropped by the event processor')); + Sentry.captureException(new Error('this should get dropped by the event processor')); + + // eslint-disable-next-line @typescript-eslint/no-floating-promises + Sentry.flush(); +})(); diff --git a/dev-packages/node-integration-tests/suites/client-reports/drop-reasons/event-processors/test.ts b/dev-packages/node-integration-tests/suites/client-reports/drop-reasons/event-processors/test.ts new file mode 100644 index 000000000000..803f1c09bafe --- /dev/null +++ b/dev-packages/node-integration-tests/suites/client-reports/drop-reasons/event-processors/test.ts @@ -0,0 +1,32 @@ +import { cleanupChildProcesses, createRunner } from '../../../../utils/runner'; + +afterAll(() => { + cleanupChildProcesses(); +}); + +test('should record client report for event processors', done => { + createRunner(__dirname, 'scenario.ts') + .expect({ + client_report: { + discarded_events: [ + { + category: 'error', + quantity: 1, + reason: 'event_processor', + }, + ], + }, + }) + .expect({ + client_report: { + discarded_events: [ + { + category: 'error', + quantity: 2, + reason: 'event_processor', + }, + ], + }, + }) + .start(done); +}); diff --git a/dev-packages/node-integration-tests/suites/client-reports/periodic-send/scenario.ts b/dev-packages/node-integration-tests/suites/client-reports/periodic-send/scenario.ts new file mode 100644 index 000000000000..ff14911469ca --- /dev/null +++ b/dev-packages/node-integration-tests/suites/client-reports/periodic-send/scenario.ts @@ -0,0 +1,13 @@ +import { loggingTransport } from '@sentry-internal/node-integration-tests'; +import * as Sentry from '@sentry/node'; + +Sentry.init({ + dsn: 'https://public@dsn.ingest.sentry.io/1337', + transport: loggingTransport, + clientReportFlushInterval: 5000, + beforeSend(event) { + return !event.type ? null : event; + }, +}); + +Sentry.captureException(new Error('this should get dropped by before send')); diff --git a/dev-packages/node-integration-tests/suites/client-reports/periodic-send/test.ts b/dev-packages/node-integration-tests/suites/client-reports/periodic-send/test.ts new file mode 100644 index 000000000000..0364f3ea01f0 --- /dev/null +++ b/dev-packages/node-integration-tests/suites/client-reports/periodic-send/test.ts @@ -0,0 +1,21 @@ +import { cleanupChildProcesses, createRunner } from '../../../utils/runner'; + +afterAll(() => { + cleanupChildProcesses(); +}); + +test('should flush client reports automatically after the timeout interval', done => { + createRunner(__dirname, 'scenario.ts') + .expect({ + client_report: { + discarded_events: [ + { + category: 'error', + quantity: 1, + reason: 'before_send', + }, + ], + }, + }) + .start(done); +}); diff --git a/dev-packages/node-integration-tests/utils/runner.ts b/dev-packages/node-integration-tests/utils/runner.ts index 6e663cd13d75..ae0451f0d576 100644 --- a/dev-packages/node-integration-tests/utils/runner.ts +++ b/dev-packages/node-integration-tests/utils/runner.ts @@ -3,6 +3,7 @@ import { spawn, spawnSync } from 'child_process'; import { join } from 'path'; import { SDK_VERSION } from '@sentry/node'; import type { + ClientReport, Envelope, EnvelopeItemType, Event, @@ -46,6 +47,12 @@ export function assertSentryCheckIn(actual: SerializedCheckIn, expected: Partial }); } +export function assertSentryClientReport(actual: ClientReport, expected: Partial): void { + expect(actual).toMatchObject({ + ...expected, + }); +} + export function assertEnvelopeHeader(actual: Envelope[0], expected: Partial): void { expect(actual).toEqual({ event_id: expect.any(String), @@ -148,6 +155,9 @@ type Expected = } | { check_in: Partial | ((event: SerializedCheckIn) => void); + } + | { + client_report: Partial | ((event: ClientReport) => void); }; type ExpectedEnvelopeHeader = @@ -332,6 +342,17 @@ export function createRunner(...paths: string[]) { expectCallbackCalled(); } + + if ('client_report' in expected) { + const clientReport = item[1] as ClientReport; + if (typeof expected.client_report === 'function') { + expected.client_report(clientReport); + } else { + assertSentryClientReport(clientReport, expected.client_report); + } + + expectCallbackCalled(); + } } catch (e) { complete(e as Error); } diff --git a/packages/browser/src/client.ts b/packages/browser/src/client.ts index c301df98f7f6..177d787a438d 100644 --- a/packages/browser/src/client.ts +++ b/packages/browser/src/client.ts @@ -12,7 +12,7 @@ import type { SeverityLevel, UserFeedback, } from '@sentry/types'; -import { createClientReportEnvelope, dsnToString, getSDKSource, logger } from '@sentry/utils'; +import { getSDKSource, logger } from '@sentry/utils'; import { DEBUG_BUILD } from './debug-build'; import { eventFromException, eventFromMessage } from './eventbuilder'; @@ -118,30 +118,4 @@ export class BrowserClient extends BaseClient { event.platform = event.platform || 'javascript'; return super._prepareEvent(event, hint, scope); } - - /** - * Sends client reports as an envelope. - */ - private _flushOutcomes(): void { - const outcomes = this._clearOutcomes(); - - if (outcomes.length === 0) { - DEBUG_BUILD && logger.log('No outcomes to send'); - return; - } - - // This is really the only place where we want to check for a DSN and only send outcomes then - if (!this._dsn) { - DEBUG_BUILD && logger.log('No dsn provided, will not send outcomes'); - return; - } - - DEBUG_BUILD && logger.log('Sending outcomes:', outcomes); - - const envelope = createClientReportEnvelope(outcomes, this._options.tunnel && dsnToString(this._dsn)); - - // sendEnvelope should not throw - // eslint-disable-next-line @typescript-eslint/no-floating-promises - this.sendEnvelope(envelope); - } } diff --git a/packages/core/src/baseclient.ts b/packages/core/src/baseclient.ts index 9c4b43bd9c9d..5122b66d3267 100644 --- a/packages/core/src/baseclient.ts +++ b/packages/core/src/baseclient.ts @@ -36,7 +36,9 @@ import { addItemToEnvelope, checkOrSetAlreadyCaught, createAttachmentEnvelopeItem, + createClientReportEnvelope, dropUndefinedKeys, + dsnToString, isParameterizedString, isPlainObject, isPrimitive, @@ -871,6 +873,34 @@ export abstract class BaseClient implements Client { }); } + /** + * Sends client reports as an envelope. + */ + protected _flushOutcomes(): void { + DEBUG_BUILD && logger.log('Flushing outcomes...'); + + const outcomes = this._clearOutcomes(); + + if (outcomes.length === 0) { + DEBUG_BUILD && logger.log('No outcomes to send'); + return; + } + + // This is really the only place where we want to check for a DSN and only send outcomes then + if (!this._dsn) { + DEBUG_BUILD && logger.log('No dsn provided, will not send outcomes'); + return; + } + + DEBUG_BUILD && logger.log('Sending outcomes:', outcomes); + + const envelope = createClientReportEnvelope(outcomes, this._options.tunnel && dsnToString(this._dsn)); + + // sendEnvelope should not throw + // eslint-disable-next-line @typescript-eslint/no-floating-promises + this.sendEnvelope(envelope); + } + /** * @inheritDoc */ diff --git a/packages/node/src/sdk/client.ts b/packages/node/src/sdk/client.ts index cf1cb3c2023a..877b363d3b2a 100644 --- a/packages/node/src/sdk/client.ts +++ b/packages/node/src/sdk/client.ts @@ -6,12 +6,17 @@ import type { ServerRuntimeClientOptions } from '@sentry/core'; import { SDK_VERSION, ServerRuntimeClient, applySdkMetadata } from '@sentry/core'; import { logger } from '@sentry/utils'; import { isMainThread, threadId } from 'worker_threads'; +import { DEBUG_BUILD } from '../debug-build'; import type { NodeClientOptions } from '../types'; +const DEFAULT_CLIENT_REPORT_FLUSH_INTERVAL_MS = 60_000; // 60s was chosen arbitrarily + /** A client for using Sentry with Node & OpenTelemetry. */ export class NodeClient extends ServerRuntimeClient { public traceProvider: BasicTracerProvider | undefined; private _tracer: Tracer | undefined; + private _clientReportInterval: NodeJS.Timeout | undefined; + private _clientReportOnExitFlushListener: (() => void) | undefined; public constructor(options: NodeClientOptions) { const clientOptions: ServerRuntimeClientOptions = { @@ -44,9 +49,8 @@ export class NodeClient extends ServerRuntimeClient { return tracer; } - /** - * @inheritDoc - */ + // Eslint ignore explanation: This is already documented in super. + // eslint-disable-next-line jsdoc/require-jsdoc public async flush(timeout?: number): Promise { const provider = this.traceProvider; const spanProcessor = provider?.activeSpanProcessor; @@ -55,6 +59,60 @@ export class NodeClient extends ServerRuntimeClient { await spanProcessor.forceFlush(); } + if (this.getOptions().sendClientReports) { + this._flushOutcomes(); + } + return super.flush(timeout); } + + // Eslint ignore explanation: This is already documented in super. + // eslint-disable-next-line jsdoc/require-jsdoc + public close(timeout?: number | undefined): PromiseLike { + if (this._clientReportInterval) { + clearInterval(this._clientReportInterval); + } + + if (this._clientReportOnExitFlushListener) { + process.off('beforeExit', this._clientReportOnExitFlushListener); + } + + return super.close(timeout); + } + + /** + * Will start tracking client reports for this client. + * + * NOTICE: This method will create an interval that is periodically called and attach a `process.on('beforeExit')` + * hook. To clean up these resources, call `.close()` when you no longer intend to use the client. Not doing so will + * result in a memory leak. + */ + // The reason client reports need to be manually activated with this method instead of just enabling them in a + // constructor, is that if users periodically and unboundedly create new clients, we will create more and more + // intervals and beforeExit listeners, thus leaking memory. In these situations, users are required to call + // `client.close()` in order to dispose of the acquired resources. + // We assume that calling this method in Sentry.init() is a sensible default, because calling Sentry.init() over and + // over again would also result in memory leaks. + // Note: We have experimented with using `FinalizationRegisty` to clear the interval when the client is garbage + // collected, but it did not work, because the cleanup function never got called. + public startClientReportTracking(): void { + const clientOptions = this.getOptions(); + if (clientOptions.sendClientReports) { + this._clientReportOnExitFlushListener = () => { + this._flushOutcomes(); + }; + + this._clientReportInterval = setInterval( + () => { + DEBUG_BUILD && logger.log('Flushing client reports based on interval.'); + this._flushOutcomes(); + }, + clientOptions.clientReportFlushInterval ?? DEFAULT_CLIENT_REPORT_FLUSH_INTERVAL_MS, + ) + // Unref is critical for not preventing the process from exiting because the interval is active. + .unref(); + + process.on('beforeExit', this._clientReportOnExitFlushListener); + } + } } diff --git a/packages/node/src/sdk/index.ts b/packages/node/src/sdk/index.ts index 7dd145854993..65a6f6768096 100644 --- a/packages/node/src/sdk/index.ts +++ b/packages/node/src/sdk/index.ts @@ -154,6 +154,8 @@ function _init( startSessionTracking(); } + client.startClientReportTracking(); + updateScopeFromEnvVariables(); if (options.spotlight) { @@ -228,6 +230,7 @@ function getClientOptions( transport: makeNodeTransport, dsn: process.env.SENTRY_DSN, environment: process.env.SENTRY_ENVIRONMENT, + sendClientReports: true, }); const overwriteOptions = dropUndefinedKeys({ diff --git a/packages/node/src/types.ts b/packages/node/src/types.ts index 9cf3047e6c0a..dbdbdd1b1178 100644 --- a/packages/node/src/types.ts +++ b/packages/node/src/types.ts @@ -109,6 +109,11 @@ export interface BaseNodeOptions { */ registerEsmLoaderHooks?: boolean | EsmLoaderHookOptions; + /** + * Configures in which interval client reports will be flushed. Defaults to `60_000` (milliseconds). + */ + clientReportFlushInterval?: number; + /** Callback that is executed when a fatal global error occurs. */ onFatalError?(this: void, error: Error): void; } diff --git a/packages/node/test/helpers/mockSdkInit.ts b/packages/node/test/helpers/mockSdkInit.ts index 0e1d23cfc73c..e482dac6ed08 100644 --- a/packages/node/test/helpers/mockSdkInit.ts +++ b/packages/node/test/helpers/mockSdkInit.ts @@ -17,7 +17,14 @@ export function resetGlobals(): void { export function mockSdkInit(options?: Partial) { resetGlobals(); - init({ dsn: PUBLIC_DSN, defaultIntegrations: false, ...options }); + init({ + dsn: PUBLIC_DSN, + defaultIntegrations: false, + // We are disabling client reports because we would be acquiring resources with every init call and that would leak + // memory every time we call init in the tests + sendClientReports: false, + ...options, + }); } export function cleanupOtel(_provider?: BasicTracerProvider): void { diff --git a/packages/types/src/options.ts b/packages/types/src/options.ts index d6c407d60bd0..82123c01a380 100644 --- a/packages/types/src/options.ts +++ b/packages/types/src/options.ts @@ -31,8 +31,7 @@ export interface ClientOptions