Capture Rails exception before default error page is rendered #1684
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1684 +/- ##
==========================================
- Coverage 98.31% 98.29% -0.02%
==========================================
Files 925 925
Lines 44564 44566 +2
==========================================
- Hits 43812 43808 -4
- Misses 752 758 +6
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
So is this what was causing error: true? Only if the "show exceptions page" feature was enabled? Although I'm not surprised that this was indeed one such case, I still wonder if this is the only such case, or whether there was something else. I didn't think this feature was widely used on production applications where error: true has been reported before.
Do we still allow the user app to consume the error with this change? Or does it change the behavior of middleware downstream from ours?
Other than that, one minor comment that probably doesn't have any impact.
| # Rails >= 3.2 | ||
| app.middleware.insert_after(::ActionDispatch::DebugExceptions, Datadog::Contrib::Rails::ExceptionMiddleware) | ||
| else | ||
| # Rails < 3.2 |
There was a problem hiding this comment.
Do we still support Rails < 3.2? Thought we dropped support for this.
There was a problem hiding this comment.
We still support it for Ruby 2.1:
Line 279 in a3236dd
But I can see it as a good candidate for deprecation. It would simplify our code base for sure.
There was a problem hiding this comment.
👍 I think dropping support for Rails 3.1 even before we drop support for Ruby 2.1 makes sense, given that the 3.2 series still works for 2.1 thus I suspect the intersection of the following groups:
- customers still on 2.1
- customers still on Rails 3.1
- customers that upgrade dd-trace-rb regularly
is probably nil or very close to it.
This is a common scenario for clients during onboarding, for staging environments, and clients that don't ever care about their error page when their Rails app is behind a proxy (thus don't ever check if their error pages are fancily rendered or not). I agree that this does not affect most well-behaved production Rails environments, and that makes sense, given this issue is only sporadically reported to us. I don't have complete confidence that this solves every single
Yes, no behavior change takes place in our middleware. |
Follow up to https://github.com/DataDog/dd-trace-rb/pull/552/files#diff-22e85fccca5492ebff5fee5e488aa5b4b2e4a62862dfc12af0ba990e0076b2c2R45-R52 and #1124.
This PR ensures we capture a Rails before some of the Rails' middlewares are able to swallow it.
In https://github.com/DataDog/dd-trace-rb/pull/552/files#diff-22e85fccca5492ebff5fee5e488aa5b4b2e4a62862dfc12af0ba990e0076b2c2R45-R52, we added a
ddtracemiddleware to catch such errors, but retrieving the error before theActionDispatch::ShowExceptionsRails middleware runs. This works the majority of cases.But if the user has action_dispatch.show_exceptions enabled, which is
trueby default, the Rails error page is rendered, like this one:This page is rendered by the
ActionDispatch::DebugExceptionsmiddleware, which consumes the error from the stack. This middleware swallows the error before it gets a chance to hitActionDispatch::ShowExceptions, and along with it our own middleware.This PR inserts our error handling middleware before
ActionDispatch::DebugExceptions, instead ofActionDispatch::ShowExceptions. This will address all Rails error page rendering cases today.One caveat for Rails 3.0 only:
ActionDispatch::DebugExceptionsdidn't exist at the time, soActionDispatch::ShowExceptionsstays as our hook point for Rails 3.0.