Why is Sentry Upset About my Sidekiq Crontab?

[jherdman]

Issue Description

Today we had an incident where we rolled out a new sidekiq-cron job with an invalid crontab. Sentry was both upset about this, and failed to notify us via Sentry itself. I cannot figure out why Sentry cares, and I cannot figure out why Sentry didn't notify us. We do not have Crons enabled with Sentry. Worse yet, if this is something Sentry is going to do I need CI level ability to check these strings.

Reproduction Steps

Cron tab error:

["'cron' -> \"* * 30 2 *\" -> ArgumentError: not cron or 'natural' cron string: \"* * 30 2 *\""]
expect the argument to be a Exception, got String ("'cron' -> \"* * 30 2 *\" -> ArgumentError: not cron or 'natural' cron string: \"* * 30 2 *\"")
/usr/local/bundle/gems/sentry-ruby-5.16.1/lib/sentry/utils/argument_checking_helper.rb:9:in `check_argument_type!'

Expected Behavior

Sentry is predictable.

Actual Behavior

Sentry is doing things I didn't expect

Ruby Version

3.3.0

SDK Version

5.16.1

Integration and Its Version

No response

Sentry Config

No response

[sl0thentr0py]

thx for the report, this doesn't have to do with the new crons features. Something in the sidekiq stack is calling capture_exception with a string and not an exception object.

https://github.com/getsentry/sentry-ruby/blob/a9fcc38c74f9d1390961040563135e66dcaefffd/sentry-ruby/lib/sentry/hub.rb#L118-L123

@st0012 wdyt about removing those check_argument_type!s in the capture_* methods or just logging and failing gracefully? I think raising on initialization on configuration parameters is ok but raising during normal functioning is a strict no-no from our side as a design principle - we should never break user apps.

[sl0thentr0py]

this is coming from https://github.com/floraison/fugit/blob/6b14af0282c7d7df93d13d265b43d7cc17bc62ec/lib/fugit/parse.rb#L46 but that error is added and suppressed here https://github.com/sidekiq-cron/sidekiq-cron/blob/b92fcee9dba261043d3f1689162a1dd7e209983b/lib/sidekiq/cron/job.rb#L456-L457

so unless you are calling load_from_array yourself somewhere in your stack and using those returned errors to re-raise I don't see how this could happen.

[st0012]

The idea was that passing incorrect data type is very likely to cause exceptions like NoMethodError down the flow, which is usually harder to trace back. So by checking it upfront, we make the same issue easier to spot and fix, especially when users have a decent coverage around the code.

I think it's especially true in this case because if we removed the check, the resulting exception would be:

Sentry.capture_exception("foo")
/Users/hung-wulo/src/github.com/getsentry/sentry-ruby/sentry-ruby/lib/sentry/utils/exception_cause_chain.rb:9:in `exception_to_array': undefined method `cause' for "foo":String (NoMethodError)

        while exception.cause
                       ^^^^^^
Did you mean?  upcase
        from /Users/hung-wulo/src/github.com/getsentry/sentry-ruby/sentry-ruby/lib/sentry/configuration.rb:597:in `matches_exception?'
        from /Users/hung-wulo/src/github.com/getsentry/sentry-ruby/sentry-ruby/lib/sentry/configuration.rb:583:in `block in excluded_exception?'

[jherdman]

Ah ha! I found the problem. I sincerely appreciate your investigation and help ❤️