listepo / sails-hook-sentry

Sails hook for Sentry(raven-node)
10 stars 9 forks source link

looks like event router:request:500 doesn't work #2

Open gvlekke opened 7 years ago

gvlekke commented 7 years ago

I added sails-hook-sentry to my project and after that it works fine with the captureException method.

in the script https://github.com/listepo/sails-hook-sentry/blob/master/index.js#L40 there is a event is called when you see a 500 page.

In my project it isn't called...

my questions:

listepo commented 7 years ago

@gvlekke PR welcome, I no longer use sails.js and no time to support this hook

Freundschaft commented 7 years ago

the reason is simple, https://github.com/balderdashy/sails/blob/master/lib/EVENTS.md#routerrequest500 states that:

Absolute last-resort handler for server errors. Called when a request encounters an error and isn't handled by other means. Should receive three arguments, err, req, and res.

Because sails doesnt use error handling middleware in the same way that express works, instead using custom error response commands like res.badRequest, the event is only triggered in a case where the normal sails error responses dont trigger.

I'm not a big fan of how sails.js handles this, but I think instead of hooking into the event listener, you should add the sentry hook in the response handlers

aliosv commented 6 years ago

There three types of errors we need to log with sentry: 1) unhandled errors(Raven do it by default) 2) server errors(i.e. 500 -> log from responses/serverError.js, instead of this https://github.com/listepo/sails-hook-sentry/blob/master/index.js#L40) 3) unhandled bluebird promise rejections(http://bluebirdjs.com/docs/api/error-management-configuration)

process.on('unhandledRejection', function(reason) {
    console.error('Unhandled rejection:', reason);
    Raven.captureException(reason);
});

pr #3

listepo commented 6 years ago

@Freundschaft @gvlekke please test the latest beta version