Skip to content

Use attempt_threshold to skip reporting on first N attempts#2503

Merged
solnic merged 2 commits intogetsentry:masterfrom
benhutton:attempt_threshold
Jan 2, 2025
Merged

Use attempt_threshold to skip reporting on first N attempts#2503
solnic merged 2 commits intogetsentry:masterfrom
benhutton:attempt_threshold

Conversation

@benhutton
Copy link
Copy Markdown
Contributor

@benhutton benhutton commented Dec 23, 2024

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
Copy Markdown

codecov bot commented Dec 24, 2024 •
edited
Loading

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

solnic
solnic reviewed Dec 24, 2024
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

benhutton commented Dec 31, 2024

@solnic sorry for the delay. Updated!

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

simoleone commented Mar 21, 2025

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
Copy Markdown
Contributor Author

benhutton commented Mar 24, 2025

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
Copy Markdown

Linuus commented Aug 26, 2025

Any reasons this is not documented anywhere?

@solnic
Copy link
Copy Markdown
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
Copy Markdown

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
Copy Markdown
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
Copy Markdown

hibachrach commented Sep 23, 2025

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

Reviewers

@solnic solnic solnic approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@benhutton @simoleone @Linuus @solnic @hibachrach