getsentry / sentry-ruby

Sentry SDK for Ruby
https://sentry.io/for/ruby
MIT License
926 stars 493 forks source link

Sentry::Cron::MonitorCheckIns breaks ActiveJob keyword arguments #2198

Closed jamesbebbington closed 9 months ago

jamesbebbington commented 9 months ago

Issue Description

It appears that Sentry::Cron::MonitorCheckIns::Patch#perform is not forwarding keyword arguments.

Reproduction Steps

include Sentry::Cron::MonitorCheckIns in an ActiveJob, and attempt to use keyword arguments.

Expected Behavior

Given a job that doesn't include Sentry::Cron::MonitorCheckIns:

class CronJob < ApplicationJob

  def work(a, b, c)
    puts "a: #{a}, b: #{b}, c: #{c}"
  end

  def perform(a, b = 42, c: 99)
    work(a, b, c)
  end

end

it behaves as expected:

3.2.2 :001 > CronJob.perform_now(2, 43)
a: 2, b: 43, c: 99
3.2.2 :002 > CronJob.perform_now(2, 43, c: 100)
a: 2, b: 43, c: 100

Actual Behavior

Given a job that includes Sentry::Cron::MonitorCheckIns:

class CronJob < ApplicationJob

  include Sentry::Cron::MonitorCheckIns

  sentry_monitor_check_ins

  def work(a, b, c)
    puts "a: #{a}, b: #{b}, c: #{c}"
  end

  def perform(a, b = 42, c: 99)
    work(a, b, c)
  end

end

Passing a keyword argument raises an ArgumentError:

3.2.2 :001 > CronJob.perform_now(2, 43)
a: 2, b: 43, c: 99
3.2.2 :002 > CronJob.perform_now(2, 43, c: 100)
ArgumentError (wrong number of arguments (given 3, expected 1..2))

Ruby Version

3.2.2

SDK Version

5.15.0

Integration and Its Version

Rails 7.1.2, GoodJob 3.21.5

Sentry Config

No response

sl0thentr0py commented 9 months ago

uhh sorry, should have written a better spec, I thought I tested this.

jamesbebbington commented 9 months ago

No probs, many thanks for the quick fix!

jamesbebbington commented 9 months ago

Sorry, has something gone wonky with getsentry-bot? I see it committed release: 5.15.1 yesterday, but the latest release is still showing as 5.15.0.

sl0thentr0py commented 9 months ago

@jamesbebbington just released

jamesbebbington commented 9 months ago

Oh I see, it needed a manual merge. Many thanks!