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

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

Closed
shalvah-gs opened this issue Dec 2, 2023 · 6 comments · Fixed by #2209
Closed

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

shalvah-gs opened this issue Dec 2, 2023 · 6 comments · Fixed by #2209
Assignees

Comments

@shalvah-gs
Copy link

shalvah-gs commented Dec 2, 2023

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:

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:

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

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

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

shalvah-gs commented Dec 5, 2023

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.

@getsantry getsantry bot moved this to Waiting for: Product Owner in GitHub Issues with 👀 Dec 5, 2023
@shalvah-gs
Copy link
Author

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

gaprl commented Dec 6, 2023

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 (getsentry/sentry#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
Copy link
Author

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.

@getsantry getsantry bot moved this to Waiting for: Product Owner in GitHub Issues with 👀 Dec 7, 2023
@getsantry getsantry bot removed the status in GitHub Issues with 👀 Dec 12, 2023
@sl0thentr0py sl0thentr0py changed the title sidekiq-scheduler automatic cron monitoring not showing all crons Support extracting timezone out of crontab in sidekiq-scheduler Dec 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants