Only capture handled exceptions if they are 5xx and handled by default handler

[greysteil]

This is an attempt to make https://github.com/getsentry/raven-ruby/pull/343 less noisy.

The Rails ShowExceptions class wraps exceptions and calls a handler to decide what to do with them. The default hander is the PublicExceptions class. We only want to send exceptions to sentry by default when that default handler hasn't been overwritten (by setting an exceptions_app other than PublicExceptions).

[nateberkopec]

Is there are a reason to change this line?

[greysteil]

Yep - latest version of RSpec doesn't allow you to specify an error class when expecting a block to_not raise an error.

[nateberkopec]

The default hander is the PublicExceptions class

What happens if someone uses a different handler? For example, would this break with a gem like better_errors? Why shouldn't someone be able to use Raven to report errors if they're using a non-default handler?

[greysteil]

If someone's written their own exceptions app they probably know what they're doing and can explicitly send errors to Sentry if they want to.

For example, at GoCardless we provide a JSON API. If someone gives us bad JSON it will throw a params parser error when Rails tries to handle it. We catch that in our exceptions app and turn it into a 400 with details of where the bad JSON was. I don't want Sentry to tell me whenever that happens, and don't want to have to exclude dozens of errors. However, if our exception app doesn't know what to do with the error then we do want to send it to sentry, and we do that explicitly.

[greysteil]

@nateberkopec - updated.

[nateberkopec]

If someone's written their own exceptions app they probably know what they're doing and can explicitly send errors to Sentry if they want to.

I don't think this is a user experience I'm comfortable with. Changing your exception application should not break your exception reporting.

I don't want Sentry to tell me whenever that happens, and don't want to have to exclude dozens of errors.

We could make excluded_exceptions also exclude exceptions that have one of the excluded_exceptions as a class ancestor, e.g.:

if configuration[:excluded_exceptions].any? do |x|
  x.to_s!
  x == exc.class.name || exc.class.ancestors.any? { |a| x == a.class.name }
end
# obviously clean that up a bit, but you get the idea

That way you could add YourApp::JSONSyntax to the list and have all of your exceptions inherit from that base class (which you may already be doing anyway).

[greysteil]

Sounds like we're coming at this from very different angles. As I see it:

• An exceptions app is designed to rescue from and handle exceptions
• If an application were doing this at the controller level (e.g., with rescue_from, or even a begin \ rescue flow) we wouldn't want to send the handled exceptions to Sentry
• The situation here is no different, except when the exception app re-raises or wants to serve an explicit 500

In the re-raise case we'll already bubble-up to the Rack::Raven middleware, so there's no issue - it's just the explicit 500 case that needs thinking about. As an integrator writing my own exceptions app, I'd expect to write the explicit capture myself. As an integrator using the standard exceptions app (PublicExceptions) I'd expect this gem to have done the work for me. (I'd probably only want to hear about 5xxs, not 404s etc., but we can make a call on filtering by status later if anyone is affected - this is still a big improvement.)

Adding all non-5xx error classes from ExceptionWrapper to the default excluded_exceptions array is another option, and integrators could do this on a case-by-case basis, but it feels duplicative and not in keeping with the intention of Rails exception apps.

Finally, it's worth noting that BetterErrors just adds a new middleware, so none of this code (or the original change to stop Rails swallowing exceptions) will make a difference to it - the error won't bubble up as high as the ShowExceptions middleware. There's nothing we can do about integrators that add new exception-handling middleware like that.

[dcramer]

I agree to some extent with the feedback here, but I dont have any input around that really.

I think this should be possible:

begin
   blah blah blah
rescue
  Raven.capture_exception(...)
  raise
done

We have this issue on other platforms where you couldn't reliably bubble a 500 up to a user without the global error handler catching it, thus you'd leave the logging up to that point (where you have less control and information).

[greysteil]

@nateberkopec - what do you reckon? Happy to closed if you're not convinced! We're working around this now by deleting the action_dispatch.exception when we handle it in an exception handler, which doesn't feel quite right.

Up to you!

[nateberkopec]

@greysteil I think I still don't fully understand the problem the whole way.

If someone gives us bad JSON it will throw a params parser error when Rails tries to handle it. We catch that in our exceptions app and turn it into a 400 with details of where the bad JSON was.

Couldn't we fix this by changing the middleware order? If your exceptions app came after ("lower" in the stack) than the Raven Rack middleware, wouldn't your exception app rescue first? I think we might also have to delete action_dispatch.exception from this line: https://github.com/getsentry/raven-ruby/blob/master/lib/raven/integrations/rack.rb#L63

EDIT: This is the default middleware order. I think we're discussing an unintended side effect of #343, see below.

[nateberkopec]

The point of #343, which introduced this problem for you, was really, I think, to report exceptions in development by default. You're encountering your issue in production. I think there's a better solution here that accommodates both use cases.

[greysteil]

Agree that changing the middleware order isn't the solution.

Would it be worth tagging @robertclancy in to understand exactly what behaviour he was looking for in #343? There's quite a lot of literature about Rails exception apps, of which I think this is the best, and I'm pretty sure we don't want to report exceptions that have been explicitly dealt with in any environment.

[robertclancy]

The intention of https://github.com/getsentry/raven-ruby/pull/343 was to capture non-rails exceptions in production that had been handled by ShowExceptions. The unintended part of that was to capture exceptions such as ActionDispatch::ParamsParser::ParseError and ActionController::RoutingError that probably shouldn't be captured. I think this change makes sense.

Should ActionDispatch::ParamsParser::ParseError be added to the default ignored exceptions list? or maybe more generally the list ActionDispatch::ExceptionWrapper.rescue_responses.keys?

[greysteil]

OK, cool.

We don't want to add all of the keys in ActionDispatch::ExceptionWrapper.rescue_responses, because they include ActionController::NotImplemented which is a 501. Adding all the rest is certainly an option here, though. I don't love it because it feels a bit duplicative, but it's definitely a simpler solution than the above. Happy to update this if you're both in agreement that that's the way to go - what do you reckon?

[nateberkopec]

@greysteil I think you should disable our debug/show exceptions integration and then deal with whatever behavior you want to implement in your own exception application.

After I merge #422, the following will work for you:

Raven.configure do |config|
  config.catch_debugged_exceptions = false
end

Currently it doesn't, because we still manually inspect action_dispatch.exception. #422 will change this. After disabling catch_debugged_exceptions, you should be able to handle exceptions in your own exception app. Re-raised exceptions are caught as normal by Raven::Rack, and if you swallow them yourself nothing is reported.

Would that work for you?

[greysteil]

👍 works perfectly. Closing this in favour of that PR.