From 1c687360d18f9181fb2cf1ad1498f2bc45a0cb10 Mon Sep 17 00:00:00 2001 From: Benjamin Eskola Date: Wed, 5 May 2021 14:45:29 +0100 Subject: [PATCH] Merge should_capture into before_send --- .../govuk_error/configuration.rb | 28 +++++++++++++------ lib/govuk_app_config/govuk_error/configure.rb | 6 ---- spec/govuk_error/configuration_spec.rb | 28 +++++++++---------- 3 files changed, 33 insertions(+), 29 deletions(-) diff --git a/lib/govuk_app_config/govuk_error/configuration.rb b/lib/govuk_app_config/govuk_error/configuration.rb index 450b6885..b64c4bab 100644 --- a/lib/govuk_app_config/govuk_error/configuration.rb +++ b/lib/govuk_app_config/govuk_error/configuration.rb @@ -12,12 +12,14 @@ 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 + self.before_send = lambda do |error_or_event, hint| + (ignore_exceptions_based_on_env_and_data_sync.call(error_or_event, hint) && increment_govuk_statsd_counters.call(error_or_event, hint)) + end 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)) + def before_send=(closure) + combined = lambda do |error_or_event, hint| + (ignore_exceptions_based_on_env_and_data_sync.call(error_or_event, hint) && increment_govuk_statsd_counters.call(error_or_event, hint) && closure.call(error_or_event, hint)) end super(combined) @@ -26,18 +28,18 @@ def should_capture=(closure) 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) + lambda do |error_or_event, hint| + error_or_event if ignore_exceptions_if_not_in_active_sentry_env.call(error_or_event, hint) && + ignore_excluded_exceptions_in_data_sync.call(error_or_event, hint) end end def ignore_exceptions_if_not_in_active_sentry_env - ->(_error_or_event) { active_sentry_environments.include?(sentry_environment) } + ->(_error_or_event, _hint) { 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) @@ -51,5 +53,13 @@ def ignore_excluded_exceptions_in_data_sync !(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 end end diff --git a/lib/govuk_app_config/govuk_error/configure.rb b/lib/govuk_app_config/govuk_error/configure.rb index 8aab3aeb..23de5702 100644 --- a/lib/govuk_app_config/govuk_error/configure.rb +++ b/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` diff --git a/spec/govuk_error/configuration_spec.rb b/spec/govuk_error/configuration_spec.rb index 39fd073d..725b7354 100644 --- a/spec/govuk_error/configuration_spec.rb +++ b/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_falsy 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_falsy 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_falsy end it "should ignore errors whose underlying cause is an exception in data_sync_excluded_exceptions" do @@ -71,7 +71,7 @@ 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_falsy end it "should ignore errors that are subclasses of an exception in data_sync_excluded_exceptions" do @@ -79,7 +79,7 @@ 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_falsy end end @@ -96,22 +96,22 @@ 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 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 both return `true`, then we should capture, but if either returns `false`, then we shouldn't." 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| + raven_configurator.before_send = lambda do |error_or_event, _hint| 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_falsy end end end