Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Conversation

natikgadzhi
Copy link
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.

Copy link

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
Collaborator

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
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
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
Copy link
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
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
96 of 97 checks passed
LeSim added a commit to demarches-simplifiees/demarches-simplifiees.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

@sl0thentr0py When will this be released?

@sl0thentr0py
Copy link
Member

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