bugfix: expected HTTP codes don't impact Apdex

If an error is either expected (which still creates an entry in the
Errors Inbox) or ignored (which does not), the transaction's Apdex
should not factor in the error.

A bug was discovered involving errors that were expected by way of
their HTTP response codes that would cause those errors to impact Apdex.
Errors that were expected via the agent's Noticed Errors API would
correctly not impact Apdex, but HTTP response code based expected errors
would incorrectly do so.

Now ignored errors and both kinds of expected errors will correctly not
impact Apdex when observed during a transaction.

resolves #2594

lib/new_relic/agent/error_collector.rb

@@ -110,6 +110,20 @@ def error_is_ignored?(error, status_code = nil)
         false
       end
 
+      # Neither ignored nor expected errors impact apdex.
+      # Errors can be expected either at notice time or via
+      # :'error_collector.exepcted_status_codes'.
+      def error_affects_apdex?(error, options)
+        return false if error_is_ignored?(error)
+        return false if options[:expected]
+
+        !expected?(error, ::NewRelic::Agent::Tracer.state.current_transaction&.http_response_code)
+      rescue => e
+        NewRelic::Agent.logger.error("Could not determine if error '#{error}' should impact Apdex - " \
+                                     "#{e.class}: #{e.message}. Defaulting to 'true' (it should impact Apdex).")
+        true
+      end
+
       # Calling instance_variable_set on a wrapped Java object in JRuby will
       # generate a warning unless that object's class has already been marked
       # as persistent, so we skip tagging of exception objects that are actually

lib/new_relic/agent/transaction.rb

@@ -816,13 +816,9 @@ def record_queue_time
       end
 
       def had_error_affecting_apdex?
-        @exceptions.each do |exception, options|
-          ignored = NewRelic::Agent.instance.error_collector.error_is_ignored?(exception)
-          expected = options[:expected]
-
-          return true unless ignored || expected
+        @exceptions.each.any? do |exception, options|
+          NewRelic::Agent.instance.error_collector.error_affects_apdex?(exception, options)
         end
-        false
       end
 
       def apdex_bucket(duration, current_apdex_t)

test/agent_helper.rb

@@ -228,7 +228,9 @@ def assert_metrics_recorded(expected)
   expected.each do |specish, expected_attrs|
     expected_spec = metric_spec_from_specish(specish)
     actual_stats = NewRelic::Agent.instance.stats_engine.to_h[expected_spec]
-    if !actual_stats
+    if actual_stats
+      assert(actual_stats)
+    else
       all_specs = NewRelic::Agent.instance.stats_engine.to_h.keys.sort
       matches = all_specs.select { |spec| spec.name == expected_spec.name }
       matches.map! { |m| "  #{m.inspect}" }

test/new_relic/agent/transaction_test.rb

@@ -324,10 +324,13 @@ def test_apdex_success_with_ignored_error
 
       with_ignore_error_filter(filter) do
         txn_name = 'Controller/whatever'
+
+        # this one doesn't impact Apdex
         in_web_transaction(txn_name) do
           Transaction.notice_error(SillyError.new)
         end
 
+        # this one does impact Apdex
         in_web_transaction(txn_name) do
           Transaction.notice_error(RuntimeError.new)
         end
@@ -339,6 +342,49 @@ def test_apdex_success_with_ignored_error
       )
     end
 
+    def test_noticed_errors_that_are_expected_do_not_impact_apdex
+      txn_name = 'Controller/BigDweebEnergy'
+      # this one doesn't impact Apdex
+      in_web_transaction(txn_name) do
+        Transaction.notice_error(RuntimeError.new, expected: true)
+      end
+
+      # this one does impact Apdex
+      in_web_transaction(txn_name) do
+        Transaction.notice_error(RuntimeError.new)
+      end
+
+      assert_metrics_recorded(
+        txn_name => {},
+        'Apdex' => {:apdex_s => 1, :apdex_t => 0, :apdex_f => 1},
+        'Apdex/BigDweebEnergy' => {:apdex_s => 1, :apdex_t => 0, :apdex_f => 1}
+      )
+    end
+
+    def test_expected_status_code_based_errors_do_not_impact_apdex
+      expected_status_code = 418
+      txn_name = 'Controller/BillAmend'
+
+      with_config(:'error_collector.expected_status_codes' => "11,38,#{expected_status_code}") do
+        # this one doesn't impact Apdex
+        in_web_transaction(txn_name) do |txn|
+          txn.http_response_code = expected_status_code
+          Transaction.notice_error(RuntimeError.new)
+        end
+
+        # this one does impact Apdex
+        in_web_transaction(txn_name) do
+          Transaction.notice_error(RuntimeError.new)
+        end
+      end
+
+      assert_metrics_recorded(
+        txn_name => {},
+        'Apdex' => {:apdex_s => 1, :apdex_t => 0, :apdex_f => 1},
+        'Apdex/BillAmend' => {:apdex_s => 1, :apdex_t => 0, :apdex_f => 1}
+      )
+    end
+
     def test_apdex_success_with_config_ignored_error
       txn_name = 'Controller/whatever'
       with_config(:'error_collector.ignore_classes' => [SillyError.name]) do