bugsnag / bugsnag-ruby

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

Sidekiq exception handlers now take three arguments (DEPRECATION since sidekiq 7.1.5, will become error in 8) #804

Closed timdiggins closed 6 months ago

timdiggins commented 8 months ago

Describe the bug

Sidekiq has (since 7.1.5) changed the api for its exception handlers. https://github.com/sidekiq/sidekiq/blob/main/Changes.md#715

Steps to reproduce

Use a version of sidekiq since 7.1.5 (e.g. 7.2.0 which is latest as of date of writing). Execute a Sidekiq job with an error like

class ErroringJob
  include Sidekiq::Job
  def perform
     raise "Deliberate error"
  end
end

ErroringJob.perform_async

it will output

DEPRECATION: Sidekiq exception handlers now take three arguments, see #<Proc:0x00007fad0325ccd8 /pathto/ruby/3.0.6/lib/ruby/gems/3.0.0/gems/bugsnag-6.26.0/lib/bugsnag/integrations/sidekiq.rb:53>

Environment

clr182 commented 8 months ago

Hi @timdiggins Thanks for reaching out. We are aware of this deprecation warning. We have an item on our backlog aimed at fixing this warning. While we don't have an ETA for the release of this functionality, you can rest assured that a fix will be in place before the warning becomes an error in v8 of Sidekiq.

stevenharman commented 6 months ago

This was addressed in #796 and released in 6.26.1.

pic commented 3 months ago

As far as I can tell, the deprecation warning is still displayed in 6.26.4. The handler is defined with a default value:

server.error_handlers << proc do |ex, _context, _config = nil|

and in sidekiq check this still has arity == 2

        if handler.arity == 2
          # TODO Remove in 8.0
          logger.info { "DEPRECATION: Sidekiq exception handlers now take three arguments, see #{handler}" }
mclack commented 1 month ago

Hi @pic

Apologies for the delay in response.

Can you please confirm whether you are still seeing this warning being displayed when using a bugsnag-ruby version of 6.26.1 or above? With the change here, we would expect this warning to no longer be logged based on the condition of handler.arity == 2, as the arity of the handler should now be -3.

Please let us know if you are still seeing this deprecation warning. It would be useful to see the version of bugsnag-ruby you are using, as well as any warnings or errors being displayed.

pic commented 1 month ago

Hi @mclack

We are using 6.27. As far as I can tell you this declaration:

server.error_handlers << proc do |ex, _context, _config = nil

is considered of arity 2, I guess because of the default value.

For comparison, I've just noticed how newrelic handles this:

        # Sidekiq 3.0.0 - 7.1.4 expect error_handlers to have 2 arguments
        # Sidekiq 7.1.5+ expect error_handlers to have 3 arguments
        config.error_handlers << proc do |error, _ctx, *_|
          NewRelic::Agent.notice_error(error)
        end

Thx

matthewjhowells commented 1 month ago

Hi @pic, This should have been fixed in bugsnag-ruby version 6.26.1 so we wouldn’t expect you to be seeing this depreciation warning any more. Could you please confirm which version of Bugsnag-ruby you are using? If you are running version 6.26.1 or higher, could you please share a reproduction with us to investigate further?

If the reproduction might contain anything sensitive, please feel free to share it with us at support@bugsnag.com instead.

pic commented 3 weeks ago
$ bundle show bugsnag
[...]rbenv/versions/3.2.2/lib/ruby/gems/3.2.0/gems/bugsnag-6.27.1

Still displaying the deprecation warning. I don't have resources right now to try that, but I think that a new RoR 7.0.x application, with a worker, that is raising an exception in perform would expose this.

clr182 commented 2 weeks ago

Hi @pic

Unfortunately without a reproduction example of the case we are unable to investigate this issue further. If you are able to reproduce this behaviour then please feel free to share it with us so that we may review.