Add option to notify Sentry only after the last retry on resque#2087
Conversation
Codecov ReportAll modified lines are covered by tests ✅
... and 50 files with indirect coverage changes 📢 Thoughts on this report? Let us know!. |
|
Hello, any update on this? |
|
/cc @sl0thentr0py |
36f916f to
27beb99
Compare
sl0thentr0py
left a comment
There was a problem hiding this comment.
hey sorry for the delay on this but I don't think this solution is something we can put in the SDK (see comment)
If there's another possibility to fix the logic, I can take that otherwise I'll dig into resque later.
sentry-resque/lib/sentry/resque.rb
Outdated
|
|
||
| if Sentry.configuration.resque.report_after_job_retries && defined?(::Resque::Plugins::Retry) == 'constant' && klass.is_a?(::Resque::Plugins::Retry) | ||
| retry_key = klass.redis_retry_key(payload['args']) | ||
| retry_attempt = ::Resque.redis.get(retry_key) |
There was a problem hiding this comment.
the SDK should never do its own database calls by any chance, that's a side effect that breaks all sorts of contracts.
Is there no way of knowing the retry status from the stuff available to this function?
There was a problem hiding this comment.
@sl0thentr0py In fact, there is. I didn't realize it, but we are catching an exception here (so the processed job context is available) and when we use constantize against the class name, resque-retry will capture the job context, so methods retry_limit_reached? and retry_limit_attempt will expose the current "status" of the job.
I just pushed a new commit that fixes it. Hopefully it will match the requirements
76fa859 to
2b1ab7d
Compare
sl0thentr0py
left a comment
There was a problem hiding this comment.
sorry for the delay again and ty @GlauberrBatista !
|
@sl0thentr0py not a problem and thank you 🙌 |
sentry-sidekiqandsentry-delayed_jobhave the option to report the failure only after the retry process has finished.With
sentry-resquewe can do the same when using theresque-retrygem. The implementation is not as clean asSidekiqandDelayedJob, which expose the attempts in a better way, but still, I was able to make it work.I noticed the configuration option was there but not implemented. I added a conditional inside the exception block to check if the job is an instance of
Resque::Plugins::Retryand if so, it checks the retry key.An odd behavior:
resque-retrycleans the key on the last try, before being caught on the exception block. Thus, when the key no longer exists on Redis, it means the retries were exhausted.I understand this change implies using the
resque-retrygem, but I'm unaware of a similar gem widely used asresque-retry.