refactor(core): Unify error reporters in core and task runners (#12168)

Co-authored-by: Tomi Turtiainen <10324676+tomi@users.noreply.github.com>

packages/@n8n/task-runner/src/__tests__/error-reporter.test.ts

@@ -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();
-		});
-	});
-});

packages/@n8n/task-runner/src/error-reporter.ts

@@ -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<string>();
-
-	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;
-	}
-}

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');
+		const { ErrorReporter } = await import('n8n-core');
-		errorReporter = new ErrorReporter(config.sentryConfig);
+		errorReporter = new ErrorReporter();
-		await errorReporter.start();
+		await errorReporter.init('task_runner', config.sentryConfig.sentryDsn);
 	}
 
 	runner = new JsTaskRunner(config);

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<void> {
-		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'));

packages/cli/test/integration/shared/retry-until.ts

@@ -8,7 +8,7 @@
  */
 export const retryUntil = async (
 	assertion: () => Promise<void> | 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();

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<string>();
 
 	private report: (error: Error | string, options?: ReportingOptions) => void;
 
-	constructor(
+	constructor() {
-		private readonly globalConfig: GlobalConfig,
-		private readonly instanceSettings: InstanceSettings,
-	) {
 		// eslint-disable-next-line @typescript-eslint/unbound-method
 		this.report = this.defaultReport;
 	}
@@ -41,18 +36,16 @@ export class ErrorReporter {
 		}
 	}
 
-	async init() {
+	async shutdown(timeoutInMs = 1000) {
-		if (this.initialized) return;
+		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) return;
-		if (!dsn) {
-			this.initialized = true;
-			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) {

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<GlobalConfig>();
+	const errorReporter = new ErrorReporter();
-	const instanceSettings = mock<InstanceSettings>();
-	const errorReporting = new ErrorReporter(globalConfig, instanceSettings);
 	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();
 		});
 	});