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

Add timezone support for Oban cron monitoring #830

Open
Flo0807 opened this issue Dec 10, 2024 · 5 comments
Open

Add timezone support for Oban cron monitoring #830

Flo0807 opened this issue Dec 10, 2024 · 5 comments

Comments

@Flo0807
Copy link

Flo0807 commented Dec 10, 2024

Sentry supports monitoring scheduled Oban jobs. I wanted to use this feature in my application to monitor Oban cron jobs, but Sentry always threw A missed check-in was detected errors. I looked into the problem and noticed in the web interface that the configured timezone for all cron monitors was set to UTC while I configured the Oban cron jobs to use the Europe/Berlin timezone. Therefore, I needed to change the timezone of the created cron monitors to Europe/Berlin manually to be in sync with my jobs.

I figured out that you can set the timezone with a Manual Setup, but I'd like to have a timezone setting for my config that allows me to set the timezone for the oban cron integrations. I would rather not do a manual setup only to set the timezone value.

This is my setup:

# config/config.exs
config :sentry,
  ...,
  integrations: [
    oban: [
      cron: [enabled: true],
      capture_errors: true
    ]
  ]
# config/config.exs
config :my_app, Oban,
  repo: MyApp.Repo,
  queues: [default: 10],
  plugins: [
    {Oban.Plugins.Pruner, max_age: :timer.hours(24) * 90},
    {Oban.Plugins.Lifeline, rescue_after: :timer.minutes(5)},
    {Oban.Plugins.Cron,
     crontab: [
       {"0 8 * * 1", MyApp.Workers.ExampleWorker}
     ],
     timezone: "Europe/Berlin"}
  ]
@getsantry getsantry bot moved this to Waiting for: Product Owner in GitHub Issues with 👀 3 Dec 10, 2024
@whatyouhide
Copy link
Collaborator

@savhappy @solnic I think it's time we consider a per-job configuration when necessary.

One option is to do it via a behaviour that jobs (not just Oban jobs) can implement. The behaviour can be Sentry.CheckIn too.

defmodule Sentry.CheckIn do
  @callback sentry_check_in_configuration() :: (options_to_merge :: keyword())

  # ...
end

Then, an Oban worker could implement it like this:

defmodule MyApp.Worker do
  use Oban.Worker
 
  @behaviour Sentry.CheckIn

  @impl Oban.Worker
  def process(...)

  @impl Sentry.CheckIn
  def sentry_check_in_configuration do
    [timezone: "Europe/Berlin"]
  end
end

Then, integrations like the Oban one can essentially do:

config =
  if function_exported?(job.module, :sentry_check_in_configuration, 0) do
    Keyword.merge(config, job.module.sentry_check_in_configuration())
  else
    config
  end

This is a basic approach, but I think it works and it's going to scale out pretty well. The callback could also take an argument that is decided by the integration itself, like

@callbac sentry_check_in_configuration(info :: term()) :: keyword()

In Oban's case, it would be the Oban.Job.

Thoughts?

@solnic
Copy link
Collaborator

solnic commented Dec 11, 2024

@savhappy @solnic I think it's time we consider a per-job configuration when necessary.

I can see this being useful (in general, not just for setting TZ for cron check-ins) but I'm wondering if this means you don't think that a top-level default for TZ that we could set via config would be a good idea? We could just have this:

config :sentry,
  # ...,
  integrations: [
    oban: [
      cron: [enabled: true, time_zone: "Europe/Berlin"],
      capture_errors: true
    ]
  ]

...and that could be overridden by a job's custom check-in config, what do you think? From what I see in the code, it seems doable, since we have Sentry.Integrations.Oban.Cron.job_to_check_in_opts that receives an Oban job, config (which is cron[:oban][:cron]) and then it also has access to Sentry.Config.integrations()[:monitor_config_defaults].

@whatyouhide
Copy link
Collaborator

It's doable but I would prioritize it lower than a per-job config (see also #829 which would be solved by the per-job config).

@savhappy
Copy link
Collaborator

This seems like a good use for a behaviour like @whatyouhide mentioned so we don't have to make additional considerations later. I can change the in-progress #829 to handle the new implementation. Or should we separate it?

@whatyouhide
Copy link
Collaborator

@savhappy I'd make a new PR with this change and then refactor #829 as needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: No status
Development

No branches or pull requests

4 participants