first cut at nested exception support

[sahglie]

I'd like to try and get support for nested exceptions into errbit. This current pull request is just a first cut to get some feedback. Currently there are no tests, but once I get some clarity on my approach I will definitely add them.

Here is the exception I'm raising:

#!/usr/bin/env ruby
require_relative "../config/environment"
require 'airbrake/logger'

Rails.logger = Airbrake::AirbrakeLogger.new(Rails.logger)

class One
  Error = Class.new(StandardError)

  def self.run
    raise Error.new("raise one error")
  end
end

class Two
  Error = Class.new(StandardError)

  def self.run
    One.run
  rescue One::Error => e
    raise Error.new("re-raise and wrap one error")
  end
end

begin
  Two.run
rescue Two::Error => e
  Rails.logger.error(e)
end

and here is a screenshot of the backtrace in errbit using my nested exception change:

[stevecrozz]

Thanks for picking this up! I'm sure our users will love it if we can get support for multiple exceptions. The first thing I'm thinking is about how to best model these data.

Right now we have a Backtrace model and in this first iteration I see that you're using one Backtrace model for containing the backtraces from multiple exceptions. Currently, we have a model called Notice which represents the notice received and also has properties like message and error_class which are properties of the exception, and it also belongs_to one backtrace.

The thing we want to add involves augmenting this Notice with the concept of multiple exceptions. Does it make sense to store the first exception's metadata in the message and error_class fields while storing all the backtraces in the backtrace model? I'm not sure yet. It does seem to offer a quick win so that's worth considering.

Maybe a more "correct" thing to do would be to create a new model for representing an exception (error_class, message and backtrace) and embeds_many of them in the Notice model. It seems like a nice change, but it would also be a pretty big change. In a perfect world, I think this would be my preference, but I also don't have the time to make this bigger change so that's one point against this approach.

Does that spark any thoughts for you?

[sahglie]

Thanks for taking a look at my pull request. I definitely did the simplest thing that would possibly work, even though it was a bit of a hack. I like your idea of creating a separate exception model to encapsulate the exception behavior. I'll start digging into the code and see how much effort it will take to put this all together. This is definitely something I have the time/interest to put together.

On Wed, Feb 13, 2019 at 3:33 PM Stephen Crosby [email protected] wrote:

Thanks for picking this up! I'm sure our users will love it if we can get support for multiple exceptions. The first thing I'm thinking is about how to best model these data.

Right now we have a Backtrace model and in this first iteration I see that you're using one Backtrace model for containing the backtraces from multiple exceptions. Currently, we have a model called Notice which represents the notice received and also has properties like message and error_class which are properties of the exception, and it also belongs_to one backtrace.

The thing we want to add involves augmenting this Notice with the concept of multiple exceptions. Does it make sense to store the first exception's metadata in the message and error_class fields while storing all the backtraces in the backtrace model? I'm not sure yet. It does seem to offer a quick win so that's worth considering.

Maybe a more "correct" thing to do would be to create a new model for representing an exception (error_class, message and backtrace) and embeds_many of them in the Notice model. It seems like a nice change, but it would also be a pretty big change. In a perfect world, I think this would be my preference, but I also don't have the time to make this bigger change so that's one point against this approach.

Does that spark any thoughts for you?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/errbit/errbit/pull/1422#issuecomment-463419702, or mute the thread https://github.com/notifications/unsubscribe-auth/AABJC-NW-qivIMYnO6C3MxtZv71p9fhJks5vNKDbgaJpZM4a1PkA .