diff --git a/CHANGELOG.md b/CHANGELOG.md index 100254e4..d0253d02 100644 --- a/CHANGELOG.md +++ b/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)). diff --git a/README.md b/README.md index e8f8e10a..408ad794 100644 --- a/README.md +++ b/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 ``` diff --git a/lib/govuk_app_config.rb b/lib/govuk_app_config.rb index 6c8939ec..4ee13076 100644 --- a/lib/govuk_app_config.rb +++ b/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 diff --git a/lib/govuk_app_config/govuk_error.rb b/lib/govuk_app_config/govuk_error.rb index 448e6bb5..8b8c6f39 100644 --- a/lib/govuk_app_config/govuk_error.rb +++ b/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 diff --git a/lib/govuk_app_config/govuk_error/configuration.rb b/lib/govuk_app_config/govuk_error/configuration.rb index 30e54615..eafeb133 100644 --- a/lib/govuk_app_config/govuk_error/configuration.rb +++ b/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| diff --git a/lib/govuk_app_config/govuk_error/configure.rb b/lib/govuk_app_config/govuk_error/configure.rb deleted file mode 100644 index df5fa737..00000000 --- a/lib/govuk_app_config/govuk_error/configure.rb +++ /dev/null @@ -1,62 +0,0 @@ -GovukError.configure do |config| - # 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. - config.active_sentry_environments = %w[ - integration-blue-aws - staging - production - ] - - config.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. - 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", - "GdsApi::ContentStore::ItemNotFound", - ] - - config.before_send = lambda { |error_or_event, _hint| - error_or_event - } -end diff --git a/lib/govuk_app_config/railtie.rb b/lib/govuk_app_config/railtie.rb index 6d74f706..16b2dbde 100644 --- a/lib/govuk_app_config/railtie.rb +++ b/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 diff --git a/lib/govuk_app_config/version.rb b/lib/govuk_app_config/version.rb index 5b88ab24..617cde6b 100644 --- a/lib/govuk_app_config/version.rb +++ b/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 diff --git a/spec/govuk_error/configuration_spec.rb b/spec/govuk_error/configuration_spec.rb index 39ac8dec..ba68ca82 100644 --- a/spec/govuk_error/configuration_spec.rb +++ b/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,7 +30,7 @@ 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 @@ -34,7 +38,7 @@ 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 @@ -42,7 +46,7 @@ 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 diff --git a/spec/govuk_error/configure_spec.rb b/spec/govuk_error/configure_spec.rb deleted file mode 100644 index 5495c479..00000000 --- a/spec/govuk_error/configure_spec.rb +++ /dev/null @@ -1,9 +0,0 @@ -require "spec_helper" -require "sentry-ruby" -require "govuk_app_config/govuk_error" - -RSpec.describe "GovukError.configure" do - it "should contain only valid Sentry config" do - require "govuk_app_config/govuk_error/configure" - end -end diff --git a/spec/lib/govuk_error_spec.rb b/spec/lib/govuk_error_spec.rb index 2e1dbcd6..ad2f4b89 100644 --- a/spec/lib/govuk_error_spec.rb +++ b/spec/lib/govuk_error_spec.rb @@ -11,6 +11,14 @@ expect(Sentry).to have_received(:capture_exception) end + it "forwards the string" do + allow(Sentry).to receive(:capture_message) + + GovukError.notify("Foo") + + expect(Sentry).to have_received(:capture_message) + end + it "allows Airbrake-style parameters" do allow(Sentry).to receive(:capture_exception) @@ -29,9 +37,26 @@ end describe ".configure" do - it "configures Sentry via the Configuration" do + it "configures Sentry via the Configuration, and raises exception for subsequent calls" do expect { |b| GovukError.configure(&b) } .to yield_with_args(instance_of(GovukError::Configuration)) + + expect { GovukError.configure { |_config| } } + .to raise_exception(GovukError::AlreadyInitialised) + end + end + + describe ".is_configured?" do + it "returns false if not configured" do + allow(Sentry).to receive(:get_current_client).and_return(nil) + + expect(GovukError.is_configured?).to eq(false) + end + + it "returns true if configured" do + allow(Sentry).to receive(:get_current_client).and_return(double("Sentry::Client")) + + expect(GovukError.is_configured?).to eq(true) end end end