From 2316f0e59b35da78e29cd604dd11d48224a7efc0 Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Thu, 20 Mar 2025 22:41:04 -0400 Subject: [PATCH 1/3] feat(node): Add fastify `shouldHandleError` --- .../node-fastify/tests/errors.test.ts | 22 +++++ .../node/src/integrations/tracing/fastify.ts | 97 ++++++++++++++++--- 2 files changed, 103 insertions(+), 16 deletions(-) diff --git a/dev-packages/e2e-tests/test-applications/node-fastify/tests/errors.test.ts b/dev-packages/e2e-tests/test-applications/node-fastify/tests/errors.test.ts index 1b63fe0e0c55..ed46bffeeee3 100644 --- a/dev-packages/e2e-tests/test-applications/node-fastify/tests/errors.test.ts +++ b/dev-packages/e2e-tests/test-applications/node-fastify/tests/errors.test.ts @@ -28,3 +28,25 @@ test('Sends correct error event', async ({ baseURL }) => { parent_span_id: expect.stringMatching(/[a-f0-9]{16}/), }); }); + +test('Does not send 4xx errors by default', async ({ baseURL }) => { + // Define our test approach: we'll send both a 5xx and a 4xx request + // We should only see the 5xx error captured due to shouldHandleError's default behavior + + // Create a promise to wait for the 500 error + const serverErrorPromise = waitForError('node-fastify', event => { + // Looking for a 500 error that should be captured + return !!event.exception?.values?.[0]?.value?.includes('Internal Server Error'); + }); + + // Make a request that will trigger a 404 error + const notFoundResponse = await fetch(`${baseURL}/non-existent-route`); + expect(notFoundResponse.status).toBe(404); + + // Make a request that will trigger a 500 error + await fetch(`${baseURL}/test-server-error`); + + // Verify we receive the 500 error + const errorEvent = await serverErrorPromise; + expect(errorEvent.exception?.values?.[0]?.value).toContain('Internal Server Error'); +}); diff --git a/packages/node/src/integrations/tracing/fastify.ts b/packages/node/src/integrations/tracing/fastify.ts index 2920b134d82d..9f3a98eb3cf9 100644 --- a/packages/node/src/integrations/tracing/fastify.ts +++ b/packages/node/src/integrations/tracing/fastify.ts @@ -12,19 +12,13 @@ import type { IntegrationFn, Span } from '@sentry/core'; import { generateInstrumentOnce } from '../../otel/instrument'; import { ensureIsWrapped } from '../../utils/ensureIsWrapped'; -// We inline the types we care about here -interface Fastify { - // eslint-disable-next-line @typescript-eslint/no-explicit-any - register: (plugin: any) => void; - // eslint-disable-next-line @typescript-eslint/no-explicit-any - addHook: (hook: string, handler: (request: any, reply: any, error: Error) => void) => void; -} - /** * Minimal request type containing properties around route information. * Works for Fastify 3, 4 and presumably 5. + * + * Based on https://github.com/fastify/fastify/blob/ce3811f5f718be278bbcd4392c615d64230065a6/types/request.d.ts */ -interface FastifyRequestRouteInfo { +interface MinimalFastifyRequest { method?: string; // since fastify@4.10.0 routeOptions?: { @@ -33,6 +27,64 @@ interface FastifyRequestRouteInfo { routerPath?: string; } +/** + * Minimal reply type containing properties needed for error handling. + * + * Based on https://github.com/fastify/fastify/blob/ce3811f5f718be278bbcd4392c615d64230065a6/types/reply.d.ts + */ +interface MinimalFastifyReply { + statusCode: number; +} + +// We inline the types we care about here +interface Fastify { + // eslint-disable-next-line @typescript-eslint/no-explicit-any + register: (plugin: any) => void; + addHook( + hook: 'onError', + handler: (request: MinimalFastifyRequest, reply: MinimalFastifyReply, error: Error) => void, + ): void; + addHook(hook: 'onRequest', handler: (request: MinimalFastifyRequest, reply: MinimalFastifyReply) => void): void; +} + +interface FastifyHandlerOptions { + /** + * Callback method deciding whether error should be captured and sent to Sentry + * + * @param error Captured Fastify error + * @param request Fastify request + * @param reply Fastify reply + * + * @example + * + * ```javascript + * setupFastifyErrorHandler(app, { + * shouldHandleError(_error, _request, reply) { + * return reply.statusCode >= 400; + * }, + * }); + * ``` + * + * If using TypeScript, pass in the `FastifyRequest` and `FastifyReply` types to get type safety. + * + * ```typescript + * import type { FastifyRequest, FastifyReply } from 'fastify'; + * + * setupFastifyErrorHandler(app, { + * shouldHandleError(error, request, reply) { + * return reply.statusCode >= 500; + * }, + * }); + * ``` + */ + shouldHandleError( + this: void, + error: Error, + request: FastifyRequest, + reply: FastifyReply, + ): boolean; +} + const INTEGRATION_NAME = 'Fastify'; export const instrumentFastify = generateInstrumentOnce( @@ -73,10 +125,20 @@ const _fastifyIntegration = (() => { */ export const fastifyIntegration = defineIntegration(_fastifyIntegration); +/** + * Default function to determine if an error should be sent to Sentry + * Only sends 5xx errors by default + */ +function defaultShouldHandleError(_error: Error, _request: MinimalFastifyRequest, reply: MinimalFastifyReply): boolean { + const statusCode = reply.statusCode; + return statusCode >= 500; +} + /** * Add an Fastify error handler to capture errors to Sentry. * * @param fastify The Fastify instance to which to add the error handler + * @param options Configuration options for the handler * * @example * ```javascript @@ -92,23 +154,25 @@ export const fastifyIntegration = defineIntegration(_fastifyIntegration); * app.listen({ port: 3000 }); * ``` */ -export function setupFastifyErrorHandler(fastify: Fastify): void { +export function setupFastifyErrorHandler(fastify: Fastify, options?: Partial): void { + const shouldHandleError = options?.shouldHandleError || defaultShouldHandleError; + const plugin = Object.assign( function (fastify: Fastify, _options: unknown, done: () => void): void { - fastify.addHook('onError', async (_request, _reply, error) => { - captureException(error); + fastify.addHook('onError', async (request, reply, error) => { + if (shouldHandleError(error, request, reply)) { + captureException(error); + } }); // registering `onRequest` hook here instead of using Otel `onRequest` callback b/c `onRequest` hook // is ironically called in the fastify `preHandler` hook which is called later in the lifecycle: // https://fastify.dev/docs/latest/Reference/Lifecycle/ fastify.addHook('onRequest', async (request, _reply) => { - const reqWithRouteInfo = request as FastifyRequestRouteInfo; - // Taken from Otel Fastify instrumentation: // https://github.com/open-telemetry/opentelemetry-js-contrib/blob/main/plugins/node/opentelemetry-instrumentation-fastify/src/instrumentation.ts#L94-L96 - const routeName = reqWithRouteInfo.routeOptions?.url || reqWithRouteInfo.routerPath; - const method = reqWithRouteInfo.method || 'GET'; + const routeName = request.routeOptions?.url || request.routerPath; + const method = request.method || 'GET'; getIsolationScope().setTransactionName(`${method} ${routeName}`); }); @@ -133,6 +197,7 @@ export function setupFastifyErrorHandler(fastify: Fastify): void { }); } + // eslint-disable-next-line @typescript-eslint/unbound-method ensureIsWrapped(fastify.addHook, 'fastify'); } From eeec3d941e27e020b67bb3662bb3f01e3a07ea72 Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Fri, 21 Mar 2025 18:59:49 -0400 Subject: [PATCH 2/3] fix types and update handler definition --- .../node/src/integrations/tracing/fastify.ts | 36 ++++++++++--------- 1 file changed, 20 insertions(+), 16 deletions(-) diff --git a/packages/node/src/integrations/tracing/fastify.ts b/packages/node/src/integrations/tracing/fastify.ts index 9f3a98eb3cf9..220428cf91d4 100644 --- a/packages/node/src/integrations/tracing/fastify.ts +++ b/packages/node/src/integrations/tracing/fastify.ts @@ -18,7 +18,8 @@ import { ensureIsWrapped } from '../../utils/ensureIsWrapped'; * * Based on https://github.com/fastify/fastify/blob/ce3811f5f718be278bbcd4392c615d64230065a6/types/request.d.ts */ -interface MinimalFastifyRequest { +// eslint-disable-next-line @typescript-eslint/no-explicit-any +interface MinimalFastifyRequest extends Record { method?: string; // since fastify@4.10.0 routeOptions?: { @@ -32,7 +33,8 @@ interface MinimalFastifyRequest { * * Based on https://github.com/fastify/fastify/blob/ce3811f5f718be278bbcd4392c615d64230065a6/types/reply.d.ts */ -interface MinimalFastifyReply { +// eslint-disable-next-line @typescript-eslint/no-explicit-any +interface MinimalFastifyReply extends Record { statusCode: number; } @@ -40,6 +42,10 @@ interface MinimalFastifyReply { interface Fastify { // eslint-disable-next-line @typescript-eslint/no-explicit-any register: (plugin: any) => void; + addHook: (hook: string, handler: (...params: unknown[]) => void) => void; +} + +interface FastifyWithHooks extends Omit { addHook( hook: 'onError', handler: (request: MinimalFastifyRequest, reply: MinimalFastifyReply, error: Error) => void, @@ -52,8 +58,8 @@ interface FastifyHandlerOptions { * Callback method deciding whether error should be captured and sent to Sentry * * @param error Captured Fastify error - * @param request Fastify request - * @param reply Fastify reply + * @param request Fastify request (or any object containing at least method, routeOptions.url, and routerPath) + * @param reply Fastify reply (or any object containing at least statusCode) * * @example * @@ -65,24 +71,21 @@ interface FastifyHandlerOptions { * }); * ``` * - * If using TypeScript, pass in the `FastifyRequest` and `FastifyReply` types to get type safety. + * If using TypeScript, you can cast the request and reply to get full type safety. * * ```typescript * import type { FastifyRequest, FastifyReply } from 'fastify'; * * setupFastifyErrorHandler(app, { - * shouldHandleError(error, request, reply) { + * shouldHandleError(error, minimalRequest, minimalReply) { + * const request = minimalRequest as FastifyRequest; + * const reply = minimalReply as FastifyReply; * return reply.statusCode >= 500; * }, * }); * ``` */ - shouldHandleError( - this: void, - error: Error, - request: FastifyRequest, - reply: FastifyReply, - ): boolean; + shouldHandleError: (error: Error, request: MinimalFastifyRequest, reply: MinimalFastifyReply) => boolean; } const INTEGRATION_NAME = 'Fastify'; @@ -127,11 +130,13 @@ export const fastifyIntegration = defineIntegration(_fastifyIntegration); /** * Default function to determine if an error should be sent to Sentry - * Only sends 5xx errors by default + * + * 3xx and 4xx errors are not sent by default. */ function defaultShouldHandleError(_error: Error, _request: MinimalFastifyRequest, reply: MinimalFastifyReply): boolean { const statusCode = reply.statusCode; - return statusCode >= 500; + // 3xx and 4xx errors are not sent by default. + return statusCode >= 500 || statusCode <= 299; } /** @@ -158,7 +163,7 @@ export function setupFastifyErrorHandler(fastify: Fastify, options?: Partial void): void { + function (fastify: FastifyWithHooks, _options: unknown, done: () => void): void { fastify.addHook('onError', async (request, reply, error) => { if (shouldHandleError(error, request, reply)) { captureException(error); @@ -197,7 +202,6 @@ export function setupFastifyErrorHandler(fastify: Fastify, options?: Partial Date: Fri, 21 Mar 2025 19:09:50 -0400 Subject: [PATCH 3/3] restructure e2e test --- .../test-applications/node-fastify/src/app.ts | 10 ++++++++++ .../node-fastify/tests/errors.test.ts | 12 ++++++------ 2 files changed, 16 insertions(+), 6 deletions(-) diff --git a/dev-packages/e2e-tests/test-applications/node-fastify/src/app.ts b/dev-packages/e2e-tests/test-applications/node-fastify/src/app.ts index 275dfa786ca3..0f37bc33b90a 100644 --- a/dev-packages/e2e-tests/test-applications/node-fastify/src/app.ts +++ b/dev-packages/e2e-tests/test-applications/node-fastify/src/app.ts @@ -81,6 +81,16 @@ app.get('/test-error', async function (req, res) { res.send({ exceptionId }); }); +app.get('/test-4xx-error', async function (req, res) { + res.code(400); + throw new Error('This is a 4xx error'); +}); + +app.get('/test-5xx-error', async function (req, res) { + res.code(500); + throw new Error('This is a 5xx error'); +}); + app.get<{ Params: { id: string } }>('/test-exception/:id', async function (req, res) { throw new Error(`This is an exception with id ${req.params.id}`); }); diff --git a/dev-packages/e2e-tests/test-applications/node-fastify/tests/errors.test.ts b/dev-packages/e2e-tests/test-applications/node-fastify/tests/errors.test.ts index ed46bffeeee3..1ac1d3d88234 100644 --- a/dev-packages/e2e-tests/test-applications/node-fastify/tests/errors.test.ts +++ b/dev-packages/e2e-tests/test-applications/node-fastify/tests/errors.test.ts @@ -36,17 +36,17 @@ test('Does not send 4xx errors by default', async ({ baseURL }) => { // Create a promise to wait for the 500 error const serverErrorPromise = waitForError('node-fastify', event => { // Looking for a 500 error that should be captured - return !!event.exception?.values?.[0]?.value?.includes('Internal Server Error'); + return !!event.exception?.values?.[0]?.value?.includes('This is a 5xx error'); }); - // Make a request that will trigger a 404 error - const notFoundResponse = await fetch(`${baseURL}/non-existent-route`); - expect(notFoundResponse.status).toBe(404); + // Make a request that will trigger a 400 error + const notFoundResponse = await fetch(`${baseURL}/test-4xx-error`); + expect(notFoundResponse.status).toBe(400); // Make a request that will trigger a 500 error - await fetch(`${baseURL}/test-server-error`); + await fetch(`${baseURL}/test-5xx-error`); // Verify we receive the 500 error const errorEvent = await serverErrorPromise; - expect(errorEvent.exception?.values?.[0]?.value).toContain('Internal Server Error'); + expect(errorEvent.exception?.values?.[0]?.value).toContain('This is a 5xx error'); });