Skip to content

Update cron monitor script #11632

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.

Sign up for GitHub

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

Open
jimchamp wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Update cron monitor script #11632

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
9bb6df2
Use `openlibrary.yml` to configure script
jimchamp Dec 26, 2025
967a2d7
Replace `job_name` with `monitor_slug`
jimchamp Dec 26, 2025
9bdaa3f
Add Sentry cron monitoring
jimchamp Dec 26, 2025
ea3a22b
Remove exception capturing
jimchamp Dec 26, 2025
375ea51
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Dec 26, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 23 additions & 22 deletions scripts/cron_wrapper.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,34 +6,32 @@
import argparse
import subprocess
import sys
from configparser import ConfigParser
from pathlib import Path

import sentry_sdk
import yaml
from statsd import StatsClient

DEFAULT_CONFIG_PATH = "/olsystem/etc/cron-wrapper.ini"
DEFAULT_CONFIG_PATH = "/olsystem/etc/openlibrary.yml"


class MonitoredJob:
def __init__(self, command, sentry_cfg, statsd_cfg, job_name):
def __init__(self, command, sentry_cfg, statsd_server, monitor_slug):
self.command = command
self.statsd_client = (
self._setup_statsd(statsd_cfg.get("host", ""), statsd_cfg.get("port", ""))
if statsd_cfg
else None
statsd_host_and_port = statsd_server.split(":")
self.statsd_client = self._setup_statsd(
statsd_host_and_port[0], statsd_host_and_port[1]
)
Comment on lines +21 to 24
Copy link

Copilot AI Dec 27, 2025

Choose a reason for hiding this comment

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

The statsd_server string is split without validation, which will cause an IndexError if the string is empty or doesn't contain a colon separator. Additionally, the port value needs to be converted to an integer for StatsClient. Add validation to ensure statsd_server is in the expected "host:port" format and convert the port to int before passing to _setup_statsd.

Suggested change
statsd_host_and_port = statsd_server.split(":")
self.statsd_client = self._setup_statsd(
statsd_host_and_port[0], statsd_host_and_port[1]
)
self.statsd_client = None
if statsd_server and ":" in statsd_server:
host, port_str = statsd_server.split(":", 1)
try:
port = int(port_str)
except ValueError:
port = None
if host and port is not None:
self.statsd_client = self._setup_statsd(host, port)

Copilot uses AI. Check for mistakes.
self._setup_sentry(sentry_cfg.get("dsn", ""))
self.job_name = job_name
self.monitor_slug = monitor_slug
self.job_failed = False

def run(self):
self._before_run()
try:
self._run_script()
with sentry_sdk.monitor(self.monitor_slug):
self._run_script()
except subprocess.CalledProcessError as e:
se = RuntimeError(f"Subprocess failed: {e}\n{e.stderr}")
sentry_sdk.capture_exception(se)
self.job_failed = True
sys.stderr.write(e.stderr)
Copy link

Copilot AI Dec 27, 2025

Choose a reason for hiding this comment

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

The stderr attribute may be None if the subprocess doesn't capture stderr or if the error occurs before stderr is populated. Attempting to write None to sys.stderr.write() will cause a TypeError. Add a check to ensure e.stderr is not None before writing it.

Suggested change
sys.stderr.write(e.stderr)
if e.stderr is not None:
sys.stderr.write(e.stderr)
else:
sys.stderr.write(str(e))

Copilot uses AI. Check for mistakes.
sys.stderr.flush()
Expand All @@ -43,15 +41,17 @@ def run(self):

def _before_run(self):
if self.statsd_client:
self.statsd_client.incr(f'cron.{self.job_name}.start')
self.job_timer = self.statsd_client.timer(f'cron.{self.job_name}.duration')
self.statsd_client.incr(f'ol.cron.{self.monitor_slug}.start')
self.job_timer = self.statsd_client.timer(
f'ol.cron.{self.monitor_slug}.duration'
)
self.job_timer.start()

def _after_run(self):
if self.statsd_client:
self.job_timer.stop()
status = "failure" if self.job_failed else "stop"
Copy link

Copilot AI Dec 27, 2025

Choose a reason for hiding this comment

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

The status label "stop" for successful job completion is unclear and inconsistent with conventional metric naming. Consider using "success" instead to make the metric's meaning more explicit and align with common monitoring conventions where job outcomes are typically labeled as "success" or "failure".

Suggested change
status = "failure" if self.job_failed else "stop"
status = "failure" if self.job_failed else "success"

Copilot uses AI. Check for mistakes.
self.statsd_client.incr(f'cron.{self.job_name}.{status}')
self.statsd_client.incr(f'ol.cron.{self.monitor_slug}.{status}')

def _run_script(self):
return subprocess.run(
Expand All @@ -71,21 +71,22 @@ def _setup_statsd(self, host, port):

def main(args):
config = _read_config(args.config)
sentry_cfg = dict(config["sentry"]) if config.has_section("sentry") else None
statsd_cfg = dict(config["statsd"]) if config.has_section("statsd") else None
sentry_cfg = config.get("sentry_cron_jobs", {})
statsd_server = config.get("admin", {}).get("statsd_server", "")
config = None
Copy link

Copilot AI Dec 27, 2025

Choose a reason for hiding this comment

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

Setting config to None immediately after reading it creates potential issues if the config needs to be referenced later and makes the code confusing. This line appears to be an attempt at memory cleanup but is unnecessary in Python's garbage collection model. Consider removing this line as it reduces code clarity without providing benefits.

Suggested change
config = None

Copilot uses AI. Check for mistakes.
command = [args.script] + args.script_args
job_name = args.job_name
monitor_slug = args.monitor_slug

job = MonitoredJob(command, sentry_cfg, statsd_cfg, job_name)
job = MonitoredJob(command, sentry_cfg, statsd_server, monitor_slug)
job.run()


def _read_config(config_path):
if not Path(config_path).exists():
raise FileNotFoundError("Missing cron-wrapper configuration file")
Copy link

Copilot AI Dec 27, 2025

Choose a reason for hiding this comment

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

The error message "Missing cron-wrapper configuration file" is misleading because it doesn't indicate which file path was checked. Consider including the config_path in the error message to help users diagnose configuration issues more easily.

Suggested change
raise FileNotFoundError("Missing cron-wrapper configuration file")
raise FileNotFoundError(f"Missing cron-wrapper configuration file: {config_path}")

Copilot uses AI. Check for mistakes.
config_parser = ConfigParser()
config_parser.read(config_path)
return config_parser
with open(config_path) as in_file:
config = yaml.safe_load(in_file)
return config


def _parse_args():
Expand All @@ -96,7 +97,7 @@ def _parse_args():
default=DEFAULT_CONFIG_PATH,
help=f"Path to cron-wrapper configuration file. Defaults to \"{DEFAULT_CONFIG_PATH}\"",
)
_parser.add_argument("job_name", help="Name of the job to be monitored")
_parser.add_argument("monitor_slug", help="Monitor slug of the Sentry cron monitor")
_parser.add_argument(
"script", help="Path to script that will be wrapped and monitored"
)
Expand Down
Loading