Merge pull request #2619 from newrelic/expectional

bugfix: expected HTTP codes / error classes / error messages shouldn't impact Apdex

CHANGELOG.md

@@ -1,5 +1,14 @@
 # New Relic Ruby Agent Release Notes
 
+## dev
+
+Version <dev> fixes a bug that would cause an expected error to negatively impact a transaction's Apdex.
+
+- **Bugfix: Expected errors related to HTTP status code, class, and message won't impact Apdex
+
+  The agent is supposed to prevent observed application errors from negatively impacting Apdex if the errors are either ignored or expected. There are two ways for the agent to expect an error: via the `notice_error` API receiving an `expected: true` argument or via matches made against user-configured lists for expected HTTP status codes (`:'error_collector.expected_status_codes'`), expected error classes (`:'error_collector.expected_classes'`), or expected error messages (`:'error_collector.expected_messages'`). Previously, only errors expected via the `notice_error` API were correctly prevented from impacting Apdex. Expected errors set by configuration incorrectly impacted Apdex. This behavior has been fixed and now both types of expected errors will correctly not impact Apdex. Thanks very much to [@florianpilz](https://github.com/florianpilz) for bringing this issue to our attention. [PR#2619](https://github.com/newrelic/newrelic-ruby-agent/pull/2619)
+
+
 ## v9.9.0
 
 Version 9.9.0 introduces support for AWS Lambda serverless function observability, adds support for Elasticsearch 8.13.0, and adds the 'request.temperature' attribute to chat completion summaries in ruby-openai instrumentation.

lib/new_relic/agent/error_collector.rb

@@ -110,6 +110,29 @@ def error_is_ignored?(error, status_code = nil)
         false
       end
 
+      # Neither ignored nor expected errors impact apdex.
+      #
+      # Ignored errors are checked via `#error_is_ignored?`
+      # Expected errors are checked in 2 separate ways:
+      #   1. The presence of an `expected: true` attribute key/value pair in the
+      #      options hash, which will be set if that key/value pair was used in
+      #      the `notice_error` public API.
+      #   2. By calling `#expected?` which in turn calls `ErrorFilter#expected?`
+      #      which checks for 3 things:
+      #        - A match for user-defined HTTP status codes to expect
+      #        - A match for user-defined error classes to expect
+      #        - A match for user-defined error messages to expect
+      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