Merge pull request #1489 from DataDog/fix/rails_controller_error_dete…

…ction

Fix ActionPack instrumentation #starts_with? error

.rubocop.yml

@@ -239,6 +239,10 @@ RSpec/LeadingSubject:
 RSpec/ImplicitSubject:
   Enabled: false
 
+# Enforces empty line after hook declaration.
+RSpec/EmptyLineAfterHook:
+  Enabled: false
+
 # Enforces empty line after subject declaration.
 RSpec/EmptyLineAfterSubject:
   Enabled: false

lib/ddtrace/contrib/action_pack/action_controller/instrumentation.rb

@@ -61,7 +61,7 @@ def finish_processing(payload)
                 # [christian] in some cases :status is not defined,
                 # rather than firing an error, simply acknowledge we don't know it.
                 status = payload.fetch(:status, '?').to_s
-                span.status = 1 if status.starts_with?('5')
+                span.status = 1 if status.start_with?('5')
               elsif Utils.exception_is_error?(exception)
                 span.set_error(exception)
               end

lib/ddtrace/contrib/action_pack/utils.rb

@@ -11,7 +11,7 @@ def self.exception_is_error?(exception)
             # You can add custom errors via `config.action_dispatch.rescue_responses`
             status = ::ActionDispatch::ExceptionWrapper.status_code_for_exception(exception.class.name)
             # Only 5XX exceptions are actually errors (e.g. don't flag 404s)
-            status.to_s.starts_with?('5')
+            status.to_s.start_with?('5')
           else
             true
           end

spec/ddtrace/contrib/action_pack/action_controller/instrumentation_spec.rb

@@ -0,0 +1,78 @@
+require 'ddtrace/contrib/support/spec_helper'
+
+require 'action_controller'
+require 'ddtrace'
+
+# TODO: We plan on rewriting much of this instrumentation to bring it up to
+#       present day patterns/conventions. For now, just test a few known cases.
+RSpec.describe Datadog::Contrib::ActionPack::ActionController::Instrumentation do
+  describe '::finish_processing' do
+    subject(:finish_processing) { described_class.finish_processing(payload) }
+
+    context 'given a payload that been started' do
+      before { described_class.start_processing(payload) }
+      after { span.finish }
+
+      let(:action_dispatch_exception) { nil }
+      let(:action_name) { 'index' }
+      let(:controller_class) { stub_const('TestController', Class.new(ActionController::Base)) }
+      let(:env) { { 'rack.url_scheme' => 'http' } }
+      let(:payload) do
+        {
+          controller: controller_class,
+          action: action_name,
+          env: env,
+          headers: {
+            # The exception this controller was given in the request,
+            # which is typical if the controller is configured to handle exceptions.
+            request_exception: action_dispatch_exception
+          },
+          tracing_context: {}
+        }
+      end
+
+      let(:span) { payload[:tracing_context][:dd_request_span] }
+
+      context 'with a 200 OK response' do
+        before do
+          expect(Datadog.logger).to_not receive(:error)
+          finish_processing
+        end
+
+        describe 'the Datadog span' do
+          it do
+            expect(span).to_not have_error
+          end
+        end
+      end
+
+      context 'with a 500 Server Error response' do
+        let(:error) do
+          begin
+            raise 'Test error'
+          rescue StandardError => e
+            e
+          end
+        end
+
+        let(:payload) do
+          super().merge(
+            exception: [error.class.name, error.message],
+            exception_object: error
+          )
+        end
+
+        before do
+          expect(Datadog.logger).to_not receive(:error)
+          finish_processing
+        end
+
+        describe 'the Datadog span' do
+          it do
+            expect(span).to have_error
+          end
+        end
+      end
+    end
+  end
+end