Skip to content

feat(node): Add shouldHandleError option to fastifyIntegration #16845

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

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from

Conversation

onurtemizkan
Copy link
Collaborator

@onurtemizkan onurtemizkan commented Jul 8, 2025

Resolves: #16819

This requires the Fastify integration to be manually added while initialising Sentry.

Sentry.init({
  integrations: [
    Sentry.fastifyIntegration({
      shouldHandleError(_error, _request, reply) {
        return reply.statusCode >= 500;
      },
    });
  },
});

If shouldHandleError is provided in setupFastifyErrorHandler, it overrides shouldHandleError provided from fastifyIntegration. So this should not break any existing (pre-DC) usage.

In summary:

  • When setupFastifyErrorHandler is not used, shouldHandleError at Sentry.init will be used.
  • When setupFastifyErrorHandler is used but without shouldHandleError, shouldHandleError at Sentry.init will be used.
  • When in both setupFastifyErrorHandler and Sentry.init are used with shouldHandleError, the one in setupFastifyErrorHandler is used.

Works for all Fastify versions supported (v3, v4, v5)

Note: E2E tests for this now requires to test overrides, so they need to be run twice with both overriden and non-overriden applications. Not the prettiest solution, but I could not figure out an alternative.

@onurtemizkan onurtemizkan requested review from mydea and andreiborza July 8, 2025 15:32
cursor[bot]

This comment was marked as outdated.

Copy link
Contributor

github-actions bot commented Jul 8, 2025

size-limit report 📦

Path Size % Change Change
@sentry/browser 23.78 kB - -
@sentry/browser - with treeshaking flags 22.34 kB - -
@sentry/browser (incl. Tracing) 39.66 kB - -
@sentry/browser (incl. Tracing, Replay) 77.79 kB - -
@sentry/browser (incl. Tracing, Replay) - with treeshaking flags 67.59 kB - -
@sentry/browser (incl. Tracing, Replay with Canvas) 82.49 kB - -
@sentry/browser (incl. Tracing, Replay, Feedback) 94.59 kB - -
@sentry/browser (incl. Feedback) 40.47 kB - -
@sentry/browser (incl. sendFeedback) 28.46 kB - -
@sentry/browser (incl. FeedbackAsync) 33.36 kB - -
@sentry/react 25.54 kB - -
@sentry/react (incl. Tracing) 41.62 kB - -
@sentry/vue 28.23 kB - -
@sentry/vue (incl. Tracing) 41.45 kB - -
@sentry/svelte 23.81 kB - -
CDN Bundle 25.18 kB - -
CDN Bundle (incl. Tracing) 39.42 kB - -
CDN Bundle (incl. Tracing, Replay) 75.42 kB - -
CDN Bundle (incl. Tracing, Replay, Feedback) 80.89 kB - -
CDN Bundle - uncompressed 73.44 kB - -
CDN Bundle (incl. Tracing) - uncompressed 116.86 kB - -
CDN Bundle (incl. Tracing, Replay) - uncompressed 231 kB - -
CDN Bundle (incl. Tracing, Replay, Feedback) - uncompressed 243.81 kB - -
@sentry/nextjs (client) 43.69 kB - -
@sentry/sveltekit (client) 40.08 kB - -
@sentry/node 167.88 kB +0.05% +71 B 🔺
@sentry/node - without tracing 100.2 kB - -
@sentry/aws-serverless 128.36 kB +0.01% +7 B 🔺

View base workflow run

cursor[bot]

This comment was marked as outdated.

@AbhiPrasad
Copy link
Member

Though I think this may be a bit confusing for Fastify v4/v3 setupFastifyErrorHandler users, as this will be a no-op in these cases, maybe we can change the name to shouldHandleDiagnosticsChannelError or something like that?

Yeah good point. I wouldn't mind changing the name to shouldHandleDiagnosticsChannelError. And then adding a doc string that explicitly says this option only works in Fastify v5

@onurtemizkan onurtemizkan changed the title feat(node): Add shouldHandleError option to fastifyIntegration feat(node): Add shouldHandleDiagnosticsChannelError option to fastifyIntegration Jul 8, 2025
@onurtemizkan onurtemizkan force-pushed the onur/fastify-dc-shouldHandleError branch from 31d50b1 to fc2159e Compare July 8, 2025 20:44
@mydea
Copy link
Member

mydea commented Jul 10, 2025

Thinking about this some more, can we make this work in a way that also works in v3/v4? By storing this in some variable that the setupErrorHandler accesses...? IMHO this would be the cleanest solution, from an API perspective - why should I know or care, as a user, that this uses diagnostics channel under the hood 😅

I think the way to go is, to store & expose this method on the integration itself, making this the owner of the handler. Then it can be set in the integration, or in the handler (which updates it on the integration), and it's used both by the v5 as well as the v3/v4 approach.

cursor[bot]

This comment was marked as outdated.

@onurtemizkan onurtemizkan force-pushed the onur/fastify-dc-shouldHandleError branch from b9178db to 8054696 Compare July 16, 2025 08:39
Copy link

@cursor cursor bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: Unhandled Promise Rejection Masks Test Failures

The test creates an unhandled promise rejection. It attempts to verify an error is not captured by throwing an error in an unawaited .then() callback if the error is unexpectedly captured. This results in an unhandled promise rejection, obscuring the actual test failure.

dev-packages/e2e-tests/test-applications/node-fastify-3/tests/errors.test.ts#L36-L39

errorEventPromise.then(() => {
throw new Error('This error should not be captured');
});

dev-packages/e2e-tests/test-applications/node-fastify-4/tests/errors.test.ts#L58-L61

errorEventPromise.then(() => {
throw new Error('This error should not be captured');
});

dev-packages/e2e-tests/test-applications/node-fastify-5/tests/errors.test.ts#L36-L39

errorEventPromise.then(() => {
throw new Error('This error should not be captured');
});

Fix in CursorFix in Web


Bug: Playwright Config Uses Incorrect Start Script

The node-fastify-5 Playwright override config (playwright.override.config.mjs) incorrectly uses pnpm start instead of pnpm start:override. This is inconsistent with Fastify v3 and v4 configurations, causing override tests to run the default app.ts instead of src/app-handle-error-override.ts, which prevents proper testing of the shouldHandleError functionality.

dev-packages/e2e-tests/test-applications/node-fastify-5/playwright.override.config.mjs#L3-L4

const config = getPlaywrightConfig({
startCommand: `pnpm start`,

Fix in CursorFix in Web


Was this report helpful? Give feedback by reacting with 👍 or 👎

@onurtemizkan onurtemizkan changed the title feat(node): Add shouldHandleDiagnosticsChannelError option to fastifyIntegration feat(node): Add shouldHandleError option to fastifyIntegration Jul 16, 2025
@onurtemizkan
Copy link
Collaborator Author

@mydea, @AbhiPrasad - I updated the PR, it should now work for all Fastify versions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

diagnostics channel for fastify v5 introduced a breaking change by removing custom shouldHandleError support
3 participants