Error handling within Hapi Sentry integration #11069
Comments
joshkel
changed the title
|
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? |
|
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 |
|
@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.) |
Resolves: #11069 After checking the behaviour, it seems to me that we don't need to capture any Boom responses, as the errors that may cause a `5xx` response are already captured by the core logic. To add an option to control this behaviour, we need to update the usage of `hapiErrorPlugin`, converting it to a function signature, which IMO may not worth doing, as I'm not sure if users in general would need to use it. This also adds `expectError()` to the `node-integration-tests` runner to allow `5xx` responses to be tested.
Resolves: #11069 After checking the behaviour, it seems to me that we don't need to capture any Boom responses, as the errors that may cause a `5xx` response are already captured by the core logic. To add an option to control this behaviour, we need to update the usage of `hapiErrorPlugin`, converting it to a function signature, which IMO may not worth doing, as I'm not sure if users in general would need to use it. This also adds `expectError()` to the `node-integration-tests` runner to allow `5xx` responses to be tested.
Resolves: #11069 After checking the behaviour, it seems to me that we don't need to capture any Boom responses, as the errors that may cause a `5xx` response are already captured by the core logic. To add an option to control this behaviour, we need to update the usage of `hapiErrorPlugin`, converting it to a function signature, which IMO may not worth doing, as I'm not sure if users in general would need to use it. This also adds `expectError()` to the `node-integration-tests` runner to allow `5xx` responses to be tested.
Resolves: getsentry#11069 After checking the behaviour, it seems to me that we don't need to capture any Boom responses, as the errors that may cause a `5xx` response are already captured by the core logic. To add an option to control this behaviour, we need to update the usage of `hapiErrorPlugin`, converting it to a function signature, which IMO may not worth doing, as I'm not sure if users in general would need to use it. This also adds `expectError()` to the `node-integration-tests` runner to allow `5xx` responses to be tested.
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
Integrations.Hapito configure the error plugin and tracing plugin separately (so we could continue to use the third-party hapi-sentry).hapiErrorPluginandhapiTracingPluginso that I can add one or the other or both to my Hapi server myself (and can then pick hapi-sentry overhapiErrorPlugin).Integrations.Hapito control whether the error plugin captures Boom replies as exceptions.The text was updated successfully, but these errors were encountered: