Added unhandled functionality, updated tests and integrations #368

Cawllec · 2017-09-13T14:29:03Z

Adds defaultSeverity, unhandled, and severityReasons payload properties to support upcoming handled/unhandled functionality
Created separate notifier functions, notify for handled notifications and auto_notify for automated/middleware notifications
Middleware set up with correct severity reasons
Discussion needed around language-level ruby error handling, and how to allow the user to this up without delving into severity reasons
references PLAT_207

snmaynard

I'll take a look at the non v5 one in a sec. Looks good, I think the main point of discussion is the auto_notify method. LMK your thoughts.

snmaynard · 2017-09-13T20:25:36Z

lib/bugsnag/integrations/resque.rb

+      Bugsnag.auto_notify(exception, {
+        :type => "middleware",
+        :attributes => {
+          :name => "mailman"


snmaynard · 2017-09-14T18:59:35Z

lib/bugsnag/integrations/mailman.rb

@@ -13,7 +13,12 @@ def call(mail)
        yield
      rescue Exception => ex
        raise ex if [Interrupt, SystemExit, SignalException].include? ex.class
-        Bugsnag.notify(ex, true) do |report|
+        Bugsnag.auto_notify(ex, {
+            :type => "middleware",


middleware_handler

snmaynard · 2017-09-14T18:59:41Z

lib/bugsnag/integrations/rack.rb

@@ -43,7 +48,12 @@ def call(env)

      # Notify bugsnag of rack exceptions
      if env["rack.exception"]
-        Bugsnag.notify(env["rack.exception"], true) do |report|
+        Bugsnag.auto_notify(env["rack.exception"], {
+          :type => "middleware",


middleware_handler

snmaynard · 2017-09-14T18:59:46Z

lib/bugsnag/integrations/rails/active_record_rescue.rb

@@ -8,7 +8,12 @@ def run_callbacks(kind, *args, &block)
          super
        rescue StandardError => exception
          # This exception will NOT be escalated, so notify it here.
-          Bugsnag.notify(exception, true) do |report|
+          Bugsnag.auto_notify(exception, {
+            :type => "middleware",


middleware_handler

snmaynard · 2017-09-14T18:59:50Z

lib/bugsnag/integrations/railtie.rb

@@ -17,7 +17,12 @@ class Railtie < Rails::Railtie
      runner do
        at_exit do
          if $!
-            Bugsnag.notify($!, true) do |report|
+            Bugsnag.auto_notify($!, {
+              :type => "middleware",


middleware_handler

snmaynard · 2017-09-14T19:00:00Z

lib/bugsnag/integrations/resque.rb

@@ -26,7 +26,12 @@ def self.add_failure_backend
    end

    def save
-      Bugsnag.notify(exception, true) do |report|
+      Bugsnag.auto_notify(exception, {
+        :type => "middleware",


middleware_handler

snmaynard · 2017-09-14T19:04:44Z

lib/bugsnag.rb

        configuration.debug("Not notifying because auto_notify is disabled")
        return
      end

+      report = Report.new(exception, configuration, true, severity_reason)


You could set the severity reason in here to be error?

snmaynard · 2017-09-14T19:08:55Z

lib/bugsnag/tasks/bugsnag.rake

@@ -6,7 +6,14 @@ namespace :bugsnag do
    begin
      raise RuntimeError.new("Bugsnag test exception")
    rescue => e
-      Bugsnag.notify(e, {:context => "rake#test_exception"})
+      Bugsnag.auto_notify(e, {


This is fine to just be a normal notify, doesnt need to be an auto_notify. Its just a way of people testing their bugsnag installations

snmaynard · 2017-09-14T19:16:31Z

lib/bugsnag.rb

+    end
+
+    # For automatic notification of exceptions
+    def auto_notify(exception, severity_reason, &block)


I dont think we need the auto_notify method. If you look where we build and send the report blocks when auto_notify is set to true are run first so we can change the reports in the block for auto notifications.

So I would just call notify like we did before, just telling notify that this is an auto_notify. Then in the block you can set the severity reason and the severity. Its weird that severity is set in the block, and severity reason outside it right now.

snmaynard · 2017-09-14T19:17:59Z

lib/bugsnag.rb

@@ -80,24 +106,18 @@ def notify(exception, auto_notify=false, &block)
          return
        end

+        # Test whether severity has been changed
+        if report.severity != initial_severity
+          report.default_severity = false


You could make this a little clearer imo with report.default_severity = report.severity == initial_severity

…case

Cawllec · 2017-09-15T10:43:29Z

I agree with re-merging the auto-notify functions, and with passing the severity_reason through a block, as passing it along with the severity makes the most sense.

kattrali

Left some inline comments, I think the set_handled_state method got removed somewhere.

kattrali · 2017-09-21T09:56:54Z

lib/bugsnag/integrations/delayed_job.rb

@@ -36,6 +36,12 @@ def error(job, error)

            ::Bugsnag.notify(error, true) do |report|
              report.severity = "error"
+              report.set_handled_state({


set_handled_state doesn't seem to be defined anywhere.

kattrali · 2017-09-21T09:57:19Z

lib/bugsnag/integrations/sidekiq.rb

@@ -18,6 +18,12 @@ def call(worker, msg, queue)
        raise ex if [Interrupt, SystemExit, SignalException].include? ex.class
        Bugsnag.notify(ex, true) do |report|
          report.severity = "error"
+          report.set_handled_states({


Similar to above, set_handled_states (trailing 's') doesn't seem to be defined anywhere.

kattrali · 2017-09-21T09:57:38Z

lib/bugsnag/middleware/classify_error.rb

+              }
+            )
+            report.severity = 'info'
+            break;


Unneeded semicolon

kattrali · 2017-09-21T09:59:23Z

lib/bugsnag/report.rb

@@ -25,10 +25,12 @@ class Report
    attr_accessor :raw_exceptions
    attr_accessor :release_stage
    attr_accessor :severity
+    attr_accessor :severity_reason


This can be declared as attr_reader, since its only being set internally by a method (I think that was the intent of set_handled_states). It would prevent accidental overrides by other code/callback writers.

kattrali · 2017-09-21T10:03:36Z

lib/bugsnag.rb

@@ -34,7 +34,9 @@ def configure

    # Explicitly notify of an exception
    def notify(exception, auto_notify=false, &block)
-      if auto_notify && !configuration.auto_notify
+      report = Report.new(exception, configuration, auto_notify)


Why create the report before the validation blocks? This is extra processing when a user is running in a testing/debug mode.

kattrali · 2017-09-21T10:06:04Z

lib/bugsnag.rb

+        # Update severity reason if necessary
+        if report.severity != before_middleware_severity
+          report.severity_reason = {
+            :type => "userSpecifiedSeverity"


The values for severity_reason should be defined as constants to avoid typos and to aggregate all possible values. Currently these values are scattered between different files.

kattrali · 2017-09-21T10:07:09Z

lib/bugsnag/integrations/rack.rb

@@ -35,6 +35,12 @@ def call(env)
        # Notify bugsnag of rack exceptions
        Bugsnag.notify(raised, true) do |report|
          report.severity = "error"
+          report.set_handled_state({
+            :type => "unhandledExceptionMiddleware",
+            :attirbutes => {


"attirbutes"

kattrali · 2017-09-21T10:07:34Z

lib/bugsnag/integrations/rack.rb

+          report.set_handled_state({
+            :type => "middleware_handler",
+            :attributes => {
+              :name => "rack"


Rack above is capitalized, while this is not. Integration names are another candidate for a constant enumeration.

kattrali · 2017-09-21T10:09:38Z

lib/bugsnag/middleware/classify_error.rb

+
+    def call(report)
+      report.raw_exceptions.each do |ex|
+        ancestor_chain = ex.class.ancestors.select { |ancestor| ancestor.is_a?(Class) }.map { |ancestor| ancestor.to_s }


Breaking the blocks onto separate lines would make this expression more readable.

Added unhandled functionality, updated tests and intergrations

7e399e3

Cawllec added needs discussion Requires internal analysis/discussion and removed needs discussion Requires internal analysis/discussion labels Sep 13, 2017

snmaynard suggested changes Sep 14, 2017

View reviewed changes

Cawllec added 7 commits September 15, 2017 10:25

Recombined auto_notify and notify, using auto_notify flag

4e40c4b

Made unhandled & severity_reason fields uneditable by user

5d0411e

Changed auto_notifies to notify with auto_notify enabled

880affa

Removed auto-notify from integration test

cdb80c4

Tracking auto_notify in report to stop unhandled being set in manual …

2aa48ca

…case

Ensures handledState properties only set once

4d1770d

Modified and added tests to reflect new layout

a7799ee

Cawllec added 4 commits September 20, 2017 13:37

Added info error classifier middleware

725f7f1

Updated integrations severityReasons

7023dab

Removed defaultSeverity and enabled modifying of severity-reason

8cba071

Added severityReason changes based on callbacks

f7db8af

kattrali reviewed Sep 21, 2017

View reviewed changes

Cawllec added 5 commits September 21, 2017 17:10

Updated to correct functions calls

9d4f95e

updated tests

391a755

More sensible defaults in auto_notify cases

464ac7f

Incorrect setting

82519e3

Ensured Bugsnag refered to correctly as const

085108c

kattrali approved these changes Oct 2, 2017

View reviewed changes

kattrali merged commit 643ae3a into v5 Oct 2, 2017

kattrali deleted the v5-unhandled branch October 2, 2017 16:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added unhandled functionality, updated tests and integrations #368

Added unhandled functionality, updated tests and integrations #368

Cawllec commented Sep 13, 2017

snmaynard left a comment

snmaynard Sep 13, 2017

snmaynard Sep 14, 2017

snmaynard Sep 14, 2017

snmaynard Sep 14, 2017

snmaynard Sep 14, 2017

snmaynard Sep 14, 2017

snmaynard Sep 14, 2017

snmaynard Sep 14, 2017

snmaynard Sep 14, 2017

snmaynard Sep 14, 2017

snmaynard Sep 14, 2017

Cawllec commented Sep 15, 2017

kattrali left a comment

kattrali Sep 21, 2017

kattrali Sep 21, 2017

kattrali Sep 21, 2017

kattrali Sep 21, 2017

kattrali Sep 21, 2017

kattrali Sep 21, 2017

kattrali Sep 21, 2017

kattrali Sep 21, 2017

kattrali Sep 21, 2017

Added unhandled functionality, updated tests and integrations #368

Added unhandled functionality, updated tests and integrations #368

Conversation

Cawllec commented Sep 13, 2017

snmaynard left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Cawllec commented Sep 15, 2017

kattrali left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment