resque: be more specific when handling ActiveJob wrapped jobs

[isnotajoke]

Goal

The Resque Bugsnag integration adds helpful context and payload information to reports from Resque tasks. A check within the integration attempts to unwrap ActiveJob jobs run via Resque. The check used to perform this unwrapping assumes that all jobs will run via ActiveJob, and causes errors when used with jobs other than ActiveJob whose first argument does not respond sensibly to []. For example:

Error in internal notify block: no implicit conversion of String into Integer

after bubbling up to the top-level Bugsnag notify method. This occurs before any Resque-specific context can be added to the report, and yields reports that are less useful than they could be. We want to prevent these errors so Resque reports have more uniform context.

Design

The comment in the current implementation asserts an expectation about the payload of ActiveJob reports, but doesn't actually assert it or guard the following code by checking that assumption. It seemed reasonable to me to guard the error-producing conditional with an explicit assertion following the comment – restricting it to only those payloads for which it makes sense.

Changeset

• Guard ActiveJob-specific Resque handling with an assertion that we're handling an ActiveJob report.

Testing

• Confirmed in my environment that Resque jobs more reliably have Resque context associated with them
• Confirmed in my environment that "Error in internal notify block" errors (associated with this issue) are reduced/eliminated after this fix
• Confirmed in my environment that errors originating in ActiveJob tasks are properly unwrapped
• Added specs for this case

[isnotajoke]

Self-review. I still need to add specs and finish testing this; wanted to open the PR to get feedback on the implementation while I do that.

[isnotajoke]

There's a risk here that this may be too specific or accidentally a breaking change – either because it doesn't account for an ActiveJob-specific quirk, or because the existing code was coincidentally unwrapping exceptions from other frameworks. An alternative implementation would be wrapping this existing logic in a begin/rescue; it would still fail, but in a way that is less disruptive than the existing implementation. I'm open to going that way if it's preferred by maintainers

[imjoehaines]

I think this is a good way to do it 🙂

I was considering checking if ['args'][0] is a hash or responds to [] but, given this is specifically to unwrap the Active Job class, it makes sense to me to make it specific to Active Job

[imjoehaines]

Thanks for the PR, @isnotajoke

Sorry for this, we do have tests that run Resque jobs for real, but unfortunately the jobs aren't passed arguments so didn't trigger this bug 😕

bugsnag-ruby/features/rails_features/integrations.feature

Lines 72 to 111 in 94da895

	@rails_integrations
	Scenario: Resque (no on_exit hooks)
	When I run "bundle exec rake resque:work" in the rails app
	And I run "Resque.enqueue(ResqueWorker)" with the rails runner
	And I wait to receive a request
	Then the request is valid for the error reporting API version "4.0" for the "Ruby Bugsnag Notifier"
	And the event "unhandled" is true
	And the event "context" equals "ResqueWorker@crash"
	And the event "severity" equals "error"
	And the event "severityReason.type" equals "unhandledExceptionMiddleware"
	And the event "severityReason.attributes.framework" equals "Resque"
	And the event "app.type" equals "resque"
	And the exception "errorClass" equals "RuntimeError"
	And the event "metaData.config.delivery_method" equals "synchronous"
	And the event "metaData.context" equals "ResqueWorker@crash"
	And the event "metaData.payload.class" equals "ResqueWorker"
	And the event "metaData.rake_task.name" equals "resque:work"
	And the event "metaData.rake_task.description" equals "Start a Resque worker"
	And the event "metaData.rake_task.arguments" is null
	@rails_integrations
	Scenario: Resque (with on_exit hooks)
	Given I set environment variable "RUN_AT_EXIT_HOOKS" to "1"
	When I run "bundle exec rake resque:work" in the rails app
	And I run "Resque.enqueue(ResqueWorker)" with the rails runner
	And I wait to receive a request
	Then the request is valid for the error reporting API version "4.0" for the "Ruby Bugsnag Notifier"
	And the event "unhandled" is true
	And the event "context" equals "ResqueWorker@crash"
	And the event "severity" equals "error"
	And the event "severityReason.type" equals "unhandledExceptionMiddleware"
	And the event "severityReason.attributes.framework" equals "Resque"
	And the event "app.type" equals "resque"
	And the exception "errorClass" equals "RuntimeError"
	And the event "metaData.config.delivery_method" equals "thread_queue"
	And the event "metaData.context" equals "ResqueWorker@crash"
	And the event "metaData.payload.class" equals "ResqueWorker"
	And the event "metaData.rake_task.name" equals "resque:work"
	And the event "metaData.rake_task.description" equals "Start a Resque worker"
	And the event "metaData.rake_task.arguments" is null

I'll make sure to change them to have arguments to check for this case and I can add some specs for this as well

[imjoehaines]

I think this is a good way to do it 🙂

I was considering checking if ['args'][0] is a hash or responds to [] but, given this is specifically to unwrap the Active Job class, it makes sense to me to make it specific to Active Job

[imjoehaines]

I'll merge this in #711 where I've added & updated some tests

Thanks again!

[xljones]

@isnotajoke this has been released in https://github.com/bugsnag/bugsnag-ruby/releases/tag/v6.24.1 thanks for the contribution! 🎉