From 120499291df07a9291efbf763d9d789cef181bac Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Iv=C3=A1n=20Ovejero?= Date: Fri, 13 Dec 2024 17:34:37 +0100 Subject: [PATCH] refactor(core): Unify error reporters in core and task runners (#12168) Co-authored-by: Tomi Turtiainen <10324676+tomi@users.noreply.github.com> --- .../src/__tests__/error-reporter.test.ts | 31 ------ .../@n8n/task-runner/src/error-reporter.ts | 99 ------------------- packages/@n8n/task-runner/src/start.ts | 10 +- packages/cli/src/commands/base-command.ts | 8 +- .../test/integration/shared/retry-until.ts | 6 +- packages/core/src/error-reporter.ts | 27 ++--- packages/core/test/error-reporter.test.ts | 27 ++--- 7 files changed, 33 insertions(+), 175 deletions(-) delete mode 100644 packages/@n8n/task-runner/src/__tests__/error-reporter.test.ts delete mode 100644 packages/@n8n/task-runner/src/error-reporter.ts diff --git a/packages/@n8n/task-runner/src/__tests__/error-reporter.test.ts b/packages/@n8n/task-runner/src/__tests__/error-reporter.test.ts deleted file mode 100644 index 9345819329..0000000000 --- a/packages/@n8n/task-runner/src/__tests__/error-reporter.test.ts +++ /dev/null @@ -1,31 +0,0 @@ -import { mock } from 'jest-mock-extended'; -import { ApplicationError } from 'n8n-workflow'; - -import { ErrorReporter } from '../error-reporter'; - -describe('ErrorReporter', () => { - const errorReporting = new ErrorReporter(mock()); - - describe('beforeSend', () => { - it('should return null if originalException is an ApplicationError with level warning', () => { - const hint = { originalException: new ApplicationError('Test error', { level: 'warning' }) }; - expect(errorReporting.beforeSend(mock(), hint)).toBeNull(); - }); - - it('should return event if originalException is an ApplicationError with level error', () => { - const hint = { originalException: new ApplicationError('Test error', { level: 'error' }) }; - expect(errorReporting.beforeSend(mock(), hint)).not.toBeNull(); - }); - - it('should return null if originalException is an Error with a non-unique stack', () => { - const hint = { originalException: new Error('Test error') }; - errorReporting.beforeSend(mock(), hint); - expect(errorReporting.beforeSend(mock(), hint)).toBeNull(); - }); - - it('should return event if originalException is an Error with a unique stack', () => { - const hint = { originalException: new Error('Test error') }; - expect(errorReporting.beforeSend(mock(), hint)).not.toBeNull(); - }); - }); -}); diff --git a/packages/@n8n/task-runner/src/error-reporter.ts b/packages/@n8n/task-runner/src/error-reporter.ts deleted file mode 100644 index 33ca8bf5dd..0000000000 --- a/packages/@n8n/task-runner/src/error-reporter.ts +++ /dev/null @@ -1,99 +0,0 @@ -import { - init, - setTag, - captureException, - close, - rewriteFramesIntegration, - type EventHint, - type ErrorEvent, -} from '@sentry/node'; -import * as a from 'assert/strict'; -import { createHash } from 'crypto'; -import { ApplicationError } from 'n8n-workflow'; - -import type { SentryConfig } from '@/config/sentry-config'; - -/** - * Handles error reporting using Sentry - */ -export class ErrorReporter { - private isInitialized = false; - - /** Hashes of error stack traces, to deduplicate error reports. */ - private readonly seenErrors = new Set(); - - private get dsn() { - return this.sentryConfig.sentryDsn; - } - - constructor(private readonly sentryConfig: SentryConfig) { - a.ok(this.dsn, 'Sentry DSN is required to initialize Sentry'); - } - - async start() { - if (this.isInitialized) return; - - // Collect longer stacktraces - Error.stackTraceLimit = 50; - - process.on('uncaughtException', captureException); - - const ENABLED_INTEGRATIONS = [ - 'InboundFilters', - 'FunctionToString', - 'LinkedErrors', - 'OnUnhandledRejection', - 'ContextLines', - ]; - - setTag('server_type', 'task_runner'); - - init({ - dsn: this.dsn, - release: this.sentryConfig.n8nVersion, - environment: this.sentryConfig.environment, - enableTracing: false, - serverName: this.sentryConfig.deploymentName, - beforeBreadcrumb: () => null, - beforeSend: async (event, hint) => await this.beforeSend(event, hint), - integrations: (integrations) => [ - ...integrations.filter(({ name }) => ENABLED_INTEGRATIONS.includes(name)), - rewriteFramesIntegration({ root: process.cwd() }), - ], - }); - - this.isInitialized = true; - } - - async stop() { - if (!this.isInitialized) { - return; - } - - await close(1000); - } - - async beforeSend(event: ErrorEvent, { originalException }: EventHint) { - if (!originalException) return null; - - if (originalException instanceof Promise) { - originalException = await originalException.catch((error) => error as Error); - } - - if (originalException instanceof ApplicationError) { - const { level, extra, tags } = originalException; - if (level === 'warning') return null; - event.level = level; - if (extra) event.extra = { ...event.extra, ...extra }; - if (tags) event.tags = { ...event.tags, ...tags }; - } - - if (originalException instanceof Error && originalException.stack) { - const eventHash = createHash('sha1').update(originalException.stack).digest('base64'); - if (this.seenErrors.has(eventHash)) return null; - this.seenErrors.add(eventHash); - } - - return event; - } -} diff --git a/packages/@n8n/task-runner/src/start.ts b/packages/@n8n/task-runner/src/start.ts index 84226449be..69e9462516 100644 --- a/packages/@n8n/task-runner/src/start.ts +++ b/packages/@n8n/task-runner/src/start.ts @@ -1,8 +1,8 @@ +import type { ErrorReporter } from 'n8n-core'; import { ensureError, setGlobalState } from 'n8n-workflow'; import Container from 'typedi'; import { MainConfig } from './config/main-config'; -import type { ErrorReporter } from './error-reporter'; import type { HealthCheckServer } from './health-check-server'; import { JsTaskRunner } from './js-task-runner/js-task-runner'; @@ -33,7 +33,7 @@ function createSignalHandler(signal: string, timeoutInS = 10) { } if (errorReporter) { - await errorReporter.stop(); + await errorReporter.shutdown(); errorReporter = undefined; } } catch (e) { @@ -54,9 +54,9 @@ void (async function start() { }); if (config.sentryConfig.sentryDsn) { - const { ErrorReporter } = await import('@/error-reporter'); - errorReporter = new ErrorReporter(config.sentryConfig); - await errorReporter.start(); + const { ErrorReporter } = await import('n8n-core'); + errorReporter = new ErrorReporter(); + await errorReporter.init('task_runner', config.sentryConfig.sentryDsn); } runner = new JsTaskRunner(config); diff --git a/packages/cli/src/commands/base-command.ts b/packages/cli/src/commands/base-command.ts index 33b7a28bf3..286fec1de6 100644 --- a/packages/cli/src/commands/base-command.ts +++ b/packages/cli/src/commands/base-command.ts @@ -34,7 +34,7 @@ import { WorkflowHistoryManager } from '@/workflows/workflow-history/workflow-hi export abstract class BaseCommand extends Command { protected logger = Container.get(Logger); - protected readonly errorReporter = Container.get(ErrorReporter); + protected errorReporter: ErrorReporter; protected externalHooks?: ExternalHooks; @@ -60,7 +60,11 @@ export abstract class BaseCommand extends Command { protected needsCommunityPackages = false; async init(): Promise { - await this.errorReporter.init(); + this.errorReporter = Container.get(ErrorReporter); + await this.errorReporter.init( + this.instanceSettings.instanceType, + this.globalConfig.sentry.backendDsn, + ); initExpressionEvaluator(); process.once('SIGTERM', this.onTerminationSignal('SIGTERM')); diff --git a/packages/cli/test/integration/shared/retry-until.ts b/packages/cli/test/integration/shared/retry-until.ts index f469149b31..b6f375ecb4 100644 --- a/packages/cli/test/integration/shared/retry-until.ts +++ b/packages/cli/test/integration/shared/retry-until.ts @@ -8,7 +8,7 @@ */ export const retryUntil = async ( assertion: () => Promise | void, - { interval = 20, timeout = 1000 } = {}, + { intervalMs = 200, timeoutMs = 5000 } = {}, ) => { return await new Promise((resolve, reject) => { const startTime = Date.now(); @@ -18,13 +18,13 @@ export const retryUntil = async ( try { resolve(await assertion()); } catch (error) { - if (Date.now() - startTime > timeout) { + if (Date.now() - startTime > timeoutMs) { reject(error); } else { tryAgain(); } } - }, interval); + }, intervalMs); }; tryAgain(); diff --git a/packages/core/src/error-reporter.ts b/packages/core/src/error-reporter.ts index 10e58b1f45..b6fc936daa 100644 --- a/packages/core/src/error-reporter.ts +++ b/packages/core/src/error-reporter.ts @@ -1,26 +1,21 @@ -import { GlobalConfig } from '@n8n/config'; import type { NodeOptions } from '@sentry/node'; +import { close } from '@sentry/node'; import type { ErrorEvent, EventHint } from '@sentry/types'; import { AxiosError } from 'axios'; import { ApplicationError, LoggerProxy, type ReportingOptions } from 'n8n-workflow'; import { createHash } from 'node:crypto'; import { Service } from 'typedi'; -import { InstanceSettings } from './InstanceSettings'; +import type { InstanceType } from './InstanceSettings'; @Service() export class ErrorReporter { - private initialized = false; - /** Hashes of error stack traces, to deduplicate error reports. */ private seenErrors = new Set(); private report: (error: Error | string, options?: ReportingOptions) => void; - constructor( - private readonly globalConfig: GlobalConfig, - private readonly instanceSettings: InstanceSettings, - ) { + constructor() { // eslint-disable-next-line @typescript-eslint/unbound-method this.report = this.defaultReport; } @@ -41,18 +36,16 @@ export class ErrorReporter { } } - async init() { - if (this.initialized) return; + async shutdown(timeoutInMs = 1000) { + await close(timeoutInMs); + } + async init(instanceType: InstanceType | 'task_runner', dsn: string) { process.on('uncaughtException', (error) => { this.error(error); }); - const dsn = this.globalConfig.sentry.backendDsn; - if (!dsn) { - this.initialized = true; - return; - } + if (!dsn) return; // Collect longer stacktraces Error.stackTraceLimit = 50; @@ -98,11 +91,9 @@ export class ErrorReporter { ], }); - setTag('server_type', this.instanceSettings.instanceType); + setTag('server_type', instanceType); this.report = (error, options) => captureException(error, options); - - this.initialized = true; } async beforeSend(event: ErrorEvent, { originalException }: EventHint) { diff --git a/packages/core/test/error-reporter.test.ts b/packages/core/test/error-reporter.test.ts index b1ad419bb4..1f507ab5c0 100644 --- a/packages/core/test/error-reporter.test.ts +++ b/packages/core/test/error-reporter.test.ts @@ -1,17 +1,12 @@ -import type { GlobalConfig } from '@n8n/config'; import { QueryFailedError } from '@n8n/typeorm'; import type { ErrorEvent } from '@sentry/types'; import { AxiosError } from 'axios'; -import { mock } from 'jest-mock-extended'; import { ApplicationError } from 'n8n-workflow'; import { ErrorReporter } from '@/error-reporter'; -import type { InstanceSettings } from '@/InstanceSettings'; - -const init = jest.fn(); jest.mock('@sentry/node', () => ({ - init, + init: jest.fn(), setTag: jest.fn(), captureException: jest.fn(), Integrations: {}, @@ -20,9 +15,7 @@ jest.mock('@sentry/node', () => ({ jest.spyOn(process, 'on'); describe('ErrorReporter', () => { - const globalConfig = mock(); - const instanceSettings = mock(); - const errorReporting = new ErrorReporter(globalConfig, instanceSettings); + const errorReporter = new ErrorReporter(); const event = {} as ErrorEvent; describe('beforeSend', () => { @@ -30,14 +23,14 @@ describe('ErrorReporter', () => { const originalException = new ApplicationError('test'); originalException.level = 'warning'; - expect(await errorReporting.beforeSend(event, { originalException })).toEqual(null); + expect(await errorReporter.beforeSend(event, { originalException })).toEqual(null); }); it('should keep events with a cause with error level', async () => { const cause = new Error('cause-error'); const originalException = new ApplicationError('test', cause); - expect(await errorReporting.beforeSend(event, { originalException })).toEqual(event); + expect(await errorReporter.beforeSend(event, { originalException })).toEqual(event); }); it('should ignore events with error cause with warning level', async () => { @@ -45,7 +38,7 @@ describe('ErrorReporter', () => { cause.level = 'warning'; const originalException = new ApplicationError('test', cause); - expect(await errorReporting.beforeSend(event, { originalException })).toEqual(null); + expect(await errorReporter.beforeSend(event, { originalException })).toEqual(null); }); it('should set level, extra, and tags from ApplicationError', async () => { @@ -57,7 +50,7 @@ describe('ErrorReporter', () => { const testEvent = {} as ErrorEvent; - const result = await errorReporting.beforeSend(testEvent, { originalException }); + const result = await errorReporter.beforeSend(testEvent, { originalException }); expect(result).toEqual({ level: 'error', @@ -69,17 +62,17 @@ describe('ErrorReporter', () => { it('should deduplicate errors with same stack trace', async () => { const originalException = new Error(); - const firstResult = await errorReporting.beforeSend(event, { originalException }); + const firstResult = await errorReporter.beforeSend(event, { originalException }); expect(firstResult).toEqual(event); - const secondResult = await errorReporting.beforeSend(event, { originalException }); + const secondResult = await errorReporter.beforeSend(event, { originalException }); expect(secondResult).toBeNull(); }); it('should handle Promise rejections', async () => { const originalException = Promise.reject(new Error()); - const result = await errorReporting.beforeSend(event, { originalException }); + const result = await errorReporter.beforeSend(event, { originalException }); expect(result).toEqual(event); }); @@ -103,7 +96,7 @@ describe('ErrorReporter', () => { new Error('', { cause: new ApplicationError('', { level: 'warning' }) }), ], ])('should ignore if originalException is %s', async (_, originalException) => { - const result = await errorReporting.beforeSend(event, { originalException }); + const result = await errorReporter.beforeSend(event, { originalException }); expect(result).toBeNull(); }); });