Merge should_capture into before_send

CHANGELOG.md

@@ -1,3 +1,7 @@
+# 3.1.0
+
+- Remove support for `should_capture` callbacks in favour of `before_send` ([#196](https://github.com/alphagov/govuk_app_config/pull/196))
+
 # 3.0.0
 
 * BREAKING: Implement RFC 141 - remove unsuitable healthchecks and return a 500 on healthcheck failure ([#193](https://github.com/alphagov/govuk_app_config/pull/193))

lib/govuk_app_config/govuk_error/configuration.rb

@@ -12,44 +12,57 @@ def initialize(_raven_configuration)
       @data_sync = GovukDataSync.new(ENV["GOVUK_DATA_SYNC_PERIOD"])
       self.active_sentry_environments = []
       self.data_sync_excluded_exceptions = []
-      self.should_capture = ignore_exceptions_based_on_env_and_data_sync
+      @before_send_callbacks = [
+        ignore_exceptions_if_not_in_active_sentry_env,
+        ignore_excluded_exceptions_in_data_sync,
+        increment_govuk_statsd_counters,
+      ]
+      super.before_send = run_before_send_callbacks
     end
 
-    def should_capture=(closure)
-      combined = lambda do |error_or_event|
-        (ignore_exceptions_based_on_env_and_data_sync.call(error_or_event) && closure.call(error_or_event))
-      end
-
-      super(combined)
+    def before_send=(closure)
+      @before_send_callbacks.insert(-2, closure)
     end
 
   protected
 
-    def ignore_exceptions_based_on_env_and_data_sync
-      lambda do |error_or_event|
-        ignore_exceptions_if_not_in_active_sentry_env.call(error_or_event) &&
-          ignore_excluded_exceptions_in_data_sync.call(error_or_event)
-      end
-    end
-
     def ignore_exceptions_if_not_in_active_sentry_env
-      ->(_error_or_event) { active_sentry_environments.include?(sentry_environment) }
+      ->(error_or_event, _hint) { error_or_event if active_sentry_environments.include?(sentry_environment) }
     end
 
     def ignore_excluded_exceptions_in_data_sync
-      lambda { |error_or_event|
+      lambda { |error_or_event, _hint|
         data_sync_ignored_error = data_sync_excluded_exceptions.any? do |exception_to_ignore|
           exception_to_ignore = Object.const_get(exception_to_ignore) unless exception_to_ignore.is_a?(Module)
           exception_chain = Raven::Utils::ExceptionCauseChain.exception_to_array(error_or_event)
           exception_chain.any? { |exception| exception.is_a?(exception_to_ignore) }
         rescue NameError
           # the exception type represented by the exception_to_ignore string
           # doesn't even exist in this environment, so won't be found in the chain
-          false
+          nil
         end
 
-        !(data_sync.in_progress? && data_sync_ignored_error)
+        error_or_event unless data_sync.in_progress? && data_sync_ignored_error
+      }
+    end
+
+    def increment_govuk_statsd_counters
+      lambda { |error_or_event, _hint|
+        GovukStatsd.increment("errors_occurred")
+        GovukStatsd.increment("error_types.#{error_or_event.class.name.demodulize.underscore}")
+        error_or_event
       }
     end
+
+    def run_before_send_callbacks
+      lambda do |error_or_event, hint|
+        result = error_or_event
+        @before_send_callbacks.each do |callback|
+          result = callback.call(error_or_event, hint)
+          break if result.nil?
+        end
+        result
+      end
+    end
   end
 end

lib/govuk_app_config/govuk_error/configure.rb

@@ -1,10 +1,4 @@
 GovukError.configure do |config|
-  config.before_send = proc { |e|
-    GovukStatsd.increment("errors_occurred")
-    GovukStatsd.increment("error_types.#{e.class.name.demodulize.underscore}")
-    e
-  }
-
   config.silence_ready = !Rails.env.production? if defined?(Rails)
 
   # These are the environments (described by the `SENTRY_CURRENT_ENV`

lib/govuk_app_config/version.rb

@@ -1,3 +1,3 @@
 module GovukAppConfig
-  VERSION = "3.0.0".freeze
+  VERSION = "3.1.0".freeze
 end

spec/govuk_error/configuration_spec.rb

@@ -11,20 +11,20 @@
     end
   end
 
-  describe ".should_capture" do
+  describe ".before_send" do
     let(:configuration) { GovukError::Configuration.new(Raven.configuration) }
 
     it "ignores errors if they happen in an environment we don't care about" do
       ClimateControl.modify SENTRY_CURRENT_ENV: "some-temporary-environment" do
         configuration.active_sentry_environments << "production"
-        expect(configuration.should_capture.call(StandardError.new)).to eq(false)
+        expect(configuration.before_send.call(StandardError.new)).to be_nil
       end
     end
 
     it "captures errors if they happen in an environment we care about" do
       ClimateControl.modify SENTRY_CURRENT_ENV: "production" do
         configuration.active_sentry_environments << "production"
-        expect(configuration.should_capture.call(StandardError.new)).to eq(true)
+        expect(configuration.before_send.call(StandardError.new)).to be_truthy
       end
     end
 
@@ -39,19 +39,19 @@
       end
 
       it "should capture errors by default" do
-        expect(configuration.should_capture.call(StandardError.new)).to eq(true)
+        expect(configuration.before_send.call(StandardError.new)).to be_truthy
       end
 
       it "should ignore errors that have been added as a string to data_sync_excluded_exceptions" do
         configuration.data_sync_excluded_exceptions << "StandardError"
 
-        expect(configuration.should_capture.call(StandardError.new)).to eq(false)
+        expect(configuration.before_send.call(StandardError.new)).to be_nil
       end
 
       it "should ignore errors that have been added as a class to data_sync_excluded_exceptions" do
         configuration.data_sync_excluded_exceptions << StandardError
 
-        expect(configuration.should_capture.call(StandardError.new)).to eq(false)
+        expect(configuration.before_send.call(StandardError.new)).to be_nil
       end
 
       it "should ignore errors whose underlying cause is an exception in data_sync_excluded_exceptions" do
@@ -71,15 +71,15 @@
         end
 
         expect(chained_exception).to be_an_instance_of(SomeOtherError)
-        expect(configuration.should_capture.call(chained_exception)).to eq(false)
+        expect(configuration.before_send.call(chained_exception)).to be_nil
       end
 
       it "should ignore errors that are subclasses of an exception in data_sync_excluded_exceptions" do
         stub_const("SomeClass", Class.new(StandardError))
         stub_const("SomeInheritedClass", Class.new(SomeClass))
 
         configuration.data_sync_excluded_exceptions << "SomeClass"
-        expect(configuration.should_capture.call(SomeInheritedClass.new)).to eq(false)
+        expect(configuration.before_send.call(SomeInheritedClass.new)).to be_nil
       end
     end
 
@@ -96,22 +96,92 @@
       it "should capture errors even if they are in the list of data_sync_excluded_exceptions" do
         configuration.data_sync_excluded_exceptions << "StandardError"
 
-        expect(configuration.should_capture.call(StandardError.new)).to eq(true)
+        expect(configuration.before_send.call(StandardError.new)).to be_truthy
+      end
+    end
+
+    context "when the before_send lambda has not been overridden" do
+      before { stub_const("GovukStatsd", double(Module)) }
+      it "increments the appropriate counters" do
+        ClimateControl.modify SENTRY_CURRENT_ENV: "production" do
+          configuration.active_sentry_environments << "production"
+          expect(GovukStatsd).to receive(:increment).exactly(1).times.with("errors_occurred")
+          expect(GovukStatsd).to receive(:increment).exactly(1).times.with("error_types.standard_error")
+          configuration.before_send.call(StandardError.new)
+        end
+      end
+    end
+
+    context "when the before_send lambda has been overridden" do
+      before { stub_const("GovukStatsd", double(Module)) }
+      it "increments the appropriate counters" do
+        ClimateControl.modify SENTRY_CURRENT_ENV: "production" do
+          configuration.active_sentry_environments << "production"
+          expect(GovukStatsd).to receive(:increment).exactly(1).times.with("errors_occurred")
+          expect(GovukStatsd).to receive(:increment).exactly(1).times.with("error_types.standard_error")
+          expect(GovukStatsd).to receive(:increment).exactly(1).times.with("hello_world")
+
+          configuration.before_send = lambda do |error_or_event, _hint|
+            GovukStatsd.increment("hello_world")
+            error_or_event
+          end
+
+          configuration.before_send.call(StandardError.new)
+        end
+      end
+    end
+
+    context "when the before_send lambda has been overridden several times, all take effect" do
+      before { stub_const("GovukStatsd", double(Module)) }
+      it "increments the appropriate counters" do
+        ClimateControl.modify SENTRY_CURRENT_ENV: "production" do
+          configuration.active_sentry_environments << "production"
+          expect(GovukStatsd).to receive(:increment).exactly(1).times.with("errors_occurred")
+          expect(GovukStatsd).to receive(:increment).exactly(1).times.with("error_types.standard_error")
+          expect(GovukStatsd).to receive(:increment).exactly(1).times.with("hello_world")
+          expect(GovukStatsd).to receive(:increment).exactly(1).times.with("hello_world_again")
+
+          configuration.before_send = lambda do |error_or_event, _hint|
+            GovukStatsd.increment("hello_world")
+            error_or_event
+          end
+          configuration.before_send = lambda do |error_or_event, _hint|
+            GovukStatsd.increment("hello_world_again")
+            error_or_event
+          end
+
+          configuration.before_send.call(StandardError.new)
+        end
       end
     end
   end
 
-  describe ".should_capture=" do
-    it "Allows apps to add their own `should_capture` callback, that is evaluated alongside the default. If both return `true`, then we should capture, but if either returns `false`, then we shouldn't." do
+  describe ".before_send=" do
+    it "Allows apps to add their own `before_send` callback, that is evaluated alongside the default. If all return their parameter, then the chain continues, but if any returns `nil`, then it ends and the error is dropped" do
       ClimateControl.modify SENTRY_CURRENT_ENV: "production" do
         raven_configurator = GovukError::Configuration.new(Raven.configuration)
         raven_configurator.active_sentry_environments << "production"
-        raven_configurator.should_capture = lambda do |error_or_event|
-          error_or_event == "do capture"
+        raven_configurator.before_send = lambda do |error_or_event, _hint|
+          error_or_event if error_or_event == "do capture"
         end
 
-        expect(raven_configurator.should_capture.call("do capture")).to eq(true)
-        expect(raven_configurator.should_capture.call("don't capture")).to eq(false)
+        expect(raven_configurator.before_send.call("do capture")).to be_truthy
+        expect(raven_configurator.before_send.call("don't capture", {})).to be_nil
+      end
+    end
+
+    it "does not increment the counters if the callback returns nil" do
+      ClimateControl.modify SENTRY_CURRENT_ENV: "production" do
+        raven_configurator = GovukError::Configuration.new(Raven.configuration)
+        raven_configurator.active_sentry_environments << "production"
+        raven_configurator.before_send = lambda do |_error_or_event, _hint|
+          nil
+        end
+
+        expect(GovukStatsd).not_to receive(:increment).with("errors_occurred")
+        expect(GovukStatsd).not_to receive(:increment).with("error_types.standard_error")
+
+        raven_configurator.before_send.call(StandardError.new)
       end
     end
   end