From 4fceebf8c2374556953f1dd1ceff0f0a444fb6ff Mon Sep 17 00:00:00 2001 From: Tomi Turtiainen <10324676+tomi@users.noreply.github.com> Date: Tue, 12 Nov 2024 10:45:57 +0200 Subject: [PATCH] Rename to ErrorReporter and improve code --- packages/@n8n/task-runner/package.json | 4 +- .../src/__tests__/error-reporter.test.ts | 31 +++++++++++ .../{error-reporting.ts => error-reporter.ts} | 55 ++++++++++--------- packages/@n8n/task-runner/src/start.ts | 16 +++--- 4 files changed, 69 insertions(+), 37 deletions(-) create mode 100644 packages/@n8n/task-runner/src/__tests__/error-reporter.test.ts rename packages/@n8n/task-runner/src/{error-reporting.ts => error-reporter.ts} (61%) diff --git a/packages/@n8n/task-runner/package.json b/packages/@n8n/task-runner/package.json index 1b4d34326a..fc06d7bd32 100644 --- a/packages/@n8n/task-runner/package.json +++ b/packages/@n8n/task-runner/package.json @@ -35,10 +35,10 @@ }, "dependencies": { "@n8n/config": "workspace:*", - "acorn": "8.14.0", - "acorn-walk": "8.3.4", "@sentry/integrations": "catalog:", "@sentry/node": "catalog:", + "acorn": "8.14.0", + "acorn-walk": "8.3.4", "n8n-core": "workspace:*", "n8n-workflow": "workspace:*", "nanoid": "^3.3.6", diff --git a/packages/@n8n/task-runner/src/__tests__/error-reporter.test.ts b/packages/@n8n/task-runner/src/__tests__/error-reporter.test.ts new file mode 100644 index 0000000000..9345819329 --- /dev/null +++ b/packages/@n8n/task-runner/src/__tests__/error-reporter.test.ts @@ -0,0 +1,31 @@ +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-reporting.ts b/packages/@n8n/task-runner/src/error-reporter.ts similarity index 61% rename from packages/@n8n/task-runner/src/error-reporting.ts rename to packages/@n8n/task-runner/src/error-reporter.ts index 1c74d5b971..e9c935d872 100644 --- a/packages/@n8n/task-runner/src/error-reporting.ts +++ b/packages/@n8n/task-runner/src/error-reporter.ts @@ -1,5 +1,6 @@ import { RewriteFrames } from '@sentry/integrations'; import { init, setTag, captureException, close } from '@sentry/node'; +import type { ErrorEvent, EventHint } from '@sentry/types'; import * as a from 'assert/strict'; import { createHash } from 'crypto'; import { ApplicationError } from 'n8n-workflow'; @@ -9,9 +10,12 @@ import type { SentryConfig } from '@/config/sentry-config'; /** * Handles error reporting using Sentry */ -export class ErrorReporting { +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; } @@ -37,7 +41,8 @@ export class ErrorReporting { 'OnUnhandledRejection', 'ContextLines', ]; - const seenErrors = new Set(); + + setTag('server_type', 'task_runner'); init({ dsn: this.dsn, @@ -46,37 +51,13 @@ export class ErrorReporting { enableTracing: false, serverName: this.sentryConfig.deploymentName, beforeBreadcrumb: () => null, + beforeSend: (event, hint) => this.beforeSend(event, hint), integrations: (integrations) => [ ...integrations.filter(({ name }) => enabledIntegrations.includes(name)), new RewriteFrames({ root: process.cwd() }), ], - async beforeSend(event, { originalException }) { - 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 (seenErrors.has(eventHash)) return null; - seenErrors.add(eventHash); - } - - return event; - }, }); - setTag('server_type', 'task_runner'); - this.isInitialized = true; } @@ -87,4 +68,24 @@ export class ErrorReporting { await close(1000); } + + beforeSend(event: ErrorEvent, { originalException }: EventHint) { + if (!originalException) return null; + + 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 29ccecc61a..c6e8cb314c 100644 --- a/packages/@n8n/task-runner/src/start.ts +++ b/packages/@n8n/task-runner/src/start.ts @@ -2,12 +2,12 @@ import { ensureError } from 'n8n-workflow'; import Container from 'typedi'; import { MainConfig } from './config/main-config'; -import type { ErrorReporting } from './error-reporting'; +import type { ErrorReporter } from './error-reporter'; import { JsTaskRunner } from './js-task-runner/js-task-runner'; let runner: JsTaskRunner | undefined; let isShuttingDown = false; -let errorReporting: ErrorReporting | undefined; +let errorReporter: ErrorReporter | undefined; function createSignalHandler(signal: string) { return async function onSignal() { @@ -24,9 +24,9 @@ function createSignalHandler(signal: string) { runner = undefined; } - if (errorReporting) { - await errorReporting.stop(); - errorReporting = undefined; + if (errorReporter) { + await errorReporter.stop(); + errorReporter = undefined; } } catch (e) { const error = ensureError(e); @@ -42,9 +42,9 @@ void (async function start() { const config = Container.get(MainConfig); if (config.sentryConfig.sentryDsn) { - const { ErrorReporting } = await import('@/error-reporting'); - errorReporting = new ErrorReporting(config.sentryConfig); - await errorReporting.start(); + const { ErrorReporter } = await import('@/error-reporter'); + errorReporter = new ErrorReporter(config.sentryConfig); + await errorReporter.start(); } runner = new JsTaskRunner(config);