From 04fd2912a49e1fc4bf542dd3c4406565286ae4ff Mon Sep 17 00:00:00 2001 From: ChrisBAshton Date: Fri, 21 May 2021 08:25:25 +0100 Subject: [PATCH 1/4] Allow configuration_spec.rb to be run in isolation Running `bundle exec rspec spec/govuk_error/configuration_spec.rb` resulted in errors where `GovukStatsd` wasn't defined. I've now moved this stub to the top of the test file and indicated that it has a `increment` method. I've also fixed the return value of `nil` in configuration.rb, since this value is only used on line 45 as a boolean, so it's good that we return an explicit `false`. (The return value of `nil` is what we want from the _lambda_ return values, but not this internal bit of logic). --- lib/govuk_app_config/govuk_error/configuration.rb | 2 +- spec/govuk_error/configuration_spec.rb | 7 ++++--- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/lib/govuk_app_config/govuk_error/configuration.rb b/lib/govuk_app_config/govuk_error/configuration.rb index c23b8517..35789fb6 100644 --- a/lib/govuk_app_config/govuk_error/configuration.rb +++ b/lib/govuk_app_config/govuk_error/configuration.rb @@ -39,7 +39,7 @@ 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 - nil + false end error_or_event unless data_sync.in_progress? && data_sync_ignored_error diff --git a/spec/govuk_error/configuration_spec.rb b/spec/govuk_error/configuration_spec.rb index 035aec3d..441de639 100644 --- a/spec/govuk_error/configuration_spec.rb +++ b/spec/govuk_error/configuration_spec.rb @@ -3,6 +3,10 @@ require "govuk_app_config/govuk_error/configuration" RSpec.describe GovukError::Configuration do + before :each do + stub_const("GovukStatsd", double(Module, increment: nil)) + end + describe ".initialize" do it "delegates to the passed object if it doesn't have the method defined" do delegated_object = double("Raven.configuration").as_null_object @@ -101,7 +105,6 @@ 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" @@ -113,7 +116,6 @@ 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" @@ -132,7 +134,6 @@ 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" From df6a3cb3535d88260524ec69f09cbc50282603a1 Mon Sep 17 00:00:00 2001 From: ChrisBAshton Date: Fri, 21 May 2021 08:36:10 +0100 Subject: [PATCH 2/4] Update README should_capture -> before_send This was changed in #196. --- README.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/README.md b/README.md index 2bc8c5d9..36167715 100644 --- a/README.md +++ b/README.md @@ -112,12 +112,12 @@ end Finally, you can pass your own callback to evaluate whether or not to capture the exception. Note that if an exception is on the `excluded_exceptions` list, or on the `data_sync_excluded_exceptions` and occurs at the time of a data sync, then it will be excluded even if the custom -`should_capture` callback returns `true`. +`before_send` callback doesn't return `nil`. ```ruby GovukError.configure do |config| - config.should_capture = lambda do |error_or_event| - error_or_event == "do capture" + config.before_send = lambda do |error_or_event| + error_or_event == "do capture" ? error_or_event : nil end end ``` From 2616f571fdb28d1ee78c8983b257c16fc64a4994 Mon Sep 17 00:00:00 2001 From: ChrisBAshton Date: Fri, 21 May 2021 10:08:47 +0100 Subject: [PATCH 3/4] Fix `before_send` lambdas not being called In #196, we changed `should_capture` to `before_send` in preparation for [the removal of the `should_capture` method](/~https://github.com/getsentry/sentry-ruby/blob/master/MIGRATION.md#removed) in sentry-ruby. Because we already had some `before_send` logic, and it wasn't idempotent (i.e. it must only be evaluated once, as it increments counters and we want to avoid double-counting), we had to re-architect how the default behaviour is merged with any custom behaviour the downstream developer may set. Since #196 was merged, we've seen errors occuring in Sentry, which should be being ignored via the datasync logic. This suggests the lambdas are not being evaluated. I believe the removal of `before_send=` in configure.rb in /~https://github.com/alphagov/govuk_app_config/pull/196/files#diff-5617fa12f2b7ce8393099a22ed6ae89a20e286273e17190f2bf9ec48ef5b46a3L2 has meant some of our setup code would not be reached, unless the downstream app calls` before_send=` (and none of them do so far). I also think the override of the `before_send =` on `super` may be causing problems. I've reverted to the previous architecture, whilst keeping the new method names, to use an invocation of `super` that we know was once working. As a caveat, we've had to pass a harmless lambda in configure.rb, to ensure the necessary setup code is called. --- lib/govuk_app_config/govuk_error/configuration.rb | 2 +- lib/govuk_app_config/govuk_error/configure.rb | 4 ++++ spec/govuk_error/configuration_spec.rb | 8 +++++++- 3 files changed, 12 insertions(+), 2 deletions(-) diff --git a/lib/govuk_app_config/govuk_error/configuration.rb b/lib/govuk_app_config/govuk_error/configuration.rb index 35789fb6..2a6ba502 100644 --- a/lib/govuk_app_config/govuk_error/configuration.rb +++ b/lib/govuk_app_config/govuk_error/configuration.rb @@ -17,11 +17,11 @@ def initialize(_raven_configuration) ignore_excluded_exceptions_in_data_sync, increment_govuk_statsd_counters, ] - super.before_send = run_before_send_callbacks end def before_send=(closure) @before_send_callbacks.insert(-2, closure) + super(run_before_send_callbacks) end protected diff --git a/lib/govuk_app_config/govuk_error/configure.rb b/lib/govuk_app_config/govuk_error/configure.rb index 23de5702..5b12e8c2 100644 --- a/lib/govuk_app_config/govuk_error/configure.rb +++ b/lib/govuk_app_config/govuk_error/configure.rb @@ -58,6 +58,10 @@ "GdsApi::ContentStore::ItemNotFound", ] + config.before_send = lambda { |error_or_event, _hint| + error_or_event + } + config.transport_failure_callback = proc { GovukStatsd.increment("error_reports_failed") } diff --git a/spec/govuk_error/configuration_spec.rb b/spec/govuk_error/configuration_spec.rb index 441de639..e61bc438 100644 --- a/spec/govuk_error/configuration_spec.rb +++ b/spec/govuk_error/configuration_spec.rb @@ -16,7 +16,13 @@ end describe ".before_send" do - let(:configuration) { GovukError::Configuration.new(Raven.configuration) } + let(:configuration) do + configuration = GovukError::Configuration.new(Raven.configuration) + configuration.before_send = lambda { |error_or_event, _hint| + error_or_event + } + configuration + end it "ignores errors if they happen in an environment we don't care about" do ClimateControl.modify SENTRY_CURRENT_ENV: "some-temporary-environment" do From d0562b81e8eb184346e320ad3ce7f9138a3908ee Mon Sep 17 00:00:00 2001 From: ChrisBAshton Date: Fri, 21 May 2021 10:21:20 +0100 Subject: [PATCH 4/4] Bump to 3.1.1 --- CHANGELOG.md | 4 ++++ lib/govuk_app_config/version.rb | 2 +- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 898a39d3..baa87e47 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,7 @@ +# 3.1.1 + +- Fix the new before_send behaviour & tests, and add documentation ([#197](/~https://github.com/alphagov/govuk_app_config/pull/197)) + # 3.1.0 - Remove support for `should_capture` callbacks in favour of `before_send` ([#196](/~https://github.com/alphagov/govuk_app_config/pull/196)) diff --git a/lib/govuk_app_config/version.rb b/lib/govuk_app_config/version.rb index 34fc43c6..22c75635 100644 --- a/lib/govuk_app_config/version.rb +++ b/lib/govuk_app_config/version.rb @@ -1,3 +1,3 @@ module GovukAppConfig - VERSION = "3.1.0".freeze + VERSION = "3.1.1".freeze end