From b353536fbaeeca68925409a6ba91db0d24f3fdc7 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 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This is necessary because `should_capture` is deprecated, but requires slight behaviour changes in order to meet the expectations of both — `should_capture` returns a boolean, but `before_send` returns nil if the error is to be dropped; we want to support both the previous `should_capture` behaviour (filtering) and the existing `before_send` usecase of incrementing statsd counters. In the process we also want to ensure that newly-added callbacks are all run in order, that the statsd counters are incremented once and only once, and that a callback returning nil prevents the final callback from incrementing the counters. `before_send` filtering behaviour is documented at https://docs.sentry.io/platforms/ruby/configuration/filtering/ --- CHANGELOG.md | 4 + .../govuk_error/configuration.rb | 49 +++++---- lib/govuk_app_config/govuk_error/configure.rb | 6 -- lib/govuk_app_config/version.rb | 2 +- spec/govuk_error/configuration_spec.rb | 100 +++++++++++++++--- 5 files changed, 121 insertions(+), 40 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2c7845a4..898a39d3 100644 --- a/CHANGELOG.md +++ b/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)) diff --git a/lib/govuk_app_config/govuk_error/configuration.rb b/lib/govuk_app_config/govuk_error/configuration.rb index 450b6885..c23b8517 100644 --- a/lib/govuk_app_config/govuk_error/configuration.rb +++ b/lib/govuk_app_config/govuk_error/configuration.rb @@ -12,32 +12,26 @@ 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) @@ -45,11 +39,30 @@ def ignore_excluded_exceptions_in_data_sync 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 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/lib/govuk_app_config/version.rb b/lib/govuk_app_config/version.rb index 046ab67f..34fc43c6 100644 --- a/lib/govuk_app_config/version.rb +++ b/lib/govuk_app_config/version.rb @@ -1,3 +1,3 @@ module GovukAppConfig - VERSION = "3.0.0".freeze + VERSION = "3.1.0".freeze end diff --git a/spec/govuk_error/configuration_spec.rb b/spec/govuk_error/configuration_spec.rb index 39fd073d..035aec3d 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_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,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_nil 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_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