Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Hapi errors are reported before onPreResponse #12702

Closed
3 tasks done
camsteffen opened this issue Jun 28, 2024 · 5 comments · Fixed by #12725
Closed
3 tasks done

Hapi errors are reported before onPreResponse #12702

camsteffen opened this issue Jun 28, 2024 · 5 comments · Fixed by #12725
Assignees
Labels
Package: node Issues related to the Sentry Node SDK Type: Bug

Comments

@camsteffen
Copy link

Is there an existing issue for this?

How do you use Sentry?

Sentry Saas (sentry.io)

Which SDK are you using?

@sentry/node

SDK Version

8.9.2

Framework Version

Hapi 21.3.10

Link to Sentry event

No response

SDK Setup/Reproduction Example

I did some debugging and found that the issue can be fixed by specifying the 'error' channel here, like this:

server.events.on({name:'request', channels: ['error']}, (request, event) => {

Otherwise, an internal channel event occurs and that event is apparently too soon in the request lifecycle. I figured this out by comparing with hapi-sentry (code).

I believe this is enough for a minimal reproducible example:

Sentry.init();
Sentry.setupHapiErrorHandler(server);
server.ext('onPreResponse', (response, h) => {
  throw Boom.notFound();
});

Steps to Reproduce

  1. Send a request where the route handler throws an Error
  2. The onPreResponse hook replaces the error with a Boom error

Expected Result

No error is reported to Sentry since Boom error responses do not generate Hapi response error events. And the error thrown from the handler should not be considered uncaught.

Actual Result

The Error thrown from the handler is reported to Sentry.

@github-actions github-actions bot added the Package: node Issues related to the Sentry Node SDK label Jun 28, 2024
@s1gr1d s1gr1d self-assigned this Jul 1, 2024
@s1gr1d
Copy link
Member

s1gr1d commented Jul 1, 2024

Thanks for reporting this! I will look into this

@s1gr1d
Copy link
Member

s1gr1d commented Jul 2, 2024

I tested this and could not reproduce it. The boom errors do not land in Sentry. In which environment did you test your application? If it was in browser, could you try again with the following snippet:

  server.route({
    method: 'GET',
    path: '/favicon.ico',
    handler: (request, h) => {
      return h.response('MOCK FAVICON').type('image/x-icon').code(200);
    }
  });

It could be that the "Not Found" errors come from the browser trying to request the favicon.

However, boom errors are always reported if they happen in a route. What would be the desired outcome? I would propose having the same behavior for error reporting regardless of the error thrown inside onPreRequest or in a route. Boom 4xx should not be reported but Boom 5xx should. What do you think of this?

@camsteffen
Copy link
Author

Sorry, I wasn't very clear. The bug is that errors thrown from the handler are reported to Sentry, even if the onPreResponse hook handles the error. For example, the handler could throw a custom DbEntityNotFoundError, the hook can convert that to a Boom 404 response, and nothing should be reported.

@s1gr1d
Copy link
Member

s1gr1d commented Jul 2, 2024

Ah, I understand this makes sense! I created a PR to fix this.

@camsteffen
Copy link
Author

Thanks @s1gr1d!

s1gr1d added a commit that referenced this issue Jul 4, 2024
If errors are handled with Boom inside `onPreResponse`, the error should
not be reported to Sentry.

fixes #12702
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Package: node Issues related to the Sentry Node SDK Type: Bug
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants