notify Sentry about RetryLimitHit only from sidekiq #393
Conversation
We were notifying Sentry both through Media.give_up and through
configuration inside 03_sidekiq. We are keeping only the one in 03_sidekiq.rb.
It’s the most specific error and the right place.
In order to try to get more information, we are replacing
Sentry.capture_exception(ex) by PenderSentry.notify(ex, { job: job })
I also tried writing a test to make sure teh Archiver isn't notifying
Sentry anymore.
config/initializers/03_sidekiq.rb
Outdated
| @@ -12,7 +12,7 @@ | |||
|
|
|||
| config.death_handlers << ->(job, ex) do | |||
| if ex.is_a?(Pender::Exception::RetryLater) | |||
| ex = Pender::Exception::RetryLimitHit.new(ex) | |||
| ex = Pender::Exception::RetryLimitHit.new(ex, {job: job}) | |||
There was a problem hiding this comment.
The exception doesn't take a second argument, it should remain like it was:
ex = Pender::Exception::RetryLimitHit.new(ex)
config/initializers/03_sidekiq.rb
Outdated
| @@ -12,7 +12,7 @@ | |||
|
|
|||
| config.death_handlers << ->(job, ex) do | |||
| if ex.is_a?(Pender::Exception::RetryLater) | |||
| ex = Pender::Exception::RetryLimitHit.new(ex) | |||
| ex = Pender::Exception::RetryLimitHit.new(ex, {job: job}) | |||
| end | |||
| Sentry.capture_exception(ex) | |||
There was a problem hiding this comment.
What changes is here. We need to be able to send additional information along with the Sentry notification. I don't know if Sentry.capture_exception does that. If not, I suggest replacing this line by:
PenderSentry.notify(ex, { job: job })
There was a problem hiding this comment.
To send the extra information we need to set the scope, we have abstracted that in PenderSentry.notify. So I updated to use it.
The exception doesn't take a second argument. We need to send the extra intormation to Sentry, that is done through the scope. Our notify method inside PenderSentry abstracts that for us.
|
Code Climate has analyzed commit 5a0da73 and detected 0 issues on this pull request. The test coverage on the diff in this pull request is 100.0% (50% is the threshold). This pull request will bring the total coverage in the repository to 98.6% (0.0% change). View more on Code Climate. |
Not sure. Since the other tests inside this file had it (the In our tests where we don't have the What do you think? |
OK, sounds good to keep it as is for now. |
Description
We were notifying Sentry both through
Media.give_upand throughconfiguration inside
03_sidekiq. We are keeping only the one in03_sidekiq.rb.It’s the most specific error and the right place.
In order to try to get more information, we are replacing
Sentry.capture_exception(ex) by PenderSentry.notify(ex, { job: job })References: 3706, 3757, 3707
How has this been tested?
I tried writing a test to make sure teh Archiver isn't notifying Sentry anymore:
test "MediaArchiver should not notify Sentry when the worker hits the maximum number of retries"
rails test test/models/archiver_test.rb:746Things to pay attention to during code review
I ran a bunch of tests to make sure the test is working, but I'm not sure about it: if it makes sense the way it's written, if it's even needed.