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 Sentry cron monitoring #193

Merged
merged 6 commits into from
Jul 1, 2024
Merged

Add Sentry cron monitoring #193

merged 6 commits into from
Jul 1, 2024

Conversation

Jongmassey
Copy link
Contributor

@Jongmassey Jongmassey commented Jun 24, 2024

Add sentry cron monitoring to metrics so we can check it's running as it should be.

Fixes #105

@Jongmassey Jongmassey force-pushed the Jongmassey/cron-sentry branch 4 times, most recently from b986cbc to 90c4a83 Compare June 25, 2024 15:56
@Jongmassey Jongmassey changed the title Jongmassey/cron-sentry Add Sentry cron monitoring Jun 25, 2024
@Jongmassey Jongmassey force-pushed the Jongmassey/cron-sentry branch 3 times, most recently from d804720 to 9c67b00 Compare June 26, 2024 15:25
Based on configuration example from Sentry docs.

Timezone and schedule match current config on dokku3

Previous runs of the cron job seem to take <10mins in total.
Therefore 10 minutes runtime for each individual task should be plenty.

The first task in the list is expected to start shortly after midnight,
but the later ones might not start until nearly 10 past.

Therefore 30 mins chosen to give a bit of leeway on the checkin time.
This method is the entry point for the cron job,
which is what we want to be monitoring

Running the tasks in isolation (e.g. for testing) would not be monitored
@Jongmassey Jongmassey marked this pull request as ready for review June 26, 2024 15:34
@Jongmassey Jongmassey force-pushed the Jongmassey/cron-sentry branch from 9c67b00 to a68aa10 Compare June 26, 2024 15:34
metrics/tasks/__main__.py Outdated Show resolved Hide resolved
metrics/tasks/__main__.py Outdated Show resolved Hide resolved
docs/adr/0003-use-sentry-cron-monitoring.md Outdated Show resolved Hide resolved
@Jongmassey Jongmassey force-pushed the Jongmassey/cron-sentry branch from c0f0de4 to 57f6304 Compare June 27, 2024 10:28
@Jongmassey
Copy link
Contributor Author

@lucyb I've refactored things in 02b85a3 .

My thinking is that the SDK is initialised once per execution of the cron job, then for each task, a new Monitor (reflecting Sentry's parlance) is created. This Monitor holds all of its own relevant state (slug and checkin id) rather than this being in the tasks main method.

metrics/sentry/cron.py Outdated Show resolved Hide resolved
@Jongmassey Jongmassey force-pushed the Jongmassey/cron-sentry branch from 02b85a3 to e1394f4 Compare June 27, 2024 21:21
Copy link
Collaborator

@lucyb lucyb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for responding to the feedback. This is a really neat piece of code.

@Jongmassey
Copy link
Contributor Author

Thanks, Lucy.

Based on my testing - the Sentry issues raised by this cron monitoring do not appear in #sentry-events on Slack. It appears that the alert configuration requires both an Event and a corresponding Issue to be triggered. We may need to change this configuration for these to appear in the slack channel. I'll create a new issue for this.

@Jongmassey Jongmassey merged commit 7fc398b into main Jul 1, 2024
9 checks passed
@Jongmassey Jongmassey deleted the Jongmassey/cron-sentry branch July 1, 2024 15:46
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.

Add monitoring for cron jobs
2 participants