Do not report rendered errors except 500

[nvasilevski]

TLDR Context

#51050 change results in error tracking service being polluted with ActionController::RoutingError which are not actionable and we don't want them there

Context

In 4067c95 ActionDispatch::Executor started to report all errors, even the ones that were "handled" by the application. This leads to errors like ActionController::RoutingError polluting error trackers while not being actionable since they do not represent an exceptional situation.

This commit changes the behavior to only report errors that are not considered "handled" based on the ActionDispatch::ExceptionWrapper.rescue_responses list.

Alternative solution

What I have considered and what might actually be a better solution for the long-term is to report "handled" exceptions as handled: true. This way subscribers can decide how they want to proceed reporting something that has already been handled. Example: While I strongly believe that ActionController::RoutingError should not be presented on the error tracker in the same way as something like raise "boom", I can see that some applications may want to soft-report them in a way that doesn't trigger any alerts and doesn't require any attention. For example It's totally expected to have ActionController::RoutingError exceptions but if the number of ActionController::RoutingError spikes it may indicate that either some link that used to be accessible became unaccessible or a wrong link was posted/rendered somewhere which makes such event actionable and worth reporting.

But regardless, ActionController::RoutingError (and similar) exceptions should not suddenly be reported as part of a patch version upgrade. It should be a major change with perhaps a configuration option to allow applications to adapt and make a decision on how to proceed.

I'm open for the discussion and eager to learn what are yours expectations on this behavior!

[rafaelfranca]

Instead of doing this here, why we just don't add it to env["action_dispatch.exception"] if it isn't rescuable?

[rafaelfranca]

We can't do that. But we can add the exception to a different key in the env only if we want it to be reported.

[nvasilevski]

I agree, my biggest concern was the use of ExceptionWrapper.rescue_response? in the executor. Feels like executor shouldn't ideally know about the wrapper.

We can't do that

Just to make sure we have the same understanding. Is that because exceptions_app (ActionDispatch::PublicExceptions by default) expects action_dispatch.exception header to be present?
What if we still pass the header to exceptions_app but drop it afterwards?

I guess using a different key makes more sense, I just wasn't sure if it's okay to add more headers. Let me try that!

[nvasilevski]

So I added a "action_dispatch.report_exception" boolean flag to indicate whether the exception should be reported or not. I felt like it's excessive to have two headers pointing to the same exception in case if it needs to be reported.

[nvasilevski]

cc @fractaledmind as you initially reported the issue and I was wondering if this tweak still aligns with your vision of the problem

[fractaledmind]

I believe this does still solve the original problem. I like your thought for a future next step to set handled to true or false depending and then letting the subscribers decide what they want to do.

I will read the details of the rescuable check to ensure that it isn't too strict, but I expect it isn't and all important errors would be reported with this change.

[byroot]

What I have considered and what might actually be a better solution for the long-term is to report "handled" exceptions as handled: true. This way subscribers can decide how they want to proceed reporting something that has already been handled.

handled: true semantic, is to say that it didn't translate to a user visible error (e.g. connection error to the cache). Here it's different, it's more to mean it didn't end up as a server error.

So I think the current patch is fine, no need for a followup.