From 41db46598ad7a530aaf435d8f9459e8f819b3628 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Tue, 10 Dec 2024 09:10:50 +0100 Subject: [PATCH 1/2] fix(node): Guard against invalid `maxSpanWaitDuration` values --- packages/node/src/sdk/initOtel.ts | 26 ++++++++- packages/node/test/sdk/initOtel.test.ts | 78 +++++++++++++++++++++++++ 2 files changed, 103 insertions(+), 1 deletion(-) create mode 100644 packages/node/test/sdk/initOtel.test.ts diff --git a/packages/node/src/sdk/initOtel.ts b/packages/node/src/sdk/initOtel.ts index f9b849dbf79a..abbe4a20120c 100644 --- a/packages/node/src/sdk/initOtel.ts +++ b/packages/node/src/sdk/initOtel.ts @@ -10,6 +10,7 @@ import { import { GLOBAL_OBJ, SDK_VERSION, consoleSandbox, logger } from '@sentry/core'; import { SentryPropagator, SentrySampler, SentrySpanProcessor } from '@sentry/opentelemetry'; import { createAddHookMessageChannel } from 'import-in-the-middle'; +import { DEBUG_BUILD } from '../debug-build'; import { getOpenTelemetryInstrumentationToPreload } from '../integrations/tracing'; import { SentryContextManager } from '../otel/contextManager'; import type { EsmLoaderHookOptions } from '../types'; @@ -18,6 +19,9 @@ import type { NodeClient } from './client'; declare const __IMPORT_META_URL_REPLACEMENT__: string; +// About 277h - this must fit into new Array(len)! +const MAX_MAX_SPAN_WAIT_DURATION = 1_000_000; + /** * Initialize OpenTelemetry for Node. */ @@ -138,7 +142,7 @@ export function setupOtel(client: NodeClient): BasicTracerProvider { forceFlushTimeoutMillis: 500, spanProcessors: [ new SentrySpanProcessor({ - timeout: client.getOptions().maxSpanWaitDuration, + timeout: _getSpanProcessorTimeout(client.getOptions().maxSpanWaitDuration), }), ], }); @@ -152,6 +156,26 @@ export function setupOtel(client: NodeClient): BasicTracerProvider { return provider; } +/** Just exported for tests. */ +export function _getSpanProcessorTimeout(maxSpanWaitDuration: number | undefined): number | undefined { + if (maxSpanWaitDuration == null) { + return undefined; + } + + // We guard for a max. value here, because we create an array with this length + // So if this value is too large, this would fail + if (maxSpanWaitDuration > MAX_MAX_SPAN_WAIT_DURATION) { + DEBUG_BUILD && + logger.warn(`\`maxSpanWaitDuration\` is too high, using the maximum value of ${MAX_MAX_SPAN_WAIT_DURATION}`); + return MAX_MAX_SPAN_WAIT_DURATION; + } else if (maxSpanWaitDuration <= 0 || Number.isNaN(maxSpanWaitDuration)) { + DEBUG_BUILD && logger.warn('`maxSpanWaitDuration` must be a positive number, using default value instead.'); + return undefined; + } + + return maxSpanWaitDuration; +} + /** * Setup the OTEL logger to use our own logger. */ diff --git a/packages/node/test/sdk/initOtel.test.ts b/packages/node/test/sdk/initOtel.test.ts new file mode 100644 index 000000000000..f678720e1429 --- /dev/null +++ b/packages/node/test/sdk/initOtel.test.ts @@ -0,0 +1,78 @@ +import { logger } from '@sentry/core'; +import { _getSpanProcessorTimeout } from '../../src/sdk/initOtel'; + +describe('_getSpanProcessorTimeout', () => { + beforeEach(() => { + jest.clearAllMocks(); + }); + + it('works with undefined', () => { + const loggerWarnSpy = jest.spyOn(logger, 'warn').mockImplementation(() => {}); + const timeout = _getSpanProcessorTimeout(undefined); + expect(timeout).toBe(undefined); + expect(loggerWarnSpy).not.toHaveBeenCalled(); + }); + + it('works with positive number', () => { + const loggerWarnSpy = jest.spyOn(logger, 'warn').mockImplementation(() => {}); + const timeout = _getSpanProcessorTimeout(10); + expect(timeout).toBe(10); + expect(loggerWarnSpy).not.toHaveBeenCalled(); + }); + + it('works with 0', () => { + const loggerWarnSpy = jest.spyOn(logger, 'warn').mockImplementation(() => {}); + const timeout = _getSpanProcessorTimeout(0); + expect(timeout).toBe(undefined); + expect(loggerWarnSpy).toHaveBeenCalledTimes(1); + expect(loggerWarnSpy).toHaveBeenCalledWith( + '`maxSpanWaitDuration` must be a positive number, using default value instead.', + ); + }); + + it('works with negative number', () => { + const loggerWarnSpy = jest.spyOn(logger, 'warn').mockImplementation(() => {}); + const timeout = _getSpanProcessorTimeout(-10); + expect(timeout).toBe(undefined); + expect(loggerWarnSpy).toHaveBeenCalledTimes(1); + expect(loggerWarnSpy).toHaveBeenCalledWith( + '`maxSpanWaitDuration` must be a positive number, using default value instead.', + ); + }); + + it('works with -Infinity', () => { + const loggerWarnSpy = jest.spyOn(logger, 'warn').mockImplementation(() => {}); + const timeout = _getSpanProcessorTimeout(-Infinity); + expect(timeout).toBe(undefined); + expect(loggerWarnSpy).toHaveBeenCalledTimes(1); + expect(loggerWarnSpy).toHaveBeenCalledWith( + '`maxSpanWaitDuration` must be a positive number, using default value instead.', + ); + }); + + it('works with Infinity', () => { + const loggerWarnSpy = jest.spyOn(logger, 'warn').mockImplementation(() => {}); + const timeout = _getSpanProcessorTimeout(Infinity); + expect(timeout).toBe(1_000_000); + expect(loggerWarnSpy).toHaveBeenCalledTimes(1); + expect(loggerWarnSpy).toHaveBeenCalledWith('`maxSpanWaitDuration` is too high, using the maximum value of 1000000'); + }); + + it('works with large number', () => { + const loggerWarnSpy = jest.spyOn(logger, 'warn').mockImplementation(() => {}); + const timeout = _getSpanProcessorTimeout(1_000_000_000); + expect(timeout).toBe(1_000_000); + expect(loggerWarnSpy).toHaveBeenCalledTimes(1); + expect(loggerWarnSpy).toHaveBeenCalledWith('`maxSpanWaitDuration` is too high, using the maximum value of 1000000'); + }); + + it('works with NaN', () => { + const loggerWarnSpy = jest.spyOn(logger, 'warn').mockImplementation(() => {}); + const timeout = _getSpanProcessorTimeout(NaN); + expect(timeout).toBe(undefined); + expect(loggerWarnSpy).toHaveBeenCalledTimes(1); + expect(loggerWarnSpy).toHaveBeenCalledWith( + '`maxSpanWaitDuration` must be a positive number, using default value instead.', + ); + }); +}); From 8af224830f303714d81c56158bb793cff837c8e5 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Tue, 10 Dec 2024 10:36:51 +0100 Subject: [PATCH 2/2] rename it --- packages/node/src/sdk/initOtel.ts | 4 ++-- packages/node/test/sdk/initOtel.test.ts | 20 ++++++++++---------- 2 files changed, 12 insertions(+), 12 deletions(-) diff --git a/packages/node/src/sdk/initOtel.ts b/packages/node/src/sdk/initOtel.ts index abbe4a20120c..4f0bb444d83d 100644 --- a/packages/node/src/sdk/initOtel.ts +++ b/packages/node/src/sdk/initOtel.ts @@ -142,7 +142,7 @@ export function setupOtel(client: NodeClient): BasicTracerProvider { forceFlushTimeoutMillis: 500, spanProcessors: [ new SentrySpanProcessor({ - timeout: _getSpanProcessorTimeout(client.getOptions().maxSpanWaitDuration), + timeout: _clampSpanProcessorTimeout(client.getOptions().maxSpanWaitDuration), }), ], }); @@ -157,7 +157,7 @@ export function setupOtel(client: NodeClient): BasicTracerProvider { } /** Just exported for tests. */ -export function _getSpanProcessorTimeout(maxSpanWaitDuration: number | undefined): number | undefined { +export function _clampSpanProcessorTimeout(maxSpanWaitDuration: number | undefined): number | undefined { if (maxSpanWaitDuration == null) { return undefined; } diff --git a/packages/node/test/sdk/initOtel.test.ts b/packages/node/test/sdk/initOtel.test.ts index f678720e1429..bb3c3a29c919 100644 --- a/packages/node/test/sdk/initOtel.test.ts +++ b/packages/node/test/sdk/initOtel.test.ts @@ -1,28 +1,28 @@ import { logger } from '@sentry/core'; -import { _getSpanProcessorTimeout } from '../../src/sdk/initOtel'; +import { _clampSpanProcessorTimeout } from '../../src/sdk/initOtel'; -describe('_getSpanProcessorTimeout', () => { +describe('_clampSpanProcessorTimeout', () => { beforeEach(() => { jest.clearAllMocks(); }); it('works with undefined', () => { const loggerWarnSpy = jest.spyOn(logger, 'warn').mockImplementation(() => {}); - const timeout = _getSpanProcessorTimeout(undefined); + const timeout = _clampSpanProcessorTimeout(undefined); expect(timeout).toBe(undefined); expect(loggerWarnSpy).not.toHaveBeenCalled(); }); it('works with positive number', () => { const loggerWarnSpy = jest.spyOn(logger, 'warn').mockImplementation(() => {}); - const timeout = _getSpanProcessorTimeout(10); + const timeout = _clampSpanProcessorTimeout(10); expect(timeout).toBe(10); expect(loggerWarnSpy).not.toHaveBeenCalled(); }); it('works with 0', () => { const loggerWarnSpy = jest.spyOn(logger, 'warn').mockImplementation(() => {}); - const timeout = _getSpanProcessorTimeout(0); + const timeout = _clampSpanProcessorTimeout(0); expect(timeout).toBe(undefined); expect(loggerWarnSpy).toHaveBeenCalledTimes(1); expect(loggerWarnSpy).toHaveBeenCalledWith( @@ -32,7 +32,7 @@ describe('_getSpanProcessorTimeout', () => { it('works with negative number', () => { const loggerWarnSpy = jest.spyOn(logger, 'warn').mockImplementation(() => {}); - const timeout = _getSpanProcessorTimeout(-10); + const timeout = _clampSpanProcessorTimeout(-10); expect(timeout).toBe(undefined); expect(loggerWarnSpy).toHaveBeenCalledTimes(1); expect(loggerWarnSpy).toHaveBeenCalledWith( @@ -42,7 +42,7 @@ describe('_getSpanProcessorTimeout', () => { it('works with -Infinity', () => { const loggerWarnSpy = jest.spyOn(logger, 'warn').mockImplementation(() => {}); - const timeout = _getSpanProcessorTimeout(-Infinity); + const timeout = _clampSpanProcessorTimeout(-Infinity); expect(timeout).toBe(undefined); expect(loggerWarnSpy).toHaveBeenCalledTimes(1); expect(loggerWarnSpy).toHaveBeenCalledWith( @@ -52,7 +52,7 @@ describe('_getSpanProcessorTimeout', () => { it('works with Infinity', () => { const loggerWarnSpy = jest.spyOn(logger, 'warn').mockImplementation(() => {}); - const timeout = _getSpanProcessorTimeout(Infinity); + const timeout = _clampSpanProcessorTimeout(Infinity); expect(timeout).toBe(1_000_000); expect(loggerWarnSpy).toHaveBeenCalledTimes(1); expect(loggerWarnSpy).toHaveBeenCalledWith('`maxSpanWaitDuration` is too high, using the maximum value of 1000000'); @@ -60,7 +60,7 @@ describe('_getSpanProcessorTimeout', () => { it('works with large number', () => { const loggerWarnSpy = jest.spyOn(logger, 'warn').mockImplementation(() => {}); - const timeout = _getSpanProcessorTimeout(1_000_000_000); + const timeout = _clampSpanProcessorTimeout(1_000_000_000); expect(timeout).toBe(1_000_000); expect(loggerWarnSpy).toHaveBeenCalledTimes(1); expect(loggerWarnSpy).toHaveBeenCalledWith('`maxSpanWaitDuration` is too high, using the maximum value of 1000000'); @@ -68,7 +68,7 @@ describe('_getSpanProcessorTimeout', () => { it('works with NaN', () => { const loggerWarnSpy = jest.spyOn(logger, 'warn').mockImplementation(() => {}); - const timeout = _getSpanProcessorTimeout(NaN); + const timeout = _clampSpanProcessorTimeout(NaN); expect(timeout).toBe(undefined); expect(loggerWarnSpy).toHaveBeenCalledTimes(1); expect(loggerWarnSpy).toHaveBeenCalledWith(