getsentry / sentry-ruby

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

Support extracting timezone out of crontab in sidekiq-scheduler #2187

Closed shalvah-gs closed 10 months ago

shalvah-gs commented 11 months ago

Issue Description

Hi, just tried out the new auto-crons feature for Sidekiq scheduler. Are there any caveats around it? We have a bunch of running jobs (Rails), but only one shows up in Sentry.

We have a sidekiq_scheduler.yml.erb:

```yaml process_webhook_requests: every: '15s' class: StagedWebhookRequestEnqueuerJob call_status_refresher: every: '2h' class: CallStatusRefresherEnqueuerJob delete_webhook_requests: cron: '0 21 * * * Europe/Berlin' # 9pm daily class: StagedWebhookRequestDeleterJob process_scheduled_call_requests: cron: '*/1 * * * * Europe/Berlin' # Runs every 1 minute class: ScheduledCallRequestsProcessorJob ```

and we have this in our Sidekiq initializer:

```rb if ENV.fetch('RUN_SIDEKIQ_CRON').to_bool && Sidekiq.server? Sidekiq.configure_server do |config| config.on(:startup) do scheduler_file_content = File.read(Rails.root.join('config/sidekiq_scheduler.yml.erb')) Sidekiq.schedule = YAML.safe_load( ERB.new(scheduler_file_content).result ) Sidekiq::Scheduler.reload_schedule! end end else Sidekiq::Scheduler.enabled = false end ```

However, only one job shows up in the UI, after 18+ hours of running:

image

I've checked the logs, and we have the "Injected Sentry Crons monitor checkins into ..." message for all four jobs. And I can also see the transport messages:

image

which makes me wonder if this is a backend issue?

Reproduction Steps

Add config.enabled_patches += [:sidekiq_scheduler]

We did not make any other changes to our individual jobs.

Expected Behavior

Expected to see all check-ins in the UI

Actual Behavior

Only saw the one with a schedule every: 2h.

Ruby Version

3.2

SDK Version

5.14.0

Integration and Its Version

Rails, Sidekiq, all version 5.14.0

Sentry Config


Sentry.init do |config|
  config.enabled_environments = %w[production]
  filterable_params = Rails.application.config.filter_parameters
  filter = ActiveSupport::ParameterFilter.new(filterable_params)
  config.before_send = lambda do |event, _hint|
    filter.filter(event.to_hash)
  end
  config.breadcrumbs_logger = [:active_support_logger]
  config.excluded_exceptions -= ['ActiveRecord::RecordNotFound']
  config.excluded_exceptions += ['Errors::ExpectedError']
  config.enabled_patches += [:sidekiq_scheduler]
end
sl0thentr0py commented 11 months ago

ok so @shalvah-gs

rocess_webhook_requests:
  every: '15s'
  class: StagedWebhookRequestEnqueuerJob

will be rejected by the server because we only support from 1 minute onwards

and these

delete_webhook_requests:
  cron: '0 21 * * * Europe/Berlin' # 9pm daily
  class: StagedWebhookRequestDeleterJob

process_scheduled_call_requests:
  cron: '*/1 * * * * Europe/Berlin' # Runs every 1 minute
  class: ScheduledCallRequestsProcessorJob

might also be getting rejected because of the timezone stuff, I will have to check with the server folks and get back to you. EIther way, I think we should just support them if it's not already, so thanks for the feedback!

sl0thentr0py commented 11 months ago

alright yea we'll need to extract those timezones and send just the raw cron value. I'll patch it up later in the week.

shalvah-gs commented 11 months ago

will be rejected by the server because we only support from 1 minute onwards

This is understandable, but couldn't we still show them? Crons of less than a minute could be rounded up to 1 minute (and this should be documented). It's better to have some alerting than none.

I guess this is something we can do with s custom monitor config (?), but it can also probably be done automatically by the gem. I can take a crack at a PR for that, but I think it should probably be a server-side fix.

shalvah-gs commented 11 months ago

Off topic, but is there an easy way to set a threshold for alerting (beyond manual Alert Rules configuration)? Apparently Heroku restarts all dynos once daily, so we sometimes get a false alert for a missed check in.

gaprl commented 11 months ago

Hey @shalvah-gs, Crons EM here 👋

This is understandable, but couldn't we still show them? Crons of less than a minute could be rounded up to 1 minute (and this should be documented). It's better to have some alerting than none.

Agreed we can do a better job in our docs, we'll get that updated. Thanks for the feedback!

Off topic, but is there an easy way to set a threshold for alerting (beyond manual Alert Rules configuration)? Apparently Heroku restarts all dynos once daily, so we sometimes get a false alert for a missed check in.

Right now thresholds can only be changed under your monitor settings, however we are working on adding two extra fields to the upsert payload so you can do it programatically (https://github.com/getsentry/sentry/issues/61290).

A different option, is to remove your current alert rules for your monitors and set up a new issue alert that covers all issues with the category "Cron" and set a minimum threshold of events before you get notified. Something like this:

CleanShot 2023-12-06 at 12 21 04@2x

shalvah-gs commented 11 months ago

Thanks for the alert suggestions. Will give it a go.

Agreed we can do a better job in our docs, we'll get that updated

Thanks. What do you think of the 1 minute rounding up, though? Any cron job less with an interval < 1 minute is treated as 1m. That makes Sentry useful for all our crons, without any special configuration.