Duplicate reporting for Sidekiq errors

[lostie]

Issue Description

After the update to version 5.1.0 I started to get Sidekiq::JobRetry::Skip errors in Sentry. After close examination, I found that for each of these errors I also had their cause reported as a separate error in Sentry.

It seems the Sidekiq error handler:

sentry-ruby/sentry-sidekiq/lib/sentry-sidekiq.rb

Line 28 in 623d046

config.error_handlers << Sentry::Sidekiq::ErrorHandler.new

is creating the sentry error for the cause, and the new error subscription for version 5.1.0:

sentry-ruby/sentry-rails/lib/sentry/rails/railtie.rb

Line 123 in 623d046

app.executor.error_reporter.subscribe(Sentry::Rails::ErrorSubscriber.new)

is creating the sentry error for the Sidekiq::JobRetry::Skip

Reproduction Steps

Use the 5.1.0 version of Sentry and raise an error within a sidekiq job.

Expected Behavior

The raised error should be logged in Sentry

Actual Behavior

Two errors are logged in sentry:

• The raised error
• The Sidekiq internal Sidekiq::JobRetry::Skip

Ruby Version

3.1.0

SDK Version

5.1.0

Integration and Its Version

Rails (7.0.2) + Sidekiq (6.4.1)

Sentry Config

I don't have any custom error handling configured

[st0012]

Hi thanks for reporting this. This is caused by the Rails 7 feature support we recently added in #1705

I've explained the problem in this comment and proposed an API that I think would be the best solution. But that could take a while to be addressed and released though (if it's accepted). So I'll also think about other workaround/short-term solutions to solve this. The worst case would be to revert #1705 .

cc @sl0thentr0py

[sl0thentr0py]

@st0012 sorry I don't understand all the details so a couple of questions.

• why does the @__sentry_captured instance var not work here? Are they two different exception objects?
• do we always want only one of the railserror_reporter and the sidekiq error_handlers to be on in an application or is this a more local disabling you're proposing? If it's the former, we could just check the version/presence of either of those objects and not add if the other exists. Or am I missing something?

[st0012]

why does the @__sentry_captured instance var not work here?

Because in this case the order is different. The exception was first caught by the error_reporter, then the sentry-sidekiq. And in this case we want sentry-sidekiq to catch it, but there's no way we can tell error_reporter to stop.

If it's the former, we could just check the version/presence of either of those objects and not add if the other exists.

Regarding this point, I've also explained it in https://github.com/rails/rails/pull/43625/files#r810051167

[sl0thentr0py]

@st0012 thx I think I get it now.
What about moving this check to capture_exception in general, so the double reporting solution is not specific to the rails error_subscriber but we will generally avoid reporting all duplicate exceptions?

This would also be consistent with other SDKs which have DedupeIntegrations.

This still would not fix the Sidekiq order precedence problem though.

[jasonrudolph]

After the update to version 5.1.0 I started to get Sidekiq::JobRetry::Skip errors in Sentry. After close examination, I found that for each of these errors I also had their cause reported as a separate error in Sentry.

I've upgraded to 5.1.1 (which includes the fixes in #1738), but I'm still experiencing this issue.

@lostie: If you've upgraded to 5.1.1, did that resolve this problem for you, or are you still seeing it as well?

[micke]

We are also still seing these after upgrading to 5.1.1

[st0012]

@jasonrudolph @micke thanks for reporting. I've done a bit more investigation and reported it in rails/rails#43625 (comment).

[st0012]

After consulting Sidekiq's author Mike, I think ignoring Sidekiq::JobRetry::Skip can be an adhoc but effective solution to this problem. So I added #1763 for it.

[st0012]

FYI, I'll change the error reporter integration in 5.3.0. More details: rails/rails#43625 (comment)