Patch ActionController::Base and API instead of Metal

[moberegger]

What does this PR do?

Makes a small adjustment to where the patch for rails.action_controller is applied. Originally it prepended ActionController::Metal, but this PR proposes to move it earlier to ActionController::Base and ActionController::API.

Motivation:

In Rails, you will commonly see something like

class MyController < ApplicationController
  # Rescue and gracefully handle error response
  rescue_from MyError with: :render_error_response

  def index
    # ...
    fail MyError if no_good # On some condition, raise an error
  end
end

These are exceptions that are being handled gracefully, but Datadog will tag the rails.action_controller with errors. This causes APM to report errors on these controller actions, which can also cause alerts via the error tracking feature. The problem here is that this is reporting on something that is in fact being handled correctly by the code, but the tracer simply isn't aware of it.

This is happening because datadog is patching ActionController::Metal, which adds the exception to the payload before the ActionController has a chance to run the rescue handler.

There is a workaround, but it requires you to configure action_dispatch.rescue_responses ; datadog will check this to determine whether or not this is an expected exception. But this isn't really tenable; that configuration is largely intended for Rails' ShowExceptions to handled raised exceptions, so you wouldn't typically configure these with exceptions that you are already handling in the controller.

By simply moving the patch, the entirety of process_action is traced, which gives Rails a chance to run the rescue handlers.

Change log entry

Additional Notes:

This may address a few existing issues:

• rack.request span that are stopped in middleware or before action doesn't have resource set #3482: This one seems to be reporting that the rack.request resource is not correctly set when a before_action raises an exception. This is because with the original patch, the action callbacks ran before the trace; now they will run under the trace. This will allow the patch to now set the rack.request resource before any before_actions run.
• Before/after actions not included in the action_pack span #2100: Reports that the action callbacks are not included in the trace (ie. they happen before the rails.action_controller span). This will no longer be the case. This is worth mentioning because there appears to be some differing opinions around whether or not this should be included as part of the rails.action_controller span. I personally think it should, because it is part of the process_action call.

With the proposed change, it means more work is being traced. Given that the rails.action_controller span is intended to measure the controller action, I do think that this is more representative of what is happening in the controller.

I have chosen to also patch ActionController::API to support Rails' api_mode. Unsure if this needs to be smarter by first checking if the module exists before attempting to apply the patch. I can make that change if need be.

How to test the change?

I did follow the guide in https://github.com/DataDog/dd-trace-rb/blob/master/docs/DevelopmentGuide.md, and it does appear that the existing suite is still green. I may not have done it right, though. The change seems pretty superficial to me, but if there are any tests that I should add or modify, please let me know. I'm really not sure what to do here, my apologies.

I have applied this patch manually in our own API application, and everything is working as expected as far as I can tell. I don't expect "just trust me!" to be super convincing, though 😆.