Update cron monitor script #11632
Update cron monitor script #11632
Conversation
jimchamp
added
the
Needs: Special Deploy
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
Pull request overview
This PR updates the cron wrapper script to integrate with Sentry cron monitoring and migrate from INI-based configuration to YAML-based configuration (openlibrary.yml). The changes replace job names with Sentry monitor slugs and add native Sentry cron monitoring while removing exception capturing for failed jobs.
Key Changes:
- Migrated configuration reading from ConfigParser (INI format) to YAML format
- Integrated Sentry's cron monitor context manager for tracking job execution
- Replaced job_name parameter with monitor_slug for better Sentry integration
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| statsd_host_and_port = statsd_server.split(":") | ||
| self.statsd_client = self._setup_statsd( | ||
| statsd_host_and_port[0], statsd_host_and_port[1] | ||
| ) |
There was a problem hiding this comment.
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.
| 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) |
| se = RuntimeError(f"Subprocess failed: {e}\n{e.stderr}") | ||
| sentry_sdk.capture_exception(se) | ||
| self.job_failed = True | ||
| sys.stderr.write(e.stderr) |
There was a problem hiding this comment.
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.
| sys.stderr.write(e.stderr) | |
| if e.stderr is not None: | |
| sys.stderr.write(e.stderr) | |
| else: | |
| sys.stderr.write(str(e)) |
| def _after_run(self): | ||
| if self.statsd_client: | ||
| self.job_timer.stop() | ||
| status = "failure" if self.job_failed else "stop" |
There was a problem hiding this comment.
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".
| status = "failure" if self.job_failed else "stop" | |
| status = "failure" if self.job_failed else "success" |
| 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 |
There was a problem hiding this comment.
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.
| config = None |
|
|
||
| def _read_config(config_path): | ||
| if not Path(config_path).exists(): | ||
| raise FileNotFoundError("Missing cron-wrapper configuration file") |
There was a problem hiding this comment.
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.
| raise FileNotFoundError("Missing cron-wrapper configuration file") | |
| raise FileNotFoundError(f"Missing cron-wrapper configuration file: {config_path}") |
|
@jimchamp can we link this to an issue (if we have one) for our sentry / cron monitoring needs? See: |
2 participants
Towards #10206
Makes the following changes to the
cronwrapper script:openlibrary.ymlSpecial Deployment Procedures
This branch must be deployed along with internetarchive/olsystem#297
Technical
Testing
Screenshot
Stakeholders