-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
feat(hono): Add shouldHandleError as middleware option
#21205
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
252cea4
54eaea5
27d9c02
52aa6ff
4d4ab5a
dc51f72
dfaf53f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,6 +2,7 @@ import { type BaseTransportOptions, debug, type Options, getClient } from '@sent | |
| import type { Env, Hono, MiddlewareHandler } from 'hono'; | ||
| import { requestHandler, responseHandler } from '../shared/middlewareHandlers'; | ||
| import { applyPatches } from '../shared/applyPatches'; | ||
| import type { SentryHonoMiddlewareOptions } from '../shared/types'; | ||
|
|
||
| export interface HonoNodeOptions extends Options<BaseTransportOptions> {} | ||
|
|
||
|
|
@@ -13,7 +14,7 @@ export interface HonoNodeOptions extends Options<BaseTransportOptions> {} | |
| * | ||
| * **Note:** You must initialize Sentry separately before using this middleware. Typically, this is done by calling `Sentry.init()` in an `instrument.ts` file and loading it via the Node `--import` flag. | ||
| */ | ||
| export const sentry = <E extends Env>(app: Hono<E>): MiddlewareHandler => { | ||
| export const sentry = <E extends Env>(app: Hono<E>, options?: SentryHonoMiddlewareOptions): MiddlewareHandler => { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. m: should this take
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, because in the Node runtime the options are passed to the |
||
| const sentryClient = getClient(); | ||
| if (sentryClient === undefined) { | ||
| debug.warn( | ||
|
|
@@ -31,6 +32,6 @@ export const sentry = <E extends Env>(app: Hono<E>): MiddlewareHandler => { | |
|
|
||
| await next(); // Handler runs in between Request above ⤴ and Response below ⤵ | ||
|
|
||
| responseHandler(context); | ||
| responseHandler(context, options?.shouldHandleError); | ||
| }; | ||
| }; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,19 @@ | ||
| /** | ||
| * Default implementation of the `shouldHandleError` callback. | ||
| * | ||
| * Returns `true` (capture) for 5xx errors and any error without a `status` property | ||
| * | ||
| * Returns `false` (skip) for 3xx and 4xx errors (they still generate spans and transactions for tracing) | ||
| * | ||
| * Checks any error-like value that carries a numeric `status` property. This covers | ||
| * Hono's `HTTPException`, third-party middleware errors, and custom error subclasses. | ||
| */ | ||
| export function defaultShouldHandleError(error: unknown): boolean { | ||
| if (typeof error !== 'object' || error === null) { | ||
| return true; | ||
| } | ||
|
|
||
| const status = (error as { status?: unknown }).status; | ||
|
|
||
| return !(typeof status === 'number' && status >= 300 && status < 500); | ||
| } |
This file was deleted.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,24 @@ | ||
| /** | ||
| * Middleware-specific options shared across all Hono runtime adapters. | ||
| * These options are distinct from the SDK initialization options (DSN, sample rates, etc.). | ||
| */ | ||
| export interface SentryHonoMiddlewareOptions { | ||
| /** | ||
| * Determines whether a given Hono error thrown in the response should be captured and sent to Sentry. | ||
| * | ||
| * When not provided, the default behavior applies: 3xx and 4xx HTTP errors are | ||
| * considered expected and are not captured. All other errors are captured. | ||
| * | ||
| * @example | ||
| * // Capture everything, including 4xx errors: | ||
| * shouldHandleError: () => true | ||
| * | ||
| * @example | ||
| * // Capture only 5xx errors and suppress everything else: | ||
| * shouldHandleError: (err) => { | ||
| * const status = (err as { status?: number })?.status; | ||
| * return typeof status === 'number' ? status >= 500 : true; | ||
| * } | ||
| */ | ||
| shouldHandleError?: (error: unknown) => boolean; | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,4 @@ | ||
| import { | ||
| captureException, | ||
| getActiveSpan, | ||
| getOriginalFunction, | ||
| getRootSpan, | ||
|
|
@@ -10,7 +9,7 @@ import { | |
| type WrappedFunction, | ||
| } from '@sentry/core'; | ||
| import { type MiddlewareHandler } from 'hono'; | ||
| import { isExpectedError } from './isExpectedError'; | ||
| import { defaultShouldHandleError } from './defaultShouldHandleError'; | ||
|
|
||
| const MIDDLEWARE_ORIGIN = 'auto.middleware.hono'; | ||
|
|
||
|
|
@@ -44,13 +43,11 @@ export function wrapMiddlewareWithSpan(handler: MiddlewareHandler): MiddlewareHa | |
| try { | ||
| return await handler(context, next); | ||
| } catch (error) { | ||
| if (!isExpectedError(error)) { | ||
| // Error capture is handled by `responseHandler` via `context.error`, so this wrapper only sets | ||
| // span status (based on our default "error" conditions) and rethrows (no `captureException`). | ||
| if (defaultShouldHandleError(error)) { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. m: is this intentionally
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just confirming that here we do not use the user-provided shouldHandleError, was that intentional? I think it is fine tbh. Just e.g. if a user wants to catch 400s as errors then span status would not be set to error here.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We'll only set the span as "errored" when our default conditions apply. |
||
| span.setStatus({ code: SPAN_STATUS_ERROR, message: 'internal_error' }); | ||
| captureException(error, { | ||
| mechanism: { handled: false, type: MIDDLEWARE_ORIGIN }, | ||
| }); | ||
| } | ||
|
|
||
| throw error; | ||
| } finally { | ||
| span.end(); | ||
|
|
||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same file as before |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,78 @@ | ||
| import { HTTPException } from 'hono/http-exception'; | ||
| import { describe, expect, it } from 'vitest'; | ||
| import { defaultShouldHandleError } from '../../src/shared/defaultShouldHandleError'; | ||
|
|
||
| describe('defaultShouldHandleError', () => { | ||
| describe('HTTPException', () => { | ||
| it('returns false for 4xx HTTPException (skip)', () => { | ||
| expect(defaultShouldHandleError(new HTTPException(400, { message: 'Bad Request' }))).toBe(false); | ||
| expect(defaultShouldHandleError(new HTTPException(401, { message: 'Unauthorized' }))).toBe(false); | ||
| expect(defaultShouldHandleError(new HTTPException(403, { message: 'Forbidden' }))).toBe(false); | ||
| expect(defaultShouldHandleError(new HTTPException(404, { message: 'Not Found' }))).toBe(false); | ||
| expect(defaultShouldHandleError(new HTTPException(422, { message: 'Unprocessable Entity' }))).toBe(false); | ||
| expect(defaultShouldHandleError(new HTTPException(499))).toBe(false); | ||
| }); | ||
|
|
||
| it('returns true for 5xx HTTPException (capture)', () => { | ||
| expect(defaultShouldHandleError(new HTTPException(500, { message: 'Internal Server Error' }))).toBe(true); | ||
| expect(defaultShouldHandleError(new HTTPException(502, { message: 'Bad Gateway' }))).toBe(true); | ||
| expect(defaultShouldHandleError(new HTTPException(503, { message: 'Service Unavailable' }))).toBe(true); | ||
| }); | ||
| }); | ||
|
|
||
| describe('custom error classes with status property', () => { | ||
| it('returns false for custom Error subclass with 4xx status (skip)', () => { | ||
| class AuthError extends Error { | ||
| status = 401; | ||
| } | ||
| expect(defaultShouldHandleError(new AuthError('unauthorized'))).toBe(false); | ||
| }); | ||
|
|
||
| it('returns true for custom Error subclass with 5xx status (capture)', () => { | ||
| class DbError extends Error { | ||
| status = 500; | ||
| } | ||
| expect(defaultShouldHandleError(new DbError('connection lost'))).toBe(true); | ||
| }); | ||
|
|
||
| it('returns false for plain object with 4xx status (skip)', () => { | ||
| expect(defaultShouldHandleError({ status: 404, message: 'Not Found' })).toBe(false); | ||
| expect(defaultShouldHandleError({ status: 400 })).toBe(false); | ||
| }); | ||
|
|
||
| it('returns true for plain object with 5xx status (capture)', () => { | ||
| expect(defaultShouldHandleError({ status: 500, message: 'Internal Server Error' })).toBe(true); | ||
| }); | ||
| }); | ||
|
|
||
| describe('non-HTTP errors', () => { | ||
| it('returns true for plain Error without status (capture)', () => { | ||
| expect(defaultShouldHandleError(new Error('something broke'))).toBe(true); | ||
| }); | ||
|
|
||
| it('returns true for non-object values (capture)', () => { | ||
| expect(defaultShouldHandleError('string error')).toBe(true); | ||
| expect(defaultShouldHandleError(42)).toBe(true); | ||
| expect(defaultShouldHandleError(null)).toBe(true); | ||
| expect(defaultShouldHandleError(undefined)).toBe(true); | ||
| expect(defaultShouldHandleError(true)).toBe(true); | ||
| }); | ||
|
|
||
| it('returns true when status is not a number (capture)', () => { | ||
| expect(defaultShouldHandleError({ status: '404' })).toBe(true); | ||
| expect(defaultShouldHandleError({ status: null })).toBe(true); | ||
| expect(defaultShouldHandleError({ status: undefined })).toBe(true); | ||
| }); | ||
|
|
||
| it('returns false for 3xx status (skip)', () => { | ||
| expect(defaultShouldHandleError({ status: 301 })).toBe(false); | ||
| expect(defaultShouldHandleError({ status: 302 })).toBe(false); | ||
| expect(defaultShouldHandleError({ status: 399 })).toBe(false); | ||
| }); | ||
|
|
||
| it('returns true for 2xx status (capture)', () => { | ||
| expect(defaultShouldHandleError({ status: 200 })).toBe(true); | ||
| expect(defaultShouldHandleError({ status: 299 })).toBe(true); | ||
| }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
l: maybe an example would help to understand how this is to be used?