From 4dad7406722a9e7e52529e07388905db6e581a4e Mon Sep 17 00:00:00 2001 From: Onur Temizkan Date: Mon, 25 Mar 2024 15:11:43 +0000 Subject: [PATCH 1/2] fix(node): Skip capturing Hapi Boom error responses. (#11151) Resolves: https://github.com/getsentry/sentry-javascript/issues/11069 After checking the behaviour, it seems to me that we don't need to capture any Boom responses, as the errors that may cause a `5xx` response are already captured by the core logic. To add an option to control this behaviour, we need to update the usage of `hapiErrorPlugin`, converting it to a function signature, which IMO may not worth doing, as I'm not sure if users in general would need to use it. This also adds `expectError()` to the `node-integration-tests` runner to allow `5xx` responses to be tested. --- .../tracing-experimental/hapi/scenario.js | 26 ++++++++++++++ .../suites/tracing-experimental/hapi/test.ts | 35 +++++++++++++++++++ .../node-integration-tests/utils/runner.ts | 18 +++++++++- packages/node/src/integrations/hapi/index.ts | 8 +---- 4 files changed, 79 insertions(+), 8 deletions(-) diff --git a/dev-packages/node-integration-tests/suites/tracing-experimental/hapi/scenario.js b/dev-packages/node-integration-tests/suites/tracing-experimental/hapi/scenario.js index 5f2c898fad60..1accd4dcc869 100644 --- a/dev-packages/node-integration-tests/suites/tracing-experimental/hapi/scenario.js +++ b/dev-packages/node-integration-tests/suites/tracing-experimental/hapi/scenario.js @@ -5,11 +5,13 @@ Sentry.init({ dsn: 'https://public@dsn.ingest.sentry.io/1337', release: '1.0', debug: true, + integrations: [Sentry.hapiIntegration()], tracesSampleRate: 1.0, transport: loggingTransport, }); const Hapi = require('@hapi/hapi'); +const Boom = require('@hapi/boom'); const port = 5999; @@ -27,6 +29,30 @@ const init = async () => { }, }); + server.route({ + method: 'GET', + path: '/error', + handler: (_request, _h) => { + return new Error('Sentry Test Error'); + }, + }); + + server.route({ + method: 'GET', + path: '/boom-error', + handler: (_request, _h) => { + return new Boom.Boom('Sentry Test Error'); + }, + }); + + server.route({ + method: 'GET', + path: '/promise-error', + handler: async (_request, _h) => { + return Promise.reject(new Error('Sentry Test Error')); + }, + }); + await server.start(); sendPortToRunner(port); diff --git a/dev-packages/node-integration-tests/suites/tracing-experimental/hapi/test.ts b/dev-packages/node-integration-tests/suites/tracing-experimental/hapi/test.ts index 148bf83bb397..47cb3f65e255 100644 --- a/dev-packages/node-integration-tests/suites/tracing-experimental/hapi/test.ts +++ b/dev-packages/node-integration-tests/suites/tracing-experimental/hapi/test.ts @@ -26,10 +26,45 @@ conditionalTest({ min: 14 })('hapi auto-instrumentation', () => { ]), }; + const EXPECTED_ERROR_EVENT = { + exception: { + values: [ + { + type: 'Error', + value: 'Sentry Test Error', + }, + ], + }, + }; + test('CJS - should auto-instrument `@hapi/hapi` package.', done => { createRunner(__dirname, 'scenario.js') .expect({ transaction: EXPECTED_TRANSACTION }) .start(done) .makeRequest('get', '/'); }); + + test('CJS - should handle returned plain errors in routes.', done => { + createRunner(__dirname, 'scenario.js') + .expect({ event: EXPECTED_ERROR_EVENT }) + .expectError() + .start(done) + .makeRequest('get', '/error'); + }); + + test('CJS - should handle returned Boom errors in routes.', done => { + createRunner(__dirname, 'scenario.js') + .expect({ event: EXPECTED_ERROR_EVENT }) + .expectError() + .start(done) + .makeRequest('get', '/boom-error'); + }); + + test('CJS - should handle promise rejections in routes.', done => { + createRunner(__dirname, 'scenario.js') + .expect({ event: EXPECTED_ERROR_EVENT }) + .expectError() + .start(done) + .makeRequest('get', '/promise-error'); + }); }); diff --git a/dev-packages/node-integration-tests/utils/runner.ts b/dev-packages/node-integration-tests/utils/runner.ts index 515a2627acf8..5da33508bda0 100644 --- a/dev-packages/node-integration-tests/utils/runner.ts +++ b/dev-packages/node-integration-tests/utils/runner.ts @@ -126,6 +126,7 @@ export function createRunner(...paths: string[]) { let withSentryServer = false; let dockerOptions: DockerOptions | undefined; let ensureNoErrorOutput = false; + let expectError = false; if (testPath.endsWith('.ts')) { flags.push('-r', 'ts-node/register'); @@ -136,6 +137,10 @@ export function createRunner(...paths: string[]) { expectedEnvelopes.push(expected); return this; }, + expectError: function () { + expectError = true; + return this; + }, withFlags: function (...args: string[]) { flags.push(...args); return this; @@ -347,7 +352,18 @@ export function createRunner(...paths: string[]) { } const url = `http://localhost:${scenarioServerPort}${path}`; - if (method === 'get') { + if (expectError) { + try { + if (method === 'get') { + await axios.get(url, { headers }); + } else { + await axios.post(url, { headers }); + } + } catch (e) { + return; + } + return; + } else if (method === 'get') { return (await axios.get(url, { headers })).data; } else { return (await axios.post(url, { headers })).data; diff --git a/packages/node/src/integrations/hapi/index.ts b/packages/node/src/integrations/hapi/index.ts index 82f8737721a9..c41b8ab080ec 100644 --- a/packages/node/src/integrations/hapi/index.ts +++ b/packages/node/src/integrations/hapi/index.ts @@ -20,10 +20,6 @@ function isResponseObject(response: ResponseObject | Boom): response is Response return response && (response as ResponseObject).statusCode !== undefined; } -function isBoomObject(response: ResponseObject | Boom): response is Boom { - return response && (response as Boom).isBoom !== undefined; -} - function isErrorEvent(event: RequestEvent): event is RequestEvent { return event && (event as RequestEvent).error !== undefined; } @@ -51,9 +47,7 @@ export const hapiErrorPlugin = { // eslint-disable-next-line deprecation/deprecation const transaction = getActiveTransaction(); - if (request.response && isBoomObject(request.response)) { - sendErrorToSentry(request.response); - } else if (isErrorEvent(event)) { + if (isErrorEvent(event)) { sendErrorToSentry(event.error); } From 5e2ffa36fea705c355e1b4455b4ae9fb8673da92 Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Thu, 28 Mar 2024 13:30:29 -0400 Subject: [PATCH 2/2] revert hapi integration test changes --- .../tracing-experimental/hapi/scenario.js | 26 -------------- .../suites/tracing-experimental/hapi/test.ts | 35 ------------------- 2 files changed, 61 deletions(-) diff --git a/dev-packages/node-integration-tests/suites/tracing-experimental/hapi/scenario.js b/dev-packages/node-integration-tests/suites/tracing-experimental/hapi/scenario.js index 1accd4dcc869..5f2c898fad60 100644 --- a/dev-packages/node-integration-tests/suites/tracing-experimental/hapi/scenario.js +++ b/dev-packages/node-integration-tests/suites/tracing-experimental/hapi/scenario.js @@ -5,13 +5,11 @@ Sentry.init({ dsn: 'https://public@dsn.ingest.sentry.io/1337', release: '1.0', debug: true, - integrations: [Sentry.hapiIntegration()], tracesSampleRate: 1.0, transport: loggingTransport, }); const Hapi = require('@hapi/hapi'); -const Boom = require('@hapi/boom'); const port = 5999; @@ -29,30 +27,6 @@ const init = async () => { }, }); - server.route({ - method: 'GET', - path: '/error', - handler: (_request, _h) => { - return new Error('Sentry Test Error'); - }, - }); - - server.route({ - method: 'GET', - path: '/boom-error', - handler: (_request, _h) => { - return new Boom.Boom('Sentry Test Error'); - }, - }); - - server.route({ - method: 'GET', - path: '/promise-error', - handler: async (_request, _h) => { - return Promise.reject(new Error('Sentry Test Error')); - }, - }); - await server.start(); sendPortToRunner(port); diff --git a/dev-packages/node-integration-tests/suites/tracing-experimental/hapi/test.ts b/dev-packages/node-integration-tests/suites/tracing-experimental/hapi/test.ts index 47cb3f65e255..148bf83bb397 100644 --- a/dev-packages/node-integration-tests/suites/tracing-experimental/hapi/test.ts +++ b/dev-packages/node-integration-tests/suites/tracing-experimental/hapi/test.ts @@ -26,45 +26,10 @@ conditionalTest({ min: 14 })('hapi auto-instrumentation', () => { ]), }; - const EXPECTED_ERROR_EVENT = { - exception: { - values: [ - { - type: 'Error', - value: 'Sentry Test Error', - }, - ], - }, - }; - test('CJS - should auto-instrument `@hapi/hapi` package.', done => { createRunner(__dirname, 'scenario.js') .expect({ transaction: EXPECTED_TRANSACTION }) .start(done) .makeRequest('get', '/'); }); - - test('CJS - should handle returned plain errors in routes.', done => { - createRunner(__dirname, 'scenario.js') - .expect({ event: EXPECTED_ERROR_EVENT }) - .expectError() - .start(done) - .makeRequest('get', '/error'); - }); - - test('CJS - should handle returned Boom errors in routes.', done => { - createRunner(__dirname, 'scenario.js') - .expect({ event: EXPECTED_ERROR_EVENT }) - .expectError() - .start(done) - .makeRequest('get', '/boom-error'); - }); - - test('CJS - should handle promise rejections in routes.', done => { - createRunner(__dirname, 'scenario.js') - .expect({ event: EXPECTED_ERROR_EVENT }) - .expectError() - .start(done) - .makeRequest('get', '/promise-error'); - }); });