From aecfb7137c10c3f32ae2d86094b33fa385cd0b8e Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Wed, 17 Jul 2024 14:07:33 +0200 Subject: [PATCH 1/2] feat(express): Allow to pass options to `setupExpressErrorHandler` Allows to pass this through to the underlying `expressErrorHandler`. --- .../setupExpressErrorHandler/server.js | 33 +++++++++++++++++++ .../express/setupExpressErrorHandler/test.ts | 30 +++++++++++++++++ .../express/tracing/withError/server.js | 2 +- .../node/src/integrations/tracing/express.ts | 7 ++-- 4 files changed, 69 insertions(+), 3 deletions(-) create mode 100644 dev-packages/node-integration-tests/suites/express/setupExpressErrorHandler/server.js create mode 100644 dev-packages/node-integration-tests/suites/express/setupExpressErrorHandler/test.ts diff --git a/dev-packages/node-integration-tests/suites/express/setupExpressErrorHandler/server.js b/dev-packages/node-integration-tests/suites/express/setupExpressErrorHandler/server.js new file mode 100644 index 000000000000..0e73923cf88a --- /dev/null +++ b/dev-packages/node-integration-tests/suites/express/setupExpressErrorHandler/server.js @@ -0,0 +1,33 @@ +const { loggingTransport } = require('@sentry-internal/node-integration-tests'); +const Sentry = require('@sentry/node'); + +Sentry.init({ + dsn: 'https://public@dsn.ingest.sentry.io/1337', + release: '1.0', + transport: loggingTransport, +}); + +// express must be required after Sentry is initialized +const express = require('express'); +const cors = require('cors'); +const { startExpressServerAndSendPortToRunner } = require('@sentry-internal/node-integration-tests'); + +const app = express(); + +app.use(cors()); + +app.get('/test1', (_req, _res) => { + throw new Error('error_1'); +}); + +app.get('/test2', (_req, _res) => { + throw new Error('error_2'); +}); + +Sentry.setupExpressErrorHandler(app, { + shouldHandleError: error => { + return error.message === 'error_2'; + }, +}); + +startExpressServerAndSendPortToRunner(app); diff --git a/dev-packages/node-integration-tests/suites/express/setupExpressErrorHandler/test.ts b/dev-packages/node-integration-tests/suites/express/setupExpressErrorHandler/test.ts new file mode 100644 index 000000000000..97ff6e3fa769 --- /dev/null +++ b/dev-packages/node-integration-tests/suites/express/setupExpressErrorHandler/test.ts @@ -0,0 +1,30 @@ +import { cleanupChildProcesses, createRunner } from '../../../utils/runner'; + +describe('express setupExpressErrorHandler', () => { + afterAll(() => { + cleanupChildProcesses(); + }); + + describe('CJS', () => { + test('allows to pass options to setupExpressErrorHandler', done => { + const runner = createRunner(__dirname, 'server.js') + .expect({ + event: { + exception: { + values: [ + { + value: 'error_2', + }, + ], + }, + }, + }) + .start(done); + + // this error is filtered & ignored + expect(() => runner.makeRequest('get', '/test1')).rejects.toThrow(); + // this error is actually captured + expect(() => runner.makeRequest('get', '/test2')).rejects.toThrow(); + }); + }); +}); diff --git a/dev-packages/node-integration-tests/suites/express/tracing/withError/server.js b/dev-packages/node-integration-tests/suites/express/tracing/withError/server.js index 3b45591ec4df..890d26cda044 100644 --- a/dev-packages/node-integration-tests/suites/express/tracing/withError/server.js +++ b/dev-packages/node-integration-tests/suites/express/tracing/withError/server.js @@ -20,7 +20,7 @@ const app = express(); app.use(cors()); app.get('/test/:id1/:id2', (_req, res) => { - Sentry.captureMessage(new Error('error_1')); + Sentry.captureException(new Error('error_1')); res.send('Success'); }); diff --git a/packages/node/src/integrations/tracing/express.ts b/packages/node/src/integrations/tracing/express.ts index 00c5735207d4..773dca32930a 100644 --- a/packages/node/src/integrations/tracing/express.ts +++ b/packages/node/src/integrations/tracing/express.ts @@ -135,8 +135,11 @@ export function expressErrorHandler(options?: { * Setup an error handler for Express. * The error handler must be before any other middleware and after all controllers. */ -export function setupExpressErrorHandler(app: { use: (middleware: ExpressMiddleware) => unknown }): void { - app.use(expressErrorHandler()); +export function setupExpressErrorHandler( + app: { use: (middleware: ExpressMiddleware) => unknown }, + options?: Parameters[0], +): void { + app.use(expressErrorHandler(options)); ensureIsWrapped(app.use, 'express'); } From 3c462a8813e6e66597947a462d0d8c144f5513c1 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Wed, 17 Jul 2024 15:30:07 +0200 Subject: [PATCH 2/2] define option types better --- packages/node/src/integrations/tracing/express.ts | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/packages/node/src/integrations/tracing/express.ts b/packages/node/src/integrations/tracing/express.ts index 773dca32930a..b8c50e0eb621 100644 --- a/packages/node/src/integrations/tracing/express.ts +++ b/packages/node/src/integrations/tracing/express.ts @@ -83,16 +83,18 @@ type ExpressMiddleware = ( next: (error: MiddlewareError) => void, ) => void; -/** - * An Express-compatible error handler. - */ -export function expressErrorHandler(options?: { +interface ExpressHandlerOptions { /** * Callback method deciding whether error should be captured and sent to Sentry * @param error Captured middleware error */ shouldHandleError?(this: void, error: MiddlewareError): boolean; -}): ExpressMiddleware { +} + +/** + * An Express-compatible error handler. + */ +export function expressErrorHandler(options?: ExpressHandlerOptions): ExpressMiddleware { return function sentryErrorMiddleware( error: MiddlewareError, _req: http.IncomingMessage, @@ -137,7 +139,7 @@ export function expressErrorHandler(options?: { */ export function setupExpressErrorHandler( app: { use: (middleware: ExpressMiddleware) => unknown }, - options?: Parameters[0], + options?: ExpressHandlerOptions, ): void { app.use(expressErrorHandler(options)); ensureIsWrapped(app.use, 'express');