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