Capture Rails exception before default error page is rendered

lib/ddtrace/contrib/rails/patcher.rb

@@ -1,4 +1,4 @@
-# typed: false
+# typed: ignore
 require 'ddtrace/contrib/rails/utils'
 require 'ddtrace/contrib/rails/framework'
 require 'ddtrace/contrib/rails/middlewares'
@@ -45,17 +45,33 @@ def before_intialize(app)
         end
 
         def add_middleware(app)
-          # Add trace middleware
+          # Add trace middleware at the top of the middleware stack,
+          # to ensure we capture the complete execution time.
           app.middleware.insert_before(0, Datadog::Contrib::Rack::TraceMiddleware)
 
-          # Insert right after Rails exception handling middleware, because if it's before,
-          # it catches and swallows the error. If it's too far after, custom middleware can find itself
-          # between, and raise exceptions that don't end up getting tagged on the request properly.
-          # e.g lost stack trace.
-          app.middleware.insert_after(
-            ActionDispatch::ShowExceptions,
-            Datadog::Contrib::Rails::ExceptionMiddleware
-          )
+          # Some Rails middleware can swallow an application error, preventing
+          # the error propagation to the encompassing Rack span.
+          #
+          # We insert our own middleware right before these Rails middleware
+          # have a chance to swallow the error.
+          #
+          # Because there's more than one possible middleware that swallows errors,
+          # we insert our middleware before all the offending ones.
+          # At this point of the application lifecycle the complete middleware stack,
+          # is not yet defined, thus we can't decide which exact Rails middleware we want to
+          # insert ourselves before. We resort to inserting ourselves as many times as needed,
+          # once per offending middleware.
+          #
+          # Note: because the middleware stack is push/pop, "before" and "after" are reversed
+          # for our use case: we insert ourselves with "after" a middleware to ensure we are
+          # able to pop the request "before" it.
+          if defined?(::ActionDispatch::DebugExceptions)
+            # Rails >= 3.2
+            app.middleware.insert_after(::ActionDispatch::DebugExceptions, Datadog::Contrib::Rails::ExceptionMiddleware)
+          else
+            # Rails < 3.2
+            app.middleware.insert_after(::ActionDispatch::ShowExceptions, Datadog::Contrib::Rails::ExceptionMiddleware)
+          end
         end
 
         def add_logger(app)

spec/ddtrace/contrib/rails/rack_spec.rb

@@ -137,13 +137,18 @@ def internal_server_error
       # We remove these during testing, as we are more interested in asserting
       # controller and template spans.
       super().reject { |s| SYNTHETIC_3_2_SPANS.include?(s.resource) }
+    # elsif true # TODO change this
+    #   super().reject { |s| SYNTHETIC_6_SPANS.include?(s.resource) }
     else
       super()
     end
   end
 
   SYNTHETIC_3_2_SPANS = %w[_request_and_response.erb missing_template.erb].freeze
 
+  # Default error page rendering spans
+  SYNTHETIC_6_SPANS = %w[_request_and_response.html.erb template_error.html.erb _source.html.erb _trace.html.erb].freeze
+
   context 'with a full request' do
     subject(:response) { get '/full' }
 

spec/ddtrace/contrib/rails/support/base.rb

@@ -35,7 +35,7 @@
 
   let(:initialize_block) do
     middleware = rails_middleware
-    debug_mw = debug_middleware
+    # debug_mw = debug_middleware
     logger = self.logger
 
     proc do
@@ -72,7 +72,7 @@
         config.lograge.keep_original_rails_log = true
       end
 
-      config.middleware.insert_after ActionDispatch::ShowExceptions, debug_mw
+      # config.middleware.insert_after ActionDispatch::ShowExceptions, debug_mw
       middleware.each { |m| config.middleware.use m }
     end
   end
@@ -92,6 +92,26 @@
 
       # Force connection to initialize, and dump some spans
       application_record.connection
+
+      # Skip default Rails exception page rendering.
+      # This avoid polluting the trace under test
+      # with render and partial_render templates for the
+      # error page.
+      #
+      # We could completely disable the {DebugExceptions} middleware,
+      # but that affects Rails' internal error propagation logic.
+      # render_for_browser_request(request, wrapper)
+      if Rails.version >= '3.2'
+        allow_any_instance_of(::ActionDispatch::DebugExceptions).to receive(:render_exception) do |this, env, exception|
+          wrapper = ::ActionDispatch::ExceptionWrapper.new(env, exception)
+
+          if Rails.version < '4.0'
+            this.send(:render, wrapper.status_code, 'Test error response body')
+          else
+            this.send(:render, wrapper.status_code, 'Test error response body', 'text/plain')
+          end
+        end
+      end
     end
   end
 end

spec/ddtrace/contrib/rails/support/rails3.rb

@@ -43,7 +43,7 @@ def inherited(base)
       config.action_view.javascript_expansions = {}
       config.action_view.stylesheet_expansions = {}
 
-      config.middleware.delete ActionDispatch::DebugExceptions if Rails.version >= '3.2.22.5'
+      # config.middleware.delete ActionDispatch::DebugExceptions if Rails.version >= '3.2.22.5'
       instance_eval(&during_init)
     end
 

spec/ddtrace/contrib/rails/support/rails4.rb

@@ -38,7 +38,7 @@ def config.database_configuration
       config.consider_all_requests_local = true
       config.active_support.test_order = :random
 
-      config.middleware.delete ActionDispatch::DebugExceptions
+      # config.middleware.delete ActionDispatch::DebugExceptions
       instance_eval(&during_init)
 
       config.active_job.queue_adapter = :inline

spec/ddtrace/contrib/rails/support/rails5.rb

@@ -41,7 +41,7 @@ def config.database_configuration
       config.eager_load = false
       config.consider_all_requests_local = true
 
-      config.middleware.delete ActionDispatch::DebugExceptions
+      # config.middleware.delete ActionDispatch::DebugExceptions
       instance_eval(&during_init)
 
       config.active_job.queue_adapter = :inline

spec/ddtrace/contrib/rails/support/rails6.rb

@@ -45,7 +45,7 @@ def config.database_configuration
       config.hosts.clear # Allow requests for any hostname during tests
 
       # Avoid eager-loading Rails sub-component, ActionDispatch, before initialization
-      config.middleware.delete ActionDispatch::DebugExceptions if defined?(ActionDispatch::DebugExceptions)
+      # config.middleware.delete ActionDispatch::DebugExceptions if defined?(ActionDispatch::DebugExceptions)
 
       instance_eval(&during_init)
 

spec/support/configuration_helpers.rb

@@ -25,7 +25,7 @@ module ConfigurationHelpers
   end
 
   def decrement_gem_version(version)
-    segments = version.dup.segments
+    segments = version.segments.dup
     segments.reverse.each_with_index do |value, i|
       if value.to_i > 0
         segments[segments.length - 1 - i] -= 1