getsentry / sentry-javascript

Official Sentry SDKs for JavaScript
https://sentry.io
MIT License
7.88k stars 1.55k forks source link

Error handling within Hapi Sentry integration #11069

Closed joshkel closed 6 months ago

joshkel commented 6 months ago

Problem Statement

We're looking into switching from the third-party hapi-sentry package to Sentry's built-in Hapi support, mainly to benefit from the added tracing support. However, the built-in Hapi support has what seems to be a potentially unwanted feature: every Hapi Boom response (i.e., every HTTP 4xx or 5xx response) is captured as a Sentry exception.

Is this intended? I would expect that Sentry exceptions are for things that are very likely programming errors, not conditions like bad or unauthorized requests (HTTP 4xx) that are often outside of the application's control.

Solution Brainstorm

  1. Add an option to Integrations.Hapi to configure the error plugin and tracing plugin separately (so we could continue to use the third-party hapi-sentry).
  2. More directly expose / export hapiErrorPlugin and hapiTracingPlugin so that I can add one or the other or both to my Hapi server myself (and can then pick hapi-sentry over hapiErrorPlugin).
  3. Add an option to Integrations.Hapi to control whether the error plugin captures Boom replies as exceptions.
  4. Don't capture Boom replies by default.
  5. Some combination of the above.
Lms24 commented 6 months ago

Hey @joshkel thanks for writing in! full disclosure, I don't have much context around Hapi and Boom but generally I agree, our integration probably shouldn't capture 400/500 responses by default.

@onurtemizkan would you mind taking a look at this? Maybe we can go with only optionally recording errors with a 4xx/5xx Http response?

mydea commented 6 months ago

Sounds reasonable to me as well.

I wonder, should we ever send on boom responses, then? Maybe the way to go is to have a config of status codes that you want to send for, and we default it to []?

joshkel commented 6 months ago

@mydea I'm not sure about the best configuration for Sentry users as a whole. For me personally, I'm unlikely to want any Boom responses sent. If I did want to send on Boom responses, I could see wanting to choose between 4xx and 5xx, without having to enumerate every 5xx status code. That could mean a few options:

interface HapiSentryOptions {
  captureBoom4xx?: boolean;
  captureBoom5xx?: boolean;
  captureBoomStatusCodes?: number[];
}

Although I'm not sure there's enough user interest to justify the complexity.

Maybe the provided integration could stop sending on Boom and could add an example in the docs on how to implement your own capturing of Boom responses from a Hapi event handler, until there's more concrete user interest?

(Just brainstorming - you all know much better what kinds of APIs are suitable for Sentry SDKs than I do.)