Merge pull request #13141 from mkllnk/dfc-error-logging

Report DFC server errors to Bugsnag

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

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

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

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

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 }
     )