payloadcms / payload

Payload is the open-source, fullstack Next.js framework, giving you instant backend superpowers. Get a full TypeScript backend and admin panel instantly. Use Payload as a headless CMS or for building powerful applications.
https://payloadcms.com
MIT License
25.67k stars 1.63k forks source link

plugin-sentry: inconsistencies on how payload capturing an error #5247

Open lukekluky opened 8 months ago

lukekluky commented 8 months ago

Link to reproduction

No response

Describe the Bug

We set a list of capture errors and pass it to the plugin, But it does not work as expected. Some error still sent to the Sentry although the error status is not in the list of captureErrors. After some investigation, we figured out that there's an incorrect order between payload and sentry on the error handling middleware. Sentry error middleware comes after payload error handling. This happening because plugin initiate after the payload. We manage to makes a temporary fix with recreate the plugin and add check on the hook.

// eslint-disable-next-line @typescript-eslint/no-explicit-any
const shouldCaptureError = (error: any, captureErrors?: number[]): boolean => {
  if (error.status === 500) {
    return true;
  }
  if (captureErrors && typeof error.status === 'number' && captureErrors.includes(error.status)) {
    return true;
  }
  return false;
};
  const captureErrors = pluginOptions.options?.captureErrors;
  config.hooks = {
    ...(incomingConfig.hooks || {}),
    // eslint-disable-next-line @typescript-eslint/no-explicit-any
    afterError: (err: any) => {
      if (!shouldCaptureError(err, captureErrors)) {
        return;
      }
      captureException(err);
    },
  };

To Reproduce

  1. Set a list of capture errors and pass it to the plugin
  2. Try to raise an error on the collection endpoint, make sure error status is not in the list of captureErrors
  3. Check the sentry issues

Payload Version

2.11.2

Adapters and Plugins

plugin-sentry

rchl commented 2 weeks ago

I've came to the same conclusion after wondering why Sentry errors don't include any request information (invoked URL, headers, etc.).

Sentry plugin must register the Sentry.Handlers.requestHandler and Sentry.Handlers.tracingHandler() middlewares before Payload registers its own handlers for /api route etc (potentially after /static route).

As for error middlewares added by the plugin, that might need more consideration since I don't know for sure how Payload processes erros but Sentry error middleware should not send its own response but let Payload respond with proper error to the client.

rchl commented 2 weeks ago

Another problem with that is that errors like incorrect password or auto-logout due to timeout are reported to sentry, even though they are not supposed to (because those are not >=500 errors).