Skip to content

Commit 4018f80

Browse files
s1gr1ds1gr1d
authored
fix(hapi): Specify error channel to filter boom errors (#12725)
If errors are handled with Boom inside `onPreResponse`, the error should not be reported to Sentry. fixes #12702
1 parent 8c0631a commit 4018f80

File tree

7 files changed

+371
-219
lines changed

7 files changed

+371
-219
lines changed

‎dev-packages/e2e-tests/test-applications/node-hapi/package.json‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,8 @@
1111
"test:assert": "pnpm test"
1212
},
1313
"dependencies": {
14-
"@hapi/hapi": "21.3.2",
14+
"@hapi/boom": "10.0.1",
15+
"@hapi/hapi": "21.3.10",
1516
"@sentry/node": "latest || *",
1617
"@sentry/types": "latest || *"
1718
},

‎dev-packages/e2e-tests/test-applications/node-hapi/src/app.js‎

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ Sentry.init({
1010
});
1111

1212
const Hapi = require('@hapi/hapi');
13+
const Boom = require('@hapi/boom');
1314

1415
const server = Hapi.server({
1516
port: 3030,
@@ -63,6 +64,55 @@ const init = async () => {
6364
throw new Error('This is an error');
6465
},
6566
});
67+
68+
server.route({
69+
method: 'GET',
70+
path: '/test-failure-boom-4xx',
71+
handler: async function (request, h) {
72+
throw new Error('This is a JS error (boom in onPreResponse)');
73+
},
74+
});
75+
76+
server.route({
77+
method: 'GET',
78+
path: '/test-failure-boom-5xx',
79+
handler: async function (request, h) {
80+
throw new Error('This is an error (boom in onPreResponse)');
81+
},
82+
});
83+
84+
server.route({
85+
method: 'GET',
86+
path: '/test-failure-JS-error-onPreResponse',
87+
handler: async function (request, h) {
88+
throw new Error('This is an error (another JS error in onPreResponse)');
89+
},
90+
});
91+
92+
server.route({
93+
method: 'GET',
94+
path: '/test-failure-2xx-override-onPreResponse',
95+
handler: async function (request, h) {
96+
throw new Error('This is a JS error (2xx override in onPreResponse)');
97+
},
98+
});
99+
100+
// This runs after the route handler
101+
server.ext('onPreResponse', (request, h) => {
102+
const path = request.route.path;
103+
104+
if (path.includes('boom-4xx')) {
105+
throw Boom.notFound('4xx not found (onPreResponse)');
106+
} else if (path.includes('boom-5xx')) {
107+
throw Boom.gatewayTimeout('5xx not implemented (onPreResponse)');
108+
} else if (path.includes('JS-error-onPreResponse')) {
109+
throw new Error('JS error (onPreResponse)');
110+
} else if (path.includes('2xx-override-onPreResponse')) {
111+
return h.response('2xx override').code(200);
112+
} else {
113+
return h.continue;
114+
}
115+
});
66116
};
67117

