From e463ca35f9fb2c45dd5eea972776856d4fda2018 Mon Sep 17 00:00:00 2001 From: Toby Hsieh Date: Wed, 14 Nov 2018 11:41:59 -0800 Subject: [PATCH] refactor: Ignore SignalException, consolidating ignore logic (#479) * Add SignalException to default ignore_classes With #397 we receive SignalException error reports when puma shuts down, which aren't really errors. * Remove exit exception checks in integrations These are now in the default ignore_classes. * Improve IgnoredExceptions: Remove exception rejection from sidekiq integration --- lib/bugsnag/configuration.rb | 6 ++--- lib/bugsnag/integrations/mailman.rb | 1 - lib/bugsnag/integrations/shoryuken.rb | 14 +++++----- lib/bugsnag/integrations/sidekiq.rb | 1 - spec/configuration_spec.rb | 2 +- spec/report_spec.rb | 39 ++++++++++++++++----------- 6 files changed, 34 insertions(+), 29 deletions(-) diff --git a/lib/bugsnag/configuration.rb b/lib/bugsnag/configuration.rb index fee5319e5..46f4b8485 100644 --- a/lib/bugsnag/configuration.rb +++ b/lib/bugsnag/configuration.rb @@ -69,9 +69,9 @@ def initialize self.auto_capture_sessions = false self.session_endpoint = DEFAULT_SESSION_ENDPOINT - # SystemExit and Interrupt are common Exception types seen with successful - # exits and are not automatically reported to Bugsnag - self.ignore_classes = Set.new([SystemExit, Interrupt]) + # SystemExit and SignalException are common Exception types seen with + # successful exits and are not automatically reported to Bugsnag + self.ignore_classes = Set.new([SystemExit, SignalException]) # Read the API key from the environment self.api_key = ENV["BUGSNAG_API_KEY"] diff --git a/lib/bugsnag/integrations/mailman.rb b/lib/bugsnag/integrations/mailman.rb index a5a84a2eb..d49530fcd 100644 --- a/lib/bugsnag/integrations/mailman.rb +++ b/lib/bugsnag/integrations/mailman.rb @@ -21,7 +21,6 @@ def call(mail) Bugsnag.configuration.set_request_data :mailman_msg, mail.to_s yield rescue Exception => ex - raise ex if [Interrupt, SystemExit, SignalException].include? ex.class Bugsnag.notify(ex, true) do |report| report.severity = "error" report.severity_reason = { diff --git a/lib/bugsnag/integrations/shoryuken.rb b/lib/bugsnag/integrations/shoryuken.rb index 2e01cfdbb..052ad3498 100644 --- a/lib/bugsnag/integrations/shoryuken.rb +++ b/lib/bugsnag/integrations/shoryuken.rb @@ -27,14 +27,12 @@ def call(_, queue, _, body) yield rescue Exception => ex - unless [Interrupt, SystemExit, SignalException].include?(ex.class) - Bugsnag.notify(ex, true) do |report| - report.severity = "error" - report.severity_reason = { - :type => Bugsnag::Report::UNHANDLED_EXCEPTION_MIDDLEWARE, - :attributes => Bugsnag::Shoryuken::FRAMEWORK_ATTRIBUTES - } - end + Bugsnag.notify(ex, true) do |report| + report.severity = "error" + report.severity_reason = { + :type => Bugsnag::Report::UNHANDLED_EXCEPTION_MIDDLEWARE, + :attributes => Bugsnag::Shoryuken::FRAMEWORK_ATTRIBUTES + } end raise ensure diff --git a/lib/bugsnag/integrations/sidekiq.rb b/lib/bugsnag/integrations/sidekiq.rb index 5566c8ee3..12dabe2c7 100644 --- a/lib/bugsnag/integrations/sidekiq.rb +++ b/lib/bugsnag/integrations/sidekiq.rb @@ -31,7 +31,6 @@ def call(worker, msg, queue) end def self.notify(exception) - return if [Interrupt, SystemExit, SignalException].include? exception.class Bugsnag.notify(exception, true) do |report| report.severity = "error" report.severity_reason = { diff --git a/spec/configuration_spec.rb b/spec/configuration_spec.rb index 0ecc7e43e..055808fc8 100644 --- a/spec/configuration_spec.rb +++ b/spec/configuration_spec.rb @@ -219,7 +219,7 @@ def debug(name, &block) end it "should have exit exception classes ignored by default" do - expect(subject.ignore_classes).to eq(Set.new([SystemExit, Interrupt])) + expect(subject.ignore_classes).to eq(Set.new([SystemExit, SignalException])) end end diff --git a/spec/report_spec.rb b/spec/report_spec.rb index 2dfe43ad5..e45a2cd51 100644 --- a/spec/report_spec.rb +++ b/spec/report_spec.rb @@ -146,18 +146,27 @@ def gloops end it "sets correct severity and reason for specific error classes" do - Bugsnag.notify(SignalException.new("TERM")) - expect(Bugsnag).to have_sent_notification{ |payload, headers| - event = get_event_from_payload(payload) - expect(event["unhandled"]).to be false - expect(event["severity"]).to eq("info") - expect(event["severityReason"]).to eq({ - "type" => "errorClass", - "attributes" => { - "errorClass" => "SignalException" - } - }) - } + original_ignore_classes = Bugsnag.configuration.ignore_classes + + begin + # The default ignore_classes includes SignalException, so we need to + # temporarily set it to something else. + Bugsnag.configuration.ignore_classes = Set[SystemExit] + Bugsnag.notify(SignalException.new("TERM")) + expect(Bugsnag).to have_sent_notification{ |payload, headers| + event = get_event_from_payload(payload) + expect(event["unhandled"]).to be false + expect(event["severity"]).to eq("info") + expect(event["severityReason"]).to eq({ + "type" => "errorClass", + "attributes" => { + "errorClass" => "SignalException" + } + }) + } + ensure + Bugsnag.configuration.ignore_classes = original_ignore_classes + end end # TODO: nested context @@ -1052,10 +1061,10 @@ def gloops expect(exception["message"]).to eq("'nil' was notified as an exception") stacktrace = exception["stacktrace"][0] - expect(stacktrace["lineNumber"]).to eq(1047) + expect(stacktrace["lineNumber"]).to eq(1056) expect(stacktrace["file"]).to end_with("spec/report_spec.rb") - expect(stacktrace["code"]["1046"]).to eq(" it 'uses an appropriate message if nil is notified' do") - expect(stacktrace["code"]["1047"]).to eq(" Bugsnag.notify(nil)") + expect(stacktrace["code"]["1055"]).to eq(" it 'uses an appropriate message if nil is notified' do") + expect(stacktrace["code"]["1056"]).to eq(" Bugsnag.notify(nil)") } end