diff --git a/app/services/alert.rb b/app/services/alert.rb index d030554298..a8ce1fbec0 100644 --- a/app/services/alert.rb +++ b/app/services/alert.rb @@ -22,9 +22,16 @@ class Alert # ) def self.raise(error, metadata = {}, &block) Bugsnag.notify(error) do |payload| + unless metadata.respond_to?(:each) + metadata = { metadata: { data: metadata } } + end + metadata.each do |name, data| + # Bugsnag only reports metadata when given a Hash. + data = { data: } unless data.is_a?(Hash) payload.add_metadata(name, data) end + block.call(payload) end end diff --git a/config/initializers/bugsnag.rb b/config/initializers/bugsnag.rb index 572681f1db..2eb0ddbe2a 100644 --- a/config/initializers/bugsnag.rb +++ b/config/initializers/bugsnag.rb @@ -1,6 +1,7 @@ Bugsnag.configure do |config| config.api_key = ENV['BUGSNAG_API_KEY'] - config.release_stage = Rails.env + config.app_version = Rails.application.config.x.git_version + # Avoid missing API key warning without changing the Rails log level. if Rails.env.development? config.logger = Logger.new(STDOUT) @@ -10,5 +11,5 @@ Bugsnag.configure do |config| # If you want to notify Bugsnag in dev or test then set the env var: # spring stop # BUGSNAG=true ./bin/rails console - config.notify_release_stages = %w(production staging) unless ENV["BUGSNAG"] + config.enabled_release_stages = %w(production staging) unless ENV["BUGSNAG"] end diff --git a/engines/dfc_provider/app/services/dfc_request.rb b/engines/dfc_provider/app/services/dfc_request.rb index c976a69787..541505f8e3 100644 --- a/engines/dfc_provider/app/services/dfc_request.rb +++ b/engines/dfc_provider/app/services/dfc_request.rb @@ -22,6 +22,9 @@ class DfcRequest # If access was denied and our token is stale then refresh and retry: refresh_access_token! response = request(url, data, method:) + rescue Faraday::ServerError => e + Alert.raise(e, { dfc_request: { data: } }) + raise end response.body diff --git a/engines/dfc_provider/spec/services/dfc_request_spec.rb b/engines/dfc_provider/spec/services/dfc_request_spec.rb index 0e6fbfb8d3..173367f410 100644 --- a/engines/dfc_provider/spec/services/dfc_request_spec.rb +++ b/engines/dfc_provider/spec/services/dfc_request_spec.rb @@ -38,15 +38,10 @@ RSpec.describe DfcRequest do account.refresh_token = ENV.fetch("OPENID_REFRESH_TOKEN") account.updated_at = 1.day.ago - expect { - api.call("http://example.net/api") - } + expect { api.call("http://example.net/api") } .to raise_error(Faraday::UnauthorizedError) - .and change { - account.token - }.and change { - account.refresh_token - } + .and change { account.token } + .and change { account.refresh_token } end it "doesn't try to refresh the token when it's still fresh" do @@ -86,4 +81,13 @@ RSpec.describe DfcRequest do products = graph.select { |s| s.semanticType == "dfc-b:SuppliedProduct" } expect(products).to be_present end + + it "reports and raises server errors" do + stub_request(:get, "http://example.net/api").to_return(status: 500) + + expect(Bugsnag).to receive(:notify) + + expect { api.call("http://example.net/api") } + .to raise_error(Faraday::ServerError) + end end diff --git a/spec/services/alert_spec.rb b/spec/services/alert_spec.rb index 2e8723fd8f..c899228e6a 100644 --- a/spec/services/alert_spec.rb +++ b/spec/services/alert_spec.rb @@ -7,16 +7,16 @@ RSpec.describe Alert do original_config = nil Bugsnag.configure do |config| original_config = config.dup - config.api_key ||= "dummy-key" - config.notify_release_stages = ["test"] + config.api_key ||= "00000000000000000000000000000000" + config.enabled_release_stages = ["test"] config.delivery_method = :synchronous end example.run Bugsnag.configure do |config| - config.api_key ||= original_config.api_key - config.notify_release_stages = original_config.notify_release_stages + config.api_key = original_config.api_key + config.enabled_release_stages = original_config.notify_release_stages config.delivery_method = original_config.delivery_method end end @@ -28,8 +28,6 @@ RSpec.describe Alert do end it "adds context" do - pending "Bugsnag calls in CI" if ENV.fetch("CI", false) - expect_any_instance_of(Bugsnag::Report).to receive(:add_metadata).with( :order, { number: "ABC123" } ) @@ -43,9 +41,23 @@ RSpec.describe Alert do ) end - it "is compatible with Bugsnag API" do - pending "Bugsnag calls in CI" if ENV.fetch("CI", false) + it "adds context given as keyword argument" do + expect_any_instance_of(Bugsnag::Report).to receive(:add_metadata).with( + :thing, { data: "ABC123" } + ) + Alert.raise("hey", thing: "ABC123") + end + + it "adds simple values as context" do + expect_any_instance_of(Bugsnag::Report).to receive(:add_metadata).with( + :metadata, { data: "ABC123" } + ) + + Alert.raise("hey", "ABC123") + end + + it "is compatible with Bugsnag API" do expect_any_instance_of(Bugsnag::Report).to receive(:add_metadata).with( :order, { number: "ABC123" } ) @@ -56,8 +68,6 @@ RSpec.describe Alert do end it "sends ActiveRecord objects" do - pending "Bugsnag calls in CI" if ENV.fetch("CI", false) - order = Spree::Order.new(number: "ABC123") expect_any_instance_of(Bugsnag::Report).to receive(:add_metadata).with( @@ -68,8 +78,6 @@ RSpec.describe Alert do end it "notifies Bugsnag when ActiveRecord object is missing" do - pending "Bugsnag calls in CI" if ENV.fetch("CI", false) - expect_any_instance_of(Bugsnag::Report).to receive(:add_metadata).with( "NilClass", { record_was_nil: true } )