bugsnag / bugsnag-ruby

BugSnag error monitoring & reporting software for rails, sinatra, rack and ruby
https://docs.bugsnag.com/platforms/ruby
MIT License
249 stars 174 forks source link

Fix deprecation warning from Sidekiq error handler #796

Closed fukayatsu closed 11 months ago

fukayatsu commented 1 year ago

Goal

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

2023-10-16T01:33:49.575Z pid=3856681 tid=2am91 INFO: DEPRECATION: Sidekiq exception handlers now take three arguments, see #<Proc:0x00007f51009a97c0 /home/me/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/bugsnag-6.26.0/lib/bugsnag/integrations/sidekiq.rb:53>

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

This warning is related to

Design

https://github.com/sidekiq/sidekiq/pull/6051#issuecomment-1733920934.

Changeset

I added a third optional argument to sidekiq's error handler.

Testing

  1. docker run --rm -it -p 16379:6379 redis:7
  2. Save below script as test.rb and run ruby test.rb
if $0 == __FILE__
  require 'bundler/inline'

  gemfile do
    source 'https://rubygems.org'
    gem 'sidekiq', github: 'sidekiq/sidekiq', branch: 'main'

    # Deprecation warning shown
    # gem 'bugsnag'

    # No deprecation warning
    gem 'bugsnag', github: 'fukayatsu/bugsnag-ruby', branch: 'fix-sidekiq-deprecation-warning'
  end

  require 'sidekiq/cli'
  require 'bugsnag'

  ::Sidekiq.configure_server do |config|
    config.redis = { url: 'redis://127.0.0.1:16379' }
    Bugsnag::Sidekiq.configure_server(config)
  end

  sidekiq_thread = Thread.start do
    cli = Sidekiq::CLI.instance
    cli.parse(['-r', File.expand_path(__FILE__)])
    cli.run
  end
  sleep 1
  Sidekiq::Client.push('class' => 'HardJob', 'args' => ['foo', 1])

  sidekiq_thread.join(1)
else
  class HardJob
    include Sidekiq::Job

    def perform(name, count)
      puts name, count
      raise 'error'
    end
  end
end
clr182 commented 1 year ago

Hi @fukayatsu Thanks for reaching out and creating a PR for this deprecation warning. We will review this when priority allows

stevenharman commented 1 year ago

Confirming that this does resolve the deprecation warnings for Sidekiq 7.2+. We'd love to get this into a release so we can stop spamming our logs with deprecation warnings.