From 528f6d60311b4a07a816a4cf1e040776d29bffcf Mon Sep 17 00:00:00 2001 From: Aaron Huntsman Date: Wed, 9 Jun 2021 16:59:26 -0400 Subject: [PATCH 01/10] add skeleton tests and ErrorFilter class --- lib/new_relic/agent/error_collector.rb | 1 + lib/new_relic/agent/error_filter.rb | 13 +++++++ lib/new_relic/noticed_error.rb | 6 ++-- test/new_relic/agent/error_filter_test.rb | 41 +++++++++++++++++++++++ 4 files changed, 58 insertions(+), 3 deletions(-) create mode 100644 lib/new_relic/agent/error_filter.rb create mode 100644 test/new_relic/agent/error_filter_test.rb diff --git a/lib/new_relic/agent/error_collector.rb b/lib/new_relic/agent/error_collector.rb index 39f01d52a0..d65f982c8c 100644 --- a/lib/new_relic/agent/error_collector.rb +++ b/lib/new_relic/agent/error_collector.rb @@ -210,6 +210,7 @@ def notice_segment_error(segment, exception, options={}) # See NewRelic::Agent.notice_error for options and commentary def notice_error(exception, options={}, span_id=nil) + # TODO - Filter ignored/expected errors here. This is where we should set options[:expected] return if skip_notice_error?(exception) tag_exception(exception) diff --git a/lib/new_relic/agent/error_filter.rb b/lib/new_relic/agent/error_filter.rb new file mode 100644 index 0000000000..5a485c504c --- /dev/null +++ b/lib/new_relic/agent/error_filter.rb @@ -0,0 +1,13 @@ +# encoding: utf-8 +# This file is distributed under New Relic's license terms. +# See https://github.com/newrelic/newrelic-ruby-agent/blob/main/LICENSE for complete details. + +module NewRelic + module Agent + + # Handles loading of ignored and expected errors from the agent configuration, and + # determining at runtime whether an exception is ignored or expected. + class ErrorFilter + end + end +end \ No newline at end of file diff --git a/lib/new_relic/noticed_error.rb b/lib/new_relic/noticed_error.rb index 9868e1494d..4f2ce4e7ea 100644 --- a/lib/new_relic/noticed_error.rb +++ b/lib/new_relic/noticed_error.rb @@ -33,7 +33,7 @@ class NewRelic::NoticedError ERROR_CLASS_KEY = "#{ERROR_PREFIX_KEY}.class" ERROR_EXPECTED_KEY = "#{ERROR_PREFIX_KEY}.expected" - def initialize(path, exception, timestamp = Time.now) + def initialize(path, exception, timestamp = Time.now, expected = false) @exception_id = exception.object_id @path = path @@ -58,7 +58,7 @@ def initialize(path, exception, timestamp = Time.now) @attributes_from_notice_error = nil @attributes = nil @timestamp = timestamp - @expected = false + @expected = expected end def ==(other) @@ -104,7 +104,7 @@ def base_parameters params[:file_name] = file_name if file_name params[:line_number] = line_number if line_number params[:stack_trace] = stack_trace if stack_trace - params[:'error.expected'] = expected + params[ERROR_EXPECTED_KEY.to_sym] = expected params end diff --git a/test/new_relic/agent/error_filter_test.rb b/test/new_relic/agent/error_filter_test.rb new file mode 100644 index 0000000000..148aee3930 --- /dev/null +++ b/test/new_relic/agent/error_filter_test.rb @@ -0,0 +1,41 @@ +# encoding: utf-8 +# This file is distributed under New Relic's license terms. +# See https://github.com/newrelic/newrelic-ruby-agent/blob/main/LICENSE for complete details. + +require File.expand_path(File.join(File.dirname(__FILE__),'..','..','test_helper')) +require 'new_relic/agent/configuration/manager' + +module NewRelic::Agent + class ErrorFilter + class ErrorFilterTest < Minitest::Test + + # TODO: setup - read ignored/expected errors from config + + # TODO: test - parses error_collector.ignore_classes as Array; i.e. + # - use test config that sets error_collector.ignore_classes = ['TestExceptionA', 'TestExceptionB'] + # - initialize error_filter object from config + # - assert error_filter.for_class('TestExceptionA') == :ignore + # - assert error_filter.for_class('TextExceptionC') == nil + + # TODO: test - parses error_collector.ignore_messages as Hash + # - as above: error_collector.ignore_messages = { + # 'TextExceptionA' => ['message one', 'message two'] + # } + # - assert error_filter.for_class('TestExceptionA', 'error message one test') == :ignore + # - assert error_filter.for_class('TestExceptionA', 'error message three test') == nil + + # TODO: test - parses error_collector.ignore_status_codes as String + # - as above: error_collector.ignore_status_codes = "404,507-511" + # - assert error_filter.for_status('404') == :ignore + # - assert error_filter.for_status('509') == :ignore + # - assert error_filter.for_status('500') == nil + + # TODO: test - parses error_collector.ignore_errors as error_collector.ignored_classes + # (compatibility for deprecated config setting; split classes by ',' and store as ignore_classes) + + # TODO: test - parses error_collector.expected_classes as Array + # TODO: test - parses error_collector.expected_messages as Hash + # TODO: test - parses error_collector.expected_status_codes as String + end + end +end \ No newline at end of file From eb5fa86daf2fc1695ded8ed41873c180e2d01270 Mon Sep 17 00:00:00 2001 From: Aaron Huntsman Date: Sun, 13 Jun 2021 20:51:26 -0400 Subject: [PATCH 02/10] add more tests; experiment with moving invoke_callbacks after reset_cache --- lib/new_relic/agent.rb | 1 + lib/new_relic/agent/configuration/manager.rb | 2 +- lib/new_relic/agent/error_collector.rb | 24 +++----- lib/new_relic/agent/error_filter.rb | 48 +++++++++++++++ test/new_relic/agent/error_filter_test.rb | 65 +++++++++++++++----- 5 files changed, 108 insertions(+), 32 deletions(-) diff --git a/lib/new_relic/agent.rb b/lib/new_relic/agent.rb index fef886879e..5c08327959 100644 --- a/lib/new_relic/agent.rb +++ b/lib/new_relic/agent.rb @@ -44,6 +44,7 @@ module Agent require 'new_relic/agent/sql_sampler' require 'new_relic/agent/commands/thread_profiler_session' require 'new_relic/agent/error_collector' + require 'new_relic/agent/error_filter' require 'new_relic/agent/sampler' require 'new_relic/agent/database' require 'new_relic/agent/database_adapter' diff --git a/lib/new_relic/agent/configuration/manager.rb b/lib/new_relic/agent/configuration/manager.rb index 1b7af0818f..8b08dfe49c 100644 --- a/lib/new_relic/agent/configuration/manager.rb +++ b/lib/new_relic/agent/configuration/manager.rb @@ -79,7 +79,6 @@ def replace_or_add_config(source) source.freeze was_finished = finished_configuring? - invoke_callbacks(:add, source) case source when SecurityPolicySource then @security_policy_source = source when HighSecuritySource then @high_security_source = source @@ -93,6 +92,7 @@ def replace_or_add_config(source) end reset_cache + invoke_callbacks(:add, source) log_config(:add, source) notify_server_source_added if ServerSource === source diff --git a/lib/new_relic/agent/error_collector.rb b/lib/new_relic/agent/error_collector.rb index d65f982c8c..4a37d3ae48 100644 --- a/lib/new_relic/agent/error_collector.rb +++ b/lib/new_relic/agent/error_collector.rb @@ -22,23 +22,19 @@ def initialize events @error_trace_aggregator = ErrorTraceAggregator.new(MAX_ERROR_QUEUE_LENGTH) @error_event_aggregator = ErrorEventAggregator.new events - # lookup of exception class names to ignore. Hash for fast access - @ignore = {} - - initialize_ignored_errors(Agent.config[:'error_collector.ignore_errors']) - - Agent.config.register_callback(:'error_collector.ignore_errors') do |ignore_errors| - initialize_ignored_errors(ignore_errors) + @error_filter = NewRelic::Agent::ErrorFilter.new + + %w( + ignore_errors + ignore_classes ignore_messages ignore_status_codes + expected_classes expected_messages expected_status_codes + ).each do |w| + Agent.config.register_callback(:"error_collector.#{w}") do |_| + @error_filter.reload + end end end - def initialize_ignored_errors(ignore_errors) - @ignore.clear - ignore_errors = ignore_errors.split(",") if ignore_errors.is_a? String - ignore_errors.each { |error| error.strip! } - ignore(ignore_errors) - end - def enabled? error_trace_aggregator.enabled? || error_event_aggregator.enabled? end diff --git a/lib/new_relic/agent/error_filter.rb b/lib/new_relic/agent/error_filter.rb index 5a485c504c..8bff4c1fd3 100644 --- a/lib/new_relic/agent/error_filter.rb +++ b/lib/new_relic/agent/error_filter.rb @@ -8,6 +8,54 @@ module Agent # Handles loading of ignored and expected errors from the agent configuration, and # determining at runtime whether an exception is ignored or expected. class ErrorFilter + def initialize + reload + end + + # Load ignored/expected errors from current agent config + def reload + @ignored_classes = fetch_agent_config(:ignore_classes) || [] + @ignored_messages = fetch_agent_config(:ignore_messages) || {} + @ignored_status_codes = fetch_agent_config(:ignore_status_codes) || '' + + @expected_classes = fetch_agent_config(:expected_classes) || [] + @expected_messages = fetch_agent_config(:expected_messages) || {} + @expected_status_codes = fetch_agent_config(:expected_status_codes) || '' + + # error_collector.ignore_errors is deprecated, but we still support it + if @ignored_classes.empty? && ignore_errors = fetch_agent_config(:ignore_errors) + @ignored_classes << ignore_errors.split(',').map!(&:strip) + @ignored_classes.flatten! + end + end + + # Takes an Exception object. Depending on whether the Agent is configured + # to treat the exception as Ignored, Expected or neither, returns :ignored, + # :expected or nil, respectively. + def type_for_exception(ex) + return nil unless ex.is_a?(Exception) + return :ignored if ignored?(ex) + return :expected if expected?(ex) + nil + end + + def fetch_agent_config(cfg) + NewRelic::Agent.config[:"error_collector.#{cfg}"] + end + + private + + def ignored?(ex) + @ignored_classes.include?(ex.class.name) || + @ignored_messages.keys.include?(ex.class.name) && + @ignored_messages[ex.class.name].any? { |m| ex.message.include?(m) } + end + + def expected?(ex) + @expected_classes.include?(ex.class.name) || + @expected_messages.keys.include?(ex.class.name) && + @expected_messages[ex.class.name].any? { |m| ex.message.include?(m) } + end end end end \ No newline at end of file diff --git a/test/new_relic/agent/error_filter_test.rb b/test/new_relic/agent/error_filter_test.rb index 148aee3930..ef28cb5d27 100644 --- a/test/new_relic/agent/error_filter_test.rb +++ b/test/new_relic/agent/error_filter_test.rb @@ -3,26 +3,35 @@ # See https://github.com/newrelic/newrelic-ruby-agent/blob/main/LICENSE for complete details. require File.expand_path(File.join(File.dirname(__FILE__),'..','..','test_helper')) -require 'new_relic/agent/configuration/manager' + +class TestExceptionA < StandardError; end +class TestExceptionB < StandardError; end module NewRelic::Agent class ErrorFilter class ErrorFilterTest < Minitest::Test - # TODO: setup - read ignored/expected errors from config + def setup + @error_filter = NewRelic::Agent::ErrorFilter.new + end - # TODO: test - parses error_collector.ignore_classes as Array; i.e. - # - use test config that sets error_collector.ignore_classes = ['TestExceptionA', 'TestExceptionB'] - # - initialize error_filter object from config - # - assert error_filter.for_class('TestExceptionA') == :ignore - # - assert error_filter.for_class('TextExceptionC') == nil + def test_ignore_classes + with_config :'error_collector.ignore_classes' => ['TestExceptionA', 'TestExceptionC'] do + @error_filter.reload + assert_equal :ignored, @error_filter.type_for_exception(TestExceptionA.new) + assert_nil @error_filter.type_for_exception(TestExceptionB.new) + end + end - # TODO: test - parses error_collector.ignore_messages as Hash - # - as above: error_collector.ignore_messages = { - # 'TextExceptionA' => ['message one', 'message two'] - # } - # - assert error_filter.for_class('TestExceptionA', 'error message one test') == :ignore - # - assert error_filter.for_class('TestExceptionA', 'error message three test') == nil + def test_ignore_messages + with_config :'error_collector.ignore_messages' => {'TestExceptionA' => ['message one', 'message two']} do + @error_filter.reload + assert_equal :ignored, @error_filter.type_for_exception(TestExceptionA.new('message one')) + assert_equal :ignored, @error_filter.type_for_exception(TestExceptionA.new('message two')) + assert_nil @error_filter.type_for_exception(TestExceptionA.new('message three')) + assert_nil @error_filter.type_for_exception(TestExceptionB.new('message one')) + end + end # TODO: test - parses error_collector.ignore_status_codes as String # - as above: error_collector.ignore_status_codes = "404,507-511" @@ -30,11 +39,33 @@ class ErrorFilterTest < Minitest::Test # - assert error_filter.for_status('509') == :ignore # - assert error_filter.for_status('500') == nil - # TODO: test - parses error_collector.ignore_errors as error_collector.ignored_classes - # (compatibility for deprecated config setting; split classes by ',' and store as ignore_classes) + # compatibility for deprecated config setting; split classes by ',' and store as ignore_classes + def test_ignore_errors + with_config :'error_collector.ignore_errors' => 'TestExceptionA,TestExceptionC' do + @error_filter.reload + assert_equal :ignored, @error_filter.type_for_exception(TestExceptionA.new) + assert_nil @error_filter.type_for_exception(TestExceptionB.new) + end + end + + def test_expected_classes + with_config :'error_collector.expected_classes' => ['TestExceptionA', 'TestExceptionC'] do + @error_filter.reload + assert_equal :expected, @error_filter.type_for_exception(TestExceptionA.new) + assert_nil @error_filter.type_for_exception(TestExceptionB.new) + end + end + + def test_expected_messages + with_config :'error_collector.expected_messages' => {'TestExceptionA' => ['message one', 'message two']} do + @error_filter.reload + assert_equal :expected, @error_filter.type_for_exception(TestExceptionA.new('message one')) + assert_equal :expected, @error_filter.type_for_exception(TestExceptionA.new('message two')) + assert_nil @error_filter.type_for_exception(TestExceptionA.new('message three')) + assert_nil @error_filter.type_for_exception(TestExceptionB.new('message one')) + end + end - # TODO: test - parses error_collector.expected_classes as Array - # TODO: test - parses error_collector.expected_messages as Hash # TODO: test - parses error_collector.expected_status_codes as String end end From 2b6cfecf2ca7433d47ca97129bab55b823e7d3ca Mon Sep 17 00:00:00 2001 From: Aaron Huntsman Date: Mon, 14 Jun 2021 09:00:31 -0400 Subject: [PATCH 03/10] implement error filtering (without status codes) --- lib/new_relic/agent/error_collector.rb | 23 +++++++++++------------ lib/new_relic/agent/error_filter.rb | 16 ++++++++++++++++ 2 files changed, 27 insertions(+), 12 deletions(-) diff --git a/lib/new_relic/agent/error_collector.rb b/lib/new_relic/agent/error_collector.rb index 4a37d3ae48..3e9128008e 100644 --- a/lib/new_relic/agent/error_collector.rb +++ b/lib/new_relic/agent/error_collector.rb @@ -72,15 +72,6 @@ def self.ignore_error_filter defined?(@ignore_filter) ? @ignore_filter : nil end - # errors is an array of Exception Class Names - # - def ignore(errors) - errors.each do |error| - @ignore[error] = true - ::NewRelic::Agent.logger.debug("Ignoring errors of type '#{error}'") - end - end - # Checks the provided error against the error filter, if there # is an error filter def filtered_by_error_filter?(error) @@ -95,7 +86,7 @@ def filtered_error?(error) # an error is ignored if it is nil or if it is filtered def error_is_ignored?(error) - error && filtered_error?(error) + error && @error_filter.type_for_exception(error) == :ignored rescue => e NewRelic::Agent.logger.error("Error '#{error}' will NOT be ignored. Exception '#{e}' while determining whether to ignore or not.", e) false @@ -170,6 +161,13 @@ def increment_error_count!(state, exception, options={}) end end + def increment_expected_error_count!(state, exception) + stats_engine = NewRelic::Agent.agent.stats_engine + stats_engine.record_unscoped_metrics(state, ['ErrorsExpected/all']) do |stats| + stats.increment_count + end + end + def skip_notice_error?(exception) disabled? || error_is_ignored?(exception) || @@ -206,14 +204,15 @@ def notice_segment_error(segment, exception, options={}) # See NewRelic::Agent.notice_error for options and commentary def notice_error(exception, options={}, span_id=nil) - # TODO - Filter ignored/expected errors here. This is where we should set options[:expected] return if skip_notice_error?(exception) tag_exception(exception) state = ::NewRelic::Agent::Tracer.state - unless options[:expected] + if options[:expected] || @error_filter.type_for_exception(exception) == :expected + increment_expected_error_count!(state, exception) + else increment_error_count!(state, exception, options) end diff --git a/lib/new_relic/agent/error_filter.rb b/lib/new_relic/agent/error_filter.rb index 8bff4c1fd3..0641a61cc2 100644 --- a/lib/new_relic/agent/error_filter.rb +++ b/lib/new_relic/agent/error_filter.rb @@ -27,6 +27,22 @@ def reload @ignored_classes << ignore_errors.split(',').map!(&:strip) @ignored_classes.flatten! end + + @ignored_classes.each do |c| + ::NewRelic::Agent.logger.debug("Ignoring errors of type '#{c}'") + end + @ignored_messages.each do |k,v| + ::NewRelic::Agent.logger.debug("Ignoring errors of type '#{k}' with messages: " + v.join(', ')) + end + ::NewRelic::Agent.logger.debug("Ignoring status codes #{@ignored_status_codes}") unless @ignored_status_codes.empty? + + @expected_classes.each do |c| + ::NewRelic::Agent.logger.debug("Expecting errors of type '#{c}'") + end + @expected_messages.each do |k,v| + ::NewRelic::Agent.logger.debug("Expecting errors of type '#{k}' with messages: " + v.join(', ')) + end + ::NewRelic::Agent.logger.debug("Expecting status codes #{@expected_status_codes}") unless @expected_status_codes.empty? end # Takes an Exception object. Depending on whether the Agent is configured From 6a8bde04baaa04441796433e4eaecb2c579af537 Mon Sep 17 00:00:00 2001 From: Aaron Huntsman Date: Mon, 14 Jun 2021 09:21:14 -0400 Subject: [PATCH 04/10] refactor old ignore methods; ensure noticed expected errors are created with error.expected --- lib/new_relic/agent/error_collector.rb | 12 ++--- lib/new_relic/agent/error_filter.rb | 53 +++++++++++++++-------- test/new_relic/agent/error_filter_test.rb | 28 ++++++------ 3 files changed, 51 insertions(+), 42 deletions(-) diff --git a/lib/new_relic/agent/error_collector.rb b/lib/new_relic/agent/error_collector.rb index 3e9128008e..95d15fbe86 100644 --- a/lib/new_relic/agent/error_collector.rb +++ b/lib/new_relic/agent/error_collector.rb @@ -74,19 +74,13 @@ def self.ignore_error_filter # Checks the provided error against the error filter, if there # is an error filter - def filtered_by_error_filter?(error) + def ignored_by_filter_proc?(error) respond_to?(:ignore_filter_proc) && !ignore_filter_proc(error) end - # Checks the array of error names and the error filter against - # the provided error - def filtered_error?(error) - @ignore[error.class.name] || filtered_by_error_filter?(error) - end - # an error is ignored if it is nil or if it is filtered def error_is_ignored?(error) - error && @error_filter.type_for_exception(error) == :ignored + error && (@error_filter.type_for_exception(error) == :ignored || ignored_by_filter_proc?(error)) rescue => e NewRelic::Agent.logger.error("Error '#{error}' will NOT be ignored. Exception '#{e}' while determining whether to ignore or not.", e) false @@ -254,7 +248,7 @@ def create_noticed_error(exception, options) noticed_error.line_number = sense_method(exception, :line_number) noticed_error.stack_trace = truncate_trace(extract_stack_trace(exception)) - noticed_error.expected = !! options.delete(:expected) + noticed_error.expected = !!options.delete(:expected) || @error_filter.expected?(exception) noticed_error.attributes_from_notice_error = options.delete(:custom_params) || {} diff --git a/lib/new_relic/agent/error_filter.rb b/lib/new_relic/agent/error_filter.rb index 0641a61cc2..9f067621e8 100644 --- a/lib/new_relic/agent/error_filter.rb +++ b/lib/new_relic/agent/error_filter.rb @@ -28,21 +28,7 @@ def reload @ignored_classes.flatten! end - @ignored_classes.each do |c| - ::NewRelic::Agent.logger.debug("Ignoring errors of type '#{c}'") - end - @ignored_messages.each do |k,v| - ::NewRelic::Agent.logger.debug("Ignoring errors of type '#{k}' with messages: " + v.join(', ')) - end - ::NewRelic::Agent.logger.debug("Ignoring status codes #{@ignored_status_codes}") unless @ignored_status_codes.empty? - - @expected_classes.each do |c| - ::NewRelic::Agent.logger.debug("Expecting errors of type '#{c}'") - end - @expected_messages.each do |k,v| - ::NewRelic::Agent.logger.debug("Expecting errors of type '#{k}' with messages: " + v.join(', ')) - end - ::NewRelic::Agent.logger.debug("Expecting status codes #{@expected_status_codes}") unless @expected_status_codes.empty? + log_filters end # Takes an Exception object. Depending on whether the Agent is configured @@ -50,28 +36,57 @@ def reload # :expected or nil, respectively. def type_for_exception(ex) return nil unless ex.is_a?(Exception) - return :ignored if ignored?(ex) - return :expected if expected?(ex) + return :ignored if _ignored?(ex) + return :expected if _expected?(ex) nil end + # Define #ignored? and #expected? in this way so that any given exception + # cannot be both ignored and expected. Ignoring takes priority. + + def ignored?(ex) + type_for_exception(ex) == :ignored + end + + def expected?(ex) + type_for_exception(ex) == :expected + end + def fetch_agent_config(cfg) NewRelic::Agent.config[:"error_collector.#{cfg}"] end private - def ignored?(ex) + def _ignored?(ex) @ignored_classes.include?(ex.class.name) || @ignored_messages.keys.include?(ex.class.name) && @ignored_messages[ex.class.name].any? { |m| ex.message.include?(m) } end - def expected?(ex) + def _expected?(ex) @expected_classes.include?(ex.class.name) || @expected_messages.keys.include?(ex.class.name) && @expected_messages[ex.class.name].any? { |m| ex.message.include?(m) } end + + def log_filters + @ignored_classes.each do |c| + ::NewRelic::Agent.logger.debug("Ignoring errors of type '#{c}'") + end + @ignored_messages.each do |k,v| + ::NewRelic::Agent.logger.debug("Ignoring errors of type '#{k}' with messages: " + v.join(', ')) + end + ::NewRelic::Agent.logger.debug("Ignoring status codes #{@ignored_status_codes}") unless @ignored_status_codes.empty? + + @expected_classes.each do |c| + ::NewRelic::Agent.logger.debug("Expecting errors of type '#{c}'") + end + @expected_messages.each do |k,v| + ::NewRelic::Agent.logger.debug("Expecting errors of type '#{k}' with messages: " + v.join(', ')) + end + ::NewRelic::Agent.logger.debug("Expecting status codes #{@expected_status_codes}") unless @expected_status_codes.empty? + end end end end \ No newline at end of file diff --git a/test/new_relic/agent/error_filter_test.rb b/test/new_relic/agent/error_filter_test.rb index ef28cb5d27..64562eaab7 100644 --- a/test/new_relic/agent/error_filter_test.rb +++ b/test/new_relic/agent/error_filter_test.rb @@ -18,18 +18,18 @@ def setup def test_ignore_classes with_config :'error_collector.ignore_classes' => ['TestExceptionA', 'TestExceptionC'] do @error_filter.reload - assert_equal :ignored, @error_filter.type_for_exception(TestExceptionA.new) - assert_nil @error_filter.type_for_exception(TestExceptionB.new) + assert @error_filter.ignored?(TestExceptionA.new) + refute @error_filter.ignored?(TestExceptionB.new) end end def test_ignore_messages with_config :'error_collector.ignore_messages' => {'TestExceptionA' => ['message one', 'message two']} do @error_filter.reload - assert_equal :ignored, @error_filter.type_for_exception(TestExceptionA.new('message one')) - assert_equal :ignored, @error_filter.type_for_exception(TestExceptionA.new('message two')) - assert_nil @error_filter.type_for_exception(TestExceptionA.new('message three')) - assert_nil @error_filter.type_for_exception(TestExceptionB.new('message one')) + assert @error_filter.ignored?(TestExceptionA.new('message one')) + assert @error_filter.ignored?(TestExceptionA.new('message two')) + refute @error_filter.ignored?(TestExceptionA.new('message three')) + refute @error_filter.ignored?(TestExceptionB.new('message one')) end end @@ -43,26 +43,26 @@ def test_ignore_messages def test_ignore_errors with_config :'error_collector.ignore_errors' => 'TestExceptionA,TestExceptionC' do @error_filter.reload - assert_equal :ignored, @error_filter.type_for_exception(TestExceptionA.new) - assert_nil @error_filter.type_for_exception(TestExceptionB.new) + assert @error_filter.ignored?(TestExceptionA.new) + refute @error_filter.ignored?(TestExceptionB.new) end end def test_expected_classes with_config :'error_collector.expected_classes' => ['TestExceptionA', 'TestExceptionC'] do @error_filter.reload - assert_equal :expected, @error_filter.type_for_exception(TestExceptionA.new) - assert_nil @error_filter.type_for_exception(TestExceptionB.new) + assert @error_filter.expected?(TestExceptionA.new) + refute @error_filter.expected?(TestExceptionB.new) end end def test_expected_messages with_config :'error_collector.expected_messages' => {'TestExceptionA' => ['message one', 'message two']} do @error_filter.reload - assert_equal :expected, @error_filter.type_for_exception(TestExceptionA.new('message one')) - assert_equal :expected, @error_filter.type_for_exception(TestExceptionA.new('message two')) - assert_nil @error_filter.type_for_exception(TestExceptionA.new('message three')) - assert_nil @error_filter.type_for_exception(TestExceptionB.new('message one')) + assert @error_filter.expected?(TestExceptionA.new('message one')) + assert @error_filter.expected?(TestExceptionA.new('message two')) + refute @error_filter.expected?(TestExceptionA.new('message three')) + refute @error_filter.expected?(TestExceptionB.new('message one')) end end From b93045354037729134edb3ad5ea5e6338220055f Mon Sep 17 00:00:00 2001 From: Aaron Huntsman Date: Mon, 14 Jun 2021 21:22:39 -0400 Subject: [PATCH 05/10] more fixes; add collector configs to default source --- .../agent/configuration/default_source.rb | 44 ++++- lib/new_relic/agent/configuration/manager.rb | 4 +- lib/new_relic/agent/error_collector.rb | 27 ++- lib/new_relic/agent/error_filter.rb | 157 ++++++++++++------ test/new_relic/agent/error_collector_test.rb | 20 ++- test/new_relic/agent/error_filter_test.rb | 28 ++-- 6 files changed, 194 insertions(+), 86 deletions(-) diff --git a/lib/new_relic/agent/configuration/default_source.rb b/lib/new_relic/agent/configuration/default_source.rb index c35b67d214..5dcc1a1844 100644 --- a/lib/new_relic/agent/configuration/default_source.rb +++ b/lib/new_relic/agent/configuration/default_source.rb @@ -1297,7 +1297,49 @@ def self.enforce_fallback(allowed_values: nil, fallback: nil) :public => true, :type => String, :allowed_from_server => true, - :description => 'Specify a comma-delimited list of error classes that the agent should ignore.' + :description => 'DEPRECATED; use `error_collector.ignore_classes` instead. Specify a comma-delimited list of error classes that the agent should ignore.' + }, + :'error_collector.ignore_classes' => { + :default => [], + :public => true, + :type => Array, + :allowed_from_server => true, + :description => 'A list of error classes that the agent should ignore.' + }, + :'error_collector.ignore_messages' => { + :default => {}, + :public => true, + :type => Hash, + :allowed_from_server => true, + :description => 'A map of error classes to a list of messages. When an error of one of the classes specified here occurs, if its error message contains one of the strings corresponding to it here, that error will be ignored.' + }, + :'error_collector.ignore_status_codes' => { + :default => '', + :public => true, + :type => String, + :allowed_from_server => true, + :description => 'A comma separated list of status codes, possibly including ranges. Errors associated with these status codes, where applicable, will be ignored.' + }, + :'error_collector.expected_classes' => { + :default => [], + :public => true, + :type => Array, + :allowed_from_server => true, + :description => 'A list of error classes that the agent should treat as expected.' + }, + :'error_collector.expected_messages' => { + :default => {}, + :public => true, + :type => Hash, + :allowed_from_server => true, + :description => 'A map of error classes to a list of messages. When an error of one of the classes specified here occurs, if its error message contains one of the strings corresponding to it here, that error will be treated as expected.' + }, + :'error_collector.expected_status_codes' => { + :default => '', + :public => true, + :type => String, + :allowed_from_server => true, + :description => 'A comma separated list of status codes, possibly including ranges. Errors associated with these status codes, where applicable, will be treated as expected.' }, :'error_collector.max_backtrace_frames' => { :default => 50, diff --git a/lib/new_relic/agent/configuration/manager.rb b/lib/new_relic/agent/configuration/manager.rb index 8b08dfe49c..8dd4c9a9e4 100644 --- a/lib/new_relic/agent/configuration/manager.rb +++ b/lib/new_relic/agent/configuration/manager.rb @@ -79,6 +79,8 @@ def replace_or_add_config(source) source.freeze was_finished = finished_configuring? + invoke_callbacks(:add, source) + case source when SecurityPolicySource then @security_policy_source = source when HighSecuritySource then @high_security_source = source @@ -92,7 +94,6 @@ def replace_or_add_config(source) end reset_cache - invoke_callbacks(:add, source) log_config(:add, source) notify_server_source_added if ServerSource === source @@ -159,7 +160,6 @@ def register_callback(key, &proc) def invoke_callbacks(direction, source) return unless source source.keys.each do |key| - if @cache[key] != source[key] @callbacks[key].each do |proc| if direction == :add diff --git a/lib/new_relic/agent/error_collector.rb b/lib/new_relic/agent/error_collector.rb index 95d15fbe86..081ae8e1f5 100644 --- a/lib/new_relic/agent/error_collector.rb +++ b/lib/new_relic/agent/error_collector.rb @@ -25,12 +25,11 @@ def initialize events @error_filter = NewRelic::Agent::ErrorFilter.new %w( - ignore_errors - ignore_classes ignore_messages ignore_status_codes + ignore_errors ignore_classes ignore_messages ignore_status_codes expected_classes expected_messages expected_status_codes ).each do |w| - Agent.config.register_callback(:"error_collector.#{w}") do |_| - @error_filter.reload + Agent.config.register_callback(:"error_collector.#{w}") do |value| + @error_filter.load_from_config(w, value) end end end @@ -72,6 +71,22 @@ def self.ignore_error_filter defined?(@ignore_filter) ? @ignore_filter : nil end + def ignore(errors) + @error_filter.ignore(errors) + end + + def ignore?(ex) + @error_filter.ignore?(ex) + end + + def expect(errors) + @error_filter.expect(errors) + end + + def expected?(ex) + @error_filter.expected?(ex) + end + # Checks the provided error against the error filter, if there # is an error filter def ignored_by_filter_proc?(error) @@ -80,7 +95,7 @@ def ignored_by_filter_proc?(error) # an error is ignored if it is nil or if it is filtered def error_is_ignored?(error) - error && (@error_filter.type_for_exception(error) == :ignored || ignored_by_filter_proc?(error)) + error && (@error_filter.ignore?(error) || ignored_by_filter_proc?(error)) rescue => e NewRelic::Agent.logger.error("Error '#{error}' will NOT be ignored. Exception '#{e}' while determining whether to ignore or not.", e) false @@ -204,7 +219,7 @@ def notice_error(exception, options={}, span_id=nil) state = ::NewRelic::Agent::Tracer.state - if options[:expected] || @error_filter.type_for_exception(exception) == :expected + if options[:expected] || @error_filter.expected?(exception) increment_expected_error_count!(state, exception) else increment_error_count!(state, exception, options) diff --git a/lib/new_relic/agent/error_filter.rb b/lib/new_relic/agent/error_filter.rb index 9f067621e8..51f5189379 100644 --- a/lib/new_relic/agent/error_filter.rb +++ b/lib/new_relic/agent/error_filter.rb @@ -8,84 +8,131 @@ module Agent # Handles loading of ignored and expected errors from the agent configuration, and # determining at runtime whether an exception is ignored or expected. class ErrorFilter + def initialize - reload + @ignore_errors, @ignore_classes, @expected_classes = [], [], [] + @ignore_messages, @expected_messages = {}, {} + @ignore_status_codes, @expected_status_codes = [], [] end - # Load ignored/expected errors from current agent config - def reload - @ignored_classes = fetch_agent_config(:ignore_classes) || [] - @ignored_messages = fetch_agent_config(:ignore_messages) || {} - @ignored_status_codes = fetch_agent_config(:ignore_status_codes) || '' - - @expected_classes = fetch_agent_config(:expected_classes) || [] - @expected_messages = fetch_agent_config(:expected_messages) || {} - @expected_status_codes = fetch_agent_config(:expected_status_codes) || '' - - # error_collector.ignore_errors is deprecated, but we still support it - if @ignored_classes.empty? && ignore_errors = fetch_agent_config(:ignore_errors) - @ignored_classes << ignore_errors.split(',').map!(&:strip) - @ignored_classes.flatten! - end - - log_filters + def load_all + %i( + ignore_errors ignore_classes ignore_messages ignore_status_codes + expected_classes expected_messages expected_status_codes + ).each { |setting| load_from_config(setting) } end - # Takes an Exception object. Depending on whether the Agent is configured - # to treat the exception as Ignored, Expected or neither, returns :ignored, - # :expected or nil, respectively. - def type_for_exception(ex) - return nil unless ex.is_a?(Exception) - return :ignored if _ignored?(ex) - return :expected if _expected?(ex) - nil + def load_from_config(setting, value = nil) + errors = nil + new_value = value || fetch_agent_config(setting.to_sym) + case setting.to_sym + when :ignore_errors # Deprecated; only use if ignore_classes isn't present + errors = @ignore_errors = new_value.to_s.split(',').map!(&:strip) + when :ignore_classes + errors = @ignore_classes = new_value || [] + when :ignore_messages + errors = @ignore_messages = new_value || {} + when :ignore_status_codes + errors = @ignore_status_codes = parse_status_codes(new_value) || [] + when :expected_classes + errors = @expected_classes = new_value || [] + when :expected_messages + errors = @expected_messages = new_value || {} + when :expected_status_codes + errors = @expected_status_codes = parse_status_codes(new_value) || [] + end + log_filter(setting, errors) end # Define #ignored? and #expected? in this way so that any given exception - # cannot be both ignored and expected. Ignoring takes priority. - - def ignored?(ex) - type_for_exception(ex) == :ignored + # cannot be both ignored and expected when using type_for_exception. + # Ignoring takes priority. + + def ignore?(ex) + @ignore_classes.include?(ex.class.name) || + (@ignore_classes.empty? && @ignore_errors.include?(ex.class.name)) || + @ignore_messages.keys.include?(ex.class.name) && + @ignore_messages[ex.class.name].any? { |m| ex.message.include?(m) } end def expected?(ex) - type_for_exception(ex) == :expected + @expected_classes.include?(ex.class.name) || + @expected_messages.keys.include?(ex.class.name) && + @expected_messages[ex.class.name].any? { |m| ex.message.include?(m) } end def fetch_agent_config(cfg) NewRelic::Agent.config[:"error_collector.#{cfg}"] end - private - - def _ignored?(ex) - @ignored_classes.include?(ex.class.name) || - @ignored_messages.keys.include?(ex.class.name) && - @ignored_messages[ex.class.name].any? { |m| ex.message.include?(m) } + def ignore(errors) + case errors + when Array + @ignore_classes += errors + log_filter(:ignore_classes, errors) + when Hash + @ignore_messages.update(errors) + log_filter(:ignore_messages, errors) + when String + if errors.match(/^[\d\,\-]+$/) + @ignore_status_codes += parse_status_codes(errors) # TODO: convert this value to a Hash + else + new_ignore_classes = errors.split(',').map!(&:strip) + @ignore_classes += new_ignore_classes + log_filter(:ignore_classes, new_ignore_classes) + end + end end - def _expected?(ex) - @expected_classes.include?(ex.class.name) || - @expected_messages.keys.include?(ex.class.name) && - @expected_messages[ex.class.name].any? { |m| ex.message.include?(m) } + def expect(errors) + case errors + when Array + @expected_classes += errors + log_filter(:expected_classes, errors) + when Hash + @expected_messages.update(errors) + log_filter(:expected_messages, errors) + when String + if errors.match(/^[\d\,\-]+$/) + @expected_status_codes += parse_status_codes(errors) + else + new_expected_classes = errors.split(',').map!(&:strip) + @expected_classes += new_expected_classes + log_filter(:expected_classes, new_expected_classes) + end + end end - def log_filters - @ignored_classes.each do |c| - ::NewRelic::Agent.logger.debug("Ignoring errors of type '#{c}'") - end - @ignored_messages.each do |k,v| - ::NewRelic::Agent.logger.debug("Ignoring errors of type '#{k}' with messages: " + v.join(', ')) - end - ::NewRelic::Agent.logger.debug("Ignoring status codes #{@ignored_status_codes}") unless @ignored_status_codes.empty? + private - @expected_classes.each do |c| - ::NewRelic::Agent.logger.debug("Expecting errors of type '#{c}'") - end - @expected_messages.each do |k,v| - ::NewRelic::Agent.logger.debug("Expecting errors of type '#{k}' with messages: " + v.join(', ')) + def log_filter(setting, errors) + case setting + when :ignore_errors, :ignore_classes + errors.each do |error| + ::NewRelic::Agent.logger.debug("Ignoring errors of type '#{error}'") + end + when :ignore_messages + errors.each do |error,messages| + ::NewRelic::Agent.logger.debug("Ignoring errors of type '#{error}' with messages: #{messages.join(',')}") + end + when :ignore_status_codes + ::NewRelic::Agent.logger.debug("Ignoring errors associated with status codes: #{errors}") + when :expected_classes + errors.each do |error| + ::NewRelic::Agent.logger.debug("Expecting errors of type '#{error}'") + end + when :expected_messages + errors.each do |error,messages| + ::NewRelic::Agent.logger.debug("Expecting errors of type '#{error}' with messages: #{messages.join(',')}") + end + when :expected_status_codes + ::NewRelic::Agent.logger.debug("Expecting errors associated with status codes: #{errors}") end - ::NewRelic::Agent.logger.debug("Expecting status codes #{@expected_status_codes}") unless @expected_status_codes.empty? + end + + def parse_status_codes(code_string) + # TODO: implement + [] end end end diff --git a/test/new_relic/agent/error_collector_test.rb b/test/new_relic/agent/error_collector_test.rb index 1dc2807809..7fd92777b8 100644 --- a/test/new_relic/agent/error_collector_test.rb +++ b/test/new_relic/agent/error_collector_test.rb @@ -342,21 +342,25 @@ def test_skip_notice_error_returns_false_for_non_nil_unignored_errors_with_an_en class ::AnError end - def test_filtered_error_positive + def test_ignore_error + error = AnError.new with_config(:'error_collector.ignore_errors' => 'AnError') do - error = AnError.new - assert @error_collector.filtered_error?(error) + assert @error_collector.ignore?(error) end + refute @error_collector.ignore?(error) end - def test_filtered_error_negative + def test_expected_classes error = AnError.new - refute @error_collector.filtered_error?(error) + with_config(:'error_collector.expected_classes' => ['AnError']) do + assert @error_collector.expected?(error) + end + refute @error_collector.expected?(error) end def test_filtered_by_error_filter_empty # should return right away when there's no filter - refute @error_collector.filtered_by_error_filter?(nil) + refute @error_collector.ignored_by_filter_proc?(nil) end def test_filtered_by_error_filter_positive @@ -367,7 +371,7 @@ def test_filtered_by_error_filter_positive end error = StandardError.new - assert @error_collector.filtered_by_error_filter?(error) + assert @error_collector.ignored_by_filter_proc?(error) assert_equal error, saw_error end @@ -380,7 +384,7 @@ def test_filtered_by_error_filter_negative end error = StandardError.new - refute @error_collector.filtered_by_error_filter?(error) + refute @error_collector.ignored_by_filter_proc?(error) assert_equal error, saw_error end diff --git a/test/new_relic/agent/error_filter_test.rb b/test/new_relic/agent/error_filter_test.rb index 64562eaab7..9fe6fbeff5 100644 --- a/test/new_relic/agent/error_filter_test.rb +++ b/test/new_relic/agent/error_filter_test.rb @@ -17,19 +17,19 @@ def setup def test_ignore_classes with_config :'error_collector.ignore_classes' => ['TestExceptionA', 'TestExceptionC'] do - @error_filter.reload - assert @error_filter.ignored?(TestExceptionA.new) - refute @error_filter.ignored?(TestExceptionB.new) + @error_filter.load_all + assert @error_filter.ignore?(TestExceptionA.new) + refute @error_filter.ignore?(TestExceptionB.new) end end def test_ignore_messages with_config :'error_collector.ignore_messages' => {'TestExceptionA' => ['message one', 'message two']} do - @error_filter.reload - assert @error_filter.ignored?(TestExceptionA.new('message one')) - assert @error_filter.ignored?(TestExceptionA.new('message two')) - refute @error_filter.ignored?(TestExceptionA.new('message three')) - refute @error_filter.ignored?(TestExceptionB.new('message one')) + @error_filter.load_all + assert @error_filter.ignore?(TestExceptionA.new('message one')) + assert @error_filter.ignore?(TestExceptionA.new('message two')) + refute @error_filter.ignore?(TestExceptionA.new('message three')) + refute @error_filter.ignore?(TestExceptionB.new('message one')) end end @@ -39,18 +39,18 @@ def test_ignore_messages # - assert error_filter.for_status('509') == :ignore # - assert error_filter.for_status('500') == nil - # compatibility for deprecated config setting; split classes by ',' and store as ignore_classes + # compatibility for deprecated config setting def test_ignore_errors with_config :'error_collector.ignore_errors' => 'TestExceptionA,TestExceptionC' do - @error_filter.reload - assert @error_filter.ignored?(TestExceptionA.new) - refute @error_filter.ignored?(TestExceptionB.new) + @error_filter.load_all + assert @error_filter.ignore?(TestExceptionA.new) + refute @error_filter.ignore?(TestExceptionB.new) end end def test_expected_classes with_config :'error_collector.expected_classes' => ['TestExceptionA', 'TestExceptionC'] do - @error_filter.reload + @error_filter.load_all assert @error_filter.expected?(TestExceptionA.new) refute @error_filter.expected?(TestExceptionB.new) end @@ -58,7 +58,7 @@ def test_expected_classes def test_expected_messages with_config :'error_collector.expected_messages' => {'TestExceptionA' => ['message one', 'message two']} do - @error_filter.reload + @error_filter.load_all assert @error_filter.expected?(TestExceptionA.new('message one')) assert @error_filter.expected?(TestExceptionA.new('message two')) refute @error_filter.expected?(TestExceptionA.new('message three')) From 8542d0154155d39d63e0615a9eb16d57ee366161 Mon Sep 17 00:00:00 2001 From: Aaron Huntsman Date: Tue, 15 Jun 2021 08:07:24 -0400 Subject: [PATCH 06/10] test for ErrorsExpected/all --- lib/new_relic/agent/error_collector.rb | 2 +- test/new_relic/agent/error_collector_test.rb | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/new_relic/agent/error_collector.rb b/lib/new_relic/agent/error_collector.rb index 081ae8e1f5..2749d0be71 100644 --- a/lib/new_relic/agent/error_collector.rb +++ b/lib/new_relic/agent/error_collector.rb @@ -263,7 +263,7 @@ def create_noticed_error(exception, options) noticed_error.line_number = sense_method(exception, :line_number) noticed_error.stack_trace = truncate_trace(extract_stack_trace(exception)) - noticed_error.expected = !!options.delete(:expected) || @error_filter.expected?(exception) + noticed_error.expected = !!options.delete(:expected) || expected?(exception) noticed_error.attributes_from_notice_error = options.delete(:custom_params) || {} diff --git a/test/new_relic/agent/error_collector_test.rb b/test/new_relic/agent/error_collector_test.rb index 7fd92777b8..d9c8e914cb 100644 --- a/test/new_relic/agent/error_collector_test.rb +++ b/test/new_relic/agent/error_collector_test.rb @@ -453,6 +453,7 @@ def test_expected_error_does_not_increment_metrics assert_equal 1, traces.length assert_equal 1, events.length assert_metrics_not_recorded ['Errors/all'] + assert_metrics_recorded ['ErrorsExpected/all'] end def test_expected_error_not_recorded_as_custom_attribute From 7e0d17352a065ccf736549376edfbb47cb4e8323 Mon Sep 17 00:00:00 2001 From: Aaron Huntsman Date: Tue, 15 Jun 2021 13:00:29 -0400 Subject: [PATCH 07/10] mark new ignore/expected error configs as dynamic --- lib/new_relic/agent/configuration/default_source.rb | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/lib/new_relic/agent/configuration/default_source.rb b/lib/new_relic/agent/configuration/default_source.rb index 5dcc1a1844..eefc482407 100644 --- a/lib/new_relic/agent/configuration/default_source.rb +++ b/lib/new_relic/agent/configuration/default_source.rb @@ -1297,6 +1297,7 @@ def self.enforce_fallback(allowed_values: nil, fallback: nil) :public => true, :type => String, :allowed_from_server => true, + :dynamic_name => true, :description => 'DEPRECATED; use `error_collector.ignore_classes` instead. Specify a comma-delimited list of error classes that the agent should ignore.' }, :'error_collector.ignore_classes' => { @@ -1304,6 +1305,7 @@ def self.enforce_fallback(allowed_values: nil, fallback: nil) :public => true, :type => Array, :allowed_from_server => true, + :dynamic_name => true, :description => 'A list of error classes that the agent should ignore.' }, :'error_collector.ignore_messages' => { @@ -1311,6 +1313,7 @@ def self.enforce_fallback(allowed_values: nil, fallback: nil) :public => true, :type => Hash, :allowed_from_server => true, + :dynamic_name => true, :description => 'A map of error classes to a list of messages. When an error of one of the classes specified here occurs, if its error message contains one of the strings corresponding to it here, that error will be ignored.' }, :'error_collector.ignore_status_codes' => { @@ -1318,6 +1321,7 @@ def self.enforce_fallback(allowed_values: nil, fallback: nil) :public => true, :type => String, :allowed_from_server => true, + :dynamic_name => true, :description => 'A comma separated list of status codes, possibly including ranges. Errors associated with these status codes, where applicable, will be ignored.' }, :'error_collector.expected_classes' => { @@ -1325,6 +1329,7 @@ def self.enforce_fallback(allowed_values: nil, fallback: nil) :public => true, :type => Array, :allowed_from_server => true, + :dynamic_name => true, :description => 'A list of error classes that the agent should treat as expected.' }, :'error_collector.expected_messages' => { @@ -1332,6 +1337,7 @@ def self.enforce_fallback(allowed_values: nil, fallback: nil) :public => true, :type => Hash, :allowed_from_server => true, + :dynamic_name => true, :description => 'A map of error classes to a list of messages. When an error of one of the classes specified here occurs, if its error message contains one of the strings corresponding to it here, that error will be treated as expected.' }, :'error_collector.expected_status_codes' => { @@ -1339,6 +1345,7 @@ def self.enforce_fallback(allowed_values: nil, fallback: nil) :public => true, :type => String, :allowed_from_server => true, + :dynamic_name => true, :description => 'A comma separated list of status codes, possibly including ranges. Errors associated with these status codes, where applicable, will be treated as expected.' }, :'error_collector.max_backtrace_frames' => { From 2d859cde95f428dbb3d27fb4aa96b63e2479694f Mon Sep 17 00:00:00 2001 From: Aaron Huntsman Date: Mon, 21 Jun 2021 01:31:59 -0400 Subject: [PATCH 08/10] add ignore/expected status codes --- lib/new_relic/agent/error_collector.rb | 26 +++++++------- lib/new_relic/agent/error_filter.rb | 32 ++++++++++++----- test/new_relic/agent/error_collector_test.rb | 13 +++++-- test/new_relic/agent/error_filter_test.rb | 36 ++++++++++++++++---- 4 files changed, 77 insertions(+), 30 deletions(-) diff --git a/lib/new_relic/agent/error_collector.rb b/lib/new_relic/agent/error_collector.rb index 2749d0be71..f8842e9cc4 100644 --- a/lib/new_relic/agent/error_collector.rb +++ b/lib/new_relic/agent/error_collector.rb @@ -75,16 +75,16 @@ def ignore(errors) @error_filter.ignore(errors) end - def ignore?(ex) - @error_filter.ignore?(ex) + def ignore?(ex, status_code = nil) + @error_filter.ignore?(ex, status_code) end def expect(errors) @error_filter.expect(errors) end - def expected?(ex) - @error_filter.expected?(ex) + def expected?(ex, status_code = nil) + @error_filter.expected?(ex, status_code) end # Checks the provided error against the error filter, if there @@ -94,8 +94,8 @@ def ignored_by_filter_proc?(error) end # an error is ignored if it is nil or if it is filtered - def error_is_ignored?(error) - error && (@error_filter.ignore?(error) || ignored_by_filter_proc?(error)) + def error_is_ignored?(error, status_code = nil) + error && (@error_filter.ignore?(error, status_code) || ignored_by_filter_proc?(error)) rescue => e NewRelic::Agent.logger.error("Error '#{error}' will NOT be ignored. Exception '#{e}' while determining whether to ignore or not.", e) false @@ -177,9 +177,9 @@ def increment_expected_error_count!(state, exception) end end - def skip_notice_error?(exception) + def skip_notice_error?(exception, status_code = nil) disabled? || - error_is_ignored?(exception) || + error_is_ignored?(exception, status_code) || exception.nil? || exception_tagged_with?(EXCEPTION_TAG_IVAR, exception) end @@ -213,13 +213,15 @@ def notice_segment_error(segment, exception, options={}) # See NewRelic::Agent.notice_error for options and commentary def notice_error(exception, options={}, span_id=nil) - return if skip_notice_error?(exception) + state = ::NewRelic::Agent::Tracer.state + transaction = state.current_transaction + status_code = transaction ? transaction.http_response_code : nil - tag_exception(exception) + return if skip_notice_error?(exception, status_code) - state = ::NewRelic::Agent::Tracer.state + tag_exception(exception) - if options[:expected] || @error_filter.expected?(exception) + if options[:expected] || @error_filter.expected?(exception, status_code) increment_expected_error_count!(state, exception) else increment_error_count!(state, exception, options) diff --git a/lib/new_relic/agent/error_filter.rb b/lib/new_relic/agent/error_filter.rb index 51f5189379..b6e335a68d 100644 --- a/lib/new_relic/agent/error_filter.rb +++ b/lib/new_relic/agent/error_filter.rb @@ -48,17 +48,19 @@ def load_from_config(setting, value = nil) # cannot be both ignored and expected when using type_for_exception. # Ignoring takes priority. - def ignore?(ex) + def ignore?(ex, status_code = nil) @ignore_classes.include?(ex.class.name) || (@ignore_classes.empty? && @ignore_errors.include?(ex.class.name)) || - @ignore_messages.keys.include?(ex.class.name) && - @ignore_messages[ex.class.name].any? { |m| ex.message.include?(m) } + (@ignore_messages.keys.include?(ex.class.name) && + @ignore_messages[ex.class.name].any? { |m| ex.message.include?(m) }) || + @ignore_status_codes.include?(status_code.to_i) end - def expected?(ex) + def expected?(ex, status_code = nil) @expected_classes.include?(ex.class.name) || - @expected_messages.keys.include?(ex.class.name) && - @expected_messages[ex.class.name].any? { |m| ex.message.include?(m) } + (@expected_messages.keys.include?(ex.class.name) && + @expected_messages[ex.class.name].any? { |m| ex.message.include?(m) }) || + @expected_status_codes.include?(status_code.to_i) end def fetch_agent_config(cfg) @@ -75,7 +77,7 @@ def ignore(errors) log_filter(:ignore_messages, errors) when String if errors.match(/^[\d\,\-]+$/) - @ignore_status_codes += parse_status_codes(errors) # TODO: convert this value to a Hash + @ignore_status_codes += parse_status_codes(errors) else new_ignore_classes = errors.split(',').map!(&:strip) @ignore_classes += new_ignore_classes @@ -131,8 +133,20 @@ def log_filter(setting, errors) end def parse_status_codes(code_string) - # TODO: implement - [] + result = [] + code_string.to_s.split(',').each do |code| + m = code.match(/(\d{3})(-\d{3})?/) + if m.nil? || m[1].nil? + ::NewRelic::Agent.logger.warn("Invalid HTTP status code string: '#{code_string}'; ignoring config") + return [] + end + if m[2] + result += (m[1]..m[2].tr('-', '')).to_a.map(&:to_i) + else + result << m[1].to_i + end + end + result.uniq end end end diff --git a/test/new_relic/agent/error_collector_test.rb b/test/new_relic/agent/error_collector_test.rb index d9c8e914cb..582907a189 100644 --- a/test/new_relic/agent/error_collector_test.rb +++ b/test/new_relic/agent/error_collector_test.rb @@ -318,7 +318,7 @@ def test_skip_notice_error_is_true_if_the_error_collector_is_disabled def test_skip_notice_error_is_true_if_the_error_is_nil error = nil with_config(:'error_collector.enabled' => true) do - @error_collector.expects(:error_is_ignored?).with(error).returns(false) + @error_collector.expects(:error_is_ignored?).with(error, nil).returns(false) assert @error_collector.skip_notice_error?(error) end end @@ -326,7 +326,7 @@ def test_skip_notice_error_is_true_if_the_error_is_nil def test_skip_notice_error_is_true_if_the_error_is_ignored error = StandardError.new with_config(:'error_collector.enabled' => true) do - @error_collector.expects(:error_is_ignored?).with(error).returns(true) + @error_collector.expects(:error_is_ignored?).with(error, nil).returns(true) assert @error_collector.skip_notice_error?(error) end end @@ -334,7 +334,7 @@ def test_skip_notice_error_is_true_if_the_error_is_ignored def test_skip_notice_error_returns_false_for_non_nil_unignored_errors_with_an_enabled_error_collector error = StandardError.new with_config(:'error_collector.enabled' => true) do - @error_collector.expects(:error_is_ignored?).with(error).returns(false) + @error_collector.expects(:error_is_ignored?).with(error, nil).returns(false) refute @error_collector.skip_notice_error?(error) end end @@ -350,6 +350,13 @@ def test_ignore_error refute @error_collector.ignore?(error) end + def test_ignore_status_codes + error = AnError.new + with_config(:'error_collector.ignore_status_codes' => '400-408') do + assert @error_collector.ignore?(error, 404) + end + end + def test_expected_classes error = AnError.new with_config(:'error_collector.expected_classes' => ['AnError']) do diff --git a/test/new_relic/agent/error_filter_test.rb b/test/new_relic/agent/error_filter_test.rb index 9fe6fbeff5..5a2ad563d1 100644 --- a/test/new_relic/agent/error_filter_test.rb +++ b/test/new_relic/agent/error_filter_test.rb @@ -33,11 +33,22 @@ def test_ignore_messages end end - # TODO: test - parses error_collector.ignore_status_codes as String - # - as above: error_collector.ignore_status_codes = "404,507-511" - # - assert error_filter.for_status('404') == :ignore - # - assert error_filter.for_status('509') == :ignore - # - assert error_filter.for_status('500') == nil + def test_ignore_status_codes + with_config :'error_collector.ignore_status_codes' => '401,405-412,500' do + @error_filter.load_all + assert @error_filter.ignore?(TestExceptionA.new, 401) + assert @error_filter.ignore?(TestExceptionA.new, 409) + refute @error_filter.ignore?(TestExceptionA.new, 404) + end + end + + def test_skip_invalid_status_codes + with_config :'error_collector.ignore_status_codes' => '401,sausage,foo-bar,500' do + @error_filter.load_all + refute @error_filter.ignore?(TestExceptionA.new, 400) + refute @error_filter.ignore?(TestExceptionA.new, 401) + end + end # compatibility for deprecated config setting def test_ignore_errors @@ -48,6 +59,12 @@ def test_ignore_errors end end + def test_ignore_from_string + @error_filter.ignore('TestExceptionA,TestExceptionC') + assert @error_filter.ignore?(TestExceptionA.new) + refute @error_filter.ignore?(TestExceptionB.new) + end + def test_expected_classes with_config :'error_collector.expected_classes' => ['TestExceptionA', 'TestExceptionC'] do @error_filter.load_all @@ -66,7 +83,14 @@ def test_expected_messages end end - # TODO: test - parses error_collector.expected_status_codes as String + def test_expected_status_codes + with_config :'error_collector.expected_status_codes' => '401,405-412,500' do + @error_filter.load_all + assert @error_filter.expected?(TestExceptionA.new, 401) + assert @error_filter.expected?(TestExceptionA.new, 409) + refute @error_filter.expected?(TestExceptionA.new, 404) + end + end end end end \ No newline at end of file From 16b53075a040739d619879f2cc005e4089ed3959 Mon Sep 17 00:00:00 2001 From: Aaron Huntsman Date: Tue, 22 Jun 2021 23:34:09 -0400 Subject: [PATCH 09/10] add note to settings re: environment; add logging where needed --- lib/new_relic/agent/configuration/default_source.rb | 8 ++++---- lib/new_relic/agent/error_filter.rb | 13 ++++++------- 2 files changed, 10 insertions(+), 11 deletions(-) diff --git a/lib/new_relic/agent/configuration/default_source.rb b/lib/new_relic/agent/configuration/default_source.rb index eefc482407..2161e04ed7 100644 --- a/lib/new_relic/agent/configuration/default_source.rb +++ b/lib/new_relic/agent/configuration/default_source.rb @@ -1306,7 +1306,7 @@ def self.enforce_fallback(allowed_values: nil, fallback: nil) :type => Array, :allowed_from_server => true, :dynamic_name => true, - :description => 'A list of error classes that the agent should ignore.' + :description => 'A list of error classes that the agent should ignore. *Note: this setting cannot be set via environment variable.*' }, :'error_collector.ignore_messages' => { :default => {}, @@ -1314,7 +1314,7 @@ def self.enforce_fallback(allowed_values: nil, fallback: nil) :type => Hash, :allowed_from_server => true, :dynamic_name => true, - :description => 'A map of error classes to a list of messages. When an error of one of the classes specified here occurs, if its error message contains one of the strings corresponding to it here, that error will be ignored.' + :description => 'A map of error classes to a list of messages. When an error of one of the classes specified here occurs, if its error message contains one of the strings corresponding to it here, that error will be ignored. *Note: this setting cannot be set via environment variable.*' }, :'error_collector.ignore_status_codes' => { :default => '', @@ -1330,7 +1330,7 @@ def self.enforce_fallback(allowed_values: nil, fallback: nil) :type => Array, :allowed_from_server => true, :dynamic_name => true, - :description => 'A list of error classes that the agent should treat as expected.' + :description => 'A list of error classes that the agent should treat as expected. *Note: this setting cannot be set via environment variable.*' }, :'error_collector.expected_messages' => { :default => {}, @@ -1338,7 +1338,7 @@ def self.enforce_fallback(allowed_values: nil, fallback: nil) :type => Hash, :allowed_from_server => true, :dynamic_name => true, - :description => 'A map of error classes to a list of messages. When an error of one of the classes specified here occurs, if its error message contains one of the strings corresponding to it here, that error will be treated as expected.' + :description => 'A map of error classes to a list of messages. When an error of one of the classes specified here occurs, if its error message contains one of the strings corresponding to it here, that error will be treated as expected. *Note: this setting cannot be set via environment variable.*' }, :'error_collector.expected_status_codes' => { :default => '', diff --git a/lib/new_relic/agent/error_filter.rb b/lib/new_relic/agent/error_filter.rb index b6e335a68d..f7cc21e8b6 100644 --- a/lib/new_relic/agent/error_filter.rb +++ b/lib/new_relic/agent/error_filter.rb @@ -44,10 +44,6 @@ def load_from_config(setting, value = nil) log_filter(setting, errors) end - # Define #ignored? and #expected? in this way so that any given exception - # cannot be both ignored and expected when using type_for_exception. - # Ignoring takes priority. - def ignore?(ex, status_code = nil) @ignore_classes.include?(ex.class.name) || (@ignore_classes.empty? && @ignore_errors.include?(ex.class.name)) || @@ -78,6 +74,7 @@ def ignore(errors) when String if errors.match(/^[\d\,\-]+$/) @ignore_status_codes += parse_status_codes(errors) + log_filter(:ignore_status_codes, errors) else new_ignore_classes = errors.split(',').map!(&:strip) @ignore_classes += new_ignore_classes @@ -97,6 +94,7 @@ def expect(errors) when String if errors.match(/^[\d\,\-]+$/) @expected_status_codes += parse_status_codes(errors) + log_filter(:expected_status_codes, errors) else new_expected_classes = errors.split(',').map!(&:strip) @expected_classes += new_expected_classes @@ -132,12 +130,13 @@ def log_filter(setting, errors) end end - def parse_status_codes(code_string) + def parse_status_codes(codes) + code_list = codes.is_a?(String) ? codes.split(',') : codes result = [] - code_string.to_s.split(',').each do |code| + code_list.each do |code| m = code.match(/(\d{3})(-\d{3})?/) if m.nil? || m[1].nil? - ::NewRelic::Agent.logger.warn("Invalid HTTP status code string: '#{code_string}'; ignoring config") + ::NewRelic::Agent.logger.warn("Invalid HTTP status code: '#{code}'; ignoring config") return [] end if m[2] From e93daffa9630e947a92c3fbec72a2c347e6088b7 Mon Sep 17 00:00:00 2001 From: Aaron Huntsman Date: Thu, 24 Jun 2021 00:00:35 -0400 Subject: [PATCH 10/10] more tests --- lib/new_relic/agent/error_filter.rb | 76 +++++++++-------- .../suites/rails/error_tracing_test.rb | 13 +++ test/new_relic/agent/error_collector_test.rb | 11 +++ test/new_relic/agent/error_filter_test.rb | 83 ++++++++++++++++--- 4 files changed, 139 insertions(+), 44 deletions(-) diff --git a/lib/new_relic/agent/error_filter.rb b/lib/new_relic/agent/error_filter.rb index f7cc21e8b6..ca0c5ff161 100644 --- a/lib/new_relic/agent/error_filter.rb +++ b/lib/new_relic/agent/error_filter.rb @@ -10,6 +10,10 @@ module Agent class ErrorFilter def initialize + reset + end + + def reset @ignore_errors, @ignore_classes, @expected_classes = [], [], [] @ignore_messages, @expected_messages = {}, {} @ignore_status_codes, @expected_status_codes = [], [] @@ -63,42 +67,48 @@ def fetch_agent_config(cfg) NewRelic::Agent.config[:"error_collector.#{cfg}"] end - def ignore(errors) - case errors - when Array - @ignore_classes += errors - log_filter(:ignore_classes, errors) - when Hash - @ignore_messages.update(errors) - log_filter(:ignore_messages, errors) - when String - if errors.match(/^[\d\,\-]+$/) - @ignore_status_codes += parse_status_codes(errors) - log_filter(:ignore_status_codes, errors) - else - new_ignore_classes = errors.split(',').map!(&:strip) - @ignore_classes += new_ignore_classes - log_filter(:ignore_classes, new_ignore_classes) + # A generic method for adding ignore filters manually. This is kept for compatibility + # with the previous ErrorCollector#ignore method, and adds some flexibility for adding + # different ignore/expected error types by examining each argument. + def ignore(*args) + args.each do |errors| + case errors + when Array + errors.each { |e| ignore(e) } + when Hash + @ignore_messages.update(errors) + log_filter(:ignore_messages, errors) + when String + if errors.match(/^[\d\,\-]+$/) + @ignore_status_codes |= parse_status_codes(errors) + log_filter(:ignore_status_codes, errors) + else + new_ignore_classes = errors.split(',').map!(&:strip) + @ignore_classes |= new_ignore_classes + log_filter(:ignore_classes, new_ignore_classes) + end end end end - def expect(errors) - case errors - when Array - @expected_classes += errors - log_filter(:expected_classes, errors) - when Hash - @expected_messages.update(errors) - log_filter(:expected_messages, errors) - when String - if errors.match(/^[\d\,\-]+$/) - @expected_status_codes += parse_status_codes(errors) - log_filter(:expected_status_codes, errors) - else - new_expected_classes = errors.split(',').map!(&:strip) - @expected_classes += new_expected_classes - log_filter(:expected_classes, new_expected_classes) + # See #ignore above. + def expect(*args) + args.each do |errors| + case errors + when Array + errors.each { |e| expect(e) } + when Hash + @expected_messages.update(errors) + log_filter(:expected_messages, errors) + when String + if errors.match(/^[\d\,\-]+$/) + @expected_status_codes |= parse_status_codes(errors) + log_filter(:expected_status_codes, errors) + else + new_expected_classes = errors.split(',').map!(&:strip) + @expected_classes |= new_expected_classes + log_filter(:expected_classes, new_expected_classes) + end end end end @@ -137,7 +147,7 @@ def parse_status_codes(codes) m = code.match(/(\d{3})(-\d{3})?/) if m.nil? || m[1].nil? ::NewRelic::Agent.logger.warn("Invalid HTTP status code: '#{code}'; ignoring config") - return [] + next end if m[2] result += (m[1]..m[2].tr('-', '')).to_a.map(&:to_i) diff --git a/test/multiverse/suites/rails/error_tracing_test.rb b/test/multiverse/suites/rails/error_tracing_test.rb index e0d6bc4d60..f540905dee 100644 --- a/test/multiverse/suites/rails/error_tracing_test.rb +++ b/test/multiverse/suites/rails/error_tracing_test.rb @@ -36,6 +36,10 @@ def ignored_error raise NewRelic::TestHelpers::Exceptions::IgnoredError.new('this error should not be noticed') end + def ignored_status_code + render body: "Protip: you probably shouldn't ignore 500 errors", status: 500 + end + def server_ignored_error raise NewRelic::TestHelpers::Exceptions::ServerIgnoredError.new('this is a server ignored error') end @@ -231,6 +235,14 @@ def test_should_not_notice_filtered_errors 'Noticed an error that should have been ignored') end + def test_should_not_notice_ignored_status_codes + with_config(:'error_collector.ignore_status_codes' => '500') do + get '/error/ignored_status_code' + + assert(errors.empty?, 'Noticed an error that should have been ignored') + end + end + def test_should_notice_server_ignored_error_if_no_server_side_config get '/error/server_ignored_error' assert_error_reported_once('this is a server ignored error') @@ -288,6 +300,7 @@ def test_should_not_increment_metrics_on_expected_error_errors ]) assert_metrics_recorded("Apdex" => { :apdex_s => 1 }) + assert_metrics_recorded(["ErrorsExpected/all"]) end diff --git a/test/new_relic/agent/error_collector_test.rb b/test/new_relic/agent/error_collector_test.rb index 582907a189..402fe9112c 100644 --- a/test/new_relic/agent/error_collector_test.rb +++ b/test/new_relic/agent/error_collector_test.rb @@ -350,6 +350,17 @@ def test_ignore_error refute @error_collector.ignore?(error) end + def test_ignored_and_expected_error_is_ignored + error = AnError.new + with_config(:'error_collector.ignore_classes' => ['AnError'], + :'error_collector.expected_classes' => ['AnError']) do + @error_collector.notice_error(AnError.new) + + events = harvest_error_events + assert_equal 0, events.length + end + end + def test_ignore_status_codes error = AnError.new with_config(:'error_collector.ignore_status_codes' => '400-408') do diff --git a/test/new_relic/agent/error_filter_test.rb b/test/new_relic/agent/error_filter_test.rb index 5a2ad563d1..6f38793df9 100644 --- a/test/new_relic/agent/error_filter_test.rb +++ b/test/new_relic/agent/error_filter_test.rb @@ -6,6 +6,7 @@ class TestExceptionA < StandardError; end class TestExceptionB < StandardError; end +class TestExceptionC < StandardError; end module NewRelic::Agent class ErrorFilter @@ -34,19 +35,58 @@ def test_ignore_messages end def test_ignore_status_codes - with_config :'error_collector.ignore_status_codes' => '401,405-412,500' do + with_config :'error_collector.ignore_status_codes' => '401,405-409,501' do @error_filter.load_all assert @error_filter.ignore?(TestExceptionA.new, 401) - assert @error_filter.ignore?(TestExceptionA.new, 409) + assert @error_filter.ignore?(TestExceptionA.new, 501) + (405..409).each do |c| + assert @error_filter.ignore?(TestExceptionA.new, c) + end refute @error_filter.ignore?(TestExceptionA.new, 404) end end + def test_ignore_status_codes_by_array + with_config :'error_collector.ignore_status_codes' => ['401', '405-409', '501'] do + @error_filter.load_all + assert @error_filter.ignore?(TestExceptionA.new, 401) + assert @error_filter.ignore?(TestExceptionA.new, 501) + (405..409).each do |c| + assert @error_filter.ignore?(TestExceptionA.new, c) + end + refute @error_filter.ignore?(TestExceptionA.new, 404) + end + end + + def test_ignore_method + @error_filter.ignore('TestExceptionA', ['ArgumentError']) + assert @error_filter.ignore?(TestExceptionA.new) + assert @error_filter.ignore?(ArgumentError.new) + refute @error_filter.ignore?(TestExceptionB.new) + + @error_filter.ignore('TestExceptionB', {'TestExceptionC' => ['message one', 'message two']}) + assert @error_filter.ignore?(TestExceptionB.new) + assert @error_filter.ignore?(TestExceptionC.new('message one')) + assert @error_filter.ignore?(TestExceptionC.new('message two')) + refute @error_filter.ignore?(TestExceptionC.new('message three')) + + @error_filter.reset + refute @error_filter.ignore?(TestExceptionA.new) + + @error_filter.ignore('401,405-409', ['500', '505-509']) + assert @error_filter.ignore?(TestExceptionA.new, 401) + assert @error_filter.ignore?(TestExceptionA.new, 407) + assert @error_filter.ignore?(TestExceptionA.new, 500) + assert @error_filter.ignore?(TestExceptionA.new, 507) + refute @error_filter.ignore?(TestExceptionA.new, 404) + end + def test_skip_invalid_status_codes with_config :'error_collector.ignore_status_codes' => '401,sausage,foo-bar,500' do @error_filter.load_all refute @error_filter.ignore?(TestExceptionA.new, 400) - refute @error_filter.ignore?(TestExceptionA.new, 401) + assert @error_filter.ignore?(TestExceptionA.new, 401) + assert @error_filter.ignore?(TestExceptionA.new, 500) end end @@ -59,12 +99,6 @@ def test_ignore_errors end end - def test_ignore_from_string - @error_filter.ignore('TestExceptionA,TestExceptionC') - assert @error_filter.ignore?(TestExceptionA.new) - refute @error_filter.ignore?(TestExceptionB.new) - end - def test_expected_classes with_config :'error_collector.expected_classes' => ['TestExceptionA', 'TestExceptionC'] do @error_filter.load_all @@ -84,13 +118,40 @@ def test_expected_messages end def test_expected_status_codes - with_config :'error_collector.expected_status_codes' => '401,405-412,500' do + with_config :'error_collector.expected_status_codes' => '401,405-409,501' do @error_filter.load_all assert @error_filter.expected?(TestExceptionA.new, 401) - assert @error_filter.expected?(TestExceptionA.new, 409) + assert @error_filter.expected?(TestExceptionA.new, 501) + (405..409).each do |c| + assert @error_filter.expected?(TestExceptionA.new, c) + end refute @error_filter.expected?(TestExceptionA.new, 404) end end + + def test_expect_method + @error_filter.expect('TestExceptionA', ['ArgumentError']) + assert @error_filter.expected?(TestExceptionA.new) + assert @error_filter.expected?(ArgumentError.new) + refute @error_filter.expected?(TestExceptionB.new) + + @error_filter.expect('TestExceptionB', {'TestExceptionC' => ['message one', 'message two']}) + assert @error_filter.expected?(TestExceptionB.new) + assert @error_filter.expected?(TestExceptionC.new('message one')) + assert @error_filter.expected?(TestExceptionC.new('message two')) + refute @error_filter.expected?(TestExceptionC.new('message three')) + + @error_filter.reset + refute @error_filter.expected?(TestExceptionA.new) + + @error_filter.expect('401,405-409', ['500', '505-509']) + assert @error_filter.expected?(TestExceptionA.new, 401) + assert @error_filter.expected?(TestExceptionA.new, 407) + assert @error_filter.expected?(TestExceptionA.new, 500) + assert @error_filter.expected?(TestExceptionA.new, 507) + refute @error_filter.expected?(TestExceptionA.new, 404) + end + end end end \ No newline at end of file