diff --git a/CHANGELOG.md b/CHANGELOG.md index 5f2d70a89d..d7835fe946 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,14 @@ # New Relic Ruby Agent Release Notes +## dev + +Version 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. diff --git a/lib/new_relic/agent/error_collector.rb b/lib/new_relic/agent/error_collector.rb index 41af8e761b..896ef91d32 100644 --- a/lib/new_relic/agent/error_collector.rb +++ b/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 diff --git a/lib/new_relic/agent/transaction.rb b/lib/new_relic/agent/transaction.rb index 7ca7ec8152..bcbf87d3da 100644 --- a/lib/new_relic/agent/transaction.rb +++ b/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) diff --git a/test/agent_helper.rb b/test/agent_helper.rb index 5d12ed5e17..9615253cf9 100644 --- a/test/agent_helper.rb +++ b/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}" } diff --git a/test/new_relic/agent/transaction_test.rb b/test/new_relic/agent/transaction_test.rb index 172ba399c5..e7ff5d4a2c 100644 --- a/test/new_relic/agent/transaction_test.rb +++ b/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