From 945ad98a2288b4645ad2dab86fb250d5513e0cec Mon Sep 17 00:00:00 2001 From: ChrisBAshton Date: Tue, 8 Jun 2021 15:09:30 +0100 Subject: [PATCH] Replace `active_sentry_environments` with `enabled_environments` The custom option `active_sentry_environments` was added in https://github.com/alphagov/govuk_app_config/pull/168. However, it turns out there is already a native option for this: `enabled_environments`. See https://docs.sentry.io/platforms/ruby/configuration/options/ This allows us to simplify our code a fair bit. I've also removed the stubbed `sending_allowed?` method as it was interfering with some tests and giving a false sense of security. We're now providing all of the necessary configuration for the `Sentry::Configuration` instance to return `true` on its `valid?` method, so no need for the hack. Each config option is documented in an inline comment as otherwise it can be quite obtuse. --- README.md | 2 +- .../govuk_error/configuration.rb | 11 ++----- lib/govuk_app_config/govuk_error/configure.rb | 2 +- spec/govuk_error/configuration_spec.rb | 33 +++++++++++-------- 4 files changed, 24 insertions(+), 24 deletions(-) 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/govuk_error/configuration.rb b/lib/govuk_app_config/govuk_error/configuration.rb index 30e54615..60c5fd30 100644 --- a/lib/govuk_app_config/govuk_error/configuration.rb +++ b/lib/govuk_app_config/govuk_error/configuration.rb @@ -3,17 +3,14 @@ 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 = [] @before_send_callbacks = [ - ignore_exceptions_if_not_in_active_sentry_env, ignore_excluded_exceptions_in_data_sync, increment_govuk_statsd_counters, ] @@ -26,10 +23,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 index df5fa737..f7895a1e 100644 --- a/lib/govuk_app_config/govuk_error/configure.rb +++ b/lib/govuk_app_config/govuk_error/configure.rb @@ -3,7 +3,7 @@ # 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[ + config.enabled_environments = %w[ integration-blue-aws staging production diff --git a/spec/govuk_error/configuration_spec.rb b/spec/govuk_error/configuration_spec.rb index 39ac8dec..9f78a8b3 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, body: "", headers: {}) 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