Merge pull request #708 from newrelic/expected_errors

Ensure blank SSC settings don't overwrite local expected errors

lib/new_relic/agent/error_collector.rb

@@ -87,6 +87,14 @@ def expected?(ex, status_code = nil)
         @error_filter.expected?(ex, status_code)
       end
 
+      def load_error_filters
+        @error_filter.load_all
+      end
+
+      def reset_error_filters
+        @error_filter.reset
+      end
+
       # Checks the provided error against the error filter, if there
       # is an error filter
       def ignored_by_filter_proc?(error)
@@ -179,9 +187,9 @@ def increment_expected_error_count!(state, exception)
 
       def skip_notice_error?(exception, status_code = nil)
         disabled? ||
-        error_is_ignored?(exception, status_code) ||
         exception.nil? ||
-        exception_tagged_with?(EXCEPTION_TAG_IVAR, exception)
+        exception_tagged_with?(EXCEPTION_TAG_IVAR, exception) ||
+        error_is_ignored?(exception, status_code)
       end
 
       # calls a method on an object, if it responds to it - used for

lib/new_relic/agent/error_filter.rb

@@ -14,7 +14,7 @@ def initialize
       end
 
       def reset
-        @ignore_errors, @ignore_classes, @expected_classes = [], [], []
+        @ignore_classes, @expected_classes = [], []
         @ignore_messages, @expected_messages = {}, {}
         @ignore_status_codes, @expected_status_codes = [], []
       end
@@ -29,11 +29,12 @@ def load_all
       def load_from_config(setting, value = nil)
         errors = nil
         new_value = value || fetch_agent_config(setting.to_sym)
+        return if new_value.nil? || new_value.empty?
+
         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_errors, :ignore_classes
+          new_value = new_value.split(',').map!(&:strip) if new_value.is_a?(String)
+          errors = @ignore_classes = new_value
         when :ignore_messages
           errors = @ignore_messages = new_value || {}
         when :ignore_status_codes
@@ -45,22 +46,21 @@ def load_from_config(setting, value = nil)
         when :expected_status_codes
           errors = @expected_status_codes = parse_status_codes(new_value) || []
         end
-        log_filter(setting, errors)
+        log_filter(setting, errors) if errors
       end
 
       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_status_codes.include?(status_code.to_i)
       end
 
       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_status_codes.include?(status_code.to_i)
+          (@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,6 +75,8 @@ def ignore(*args)
           case errors
           when Array
             errors.each { |e| ignore(e) }
+          when Integer
+            @ignore_status_codes << errors
           when Hash
             @ignore_messages.update(errors)
             log_filter(:ignore_messages, errors)
@@ -97,6 +99,8 @@ def expect(*args)
           case errors
           when Array
             errors.each { |e| expect(e) }
+          when Integer
+            @expected_status_codes << errors
           when Hash
             @expected_messages.update(errors)
             log_filter(:expected_messages, errors)
@@ -144,6 +148,7 @@ def parse_status_codes(codes)
         code_list = codes.is_a?(String) ? codes.split(',') : codes
         result = []
         code_list.each do |code|
+          result << code && next if code.is_a?(Integer)
           m = code.match(/(\d{3})(-\d{3})?/)
           if m.nil? || m[1].nil?
             ::NewRelic::Agent.logger.warn("Invalid HTTP status code: '#{code}'; ignoring config")

test/multiverse/suites/sinatra/sinatra_error_tracing_test.rb

@@ -41,7 +41,6 @@ def test_ignores_notfound_errors_by_default
     get '/ignored_boom'
     assert_equal 404, last_response.status
     assert_match %r{Sinatra doesn(’|’)t know this ditty\.}, last_response.body
-
     errors = harvest_error_traces!
     assert_equal(0, errors.size)
   end

test/new_relic/agent/error_collector_test.rb

@@ -318,7 +318,6 @@ 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, nil).returns(false)
           assert @error_collector.skip_notice_error?(error)
         end
       end
@@ -368,14 +367,6 @@ def test_ignore_status_codes
         end
       end
 
-      def test_expected_classes
-        error = AnError.new
-        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.ignored_by_filter_proc?(nil)

test/new_relic/agent/error_filter_test.rb

@@ -47,7 +47,7 @@ def test_ignore_status_codes
       end
 
       def test_ignore_status_codes_by_array
-        with_config :'error_collector.ignore_status_codes' => ['401', '405-409', '501'] 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, 501)
@@ -73,7 +73,7 @@ def test_ignore_method
         @error_filter.reset
         refute @error_filter.ignore?(TestExceptionA.new)
 
-        @error_filter.ignore('401,405-409', ['500', '505-509'])
+        @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)
@@ -152,6 +152,14 @@ def test_expect_method
         refute @error_filter.expected?(TestExceptionA.new, 404)
       end
 
+      def test_empty_settings_do_not_overwrite_existing_settings
+        @error_filter.expect(['TestExceptionA'])
+
+        with_config 'error_collector.expected_classes' => [] do
+          @error_filter.load_all
+          assert @error_filter.expected?(TestExceptionA.new)
+        end
+      end
     end
   end
 end

test/new_relic/multiverse_helpers.rb

@@ -55,6 +55,8 @@ def setup_agent(opts = {}, &block)
     # to clean up before a test too.
     NewRelic::Agent.drop_buffered_data
 
+    NewRelic::Agent.instance.error_collector.load_error_filters
+
     trigger_agent_reconnect(opts)
   end
 
@@ -69,8 +71,8 @@ def teardown_agent
     # Clear out lingering stats we didn't transmit
     NewRelic::Agent.drop_buffered_data
 
-    # Clear the error collector's ignore_filter
-    NewRelic::Agent.instance.error_collector.instance_variable_set(:@ignore_filter, nil)
+    # Clear the error collector's error filters
+    NewRelic::Agent.instance.error_collector.reset_error_filters
 
     # Clean up any thread-local variables starting with 'newrelic'
     NewRelic::Agent::Tracer.clear_state