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

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

Closed
timdiggins opened this issue Nov 17, 2023 · 8 comments
Labels
awaiting feedback Awaiting a response from a customer. Will be automatically closed after approximately 2 weeks. released This feature/bug fix has been released

Comments

@timdiggins
Copy link
Contributor

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

  • Bugsnag version: 6.26.0
  • Ruby version: 3.0.6
  • Bundle version: 2.2.33
  • Integration framework version:
    • Sidekiq: 7.2.0
@clr182 clr182 added bug Confirmed bug backlog We hope to fix this feature/bug in the future labels Nov 20, 2023
@clr182
Copy link

clr182 commented Nov 20, 2023

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
Copy link
Contributor

This was addressed in #796 and released in 6.26.1.

@clr182 clr182 added released This feature/bug fix has been released and removed bug Confirmed bug backlog We hope to fix this feature/bug in the future labels Jan 25, 2024
@clr182 clr182 closed this as completed Jan 25, 2024
@pic
Copy link

pic commented Apr 22, 2024

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

mclack commented Jun 5, 2024

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.

@mclack mclack added the awaiting feedback Awaiting a response from a customer. Will be automatically closed after approximately 2 weeks. label Jun 5, 2024
@pic
Copy link

pic commented Jun 9, 2024

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

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.

@matthewjhowells matthewjhowells added awaiting feedback Awaiting a response from a customer. Will be automatically closed after approximately 2 weeks. and removed awaiting feedback Awaiting a response from a customer. Will be automatically closed after approximately 2 weeks. labels Jun 17, 2024
@pic
Copy link

pic commented Jul 9, 2024

$ 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
Copy link

clr182 commented Jul 18, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting feedback Awaiting a response from a customer. Will be automatically closed after approximately 2 weeks. released This feature/bug fix has been released
Projects
None yet
Development

No branches or pull requests

6 participants