Merge pull request #320 from DataDog/delner/rails_error_redirection

Add exception_controller option to Rails

docs/GettingStarted.md

@@ -77,7 +77,8 @@ Where `options` is an optional `Hash` that accepts the following parameters:
 | ``service_name`` | Service name used when tracing application requests (on the `rack` level) | ``<app_name>`` (inferred from your Rails application namespace) |
 | ``controller_service`` | Service name used when tracing a Rails action controller | ``<app_name>-controller`` |
 | ``cache_service`` | Cache service name used when tracing cache activity | ``<app_name>-cache`` |
-| ``database_service`` | Database service name used when tracing database activity | ``<app_name>-<adapter_name>``. |
+| ``database_service`` | Database service name used when tracing database activity | ``<app_name>-<adapter_name>`` |
+| ``exception_controller`` | Class or Module which identifies a custom exception controller class. Tracer provides improved error behavior when it can identify custom exception controllers. By default, without this option, it 'guesses' what a custom exception controller looks like. Providing this option aids this identification. | ``nil`` |
 | ``distributed_tracing`` | Enables [distributed tracing](#Distributed_Tracing) so that this service trace is connected with a trace of another service if tracing headers are received | `false` |
 | ``template_base_path`` | Used when the template name is parsed. If you don't store your templates in the ``views/`` folder, you may need to change this value | ``views/`` |
 | ``tracer`` | A ``Datadog::Tracer`` instance used to instrument the application. Usually you don't need to set that. | ``Datadog.tracer`` |

lib/ddtrace/contrib/rails/action_controller.rb

@@ -37,8 +37,9 @@ def self.finish_processing(payload)
               span.resource = "#{payload.fetch(:controller)}##{payload.fetch(:action)}"
             end
 
-            # set the parent resource if it's a `rack.request` span
-            if !span.parent.nil? && span.parent.name == 'rack.request'
+            # Set the parent resource if it's a `rack.request` span,
+            # but not if its an exception contoller.
+            if !span.parent.nil? && span.parent.name == 'rack.request' && !exception_controller?(payload)
               span.parent.resource = span.resource
             end
 
@@ -66,6 +67,26 @@ def self.finish_processing(payload)
         rescue StandardError => e
           Datadog::Tracer.log.error(e.message)
         end
+
+        def self.exception_controller?(payload)
+          exception_controller_class = Datadog.configuration[:rails][:exception_controller]
+          controller = payload.fetch(:controller)
+          headers = payload.fetch(:headers)
+
+          # If no exception controller class has been set,
+          # guess whether this is an exception controller from the headers.
+          if exception_controller_class.nil?
+            !headers[:request_exception].nil?
+          # If an exception controller class has been specified,
+          # check if the controller is a kind of the exception controller class.
+          elsif exception_controller_class.is_a?(Class) || exception_controller_class.is_a?(Module)
+            controller <= exception_controller_class
+          # Otherwise if the exception controller class is some other value (like false)
+          # assume that this controller doesn't handle exceptions.
+          else
+            false
+          end
+        end
       end
     end
   end

lib/ddtrace/contrib/rails/core_extensions.rb

@@ -150,8 +150,13 @@ def process_action_with_datadog(*args)
           # signals; it propagates the request span so that it can be finished
           # no matter what
           payload = {
-            controller: self.class.name,
+            controller: self.class,
             action: action_name,
+            headers: {
+              # The exception this controller was given in the request,
+              # which is typical if the controller is configured to handle exceptions.
+              request_exception: request.headers['action_dispatch.exception']
+            },
             tracing_context: {}
           }
 

lib/ddtrace/contrib/rails/patcher.rb

@@ -12,6 +12,7 @@ module Patcher
         option :database_service
         option :distributed_tracing, default: false
         option :template_base_path, default: 'views/'
+        option :exception_controller, default: nil
         option :tracer, default: Datadog.tracer
 
         @patched = false

test/contrib/rails/apps/controllers.rb

@@ -96,6 +96,12 @@ def custom_resource
   end
 end
 
+class ErrorsController < ActionController::Base
+  def internal_server_error
+    head :internal_server_error
+  end
+end
+
 routes = {
   '/' => 'tracing#index',
   '/nested_partial' => 'tracing#nested_partial',
@@ -109,7 +115,8 @@ def custom_resource
   '/error_partial' => 'tracing#error_partial',
   '/missing_template' => 'tracing#missing_template',
   '/missing_partial' => 'tracing#missing_partial',
-  '/custom_resource' => 'tracing#custom_resource'
+  '/custom_resource' => 'tracing#custom_resource',
+  '/internal_server_error' => 'errors#internal_server_error'
 }
 
 if Rails.version >= '3.2.22.5'

test/contrib/rails/rack_middleware_test.rb

@@ -2,6 +2,7 @@
 
 require 'contrib/rails/test_helper'
 
+# rubocop:disable Metrics/ClassLength
 class FullStackTest < ActionDispatch::IntegrationTest
   setup do
     # store original tracers
@@ -161,6 +162,33 @@ class FullStackTest < ActionDispatch::IntegrationTest
     assert_match(/\n/, request_span.get_tag('error.stack'))
   end
 
+  test 'custom error controllers should not override trace resource names' do
+    # Simulate an error being passed to the exception controller
+    # (Syntax depends on Rails integration test version)
+    if Rails.version >= '5.0'
+      get '/internal_server_error', headers: { 'action_dispatch.exception' => ArgumentError.new }
+    else
+      get '/internal_server_error', {}, 'action_dispatch.exception' => ArgumentError.new
+    end
+
+    assert_response :error
+
+    # Check spans
+    spans = @tracer.writer.spans
+    assert_equal(2, spans.length)
+
+    rack_span = spans.first
+    controller_span = spans.last
+
+    # Rack span
+    assert_equal(rack_span.status, 1, 'span should be flagged as an error')
+    refute_equal(rack_span.resource, controller_span.resource) # We expect the resource hasn't been overriden
+
+    # Controller span
+    assert_equal(controller_span.status, 1, 'span should be flagged as an error')
+    assert_equal(controller_span.resource, 'ErrorsController#internal_server_error')
+  end
+
   test 'the status code is properly set if Rails controller is bypassed' do
     # make a request that doesn't have a route
     get '/not_existing'