[active_job] support exception reporting only after last retry failed

[solnic]

This is an updated version of #2500 which adds support for active_job_report_after_job_retries option which makes reporting an exception upon a job failure to happen only on the last attempt retry failure.

[solnic]

Please ignore Danger here, it just fails to see that I added CHANGELOG entry.

[st0012]

LGTM. Left some suggestions for tests.

[st0012]

I think with our current setup this will always be true?

[solnic]

@st0012 not really, it may or may not be defined, depending on the order of loading files which is pretty much random 🙃

[github-actions]

	Fails
🚫	Please consider adding a changelog entry for the next release.

Instructions and example for changelog

Please add an entry to CHANGELOG.md to the "Unreleased" section. Make sure the entry includes this PR's number.

Example:

## Unreleased

- [active_job] support exception reporting only after last retry failed ([#2573](https://github.com/getsentry/sentry-ruby/pull/2573))

If none of the above apply, you can opt out of this check by adding #skip-changelog to the PR description.

Generated by 🚫 dangerJS against 4e59569

[aburgel]

Hi there, I came across this issue while trying to understand why I don't see any sentry issues from failing ActiveJob jobs until the final retry attempt. I'm using sentry-rails 5.23 and based on what this PR is trying to do, it seems like I should be seeing a Sentry issue on each retry. But I don't. That's a bit confusing to me, so I did a bit of investigation.

The first thing I found is that the test setup is not representative of how ActiveJob typically runs in the real world. In particular, these lines do some subtle things that don't work well with retry_on.

assert_performed_jobs 3 do
  FailedJobWithRetryOn.perform_later rescue nil
end

With the test adapter, assert_performed_jobs will run all enqueued jobs immediately as well as all jobs that those jobs enqueue. Where things fall apart is that retry_on will reenqueue the job when it fails, but because the test adapter immediately runs the job upon enqueuing it, it will immediately fail again. This causes an exception to be raised in the ActiveJob exception handler, causing the exception to propagate.

So in this test scenario, the Sentry ActiveJob error handler will be called upon each retry, but I don't think that's what happens in the real world.

If you change this test to look like this:

-        assert_performed_jobs 3 do
-          FailedJobWithRetryOn.perform_later rescue nil
-        end
+        FailedJobWithRetryOn.perform_later
+
+        perform_enqueued_jobs
+        perform_enqueued_jobs
+        perform_enqueued_jobs rescue nil

Then you should see that only 1 call to the sentry error handler is made. This is more representative of how ActiveJob actually behaves because the retry handler will enqueue the job but not run it until the next call to perform_enqueued_jobs. In other words, with this approach, the retry handler will complete successfully and the exception will not propagate.

---

But then this leads to my original question, does sentry-rails (even with this PR) capture an exception on every retry with ActiveJob? I don't quite see how this happens.

Sentry::Rails::ActiveJobExtensions gets placed in the ancestor chain above all the ActiveJob internal modules, which means its implementation of perform_now will wrap the internal ActiveJob implementations. And since those internal ActiveJob methods are where the retry logic happens, Sentry won't even see the exception when the retry logic handles it.

So I'm left a little confused, I'm guessing I've missed something. Perhaps some ActiveJob adapters behave differently (I'm using solid_queue)?

[solnic]

@aburgel hey Alex, thanks for bringing this up. I'll look into this and re-visit this PR and related issue!

[solnic]

@aburgel I've reported an issue about your discovery and will take care of this #2597

[sfcgeorge]

Aaah thanks @solnic I had exactly the same experience as @aburgel. Our retries also don't get reported, we're using GoodJob AJ backend in case that makes a difference.