Skip to content

(sentry-sidekiq): Fixed a deprecation warning in error handler#2160

Merged
sl0thentr0py merged 3 commits intogetsentry:masterfrom
natikgadzhi:sentry-sidekiq/sidekiq-7.1.5-error-handler-deprecation
Nov 13, 2023
Merged

(sentry-sidekiq): Fixed a deprecation warning in error handler#2160
sl0thentr0py merged 3 commits intogetsentry:masterfrom
natikgadzhi:sentry-sidekiq/sidekiq-7.1.5-error-handler-deprecation

Conversation

@natikgadzhi
Copy link
Copy Markdown
Contributor

@natikgadzhi natikgadzhi commented Nov 3, 2023

Summary

This PR fixes the deprecation warning that occurs when using Sidekiq >= 7.1.5: #2157. Closes #2157.

Changes

I'm just getting started, so I documented some steps as I was onboarding:

  • Added more instructions to CONTRIBUTING.md, and bumped Rails version there.
  • Bumped default Sidekiq version to 7.0 in sentry-sidekiq Gemfile.
  • Fixed the deprecation warning in the error handler and added a comment there.
  • Also added a changelog entry linking to this pull request.

Next steps

We could in theory use some of those configuration values to enrich the reported errors: https://github.com/sidekiq/sidekiq/blob/main/lib/sidekiq/config.rb, but, they are not specific to the error at hand.

@codecov
Copy link
Copy Markdown

codecov bot commented Nov 4, 2023

Codecov Report

Merging #2160 (cc60849) into master (b2d1aef) will decrease coverage by 0.03%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #2160      +/-   ##
==========================================
- Coverage   97.33%   97.30%   -0.03%     
==========================================
  Files          97       97              
  Lines        3638     3638              
==========================================
- Hits         3541     3540       -1     
- Misses         97       98       +1     
Components Coverage Δ
sentry-ruby 98.02% <ø> (ø)
sentry-rails 94.98% <ø> (ø)
sentry-sidekiq 93.70% <100.00%> (ø)
sentry-resque 92.06% <ø> (-1.59%) ⬇️
sentry-delayed_job 94.36% <ø> (ø)
sentry-opentelemetry 100.00% <ø> (ø)
Files Coverage Δ
sentry-sidekiq/lib/sentry/sidekiq/error_handler.rb 93.10% <100.00%> (ø)

... and 1 file with indirect coverage changes

# defaults to Sidekiq's default configuration `Sidekiq.default_configuration`
# Sidekiq will pass the config in starting Sidekiq 7.1.5, see
# https://github.com/sidekiq/sidekiq/pull/6051
def call(ex, context, sidekiq_config = ::Sidekiq.default_configuration)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think ::Sidekiq.default_configuration was introduced in 7.0 so this will break older Sidekiq versions. Also, we need to update retry_limit to use the passed sidekiq_config if its present.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Hmmm. You're right — I think then we could set it to nil by default, but if it's present and has retry_limit, use that. I'll fix this up.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thank you again for pointing this out, and for the deferred cleanup of the env var. I think I got to it, not sure if my code is the most elegant.

@natikgadzhi natikgadzhi requested a review from st0012 November 10, 2023 00:16
@natikgadzhi
Copy link
Copy Markdown
Contributor Author

@st0012 that one should be good to go now, I think.

@sl0thentr0py sl0thentr0py force-pushed the sentry-sidekiq/sidekiq-7.1.5-error-handler-deprecation branch from 56ea1db to cc60849 Compare November 13, 2023 13:12
Copy link
Copy Markdown
Member

@sl0thentr0py sl0thentr0py left a comment

Choose a reason for hiding this comment

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

@sl0thentr0py sl0thentr0py merged commit e082644 into getsentry:master Nov 13, 2023
LeSim added a commit to demarche-numerique/demarche.numerique.gouv.fr that referenced this pull request Nov 22, 2023
To avoid a deprecation warning from sidekiq relative to exception handling, we need getsentry/sentry-ruby#2160 not yet released
@rafaelsales
Copy link
Copy Markdown

@sl0thentr0py When will this be released?

@sl0thentr0py
Copy link
Copy Markdown
Member

@rafaelsales this was already released in https://github.com/getsentry/sentry-ruby/releases/tag/5.14.0

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.

Deprecation warning in sentry-sidekiq: Sidekiq exception handlers now take three arguments

4 participants