sorentwo / oban

šŸ’Ž Robust job processing in Elixir, backed by modern PostgreSQL and SQLite3
https://getoban.pro
Apache License 2.0
3.18k stars 297 forks source link

Crontab information in Oban jobs (or job-related Telemetry events) #1048

Closed whatyouhide closed 4 months ago

whatyouhide commented 4 months ago

Is your feature request related to a problem? Please describe.

Iā€™m working on integrating Oban with the Sentry SDK for Elixir, in particular support for Sentry crons. To me, the obvious path forward is using Oban's Telemetry events to report started and finished jobs to Sentry.

Describe the Solution You'd Like

Sentry's Crons require information about the cron jobs to work effectively. That includes the crontab, schedule, and whatnot. Currently, there doesn't seem to be data about that in the job or in the Telemetry event. I know that the Oban config includes

plugins: [
  {Oban.Plugins.Cron, [crontab: [...]]},

but getting from there to the actual crontab of the specific job is not ideal, and Iā€™m not sure if it would be reliable.

Describe Alternatives You've Considered

Parsing the config, but as I said Iā€™m not quite sure this would work and would be reliable; any advice appreciated šŸ™ƒ

Thanks all the great work peeps! šŸ’Ÿ

sorentwo commented 4 months ago

What a coincidence, we just published an article about error reporting with Sentry: https://getoban.pro/articles/enhancing-error-reporting


That includes the crontab, schedule, and whatnot. Currently, there doesn't seem to be data about that in the job or in the Telemetry event.

That's true, there isn't any indication. Which telemetry events are you looking at using? There's a :plugin event from the Cron and DynamicCron plugins, the :insert_job/:insert_all_jobs events from the engine, or the :job event?

We could insert the schedule into the job's meta along with the cron: true field that's already in there. The job's meta bubbles up through all of the aforementioned events.

whatyouhide commented 4 months ago

There's a :plugin event from the Cron and DynamicCron plugins

Ah, interesting! I didn't even notice :plugin events. Correct me if Iā€™m wrong, but for this use case that wouldn't work because:

  1. The plugin event doesn't contain direct info about the Oban.Job, which would be awesome to have for us in order to get the job ID (which we use to construct the Sentry "check in ID") and the worker itself (which we use to construct the "monitor slug").
  2. The plugin events don't tell you about the runtime information about the job, such as its duration and all, but only about the plugin, right?

We could insert the schedule into the job's meta

Having cron: true is great, but having the schedule in the job meta would make this feature in Sentry be an absolute breeeeeeeze ā„ļø

sorentwo commented 4 months ago

The plugin event doesn't contain direct info about the Oban.Job, which would be awesome to have for us in order to get the job ID (which we use to construct the Sentry "check in ID") and the worker itself (which we use to construct the "monitor slug").

The plugin event contains a list of jobs it inserts under the :jobs key. However, they aren't guaranteed to have all the fields available, and that just means they were inserted, not actually executed.

The plugin events don't tell you about the runtime information about the job, such as its duration and all, but only about the plugin, right?

Correct. It's only saying the plugin ran and inserted some jobs, there's nothing about when the job executes.

Having cron: true is great, but having the schedule in the job meta would make this feature in Sentry be an absolute breeeeeeeze

Understood, that's what I was proposing! Upon investigation I see that only Pro's DynamicCron is setting cron: true, and it isn't used anywhere. I think it'd be nice for both Cron and DynamicCron to show the schedule instead of true, e.g.:

%Job{meta: %{"cron" => "0 0 * * *"}}

WDYT?

whatyouhide commented 4 months ago

@sorentwo yes that is truly all we need, that would be perfect! For reference, Sentry uses check-ins, with the schedule inside their monitor_config looking like this:

"schedule": {
  "type": "crontab",
  "value": "0 * * * *"
}

So what you proposed above would just fit in perfectly! šŸ’Ÿ

sorentwo commented 4 months ago

PR to add the indicator and original expression: https://github.com/sorentwo/oban/pull/1049

whatyouhide commented 4 months ago

@sorentwo any ETA on a release that includes this? Just to coordinate with the Sentry folks šŸ™ƒ

sorentwo commented 4 months ago

@whatyouhide Possibly tomorrow, depending how my investigation into idle connections from the Postgres notifier goes.

(Oban.Notifiers.Postgres is built on Postgrex.SimpleConnection. You have some expertise there, so I'm all ears if you have suggestions on how it may be accumulating idle connections under load after this change: https://github.com/sorentwo/oban/commit/fb87366516c78b2603c440bb9449338187d97602)