Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add 'on_error' callbacks, to eventually replace 'before_notify_callbacks' #608

Merged
merged 2 commits into from Jul 22, 2020

Conversation

imjoehaines
Copy link
Member

@imjoehaines imjoehaines commented Jul 20, 2020

Goal

The main problem these solve is that they are global, rather than being scoped to the current thread. This means you can add a callback in a Rails initializer and it will run as expected. With the current before_notify_callbacks, they only run in the same thread as they are added and so won't run if created in an initializer (this is a problem in most frameworks, not just Rails)

The before_notify_callbacks cause a lot of confusion because they are scoped to a single thread, e.g. see #487, #417. Typically callbacks are intended to be global to the application rather than specific to a route/worker, so the new on_error mechanism should be a lot more intuitive. This is also how callbacks work in our libraries for other platforms, like JavaScript and Android

Bugsnag.configure do |config|
  # Using 'before_notify_callbacks':
  config.before_notify_callbacks << proc do |report|
    report.add_tab(:diagnostics, { hello: "world" })
  end

  # Using 'on_error' callbacks:
  config.add_on_error(proc do |report|
    report.add_tab(:diagnostics, { world: "hello" })
  end)
end

The new callbacks can also be removed if necessary:

custom_callback = proc {|report| report.add_tab(:foo, { bar: "baz" }) }

Bugsnag.add_on_error(custom_callback)
Bugsnag.remove_on_error(custom_callback)

You can also ignore an error and stop calling any subsequent callbacks by returning false:

callback1 = proc {|report| report.add_tab(:foo, { bar: "baz" }) }
callback2 = proc {|report| false }
callback3 = proc {|report| "this will never be called because 'callback2' returns false" }

Bugsnag.add_on_error(callback1)
Bugsnag.add_on_error(callback2)
Bugsnag.add_on_error(callback3)

Tests

Most of the implementation is based on middleware, which is already covered by existing tests. I've added a bunch of tests for the new callbacks specifically and checking that they work as expected across threads. Some Maze Runner tests will be added in a separate PR

Review

For the submitter, initial self-review:

  • Commented on code changes inline explain the reasoning behind the approach
  • Reviewed the test cases added for completeness and possible points for discussion
  • A changelog entry was added for the goal of this pull request
  • Check the scope of the changeset - is everything in the diff required for the pull request?
  • This pull request is ready for:
    • Initial review of the intended approach, not yet feature complete
    • Structural review of the classes, functions, and properties modified
    • Final review

For the pull request reviewer(s), this changeset has been reviewed for:

  • Consistency across platforms for structures or concepts added or modified
  • Consistency between the changeset and the goal stated above
  • Internal consistency with the rest of the library - is there any overlap between existing interfaces and any which have been added?
  • Usage friction - is the proposed change in usage cumbersome or complicated?
  • Performance and complexity - are there any cases of unexpected O(n^3) when iterating, recursing, flat mapping, etc?
  • Concurrency concerns - if components are accessed asynchronously, what issues will arise
  • Thoroughness of added tests and any missing edge cases
  • Idiomatic use of the language

These new 'on_error' callbacks will ultimately be a replacement for
the existing 'before_notify_callbacks'

The main problem these solve is that they are global, rather than
being scoped to the current thread. This means you can add a callback
in a Rails initializer and it will run as expected. With the current
'before_notify_callbacks', they only run in the same thread as they
are added and so won't run if created in an initializer. This is a
problem in most (all ?) frameworks, not just Rails

This adds some complexity to the MiddlewareStack, because it now has
to handle procs being passed as middleware. It's important that we
don't wrap the procs earlier for two reasons:

  1. We need to be able to remove them, which is much simpler if
     we can do this by object identity (because the current method
       works that way)
  2. We need to be able to abort the 'queue' of callbacks, if one
     returns false. This is much easier if we have a list of procs
     to run
@imjoehaines imjoehaines merged commit 572a6ce into next Jul 22, 2020
@imjoehaines imjoehaines deleted the on-error-callbacks branch July 22, 2020 13:35
@imjoehaines imjoehaines mentioned this pull request Jul 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants