Release 4.0.0.pre.4

CHANGELOG.md

@@ -1,3 +1,9 @@
+# 4.0.0.pre.4
+
+- Fix Sentry client initialisation ([#205](https://github.com/alphagov/govuk_app_config/pull/205)).
+- BREAKING: non-Rails apps will need to manually call `GovukError.configure` in order to initialise Sentry.
+- BREAKING: `GovukError.configure` can only be called once by the downstream application.
+
 # 4.0.0.pre.3
 
 - Include [sentry-rails](https://github.com/getsentry/sentry-ruby/tree/master/sentry-rails) by default ([#203](https://github.com/alphagov/govuk_app_config/pull/203)).

README.md

@@ -87,7 +87,7 @@ You can add your environment to the list of active Sentry environments like so:
 
 ```ruby
 GovukError.configure do |config|
-  config.active_sentry_environments << "my-test-environment"
+  config.enabled_environments << "my-test-environment"
 end
 ```
 

lib/govuk_app_config.rb

@@ -1,7 +1,6 @@
 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"
 require "govuk_app_config/govuk_i18n"
 # This require is deprecated and should be removed on next major version bump

lib/govuk_app_config/govuk_error.rb

@@ -5,6 +5,12 @@
 require "govuk_app_config/version"
 
 module GovukError
+  class AlreadyInitialised < StandardError
+    def initialize(msg = "You can only call GovukError.configure once!")
+      super
+    end
+  end
+
   def self.notify(exception_or_message, args = {})
     # Allow users to use `parameters` as a key like the Airbrake
     # client, allowing easy upgrades.
@@ -14,11 +20,23 @@ def self.notify(exception_or_message, args = {})
     args[:tags] ||= {}
     args[:tags][:govuk_app_config_version] = GovukAppConfig::VERSION
 
-    Sentry.capture_exception(exception_or_message, args)
+    if exception_or_message.is_a?(String)
+      Sentry.capture_message(exception_or_message, args)
+    else
+      Sentry.capture_exception(exception_or_message, args)
+    end
+  end
+
+  def self.is_configured?
+    Sentry.get_current_client != nil
   end
 
   def self.configure
-    @configuration ||= Configuration.new(Sentry::Configuration.new)
-    yield @configuration
+    raise GovukError::AlreadyInitialised if is_configured?
+
+    Sentry.init do |sentry_config|
+      config = Configuration.new(sentry_config)
+      yield config if block_given?
+    end
   end
 end

lib/govuk_app_config/govuk_error/configuration.rb

@@ -3,20 +3,82 @@
 
 module GovukError
   class Configuration < SimpleDelegator
-    attr_reader :data_sync, :sentry_environment
-    attr_accessor :active_sentry_environments, :data_sync_excluded_exceptions
+    attr_reader :data_sync
+    attr_accessor :data_sync_excluded_exceptions
 
     def initialize(_sentry_configuration)
       super
-      @sentry_environment = ENV["SENTRY_CURRENT_ENV"]
       @data_sync = GovukDataSync.new(ENV["GOVUK_DATA_SYNC_PERIOD"])
-      self.active_sentry_environments = []
-      self.data_sync_excluded_exceptions = []
+      set_up_defaults
+    end
+
+    def set_up_defaults
+      # These are the environments (described by the `SENTRY_CURRENT_ENV`
+      # ENV variable) where we want to capture Sentry errors. If
+      # `SENTRY_CURRENT_ENV` isn't in this list, or isn't defined, then
+      # don't capture the error.
+      self.enabled_environments = %w[
+        integration-blue-aws
+        staging
+        production
+      ]
+
+      self.excluded_exceptions = [
+        # Default ActionDispatch rescue responses
+        "ActionController::RoutingError",
+        "AbstractController::ActionNotFound",
+        "ActionController::MethodNotAllowed",
+        "ActionController::UnknownHttpMethod",
+        "ActionController::NotImplemented",
+        "ActionController::UnknownFormat",
+        "Mime::Type::InvalidMimeType",
+        "ActionController::MissingExactTemplate",
+        "ActionController::InvalidAuthenticityToken",
+        "ActionController::InvalidCrossOriginRequest",
+        "ActionDispatch::Http::Parameters::ParseError",
+        "ActionController::BadRequest",
+        "ActionController::ParameterMissing",
+        "Rack::QueryParser::ParameterTypeError",
+        "Rack::QueryParser::InvalidParameterError",
+        # Default ActiveRecord rescue responses
+        "ActiveRecord::RecordNotFound",
+        "ActiveRecord::StaleObjectError",
+        "ActiveRecord::RecordInvalid",
+        "ActiveRecord::RecordNotSaved",
+        # Additional items
+        "ActiveJob::DeserializationError",
+        "CGI::Session::CookieStore::TamperedWithCookie",
+        "GdsApi::HTTPIntermittentServerError",
+        "GdsApi::TimedOutException",
+        "Mongoid::Errors::DocumentNotFound",
+        "Sinatra::NotFound",
+        "Slimmer::IntermittentRetrievalError",
+      ]
+
+      # This will exclude exceptions that are triggered by one of the ignored
+      # exceptions. For example, when any exception occurs in a template,
+      # Rails will raise a ActionView::Template::Error, instead of the original error.
+      self.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.
+      self.data_sync_excluded_exceptions = [
+        "PG::Error",
+        "GdsApi::ContentStore::ItemNotFound",
+      ]
+
       @before_send_callbacks = [
-        ignore_exceptions_if_not_in_active_sentry_env,
         ignore_excluded_exceptions_in_data_sync,
         increment_govuk_statsd_counters,
       ]
+      # Need to invoke an arbitrary `before_send=` in order to trigger the
+      # `before_send_callbacks` behaviour
+      self.before_send = lambda { |error_or_event, _hint|
+        error_or_event
+      }
     end
 
     def before_send=(closure)
@@ -26,10 +88,6 @@ def before_send=(closure)
 
   protected
 
-    def ignore_exceptions_if_not_in_active_sentry_env
-      ->(event, _hint) { event if active_sentry_environments.include?(sentry_environment) }
-    end
-
     def ignore_excluded_exceptions_in_data_sync
       lambda { |event, hint|
         data_sync_ignored_error = data_sync_excluded_exceptions.any? do |exception_to_ignore|

lib/govuk_app_config/railtie.rb

@@ -3,5 +3,9 @@ class Railtie < Rails::Railtie
     config.before_initialize do
       GovukLogging.configure if Rails.env.production?
     end
+
+    config.after_initialize do
+      GovukError.configure unless GovukError.is_configured?
+    end
   end
 end

lib/govuk_app_config/version.rb

@@ -1,3 +1,3 @@
 module GovukAppConfig
-  VERSION = "4.0.0.pre.3".freeze
+  VERSION = "4.0.0.pre.4".freeze
 end

spec/govuk_error/configuration_spec.rb

@@ -3,8 +3,12 @@
 require "govuk_app_config/govuk_error/configuration"
 
 RSpec.describe GovukError::Configuration do
+  let(:dummy_dsn) { "http://12345:67890@sentry.localdomain/sentry/42" }
+
   before :each do
     stub_const("GovukStatsd", double(Module, increment: nil))
+    stub_request(:post, "http://sentry.localdomain/sentry/api/42/envelope/")
+      .to_return(status: 200)
   end
 
   describe ".initialize" do
@@ -26,23 +30,23 @@
 
     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"
+        configuration.enabled_environments << "production"
         sentry_client = send_exception_to_sentry(StandardError.new, configuration)
         expect(sentry_client.transport.events).to be_empty
       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"
+        configuration.enabled_environments << "production"
         sentry_client = send_exception_to_sentry(StandardError.new, configuration)
         expect(sentry_client.transport.events.count).to eq(1)
       end
     end
 
     it "allows string messages to be sent (rather than exceptions)" do
       ClimateControl.modify SENTRY_CURRENT_ENV: "production" do
-        configuration.active_sentry_environments << "production"
+        configuration.enabled_environments << "production"
         configuration.data_sync_excluded_exceptions << "SomeError"
         sentry_client = Sentry::Client.new(optimise_configuration_for_testing(configuration))
         sentry_hub = Sentry::Hub.new(sentry_client, Sentry::Scope.new)
@@ -55,7 +59,7 @@
     context "during the data sync" do
       around do |example|
         ClimateControl.modify SENTRY_CURRENT_ENV: "production", GOVUK_DATA_SYNC_PERIOD: "22:00-08:00" do
-          configuration.active_sentry_environments << "production"
+          configuration.enabled_environments << "production"
           travel_to(Time.current.change(hour: 23)) do
             example.run
           end
@@ -115,7 +119,7 @@
     context "outside of the data sync" do
       around do |example|
         ClimateControl.modify SENTRY_CURRENT_ENV: "production", GOVUK_DATA_SYNC_PERIOD: "22:00-08:00" do
-          configuration.active_sentry_environments << "production"
+          configuration.enabled_environments << "production"
           travel_to(Time.current.change(hour: 21)) do
             example.run
           end
@@ -133,7 +137,7 @@
     context "when the before_send lambda has not been overridden" do
       it "increments the appropriate counters" do
         ClimateControl.modify SENTRY_CURRENT_ENV: "production" do
-          configuration.active_sentry_environments << "production"
+          configuration.enabled_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")
           send_exception_to_sentry(StandardError.new, configuration)
@@ -144,7 +148,7 @@
     context "when the before_send lambda has been overridden" do
       it "increments the appropriate counters" do
         ClimateControl.modify SENTRY_CURRENT_ENV: "production" do
-          configuration.active_sentry_environments << "production"
+          configuration.enabled_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")
@@ -162,7 +166,7 @@
     context "when the before_send lambda has been overridden several times, all take effect" do
       it "increments the appropriate counters" do
         ClimateControl.modify SENTRY_CURRENT_ENV: "production" do
-          configuration.active_sentry_environments << "production"
+          configuration.enabled_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")
@@ -185,7 +189,7 @@
     context "when a message rather than an exception is sent to Sentry" do
       it "does not increment the error counters" do
         ClimateControl.modify SENTRY_CURRENT_ENV: "production" do
-          configuration.active_sentry_environments << "production"
+          configuration.enabled_environments << "production"
           sentry_client = Sentry::Client.new(optimise_configuration_for_testing(configuration))
           sentry_hub = Sentry::Hub.new(sentry_client, Sentry::Scope.new)
 
@@ -202,7 +206,7 @@
     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
         sentry_configurator = GovukError::Configuration.new(Sentry::Configuration.new)
-        sentry_configurator.active_sentry_environments << "production"
+        sentry_configurator.enabled_environments << "production"
         stub_const("CustomError", Class.new(StandardError))
         sentry_configurator.before_send = lambda do |event, hint|
           event if hint[:exception].is_a?(CustomError)
@@ -218,7 +222,7 @@
     it "does not increment the counters if the callback returns nil" do
       ClimateControl.modify SENTRY_CURRENT_ENV: "production" do
         sentry_configurator = GovukError::Configuration.new(Sentry::Configuration.new)
-        sentry_configurator.active_sentry_environments << "production"
+        sentry_configurator.enabled_environments << "production"
         sentry_configurator.before_send = lambda do |_error_or_event, _hint|
           nil
         end
@@ -241,8 +245,11 @@ def send_exception_to_sentry(event, configuration)
   def optimise_configuration_for_testing(configuration)
     # prevent the sending happening in a separate worker, which would cause async results
     configuration.background_worker_threads = 0
-    # force configuration to allow sending
-    allow(configuration).to receive(:sending_allowed?).and_return(true)
+    # allows us to debug which events have been sent to Sentry
+    # https://github.com/getsentry/sentry-ruby/blob/b9aa6ca8ad2bb1965ca58c7f8fc0dd16b5df310b/sentry-ruby/lib/sentry/transport/dummy_transport.rb#L7-L11
+    configuration.transport.transport_class = Sentry::DummyTransport
+    # required for `Sentry::Configuration`'s `valid?` method to return `true`
+    configuration.dsn = dummy_dsn
     configuration
   end
 end