Improve Rails middleware error handling

[delner]

There are currently two issues with Rails middleware errors:

1. Exceptions raised in Rails middleware which sits above Datadog::Contrib::Rails::ExceptionMiddleware but below ActionDispatch::ShowExceptions can result in the exception stack trace being excluded from the trace.

2. Exceptions raised in Rails middleware that are equivalent to 4XX errors (e.g. ActionController::RoutingError) can cause the trace to incorrectly be tagged as an error.

This pull request aims to address both of these, respectively by:

1. Placing Datadog::Contrib::Rails::ExceptionMiddleware directly below ActionDispatch::ShowExceptions middleware. Users should take care to avoid placing custom middleware between these two, otherwise they'll see this issue re-occur.
2. Changing the Datadog::Contrib::Rails::ExceptionMiddleware to only tag traces whose exceptions have known equivalent statuses to 5XX. Exceptions that are not whitelisted via config.action_dispatch.rescue_responses will be tagged as errors by default. This includes custom errors: if custom middleware raises custom exceptions that should not be tagged as an error, then they should be added to config.action_dispatch.rescue_responses in application.rb.

With the changes in this pull request, Rails applications produce the following behavior:

Trace for exception raised from custom Rails middleware (with custom ErrorsController):

class SampleMiddleware
  def initialize(app)
    @app = app
  end

  def call(env)
    @app.call(env)
    raise NotImplementedError.new
  end
end

module MyRailsApp
  class Application < Rails::Application
    # Datadog::Contrib::Rails::ExceptionMiddleware
    config.middleware.insert_after Datadog::Contrib::Rails::ExceptionMiddleware, SampleMiddleware
  end
end

Trace for a known 'not found' exception raised from custom Rails middleware (with custom ErrorsController):

class SampleMiddleware
  def initialize(app)
    @app = app
  end

  def call(env)
    @app.call(env)
    raise ActionController::RoutingError.new('/missing_route')
  end
end

module MyRailsApp
  class Application < Rails::Application
    # Datadog::Contrib::Rails::ExceptionMiddleware
    config.middleware.insert_after Datadog::Contrib::Rails::ExceptionMiddleware, SampleMiddleware
  end
end

Trace for custom exception equivalent to 'not found' raised from custom Rails middleware (with custom ErrorsController):

class CustomError < StandardError
end

class SampleMiddleware
  def initialize(app)
    @app = app
  end

  def call(env)
    @app.call(env)
    raise CustomError.new
  end
end

module DdRailsTest
  class Application < Rails::Application
    # Datadog::Contrib::Rails::ExceptionMiddleware
    config.middleware.insert_after Datadog::Contrib::Rails::ExceptionMiddleware, SampleMiddleware

    config.action_dispatch.rescue_responses.merge!(
      "CustomError" => :not_found
    )
  end
end

[delner]

@palazzem We were asserting that we should not put stack traces on the controller spans when it raises an error, only its parent rack.request span. Is this right? Seems like it isn't.

[palazzem]

I think this was done to avoid having the stacktrace in 2 spans (the rack and the controller)

[delner]

Working on adding some middleware tests, to guard against regression. They're a bit trickier to implement, but I think I'm close to having something working.

[dasch]

❤️

[delner]

Middleware tests are in this pull request: #347

[palazzem]

I think we should merge the other tests PR, be sure this big PR is properly rebased and move it in 0.12.x release branch.

Left one question just to be sure we're properly testing everything.

[palazzem]

I guess this compatibility has been tested in the other PR, right?

[delner]

Yes it has. And it's kind of a copy-paste from the Rails code we had before.