From b3cfe6fcc519e1368ba266233011551fd2d54e8b Mon Sep 17 00:00:00 2001 From: Alex Moinet Date: Mon, 2 Oct 2017 18:50:27 +0100 Subject: [PATCH] Track whether an error report was captured automatically (#369) * Added support for handled-unhandled * Added severity-reasons to middleware * Moved ignored classes into middleware to set info --- .travis.yml | 3 + lib/bugsnag.rb | 18 +++- lib/bugsnag/configuration.rb | 16 +--- lib/bugsnag/delayed_job.rb | 6 ++ lib/bugsnag/mailman.rb | 9 +- lib/bugsnag/middleware/classify_error.rb | 53 ++++++++++++ lib/bugsnag/notification.rb | 46 +++++++++- lib/bugsnag/que.rb | 9 +- lib/bugsnag/rack.rb | 16 +++- lib/bugsnag/rails/action_controller_rescue.rb | 16 +++- lib/bugsnag/rails/active_record_rescue.rb | 9 +- lib/bugsnag/railtie.rb | 9 +- lib/bugsnag/rake.rb | 9 +- lib/bugsnag/resque.rb | 11 ++- lib/bugsnag/shoryuken.rb | 9 +- lib/bugsnag/sidekiq.rb | 9 +- spec/integration_spec.rb | 6 +- spec/notification_spec.rb | 83 +++++++++++++++++-- spec/rack_spec.rb | 15 ++++ 19 files changed, 315 insertions(+), 37 deletions(-) create mode 100644 lib/bugsnag/middleware/classify_error.rb diff --git a/.travis.yml b/.travis.yml index 8fd7aae3d..851a3773d 100644 --- a/.travis.yml +++ b/.travis.yml @@ -10,4 +10,7 @@ rvm: - jruby-19mode before_install: +- gem install bundler -v 1.12 +- gem update --system +- bundle --version - gem --version diff --git a/lib/bugsnag.rb b/lib/bugsnag.rb index 8c6c8b046..e14ea2407 100644 --- a/lib/bugsnag.rb +++ b/lib/bugsnag.rb @@ -24,6 +24,7 @@ require "bugsnag/middleware/mailman" require "bugsnag/middleware/rake" require "bugsnag/middleware/callbacks" +require "bugsnag/middleware/classify_error" module Bugsnag LOG_PREFIX = "** [Bugsnag] " @@ -43,6 +44,9 @@ def configure(config_hash=nil) # Use resque for asynchronous notification if required require "bugsnag/delay/resque" if configuration.delay_with_resque && defined?(Resque) + # Add info error classifier to internal middleware + configuration.internal_middleware.use(Bugsnag::Middleware::ClassifyError) + # Warn if an api_key hasn't been set @key_warning = false unless defined?(@key_warning) @@ -64,8 +68,19 @@ def configure(config_hash=nil) def notify(exception, overrides=nil, request_data=nil, &block) notification = Notification.new(exception, configuration, overrides, request_data) + initial_severity = notification.severity + initial_reason = notification.severity_reason + yield(notification) if block_given? + if notification.severity != initial_severity + notification.severity_reason = { + :type => Bugsnag::Notification::USER_CALLBACK_SET_SEVERITY + } + else + notification.severity_reason = initial_reason + end + unless notification.ignore? notification.deliver notification @@ -80,7 +95,8 @@ def notify(exception, overrides=nil, request_data=nil, &block) # error class def auto_notify(exception, overrides=nil, request_data=nil, &block) overrides ||= {} - overrides.merge!({:severity => "error"}) + overrides[:severity] = "error" unless overrides.has_key? :severity + overrides[:unhandled] = true unless overrides.has_key? :unhandled notify_or_ignore(exception, overrides, request_data, &block) if configuration.auto_notify end diff --git a/lib/bugsnag/configuration.rb b/lib/bugsnag/configuration.rb index f5a2ac159..c5c9bb9e4 100644 --- a/lib/bugsnag/configuration.rb +++ b/lib/bugsnag/configuration.rb @@ -45,20 +45,6 @@ class Configuration "rack.request.form_vars" ].freeze - DEFAULT_IGNORE_CLASSES = [ - "AbstractController::ActionNotFound", - "ActionController::InvalidAuthenticityToken", - "ActionController::ParameterMissing", - "ActionController::UnknownAction", - "ActionController::UnknownFormat", - "ActionController::UnknownHttpMethod", - "ActiveRecord::RecordNotFound", - "CGI::Session::CookieStore::TamperedWithCookie", - "Mongoid::Errors::DocumentNotFound", - "SignalException", - "SystemExit", - ].freeze - DEFAULT_IGNORE_USER_AGENTS = [].freeze DEFAULT_DELIVERY_METHOD = :thread_queue @@ -72,7 +58,7 @@ def initialize self.send_environment = false self.send_code = true self.params_filters = Set.new(DEFAULT_PARAMS_FILTERS) - self.ignore_classes = Set.new(DEFAULT_IGNORE_CLASSES) + self.ignore_classes = Set.new() self.ignore_user_agents = Set.new(DEFAULT_IGNORE_USER_AGENTS) self.endpoint = DEFAULT_ENDPOINT self.hostname = default_hostname diff --git a/lib/bugsnag/delayed_job.rb b/lib/bugsnag/delayed_job.rb index 2f842acc1..abd04d3c7 100644 --- a/lib/bugsnag/delayed_job.rb +++ b/lib/bugsnag/delayed_job.rb @@ -17,6 +17,12 @@ def error(job, error) :job => { :class => job.class.name, :id => job.id, + }, + :severity_reason => { + :type => Bugsnag::Notification::UNHANDLED_EXCEPTION_MIDDLEWARE, + :attributes => { + :framework => "DelayedJob" + } } } if job.respond_to?(:queue) && (queue = job.queue) diff --git a/lib/bugsnag/mailman.rb b/lib/bugsnag/mailman.rb index 142f59267..92515b8f5 100644 --- a/lib/bugsnag/mailman.rb +++ b/lib/bugsnag/mailman.rb @@ -15,7 +15,14 @@ def call(mail) yield rescue Exception => ex raise ex if [Interrupt, SystemExit, SignalException].include? ex.class - Bugsnag.auto_notify(ex) + Bugsnag.auto_notify(ex, { + :severity_reason => { + :type => Bugsnag::Notification::UNHANDLED_EXCEPTION_MIDDLEWARE, + :attributes => { + :framework => "Mailman" + } + } + }) raise ensure Bugsnag.clear_request_data diff --git a/lib/bugsnag/middleware/classify_error.rb b/lib/bugsnag/middleware/classify_error.rb new file mode 100644 index 000000000..e6a553110 --- /dev/null +++ b/lib/bugsnag/middleware/classify_error.rb @@ -0,0 +1,53 @@ +module Bugsnag::Middleware + class ClassifyError + INFO_CLASSES = [ + "AbstractController::ActionNotFound", + "ActionController::InvalidAuthenticityToken", + "ActionController::ParameterMissing", + "ActionController::UnknownAction", + "ActionController::UnknownFormat", + "ActionController::UnknownHttpMethod", + "ActiveRecord::RecordNotFound", + "CGI::Session::CookieStore::TamperedWithCookie", + "Mongoid::Errors::DocumentNotFound", + "SignalException", + "SystemExit" + ] + + def initialize(bugsnag) + @bugsnag = bugsnag + end + + def call(notification) + notification.exceptions.each do |ex| + + outer_break = false + + ancestor_chain = ex.class.ancestors.select { + |ancestor| ancestor.is_a?(Class) + }.map { + |ancestor| ancestor.to_s + } + + INFO_CLASSES.each do |info_class| + if ancestor_chain.include?(info_class) + notification.severity_reason = { + :type => Bugsnag::Notification::ERROR_CLASS, + :attributes => { + :errorClass => info_class + } + } + notification.severity = 'info' + outer_break = true + break + end + end + + break if outer_break + end + + @bugsnag.call(notification) + end + end +end + \ No newline at end of file diff --git a/lib/bugsnag/notification.rb b/lib/bugsnag/notification.rb index 76a44ed86..5290ab988 100644 --- a/lib/bugsnag/notification.rb +++ b/lib/bugsnag/notification.rb @@ -15,6 +15,13 @@ class Notification NOTIFIER_VERSION = Bugsnag::VERSION NOTIFIER_URL = "http://www.bugsnag.com" + HANDLED_EXCEPTION = "handledException" + UNHANDLED_EXCEPTION = "unhandledException" + UNHANDLED_EXCEPTION_MIDDLEWARE = "unhandledExceptionMiddleware" + ERROR_CLASS = "errorClass" + USER_SPECIFIED_SEVERITY = "userSpecifiedSeverity" + USER_CALLBACK_SET_SEVERITY = "userCallbackSetSeverity" + API_KEY_REGEX = /[0-9a-f]{32}/i # e.g. "org/jruby/RubyKernel.java:1264:in `catch'" @@ -31,6 +38,8 @@ class Notification attr_accessor :context attr_reader :user + attr_reader :severity + attr_accessor :severity_reason attr_accessor :configuration attr_accessor :meta_data @@ -50,10 +59,36 @@ def initialize(exception, configuration, overrides = nil, request_data = nil) @user = {} @should_ignore = false @severity = nil + @unhandled = false + @severity_reason = nil @grouping_hash = nil @delivery_method = nil - self.severity = @overrides[:severity] + if @overrides.key? :unhandled + @unhandled = @overrides[:unhandled] + @overrides.delete :unhandled + end + + valid_severity = @overrides.key?(:severity) && SUPPORTED_SEVERITIES.include?(@overrides[:severity]) + has_reason = @overrides.key? :severity_reason + + if valid_severity && has_reason + @severity = @overrides[:severity] + @severity_reason = @overrides[:severity_reason] + elsif valid_severity + @severity = @overrides[:severity] + @severity_reason = { + :type => USER_SPECIFIED_SEVERITY + } + elsif has_reason + @severity_reason = @overrides[:severity_reason] + else + @severity_reason = { + :type => HANDLED_EXCEPTION + } + end + + @overrides.delete :severity_reason @overrides.delete :severity if @overrides.key? :grouping_hash @@ -214,12 +249,19 @@ def deliver # make meta_data available to public middleware @meta_data = generate_meta_data(@exceptions, @overrides) + initial_severity = self.severity + # Run the middleware here (including Bugsnag::Middleware::Callbacks) # at the end of the middleware stack, execute the actual notification delivery @configuration.middleware.run(self) do # This supports self.ignore! for before_notify_callbacks. return if @should_ignore + # Check to see if the severity has been changed + if initial_severity != self.severity + + end + # Build the endpoint url endpoint = (@configuration.use_ssl ? "https://" : "http://") + @configuration.endpoint Bugsnag.log("Notifying #{endpoint} of #{@exceptions.last.class}") @@ -243,6 +285,8 @@ def build_exception_payload :payloadVersion => payload_version, :exceptions => exception_list, :severity => self.severity, + :unhandled => @unhandled, + :severityReason => @severity_reason, :groupingHash => self.grouping_hash, } diff --git a/lib/bugsnag/que.rb b/lib/bugsnag/que.rb index 2eaae4f07..5cdba68f6 100644 --- a/lib/bugsnag/que.rb +++ b/lib/bugsnag/que.rb @@ -3,7 +3,14 @@ begin job = job.dup # Make sure the original job object is not mutated. - Bugsnag.auto_notify(error) do |notification| + Bugsnag.auto_notify(error, { + :severity_reason => { + :type => Bugsnag::Notification::UNHANDLED_EXCEPTION_MIDDLEWARE, + :attributes => { + :framework => "Que" + } + } + }) do |notification| job[:error_count] += 1 # If the job was scheduled using ActiveJob then unwrap the job details for clarity: diff --git a/lib/bugsnag/rack.rb b/lib/bugsnag/rack.rb index 17c8f59b4..41bd15231 100644 --- a/lib/bugsnag/rack.rb +++ b/lib/bugsnag/rack.rb @@ -1,5 +1,13 @@ module Bugsnag class Rack + + SEVERITY_REASON = { + :type => Bugsnag::Notification::UNHANDLED_EXCEPTION_MIDDLEWARE, + :attributes => { + :framework => "Rack" + } + } + def initialize(app) @app = app @@ -34,7 +42,9 @@ def call(env) response = @app.call(env) rescue Exception => raised # Notify bugsnag of rack exceptions - Bugsnag.auto_notify(raised) + Bugsnag.auto_notify(raised, { + :severity_reason => SEVERITY_REASON + }) # Re-raise the exception raise @@ -42,7 +52,9 @@ def call(env) # Notify bugsnag of rack exceptions if env["rack.exception"] - Bugsnag.auto_notify(env["rack.exception"]) + Bugsnag.auto_notify(env["rack.exception"], { + :severity_reason => SEVERITY_REASON + }) end response diff --git a/lib/bugsnag/rails/action_controller_rescue.rb b/lib/bugsnag/rails/action_controller_rescue.rb index 6125e9cd6..a0c0649fc 100644 --- a/lib/bugsnag/rails/action_controller_rescue.rb +++ b/lib/bugsnag/rails/action_controller_rescue.rb @@ -1,6 +1,14 @@ # Rails 2.x only module Bugsnag::Rails module ActionControllerRescue + + SEVERITY_REASON = { + :type => Bugsnag::Notification::UNHANDLED_EXCEPTION_MIDDLEWARE, + :attributes => { + :framework => "Rails" + } + } + def self.included(base) base.extend(ClassMethods) @@ -22,13 +30,17 @@ def set_bugsnag_request_data end def rescue_action_in_public_with_bugsnag(exception) - Bugsnag.auto_notify(exception) + Bugsnag.auto_notify(exception, { + :severity_reason => SEVERITY_REASON + }) rescue_action_in_public_without_bugsnag(exception) end def rescue_action_locally_with_bugsnag(exception) - Bugsnag.auto_notify(exception) + Bugsnag.auto_notify(exception, { + :severity_reason => SEVERITY_REASON + }) rescue_action_locally_without_bugsnag(exception) end diff --git a/lib/bugsnag/rails/active_record_rescue.rb b/lib/bugsnag/rails/active_record_rescue.rb index dfc9e4745..24c6516e0 100644 --- a/lib/bugsnag/rails/active_record_rescue.rb +++ b/lib/bugsnag/rails/active_record_rescue.rb @@ -8,7 +8,14 @@ def run_callbacks(kind, *args, &block) super rescue StandardError => exception # This exception will NOT be escalated, so notify it here. - Bugsnag.auto_notify(exception) + Bugsnag.auto_notify(exception, { + :severity_reason => { + :type => Bugsnag::Notification::UNHANDLED_EXCEPTION_MIDDLEWARE, + :attributes => { + :framework => "Rails" + } + } + }) raise end else diff --git a/lib/bugsnag/railtie.rb b/lib/bugsnag/railtie.rb index 799373b31..481a433a1 100644 --- a/lib/bugsnag/railtie.rb +++ b/lib/bugsnag/railtie.rb @@ -17,7 +17,14 @@ class Railtie < Rails::Railtie runner do at_exit do if $! - Bugsnag.auto_notify($!) + Bugsnag.auto_notify($!, { + :severity_reason => { + :type => Bugsnag::Notification::UNHANDLED_EXCEPTION_MIDDLEWARE, + :attributes => { + :framework => "Rails" + } + } + }) end end end diff --git a/lib/bugsnag/rake.rb b/lib/bugsnag/rake.rb index 9a2cb75e4..74261a977 100644 --- a/lib/bugsnag/rake.rb +++ b/lib/bugsnag/rake.rb @@ -12,7 +12,14 @@ def execute_with_bugsnag(args=nil) execute_without_bugsnag(args) rescue Exception => ex - Bugsnag.auto_notify(ex) + Bugsnag.auto_notify(ex, { + :severity_reason => { + :type => Bugsnag::Notification::UNHANDLED_EXCEPTION_MIDDLEWARE, + :attributes => { + :framework => "Rake" + } + } + }) raise ensure Bugsnag.set_request_data :bugsnag_running_task, old_task diff --git a/lib/bugsnag/resque.rb b/lib/bugsnag/resque.rb index 4f9b3b428..c93d7c48c 100644 --- a/lib/bugsnag/resque.rb +++ b/lib/bugsnag/resque.rb @@ -26,7 +26,16 @@ def self.add_failure_backend end def save - Bugsnag.auto_notify(exception, {:context => "#{payload['class']}@#{queue}", :payload => payload}) + Bugsnag.auto_notify(exception, { + :context => "#{payload['class']}@#{queue}", + :payload => payload, + :severity_reason => { + :type => Bugsnag::Notification::UNHANDLED_EXCEPTION_MIDDLEWARE, + :attributes => { + :framework => "Resque" + } + } + }) end end end diff --git a/lib/bugsnag/shoryuken.rb b/lib/bugsnag/shoryuken.rb index 2a2063179..ab5182442 100644 --- a/lib/bugsnag/shoryuken.rb +++ b/lib/bugsnag/shoryuken.rb @@ -18,7 +18,14 @@ def call(_, queue, _, body) yield rescue Exception => ex - Bugsnag.auto_notify(ex) unless [Interrupt, SystemExit, SignalException].include?(ex.class) + Bugsnag.auto_notify(ex, { + :severity_reason => { + :type => Bugsnag::Notification::UNHANDLED_EXCEPTION_MIDDLEWARE, + :attributes => { + :framework => "Shoryuken" + } + } + }) unless [Interrupt, SystemExit, SignalException].include?(ex.class) raise ensure Bugsnag.clear_request_data diff --git a/lib/bugsnag/sidekiq.rb b/lib/bugsnag/sidekiq.rb index 3210cbd1e..c411db3d8 100644 --- a/lib/bugsnag/sidekiq.rb +++ b/lib/bugsnag/sidekiq.rb @@ -16,7 +16,14 @@ def call(worker, msg, queue) yield rescue Exception => ex raise ex if [Interrupt, SystemExit, SignalException].include? ex.class - Bugsnag.auto_notify(ex) + Bugsnag.auto_notify(ex, { + :severity_reason => { + :type => Bugsnag::Notification::UNHANDLED_EXCEPTION_MIDDLEWARE, + :attributes => { + :framework => "Sidekiq" + } + } + }) raise ensure Bugsnag.clear_request_data diff --git a/spec/integration_spec.rb b/spec/integration_spec.rb index 64a94dc1c..b3226ec59 100644 --- a/spec/integration_spec.rb +++ b/spec/integration_spec.rb @@ -28,8 +28,10 @@ Dir.chdir(task_fixtures_path) do system("bundle exec rake test:crash > /dev/null 2>&1") end - expect(request["events"][0]["metaData"]["rake_task"]).not_to be_nil - expect(request["events"][0]["metaData"]["rake_task"]["name"]).to eq("test:crash") + + result = request() + expect(result["events"][0]["metaData"]["rake_task"]).not_to be_nil + expect(result["events"][0]["metaData"]["rake_task"]["name"]).to eq("test:crash") end it 'should send notifications over the wire' do diff --git a/spec/notification_spec.rb b/spec/notification_spec.rb index 5b50ca18e..92c266021 100644 --- a/spec/notification_spec.rb +++ b/spec/notification_spec.rb @@ -113,6 +113,83 @@ def gloops } end + it "uses correct unhandled defaults" do + Bugsnag.notify(BugsnagTestException.new("It crashed")) + + expect(Bugsnag).to have_sent_notification{ |payload| + event = get_event_from_payload(payload) + expect(event["unhandled"]).to be false + expect(event["severity"]).to eq("warning") + expect(event["severityReason"]).to eq({ + "type" => "handledException" + }) + } + end + + it "sets correct severityReason if severity is modified" do + Bugsnag.notify(BugsnagTestException.new("It crashed"), {:severity => "info"}) + + expect(Bugsnag).to have_sent_notification{ |payload| + event = get_event_from_payload(payload) + expect(event["unhandled"]).to be false + expect(event["severity"]).to eq("info") + expect(event["severityReason"]).to eq({ + "type" => "userSpecifiedSeverity" + }) + } + end + + it "sets correct severityReason if severity is modified in a block" do + Bugsnag.notify(BugsnagTestException.new("It crashed")) do |notification| + notification.severity = "info" + end + expect(Bugsnag).to have_sent_notification{ |payload| + event = get_event_from_payload(payload) + expect(event["unhandled"]).to be false + expect(event["severity"]).to eq("info") + expect(event["severityReason"]).to eq({ + "type" => "userCallbackSetSeverity" + }) + } + end + + it "sets unhandled and severityReasons through auto_notify" do + Bugsnag.auto_notify(BugsnagTestException.new("It crashed"), { + :severity_reason => { + :type => "unhandledExceptionMiddleware", + :attributes => { + :framework => "ruby test" + } + } + }) + expect(Bugsnag).to have_sent_notification{ |payload| + event = get_event_from_payload(payload) + expect(event["unhandled"]).to be true + expect(event["severity"]).to eq("error") + expect(event["severityReason"]).to eq({ + "type" => "unhandledExceptionMiddleware", + "attributes" => { + "framework" => "ruby test" + } + }) + } + end + + it "sets correct severity and reason for specific error classes" do + Bugsnag.notify(SignalException.new("TERM")) + expect(Bugsnag).to have_sent_notification{ |payload| + 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" + } + }) + } + end + # TODO: nested context it "accepts tabs in overrides and adds them to metaData" do @@ -580,12 +657,6 @@ def gloops } end - it "does not notify if the exception class is in the default ignore_classes list" do - Bugsnag.notify_or_ignore(ActiveRecord::RecordNotFound.new("It crashed")) - - expect(Bugsnag).not_to have_sent_notification - end - it "does not notify if the non-default exception class is added to the ignore_classes" do Bugsnag.configuration.ignore_classes << "BugsnagTestException" diff --git a/spec/rack_spec.rb b/spec/rack_spec.rb index 6eb6c9309..353505a74 100644 --- a/spec/rack_spec.rb +++ b/spec/rack_spec.rb @@ -32,6 +32,21 @@ end + it "applies the correct severity reason" do + rack_stack.call(rack_env) rescue nil + + expect(Bugsnag).to have_sent_notification{ |payload| + event = get_event_from_payload(payload) + expect(event["unhandled"]).to be true + expect(event["severityReason"]).to eq({ + "type" => "unhandledExceptionMiddleware", + "attributes" => { + "framework" => "Rack" + } + }) + } + end + it "does not deliver an exception if auto_notify is disabled" do Bugsnag.configure do |config| config.auto_notify = false