feat(hono): Add shouldHandleError as middleware option#21205
Conversation
| if (!isExpectedError(error)) { | ||
| // Error capture is handled by `responseHandler` via `context.error`, so this | ||
| // wrapper only sets span status and rethrows (no `captureException`). | ||
| if (defaultShouldHandleError(error)) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
We'll only set the span as "errored" when our default conditions apply.
Users could potentially want to report 200 as error and it would be wrong to set the span as "errored".
| */ | ||
| export function responseHandler(context: Context): void { | ||
| if (context.error && !isExpectedError(context.error)) { | ||
| export function responseHandler(context: Context, shouldHandleError?: (error: unknown) => boolean): void { |
There was a problem hiding this comment.
l: should this maybe take the type directly from what we set in SentryHonoMiddlewareOptions?
| * Determines whether an error should be captured and sent to Sentry. | ||
| * Uses `shouldHandleError` when provided, otherwise falls back to `defaultShouldHandleError`. | ||
| */ | ||
| export function shouldCaptureError(error: unknown, shouldHandleError?: (error: unknown) => boolean): boolean { |
There was a problem hiding this comment.
l: maybe that's just me but would putting that in a variable not be sufficient here instead of delegating that to a separate method? 😅 I also don't see this being reused anywhere
| if (!isExpectedError(error)) { | ||
| // Error capture is handled by `responseHandler` via `context.error`, so this | ||
| // wrapper only sets span status and rethrows (no `captureException`). | ||
| if (defaultShouldHandleError(error)) { |
There was a problem hiding this comment.
m: is this intentionally defaultShouldHandleError and not shouldCaptureError?
| This release promotes the `@sentry/tanstackstart-react` SDK to beta. For details on how to use it, check out the | ||
| [Sentry TanStack Start SDK docs](https://docs.sentry.io/platforms/javascript/guides/tanstackstart-react/). Please reach out on | ||
| [GitHub](https://github.com/getsentry/sentry-javascript/issues/new/choose) if you have any feedback or concerns. | ||
| The `sentry()` middleware now accepts a `shouldHandleError` callback to control which errors are captured and sent to Sentry. By default, 3xx/4xx HTTP errors are ignored and 5xx errors and plain `Error` objects are captured. Return `true` from the callback to capture an error, `false` to suppress it. |
There was a problem hiding this comment.
l: maybe an example would help to understand how this is to be used?
| * **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 => { |
There was a problem hiding this comment.
m: should this take HonoNodeOptions (which then I think would need to extend SentryHonoMiddlewareOptions) like in the other middlewares?
There was a problem hiding this comment.
No, because in the Node runtime the options are passed to the init in the external instrumentation file.
# Conflicts: # CHANGELOG.md
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
3 participants
Adds the error filtering logic to the Hono
sentry()middleware. It already exists in the to-be deprecatedhonoIntegration: #17743isExpectedErrorwas renamed todefaultShouldHandleErrorso the name is matching with the user-provided parameter. I also removed the special catching within the middleware, as it's just "duplicated code". The error bubbles through the Hono context anyway - it's a bit simpler now.Closes #21204