-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Conversation
size-limit report 📦
|
Yeah good point. I wouldn't mind changing the name to |
shouldHandleError option to fastifyIntegrationshouldHandleDiagnosticsChannelError option to fastifyIntegration
31d50b1 to
fc2159e
Compare
|
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. |
b9178db to
8054696
Compare
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.
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
sentry-javascript/dev-packages/e2e-tests/test-applications/node-fastify-3/tests/errors.test.ts
Lines 36 to 39 in 8054696
| 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
sentry-javascript/dev-packages/e2e-tests/test-applications/node-fastify-4/tests/errors.test.ts
Lines 58 to 61 in 8054696
| 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
sentry-javascript/dev-packages/e2e-tests/test-applications/node-fastify-5/tests/errors.test.ts
Lines 36 to 39 in 8054696
| errorEventPromise.then(() => { | |
| throw new Error('This error should not be captured'); | |
| }); |
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
Lines 3 to 4 in 8054696
| const config = getPlaywrightConfig({ | |
| startCommand: `pnpm start`, |
Was this report helpful? Give feedback by reacting with 👍 or 👎
shouldHandleDiagnosticsChannelError option to fastifyIntegrationshouldHandleError option to fastifyIntegration
|
@mydea, @AbhiPrasad - I updated the PR, it should now work for all Fastify versions. |
Lms24
left a comment
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.
Sorry for the delay! Let's get this merged in!
…16845) Resolves: #16819 This requires the Fastify integration to be manually added while initialising Sentry. ```typescript 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.
…7123) Backport of #16845 closes #17123 --------- Co-authored-by: Onur Temizkan <[email protected]>
Resolves: #16819
This requires the Fastify integration to be manually added while initialising Sentry.
If
shouldHandleErroris provided insetupFastifyErrorHandler, it overridesshouldHandleErrorprovided fromfastifyIntegration. So this should not break any existing (pre-DC) usage.In summary:
setupFastifyErrorHandleris not used,shouldHandleErroratSentry.initwill be used.setupFastifyErrorHandleris used but withoutshouldHandleError,shouldHandleErroratSentry.initwill be used.setupFastifyErrorHandlerandSentry.initare used withshouldHandleError, the one insetupFastifyErrorHandleris 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.