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

misc(sidekiq): New approach for cron monitoring #2225

Merged
merged 1 commit into from
Jun 28, 2024

Conversation

vincent-pochet
Copy link
Collaborator

Context

This PR is a new approach to try to finally add cron monitoring on Sidekiq.
The logic was first introduced in #2109 and #1780

Description

The new approach uses ActiveJob.set to pass cron arguments to the job, in order to avoid the issues we faced lately with the initial approach

@julienbourdeau
Copy link
Contributor

Just some food for thoughts, since these jobs are all in the Clock module and they are only called via clock.rb, we could store the sentry config directly in the job instead of using the class variable.
It could be a method or some constants.

module Clock
  class ActivateSubscriptionsJob < ApplicationJob
    include SentryCronConcern
    queue_as 'clock'

    unique :until_executed, on_conflict: :log

+    SENTRY_SLUG = :lago_activate_subscriptions
+    SENTRY_SCHEDULE = '*/5 * * * *'

    def perform
      Subscriptions::ActivateService.new(timestamp: Time.current.to_i).activate_all_pending
    end
    
+    def sentry
+      {slug: 'lago_activate_subscriptions', cron: '*/5 * * * *'}
+    end
  end
end

The drawback is that if you call the job manually, it would mess with the monitoring data 📊

@vincent-pochet
Copy link
Collaborator Author

Just some food for thoughts, since these jobs are all in the Clock module and they are only called via clock.rb, we could store the sentry config directly in the job instead of using the class variable. It could be a method or some constants.

module Clock
  class ActivateSubscriptionsJob < ApplicationJob
    include SentryCronConcern
    queue_as 'clock'

    unique :until_executed, on_conflict: :log

+    SENTRY_SLUG = :lago_activate_subscriptions
+    SENTRY_SCHEDULE = '*/5 * * * *'

    def perform
      Subscriptions::ActivateService.new(timestamp: Time.current.to_i).activate_all_pending
    end
    
+    def sentry
+      {slug: 'lago_activate_subscriptions', cron: '*/5 * * * *'}
+    end
  end
end

The drawback is that if you call the job manually, it would mess with the monitoring data 📊

There is also a risk of de-sync with what is configured in the clock file itself. The risk still exists in the current approach but at least it done in the same place

@vincent-pochet vincent-pochet merged commit bb2bb63 into main Jun 28, 2024
6 checks passed
@vincent-pochet vincent-pochet deleted the sidekiq-cron-the-return branch June 28, 2024 14:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants