check threshold before a job is reported to Rollbar

[pacoguzman]

This PR implements a simple Sidekiq threshold similar to the existing for DelayedJob (#318).

If you want to reduce the noise of your Rollbar inbox for Sidekiq jobs that fails only on the first attempt but not the next ones you can define in the Rollbar configuration this threshold so only notify from a number of retry of the current failing job.

By default is disabled the threshold is 0. And for jobs that are not going to be retried we notify always to Rollbar.

Any feedback is welcome, thanks in advance

[jondeandres]

Can you move this to a new method? Something like skip_report? or similar.

[jondeandres]

Thanks for this. Everything looks nice, just one comment in #319 (comment).

[pacoguzman]

Opps we need to keep raising the exception in any case so the job is retried, on it

[pacoguzman]

Forgot my previous comment is not true this is only how we handle the exception to report to rollbar, everything seems fine. I just extracted the code to a method as you suggested

[jondeandres]

@pacoguzman yes, on the #call method we reraise the exception, what should be still done. I think the PR is fine 😄. We'll merge it in next days.

Thanks!

[pacoguzman]

Cool you're welcome

[jondeandres]

@pacoguzman one doubt about this, since now I'm not sure this is the behavior you expected.

First fail on a job will be always reported to Rollbar, since the msg_or_context doesn't have retry set to true.

After the first report, Rollbar reports will be disabled for repeated job until reach the threshold, 0 by default. So for a job failing 5 times and a threshold of 3 we will have:

1 fail - reported (retry false)
2 fail - skipped (retry_count 1)
3 fail - skipped (retry_count 2)
4 fail - reported (retry_count 3)
5 fail - reported (retry_count 4)

Is this the expected behavior?

Thanks.

[pacoguzman]

I'll check to be 100% sure, but I think Sidekiq alwyas provide retry key and when retry key is present the retry_count even for the first attempt. We saw first errors on Rollbar with that value (retry_count = 0)

I'll confirm asap

[jondeandres]

Yes, Sidekiq will provide key retry for first attempt with value false. So with this logic:

msg_or_context.is_a?(Hash) && msg_or_context["retry"] && msg_or_context["retry_count"] < ::Rollbar.configuration.sidekiq_threshold

For the first attempt it's always false, that's ok.

My question was about if you expect to not skip the irst report and then skip reports until reach the threshold.

[pacoguzman]

No that's not the expected behaviour sorry my fault. I'll amend the changes
On Oct 20, 2015 7:25 PM, "Jon" notifications@github.com wrote:

Yes, Sidekiq will provide key retry for first attempt with value false.
So with this logic:

msg_or_context.is_a?(Hash) && msg_or_context["retry"] &&
msg_or_context["retry_count"] < ::Rollbar.configuration.sidekiq_threshold

For the first attempt it's always false, that's ok.

My question was about if you expect to not skip the irst report and
then skip reports until reach the threshold.

—
Reply to this email directly or view it on GitHub
#319 (comment).

[pacoguzman]

I think the behaviour you described above is not what is happening. For example

class RetryableWorker
  include Sidekiq::Worker

  sidekiq_options retry: true

  def perform
  end
end

1 fail - skipped (retry true)
2 fail - skipped (retry true retry_count 1)
3 fail - skipped (retry true retry_count 2)
4 fail - reported (retry true retry_count 3)
5 fail - reported (retry true retry_count 4)

class NoneRetryableWorker
  include Sidekiq::Worker

  sidekiq_options retry: false

  def perform
  end
end

1 fail - reported (retry false)

And this is the expected behaviour, what do you think?

[jondeandres]

mmm @pacoguzman, I think you are right yes 😄, sorry for the confusion.

let me review this again and we'll merge it.

Thanks!

[aselder]

This PR has a bug and should be fixed or reverted: #345