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

Fix deprecation warning from sidekiq error handler #2261

Merged
merged 1 commit into from Oct 17, 2023
Merged

Fix deprecation warning from sidekiq error handler #2261

merged 1 commit into from Oct 17, 2023

Conversation

fukayatsu
Copy link
Contributor

Overview

Fix deprecation warning: DEPRECATION: Sidekiq exception handlers now take three arguments...

DEPRECATION: Sidekiq exception handlers now take three arguments, see #<Proc:0x00007f9015db7ef8 /home/me/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/newrelic_rpm-9.5.0/lib/new_relic/agent/instrumentation/sidekiq.rb:36>

NOTE: This warning is shown when using sidekiq main branch.

Submitter Checklist:

  • Include a link to the related GitHub issue, if applicable
  • [ ] Include a security review link, if applicable

Testing

The agent includes a suite of unit and functional tests which should be used to
verify your changes don't break existing functionality. These tests will run with
GitHub Actions when a pull request is made. More details on running the tests locally can be found
here for our unit tests,
and here for our functional tests.
For most contributions it is strongly recommended to add additional tests which
exercise your changes.

Reviewer Checklist

  • Perform code review
  • Add performance label
  • Perform appropriate level of performance testing
  • Confirm all checks passed
  • Add version label prior to acceptance

@CLAassistant
Copy link

CLAassistant commented Oct 16, 2023

CLA assistant check
All committers have signed the CLA.

@github-actions github-actions bot added the community To tag external issues and PRs submitted by the community label Oct 16, 2023
@fukayatsu
Copy link
Contributor Author

Testing

I tested locally by $ rake 'test:multiverse[sidekiq]' with below diff:

diff --git a/test/multiverse/suites/sidekiq/Envfile b/test/multiverse/suites/sidekiq/Envfile
index 56fee0a97..4f7f19566 100644
--- a/test/multiverse/suites/sidekiq/Envfile
+++ b/test/multiverse/suites/sidekiq/Envfile
@@ -8,13 +8,11 @@ end
 
 SIDEKIQ_VERSIONS = [
   [nil, 2.7],
-  ['6.4.0', 2.5],
-  ['5.0.3', 2.4, 2.5]
 ]
 
 def gem_list(sidekiq_version = nil)
   <<-RB
-    gem 'sidekiq'#{sidekiq_version}
+    gem 'sidekiq', github: 'sidekiq/sidekiq', branch: 'main'
     gem 'newrelic_rpm', :require => false, :path => File.expand_path('../../../../')
   RB
 end
diff --git a/test/multiverse/suites/sidekiq/sidekiq_test_helpers.rb b/test/multiverse/suites/sidekiq/sidekiq_test_helpers.rb
index 0d0546853..c892ddc8b 100644
--- a/test/multiverse/suites/sidekiq/sidekiq_test_helpers.rb
+++ b/test/multiverse/suites/sidekiq/sidekiq_test_helpers.rb
@@ -69,7 +69,7 @@ module SidekiqTestHelpers
     @@cli ||= begin
       cli = Sidekiq::CLI.instance
       cli.parse(['--require', File.absolute_path(__FILE__), '--queue', 'default,1'])
-      cli.logger.instance_variable_get(:@logdev).instance_variable_set(:@dev, File.new('/dev/null', 'w'))
+      # cli.logger.instance_variable_get(:@logdev).instance_variable_set(:@dev, File.new('/dev/null', 'w'))
       ensure_sidekiq_config(cli)
       cli
     end

Copy link
Contributor

@kaylareopelle kaylareopelle left a comment

Choose a reason for hiding this comment

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

Thanks for this fix and your related work in the sidekiq repo, @fukayatsu!

@kaylareopelle kaylareopelle merged commit 0f6cb1a into newrelic:dev Oct 17, 2023
26 checks passed
@fukayatsu fukayatsu deleted the fix-sidekiq-deprecation-warning branch October 18, 2023 00:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community To tag external issues and PRs submitted by the community hacktoberfest-accepted
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

3 participants