Merge pull request #160 from alphagov/datasync-errors-new

Ignore data sync errors and allow custom `should_capture` via Delegator pattern

CHANGELOG.md

@@ -1,3 +1,7 @@
+# 2.5.0
+
+* Use delegator pattern for `GovukError.configure`, to allow custom `should_capture` (/~https://github.com/alphagov/govuk_app_config/pull/160)
+
 # 2.4.1
 
 * Bump 'sentry-raven' to 3.1.1 to improve grouping of errors (/~https://github.com/alphagov/govuk_app_config/pull/162)

README.md

@@ -89,6 +89,27 @@ GovukError.configure do |config|
 end
 ```
 
+And you can exclude errors from being reported if they occur during the nightly data sync (on integration and staging):
+
+```ruby
+GovukError.configure do |config|
+  config.data_sync_excluded_exceptions << "PG::Error"
+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`.
+
+```ruby
+GovukError.configure do |config|
+  config.should_capture = lambda do |error_or_event|
+    error_or_event == "do capture"
+  end
+end
+```
+
 `GovukError.configure` has the same options as the Sentry client, Raven. See [the Raven docs for all configuration options](https://docs.sentry.io/clients/ruby/config).
 
 ## Statsd

govuk_app_config.gemspec

@@ -25,6 +25,7 @@ Gem::Specification.new do |spec|
   spec.add_dependency "statsd-ruby", "~> 1.4.0"
   spec.add_dependency "unicorn", ">= 5.4", "< 5.8"
 
+  spec.add_development_dependency "byebug"
   spec.add_development_dependency "climate_control"
   spec.add_development_dependency "rack-test", "~> 1.1.0"
   spec.add_development_dependency "rails", "~> 6"

lib/govuk_app_config.rb

@@ -1,11 +1,11 @@
 require "govuk_app_config/version"
 require "govuk_app_config/govuk_statsd"
 require "govuk_app_config/govuk_error"
+require "govuk_app_config/govuk_error/configure"
 require "govuk_app_config/govuk_healthcheck"
 # This require is deprecated and should be removed on next major version bump
 # and should be required by applications directly.
 require "govuk_app_config/govuk_unicorn"
-require "govuk_app_config/configure"
 
 if defined?(Rails)
   require "govuk_app_config/govuk_logging"

lib/govuk_app_config/govuk_error.rb

@@ -1,5 +1,6 @@
 require "sentry-raven"
 require "govuk_app_config/govuk_statsd"
+require "govuk_app_config/govuk_error/configuration"
 
 module GovukError
   def self.notify(exception_or_message, args = {})
@@ -12,8 +13,7 @@ def self.notify(exception_or_message, args = {})
   end
 
   def self.configure
-    Raven.configure do |config|
-      yield(config)
-    end
+    @configuration ||= Configuration.new(Raven.configuration)
+    yield @configuration
   end
 end

lib/govuk_app_config/govuk_error/configuration.rb

@@ -0,0 +1,36 @@
+require "govuk_app_config/govuk_error/govuk_data_sync"
+
+module GovukError
+  class Configuration < SimpleDelegator
+    attr_reader :data_sync
+    attr_accessor :data_sync_excluded_exceptions
+
+    def initialize(_raven_configuration)
+      super
+      @data_sync = GovukDataSync.new(ENV["GOVUK_DATA_SYNC_PERIOD"])
+      self.data_sync_excluded_exceptions = []
+      self.should_capture = ignore_excluded_exceptions_in_data_sync
+    end
+
+    def should_capture=(closure)
+      combined = lambda do |error_or_event|
+        (ignore_excluded_exceptions_in_data_sync.call(error_or_event) && closure.call(error_or_event))
+      end
+
+      super(combined)
+    end
+
+  protected
+
+    def ignore_excluded_exceptions_in_data_sync
+      lambda { |error_or_event|
+        data_sync_ignored_error = data_sync_excluded_exceptions.any? do |exception_to_ignore|
+          exception_chain = Raven::Utils::ExceptionCauseChain.exception_to_array(error_or_event)
+          exception_chain.any? { |exception| exception.class.to_s == exception_to_ignore }
+        end
+
+        !(data_sync.in_progress? && data_sync_ignored_error)
+      }
+    end
+  end
+end

lib/govuk_app_config/configure.rb → lib/govuk_app_config/govuk_error/configure.rb

@@ -43,6 +43,15 @@
   # Rails will raise a ActionView::Template::Error, instead of the original error.
   config.inspect_exception_causes_for_exclusion = true
 
+  # List of exceptions to ignore if they take place during the data sync.
+  # Some errors are transient in nature, e.g. PostgreSQL databases being
+  # unavailable, and add little value. In fact, their presence can greatly
+  # increase the number of errors being sent and risk genuine errors being
+  # rate-limited by Sentry.
+  config.data_sync_excluded_exceptions = [
+    "PG::Error",
+  ]
+
   config.transport_failure_callback = proc {
     GovukStatsd.increment("error_reports_failed")
   }

lib/govuk_app_config/govuk_error/govuk_data_sync.rb

@@ -0,0 +1,50 @@
+require "time"
+
+module GovukError
+  class GovukDataSync
+    class MalformedDataSyncPeriod < RuntimeError
+      attr_reader :invalid_value
+
+      def initialize(invalid_value)
+        @invalid_value = invalid_value
+      end
+
+      def message
+        "\"#{invalid_value}\" is not a valid value (should be of form '22:00-03:00')."
+      end
+    end
+
+    attr_reader :from, :to
+
+    def initialize(govuk_data_sync_period)
+      return if govuk_data_sync_period.nil?
+
+      parts = govuk_data_sync_period.split("-")
+      raise MalformedDataSyncPeriod, govuk_data_sync_period unless parts.count == 2
+
+      @from, @to = parts.map { |time| Time.parse(time) }
+    rescue ArgumentError
+      raise MalformedDataSyncPeriod, govuk_data_sync_period
+    end
+
+    def in_progress?
+      from.present? && to.present? && in_time_range?(from, to)
+    end
+
+  private
+
+    # `from`/`to` times are in relation to the local server time, which is expected to be in UTC as per:
+    # /~https://github.com/alphagov/govuk-puppet/blob/b588e4ade996e97b8975e69cb00800521fff4a48/modules/govuk_envsys/files/etc/environment#L3
+    def in_time_range?(from, to)
+      hour_is_in_range = Time.now.hour >= from.hour || Time.now.hour <= to.hour
+      minute_is_in_range = if Time.now.hour == from.hour
+                             Time.now.min >= from.min
+                           elsif Time.now.hour == to.hour
+                             Time.now.min <= to.min
+                           else
+                             true
+                           end
+      hour_is_in_range && minute_is_in_range
+    end
+  end
+end

lib/govuk_app_config/version.rb

@@ -1,3 +1,3 @@
 module GovukAppConfig
-  VERSION = "2.4.1".freeze
+  VERSION = "2.5.0".freeze
 end

spec/govuk_error/configuration_spec.rb

@@ -0,0 +1,60 @@
+require "spec_helper"
+require "sentry-raven"
+require "govuk_app_config/govuk_error/configuration"
+
+RSpec.describe GovukError::Configuration do
+  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
+      expect(delegated_object).to receive(:some_method)
+      GovukError::Configuration.new(delegated_object).some_method
+    end
+  end
+
+  describe ".should_capture" do
+    around do |example|
+      ClimateControl.modify GOVUK_DATA_SYNC_PERIOD: "22:00-08:00" do
+        example.run
+      end
+    end
+
+    let(:configuration) { GovukError::Configuration.new(Raven.configuration) }
+
+    it "should capture errors that occur during the data sync" do
+      travel_to(Time.current.change(hour: 23)) do
+        expect(configuration.should_capture.call(StandardError.new)).to eq(true)
+      end
+    end
+
+    it "should ignore errors that have been added to data_sync_excluded_exceptions, if they occurred during the data sync" do
+      configuration.data_sync_excluded_exceptions << "StandardError"
+
+      travel_to(Time.current.change(hour: 23)) do
+        expect(configuration.should_capture.call(StandardError.new)).to eq(false)
+      end
+    end
+
+    it "should ignore exceptions whose underlying cause is an ignorable error, if it occurred during the data sync" do
+      pg_error = double("Caused by PG::Error", class: "PG::Error")
+      allow(pg_error).to receive(:cause)
+      exception = double("Exception 1", cause: double("Exception 2", cause: pg_error))
+
+      configuration.data_sync_excluded_exceptions << "PG::Error"
+      travel_to(Time.current.change(hour: 23)) do
+        expect(configuration.should_capture.call(exception)).to eq(false)
+      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
+      raven_configurator = GovukError::Configuration.new(Raven.configuration)
+      raven_configurator.should_capture = lambda do |error_or_event|
+        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)
+    end
+  end
+end

spec/govuk_error/govuk_data_sync_spec.rb

@@ -0,0 +1,55 @@
+require "spec_helper"
+require "rails"
+require "govuk_app_config/govuk_error/govuk_data_sync"
+
+RSpec.describe GovukError::GovukDataSync do
+  describe ".initialize" do
+    it "does not raise an exception if data sync time period is not defined" do
+      expect { GovukError::GovukDataSync.new(nil) }.not_to raise_error
+    end
+
+    it "returns false for `in_progress?` if data sync time period is not defined" do
+      data_sync = GovukError::GovukDataSync.new(nil)
+      24.times do |n|
+        at(hour: n) { expect(data_sync.in_progress?).to eq(false) }
+      end
+    end
+
+    it "raises an exception if data sync time period is malformed" do
+      invalid_values = [
+        "foo",
+        "22:00",
+        "10:10-10:10-10:10",
+        "3:00-fish",
+      ]
+      invalid_values.each do |val|
+        expect { GovukError::GovukDataSync.new(val) }.to raise_error(
+          GovukError::GovukDataSync::MalformedDataSyncPeriod,
+          "\"#{val}\" is not a valid value (should be of form '22:00-03:00').",
+        )
+      end
+    end
+  end
+
+  describe ".in_progress?" do
+    it "returns false if we are outside of the time range" do
+      data_sync = GovukError::GovukDataSync.new("22:30-8:30")
+      at(hour: 21) { expect(data_sync.in_progress?).to eq(false) }
+      at(hour: 22, min: 29) { expect(data_sync.in_progress?).to eq(false) }
+      at(hour: 8, min: 31) { expect(data_sync.in_progress?).to eq(false) }
+    end
+
+    it "returns true if we are within the time range" do
+      data_sync = GovukError::GovukDataSync.new("22:30-8:30")
+      at(hour: 22, min: 30) { expect(data_sync.in_progress?).to eq(true) }
+      at(hour: 0) { expect(data_sync.in_progress?).to eq(true) }
+      at(hour: 8, min: 30) { expect(data_sync.in_progress?).to eq(true) }
+    end
+  end
+
+  def at(time)
+    travel_to(Time.current.change(time)) do
+      yield
+    end
+  end
+end

spec/lib/govuk_error_spec.rb

@@ -19,4 +19,11 @@
       expect(Raven).to have_received(:capture_exception).with(StandardError.new, extra: { parameters: "Something" })
     end
   end
+
+  describe ".configure" do
+    it "configures Raven via the Configuration" do
+      expect { |b| GovukError.configure(&b) }
+        .to yield_with_args(instance_of(GovukError::Configuration))
+    end
+  end
 end

spec/spec_helper.rb

@@ -1,8 +1,12 @@
+require "active_support/testing/time_helpers"
+require "byebug"
 require "climate_control"
 require "rspec/its"
 require "webmock/rspec"
 
 RSpec.configure do |config|
+  config.include(ActiveSupport::Testing::TimeHelpers)
+
   config.expect_with :rspec do |expectations|
     expectations.include_chain_clauses_in_custom_matcher_descriptions = true
   end