68118
(async () => {

‎dev-packages/e2e-tests/test-applications/node-hapi/tests/errors.test.ts‎

Lines changed: 94 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -53,3 +53,97 @@ test('sends error with parameterized transaction name', async ({ baseURL }) => {
5353

5454
expect(errorEvent?.transaction).toBe('GET /test-error/{id}');
5555
});
56+
57+
test('Does not send errors to Sentry if boom throws in "onPreResponse" after JS error in route handler', async ({
58+
baseURL,
59+
}) => {
60+
let errorEventOccurred = false;
61+
62+
waitForError('node-hapi', event => {
63+
if (event.exception?.values?.[0]?.value?.includes('This is a JS error (boom in onPreResponse)')) {
64+
errorEventOccurred = true;
65+
}
66+
return false; // expects to return a boolean (but not relevant here)
67+
});
68+
69+
const transactionEventPromise4xx = waitForTransaction('node-hapi', transactionEvent => {
70+
return transactionEvent?.transaction === 'GET /test-failure-boom-4xx';
71+
});
72+
73+
const transactionEventPromise5xx = waitForTransaction('node-hapi', transactionEvent => {
74+
return transactionEvent?.transaction === 'GET /test-failure-boom-5xx';
75+
});
76+
77+
const response4xx = await fetch(`${baseURL}/test-failure-boom-4xx`);
78+
const response5xx = await fetch(`${baseURL}/test-failure-boom-5xx`);
79+
80+
expect(response4xx.status).toBe(404);
81+
expect(response5xx.status).toBe(504);
82+
83+
const transactionEvent4xx = await transactionEventPromise4xx;
84+
const transactionEvent5xx = await transactionEventPromise5xx;
85+
86+
expect(errorEventOccurred).toBe(false);
87+
expect(transactionEvent4xx.transaction).toBe('GET /test-failure-boom-4xx');
88+
expect(transactionEvent5xx.transaction).toBe('GET /test-failure-boom-5xx');
89+
});
90+
91+
test('Does not send error to Sentry if error response is overwritten with 2xx in "onPreResponse"', async ({
92+
baseURL,
93+
}) => {
94+
let errorEventOccurred = false;
95+
96+
waitForError('node-hapi', event => {
97+
if (event.exception?.values?.[0]?.value?.includes('This is a JS error (2xx override in onPreResponse)')) {
98+
errorEventOccurred = true;
99+
}
100+
return false; // expects to return a boolean (but not relevant here)
101+
});
102+
103+
const transactionEventPromise = waitForTransaction('node-hapi', transactionEvent => {
104+
return transactionEvent?.transaction === 'GET /test-failure-2xx-override-onPreResponse';
105+
});
106+
107+
const response = await fetch(`${baseURL}/test-failure-2xx-override-onPreResponse`);
108+
109+
const transactionEvent = await transactionEventPromise;
110+
111+
expect(response.status).toBe(200);
112+
expect(errorEventOccurred).toBe(false);
113+
expect(transactionEvent.transaction).toBe('GET /test-failure-2xx-override-onPreResponse');
114+
});
115+
116+
test('Only sends onPreResponse error to Sentry if JS error is thrown in route handler AND onPreResponse', async ({
117+
baseURL,
118+
}) => {
119+
const errorEventPromise = waitForError('node-hapi', errorEvent => {
120+
return errorEvent?.exception?.values?.[0]?.value?.includes('JS error (onPreResponse)') || false;
121+
});
122+
123+
let routeHandlerErrorOccurred = false;
124+
125+
waitForError('node-hapi', event => {
126+
if (
127+
!event.type &&
128+
event.exception?.values?.[0]?.value?.includes('This is an error (another JS error in onPreResponse)')
129+
) {
130+
routeHandlerErrorOccurred = true;
131+
}
132+
return false; // expects to return a boolean (but not relevant here)
133+
});
134+
135+
const transactionEventPromise = waitForTransaction('node-hapi', transactionEvent => {
136+
return transactionEvent?.transaction === 'GET /test-failure-JS-error-onPreResponse';
137+
});
138+
139+
const response = await fetch(`${baseURL}/test-failure-JS-error-onPreResponse`);
140+
141+
expect(response.status).toBe(500);
142+
143+
const errorEvent = await errorEventPromise;
144+
const transactionEvent = await transactionEventPromise;
145+
146+
expect(routeHandlerErrorOccurred).toBe(false);
147+
expect(transactionEvent.transaction).toBe('GET /test-failure-JS-error-onPreResponse');
148+
expect(errorEvent.transaction).toEqual('GET /test-failure-JS-error-onPreResponse');
149+
});

‎dev-packages/e2e-tests/test-applications/node-hapi/tests/transactions.test.ts‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -77,6 +77,25 @@ test('Sends successful transaction', async ({ baseURL }) => {
7777
timestamp: expect.any(Number),
7878
trace_id: expect.any(String),
7979
},
80+
{
81+
// this comes from "onPreResponse"
82+
data: {
83+
'hapi.type': 'server.ext',
84+
'otel.kind': 'INTERNAL',
85+
'sentry.op': 'server.ext.hapi',
86+
'sentry.origin': 'auto.http.otel.hapi',
87+
'server.ext.type': 'onPreResponse',
88+
},
89+
description: 'ext - onPreResponse',
90+
op: 'server.ext.hapi',
91+
origin: 'auto.http.otel.hapi',
92+
parent_span_id: expect.any(String),
93+
span_id: expect.any(String),
94+
start_timestamp: expect.any(Number),
95+
status: 'ok',
96+
timestamp: expect.any(Number),
97+
trace_id: expect.any(String),
98+
},
8099
]);
81100
});
82101

‎dev-packages/node-integration-tests/package.json‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,7 @@
2626
},
2727
"dependencies": {
2828
"@aws-sdk/client-s3": "^3.552.0",
29-
"@hapi/hapi": "^20.3.0",
29+
"@hapi/hapi": "^21.3.10",
3030
"@nestjs/common": "^10.3.7",
3131
"@nestjs/core": "^10.3.3",
3232
"@nestjs/platform-express": "^10.3.3",

‎packages/node/src/integrations/tracing/hapi/index.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -61,7 +61,7 @@ export const hapiErrorPlugin = {
6161
register: async function (serverArg: Record<any, any>) {
6262
const server = serverArg as unknown as Server;
6363

64-
server.events.on('request', (request: Request, event: RequestEvent) => {
64+
server.events.on({ name: 'request', channels: ['error'] }, (request: Request, event: RequestEvent) => {
6565
if (getIsolationScope() !== getDefaultIsolationScope()) {
6666
const route = request.route;
6767
if (route && route.path) {

0 commit comments

Comments
 (0)