Skip to content

Conversation

@benhutton
Copy link
Contributor

This is an implementation for #1632.

I am bringing over the concept of attempt_threshold from Honeybadger.

Retry counting is a bit tricky, and so I included a comment based on a GH comment from Honeybadger.

There are two relatively arbitrary decisions I made here that you should validate:

  1. I am 1-indexing attempts, not 0-indexing
  2. I am skipping until the number of attempts equals the attempt_threshold. Alternatively, we could skip until number of attempts is greater than attempt_threshold.

Thanks for your consideration!

@codecov
Copy link

codecov bot commented Dec 24, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.18%. Comparing base (bf8d8c5) to head (536166e).
Report is 2 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2503   +/-   ##
=======================================
  Coverage   98.18%   98.18%           
=======================================
  Files         128      128           
  Lines        4837     4845    +8     
=======================================
+ Hits         4749     4757    +8     
  Misses         88       88           
Components Coverage Δ
sentry-ruby 98.57% <100.00%> (ø)
sentry-rails 97.07% <100.00%> (+0.15%) ⬆️
sentry-sidekiq 97.09% <100.00%> (-0.33%) ⬇️
sentry-resque 92.85% <ø> (ø)
sentry-delayed_job 95.65% <ø> (ø)
sentry-opentelemetry 99.31% <ø> (ø)
Files with missing lines Coverage Δ
sentry-sidekiq/lib/sentry/sidekiq/error_handler.rb 94.73% <100.00%> (-1.93%) ⬇️

... and 1 file with indirect coverage changes

Copy link
Collaborator

@solnic solnic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, this LGTM. Could you update the CHANGELOG.md (top level one) too so that we can get a green build?

@benhutton
Copy link
Contributor Author

@solnic sorry for the delay. Updated!

@solnic solnic merged commit 4a0f70e into getsentry:master Jan 2, 2025
147 checks passed
@simoleone
Copy link

What's the intended interaction with report_after_job_retries? At the moment report_after_job_retries takes precedence and reports aren't sent until retries are exhausted regardless of attempt_threshold.

I think it would be useful if when report_after_job_retries: true then attempt_threshold could be used to selectively lower the threshold for some jobs (so attempt_threshold would take precedence).

What do you think? It's just a matter of flipping the order of the conditionals.

@benhutton
Copy link
Contributor Author

personally I have no objection to that... I think my use case would be supported either way!

@benhutton benhutton deleted the attempt_threshold branch March 24, 2025 13:32
@Linuus
Copy link

Linuus commented Aug 26, 2025

Any reasons this is not documented anywhere?

@solnic
Copy link
Collaborator

solnic commented Sep 2, 2025

Any reasons this is not documented anywhere?

@Linuus good point, we should definitely document this and related features as it's kinda tricky 😅 I'll make sure we get it done!

@Linuus
Copy link

Linuus commented Sep 2, 2025

Any reasons this is not documented anywhere?

@Linuus good point, we should definitely document this and related features as it's kinda tricky 😅 I'll make sure we get it done!

Thanks :) I was mostly curious if it was some beta/internal thing. Just to make sure we don't depend on something we shouldn't.

@solnic
Copy link
Collaborator

solnic commented Sep 3, 2025

@Linuus oh, sorry about the confusion. Let me clarify then - it is a first-class feature and you can rely on this 🙂

@hibachrach
Copy link

One issue with this solution is it means jobs that are out of the developer's control (e.g. Sidekiq::Batch::Callback) cannot be configured with sidekiq_options. Instead, you need to set via some Sidekiq middleware.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants