ref(node)!: Remove deprecated fastify exports, deprecate setupFastifyErrorHandler - #23460
ref(node)!: Remove deprecated fastify exports, deprecate setupFastifyErrorHandler#23460mydea wants to merge 10 commits into
setupFastifyErrorHandler#23460Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
size-limit report 📦
|
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit d9cdd33. Configure here.
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
3004b8c to
3a04de5
Compare
3a04de5 to
07fbd58
Compare
isaacs
left a comment
isaacs
left a comment
There was a problem hiding this comment.
Found a few nits and questions/cleanup opportunities, mostly fairly low severity.
This is a really nice cleanup, very deep cuts. Love to see it!
The only blocking issue imo is that Fastify will silently not track errors anymore unless tracing is enabled, and requiring someone to specifically opt into the integration isn't meaningfully easier than requiring them to attach a custom error handler. I'd recommend just having it turned on all the time.
Also, there's some orphaned e2e test files that I think can be deleted:
dev-packages/e2e-tests/test-applications/node-fastify-3/playwright.override.config.mjsdev-packages/e2e-tests/test-applications/node-fastify-4/playwright.override.config.mjsdev-packages/e2e-tests/test-applications/node-fastify-5/playwright.override.config.mjs
| * `captureException` deduplicates by object identity (`__sentry_captured__`), so | ||
| * only the first call sends an event. Errors that reach only one path (e.g. | ||
| * thrown in an `onRequest` hook, or on Fastify v3/v4 which has no channel) are | ||
| * captured once. |
There was a problem hiding this comment.
This is not exactly true. It'll dedupe based on the __sentry_captured__ flag, but if the object is frozen or a scalar value that won't matter.
If we're ok with duplicate events, then it's worth calling out in the docs, but I think it'd be cheap to just put a flag on the request (especially since we're already decorating it with kRequestSpan), so we could set request[kErrorCaptured] = true and use that as a guard here.
| } from '@sentry/server-utils/orchestrion'; | ||
| import { fastifyIntegration } from './fastify'; | ||
|
|
||
| export function getAutoPerformanceIntegrations(): Integration[] { |
There was a problem hiding this comment.
If fastifyIntegration() is only in the auto performance integrations, won't it mean that fastify error capture is now disabled if you turn off tracing? That seems surprising.
I'd suggest we have it on all the time (but not doing any tracing if tracing is disabled), rather than having error handling be disabled unless tracing is enabled and options.shouldHandleError is set.
| }; | ||
|
|
||
| function otelWireRoute(this: any, routeOptions: any): void { | ||
| function onRoute(this: any, routeOptions: any): void { |
| fastifyInstance?.register(fastifyTracingPlugin); | ||
| fastifyInstance?.register(fastifyErrorHandlerPlugin); |
There was a problem hiding this comment.
I thought from the commit messages that a goal of this is to consolidate these two things into a single plugin, no? Can they be combined into one?
| const routeName = getRequestRouteUrl(request); | ||
| const method = request.method || 'GET'; | ||
|
|
||
| getIsolationScope().setTransactionName(`${method} ${routeName}`); |
There was a problem hiding this comment.
The name fastifyErrorHandlerPlugin seems inconsistent, if it's also setting the transaction name and managing isolation scopes...?
| function getFastifyIntegration(): FastifyIntegration | undefined { | ||
| const client = getClient(); | ||
| return client?.getIntegrationByName(INTEGRATION_NAME); | ||
| return client?.getIntegrationByName(INTEGRATION_NAME) as FastifyIntegration | undefined; |
There was a problem hiding this comment.
I think this cast is only necessary because the satisfies was removed from packages/server-utils/src/integrations/fastify/index.ts line 57. It's probably better to keep the satisfies and make this client?.getIntegrationByName<FastifyIntegration>(INTEGRATION_NAME) so it's type checked.
| fastifyInstance?.register(fastifyErrorHandlerPlugin); | ||
| }); | ||
| }, | ||
| { id: 'Fastify.v5' }, |
There was a problem hiding this comment.
I think this is stale now that the same code path covers v3 and v4, right?
| done(); | ||
| }, | ||
| { | ||
| [Symbol.for('skip-override')]: true, |
There was a problem hiding this comment.
Should this also have a Symbol.for('plugin-meta') like the other one? (Recommend adding here, or deleting there, unless there's a reason for one to have it and the other not to.)
| * This is used on Fastify v5 where Sentry handles errors in the diagnostics channel. | ||
| * Fastify v3 and v4 use `setupFastifyErrorHandler` instead. |
There was a problem hiding this comment.
setupFastifyErrorHandler is deprecated as of this PR, so this option is simply The Way To Do It.
| return false; | ||
| } | ||
|
|
||
| // @ts-ignore // Fastify V5 is not typed correctly |
2 participants
This removes deprecated/unneeded fastify exports.
It also deprecates
setupFastifyErrorHandler- it is no longer needed. instead, you can fully configure the error handler for all versions of fastify now via the integration.I also removed the override tests from e2e tests, instead testing that you can still override the error handler in a node-integration-test.