honeybadger-io / honeybadger-ruby

Ruby gem for reporting errors to honeybadger.io
https://docs.honeybadger.io/lib/ruby/
MIT License
251 stars 145 forks source link

Honeybadger Rack middleware ignores Rails error context when creating notice for unhandled error #648

Closed douglasshuang closed 4 days ago

douglasshuang commented 1 week ago

We're taking advantage of Honeybadger's integration with the Rails error reporter wherever possible. For example, instead of calling Honeybadger.notify, we call Rails.error.report.

We discovered that when creating a notice for an unhandled error, Honeybadger's Rack middleware ignores any Rails error context we may have set by calling Rails.error.set_context. We know that we could use Honeybadger.context instead, but we'd rather use the built-in Rails mechanism.

After studying the configuration documentation and examining the source code in lib/honeybadger/rack/error_notifier.rb, we were unable to find a way to coax the middleware to use the Rails error context. We ended up implementing a workaround as follows in our config/initializers.honeybadger.rb:

if defined?(Honeybadger)
  Honeybadger.configure do |config|
    ...
  end

  class HoneybadgerContextSetter
    def self.report(exception, handled:, severity:, context: {}, source: nil)
      Honeybadger.context(context) unless handled
    end
  end

  Rails.error.subscribe(HoneybadgerContextSetter)
end

The dummy subscriber sets the Honeybadger context from the provided Rails error context, so that it's available to Honeybadger::Rack::Notifier#call when it gets invoked.

Is there a better way to accomplish what we want?

We see the comment in Honeybadger::Plugins::Rails::ErrorSubscriber that says, "Unhandled errors will be caught by our integrations (eg middleware), which have richer context than the Rails error reporter", but the middleware is ignoring the context we've provided to the Rails error reporter. What are we missing?

roelbondoc commented 1 week ago

Hi @douglasshuang,

If you take a look at the ErrorSubscriber middleware here, the context is passed along to Honeybadger.notify. So this should work if you have called #set_context before reporting the error.

I did the following a rails console:

Rails.error.set_context(test: 'foo')
Rails.error.report(Error)

This context shows up for me in the error details:

image

Perhaps there is another scenario where this doesn't work? Would you be able to provide an example of how you set the context and then report the error?

douglasshuang commented 6 days ago

Hi @roelbondoc,

Thank you for the response. We know that the Rails error context is passed along when calling Rails.error.report. The scenario I'm describing involves the Honeybadger Rack middleware and its reporting of unhandled errors. Here is some example Rails controller code to illustrate:

class FooController < ApplicationController
  def index
    Rails.error.set_context(test: 'foo')
    raise 'bar'
  end
end

Making a request for that index page should trigger a Honeybadger notice that is missing the context. If you're still not able to reproduce, please let me know, and I can try to create a minimal Rails app that will do it.

roelbondoc commented 6 days ago

Hey @douglasshuang, sorry about that, I understand now. Your example is perfect.

I think what you are doing with the subscriber to set the context makes sense. In fact, perhaps the context setting can be done in the Honeybadger::Plugins::Rails::ErrorSubscriber. If I get some time this week I'll see about modifying that. If you have any thoughts about it, feel free to open a PR as well!

Here's an alternative using the before_notify callback with our gem:

Honeybadger.configure do |config|
  config.before_notify do |notice|
    notice.context.merge!(ActiveSupport::ExecutionContext.to_h)
  end
end

Not sure if this any better as it requires to dig into the internals of ActiveSupport::ExecutionContext. But at least this will allow your errors to still be able to report the context if for some reason the Rails error handler doesn't catch it (if that's a concern at all).

douglasshuang commented 6 days ago

@roelbondoc Thank you!

We are seeing a similar problem with Honeybadger::Plugins::ActiveJob. It is ignoring the Rails error context when creating a notice for an unhandled error. Here is an example job that should reproduce the problem:

class MyJob < ApplicationJob
  def perform
    Rails.error.set_config(test: 'foo')
    raise 'bar'
  end
end

Is that something you can address as well?

roelbondoc commented 5 days ago

Hey @douglasshuang, I released v5.25.0 with a change similar to what you have implemented above. Let me know how that works for you!

Let me take a deeper look into the issue with ActiveJob.

douglasshuang commented 4 days ago

@roelbondoc The change in v5.25.0 works great, thank you! Please let me know your thoughts about the Active Job issue when you get a chance.

roelbondoc commented 4 days ago

@roelbondoc The change in v5.25.0 works great, thank you! Please let me know your thoughts about the Active Job issue when you get a chance.

@douglasshuang we just released v5.26.0 that changes the way the context is set. I believe this should now work within ActiveJob as well!