Remove RISC network errors from NewRelic

[vrajmohan]

🎫 Ticket

Link to the relevant ticket:
https://gitlab.login.gov/lg-people/lg-people-appdev/Melba/backlog-fy24/-/issues/37

🛠 Summary of changes

Don't bubble up the network errors. Let ActiveJob retry based on its configuration.

📜 Testing Plan

Provide a checklist of steps to confirm the changes.

• Network errors in the RISC delivery job should not show up in NewRelic errors dashboard

[mitchellhenke]

Does removing this affect the retry logic? I think it may depend on the job failing.

[vrajmohan]

I was wondering about that as well.

I also have a question here about whether we would also like to reduce the events logged.

The best approach seems to be:
Provide a block to retry_on to log the event after the last attempt fails. Don't do any rescue within the perform.

[vrajmohan]

These 2 statements in the Rails ActiveJob guide do seem to indicate that @mitchellhenke is right:

If an exception from a job is not rescued, then the job is referred to as "failed".

A failed job will not be retried, unless configured otherwise.

[mitchellhenke]

Logging is fine to keep for every try imo

[zachmargolis]

Looking at this, and looking at the ActiveJob source code for retry_on, I think the way to stop reporting exceptions to NewRelic would be to add a block to our retry_on up top and make it a no-op, because the block is only called on the last/final time, so providing a block would be tell it to not keep raising

[vrajmohan]

From the GoodJob documentation:

When using retry_on with a limited number of retries, the final exception will not be rescued and will raise to GoodJob's error handler. To avoid this, pass a block to retry_on to handle the final exception instead of raising it to GoodJob:

class ApplicationJob < ActiveJob::Base
  retry_on StandardError, attempts: 5 do |_job, _exception|
    # Log error, do nothing, etc.
  end
  # ...
end

Perhaps the right thing to do is to leave the existing rescue handler in, and use the block argument to retry_on to do nothing. This would work if the errors we are seeing on NewRelic is only after the retries have failed. I'll verify that this is true.

[vrajmohan]

Jinx

[zachmargolis]

It's a little unclear from this why we have a blank block, I think this would be a good candidate for a comment, or since we have 2, extracing a proc into a variable with a clear name that indicates what's going on

  FINAL_EXCEPTION_HANDLER_NO_OP = proc { |_job, _exception| }.freeze

  retry_on(
    *NETWORK_ERRORS,
    wait: :polynomially_longer,
    attempts: 2,
    &FINAL_EXCEPTION_HANDLER_NO_OP
  )

[vrajmohan]

To me, a comment would be clear and concise, but if a named Proc would be idiomatic, I'll go with that.

[zachmargolis]

It's 6 of one, half a dozen of the other. I don't think one is more idiomatic

[zachmargolis]

So the code in this test never triggers a call to NewRelic (that happens as part of the GoodJob.on_thread_error that we set elsewhere)

So since this wouldn't catch a regression, I would remove it