How to prevent handleFastifyError.handleFastifyError errors in Sentry?

[punkpeye]

Problem Statement

In my route handles, I throw Response objects like:

throw Response.json(
    {
      error: {
        code,
        details,
        id: sentryErrorForeignId,
        message,
      },
    } satisfies ApiErrorResponse,
    {
      headers,
      status,
    },
  );

It works as expected with Fastify, but the problem is that Sentry is catching these as if they are errors.

I get lots of these issues, like:

handleFastifyError.handleFastifyError
Level: Error
[object Response]
GET /api/gateway/v1/models

with this stack trace:

… es/.pnpm/@sentry+node@9.40.0/node_modules/@sentry/node/src/integrations/tracing/fastify/index.ts in handleFastifyError.handleFastifyError at line 87:5
… es/.pnpm/@sentry+node@9.40.0/node_modules/@sentry/node/src/integrations/tracing/fastify/index.ts in <anonymous> at line 123:24
node:diagnostics_channel in Channel.publish at line 150:9
/srv/node_modules/.pnpm/fastify@5.4.0/node_modules/fastify/lib/wrapThenable.js at line 55:22

node:internal/process/task_queues in process.processTicksAndRejections at line 105:5

https://glama.sentry.io/issues/6760827050/?project=4507975514849280&query=is%3Aunresolved&referrer=issue-stream

I am already returning false from shouldHandleError:

setupFastifyErrorHandler(app, {
    shouldHandleError: () => {
      // We are handling errors ourselves.
      // We are only using `setupFastifyErrorHandler` to setup request isolation context.
      return false;
    },
  });

  app.setErrorHandler((error, request, reply) => {
    return replyWithErrorPage({ error, origin: 'setErrorHandler', reply });
  });

Solution Brainstorm

Come to think about it, this is likely a bug.

Or maybe the ask is that at the moment setupFastifyErrorHandler is overloaded with error handling logic and also logic for starting a context for every request. It would be nice to separate out those two.

[linear]

JS-760 How to prevent handleFastifyError.handleFastifyError errors in Sentry?

[punkpeye]

Looking at the source code, this should never report. So this is a bug.

sentry-javascript/packages/node/src/integrations/tracing/fastify/index.ts

Lines 86 to 88 in 726c868

	if (shouldHandleError(error, request, reply)) {
	captureException(error);
	}

[punkpeye]

Reading the source code of @sentry/node/src/integrations/tracing/fastify/index.ts is making me realize that I don't need to add setupFastifyErrorHandler as it is automatically added by the instrumentation. However, the default instrumentation does not allow to configure shouldHandleError, so it seems I should remove the default integration and add it manually as needed.

[punkpeye]

Maybe log a warning if user uses setupFastifyErrorHandler if there is an existing instrumentation?

[Lms24]

Maybe log a warning if user uses setupFastifyErrorHandler if there is an existing instrumentation?

This should already be logged out if you have debug: true enabled:

sentry-javascript/packages/node/src/integrations/tracing/fastify/index.ts

Lines 124 to 127 in 549ca39

	debug.warn(
	'Fastify error handler was already registered via diagnostics channel.',
	'You can safely remove `setupFastifyErrorHandler` call and set `shouldHandleError` on the integration options.',
	);

However, the default instrumentation does not allow to configure shouldHandleError, so it seems I should remove the default integration and add it manually as needed.

This is still true for the moment but we're in the process of merging the PRs to expose shouldHandleError on the fastifyIntegration as an option. Expect this to be released by ~tomorrow.

Gonna try to reproduce the original issue, will report back

[kblcuk]

@Lms24 any update on this? At least release that happened 20 hours ago didn't seem to have anything related to this issue.