Add support for interval cron expressions#93
Conversation
|
Bump for @klen @garrettheel, any interest? |
|
Thanks for the PR @jbrody1. This looks useful - I'm just waiting to see how some other changes fall out before looking at this more seriously. Would the ability to write more expressive rules also fit your need? (E.g "warning: < 200MB AND it is Wednesday") |
|
More expressive rules would be great. In my case, I need rules that are enforced not only on certain days (weekdays), but certain times of day (working hours), excluding specific days (holidays), etc. Thinking about the best way to express these conditions, it could be either a new, custom syntax for chronological expressions, or cron itself. Given this choice, I went with cron. I'm interested to see what else is coming for graphite-beacon. In the meantime I'll resolve the conflicts in case you want this PR. |
# Conflicts: # requirements.txt
|
Bump. Any interest? I've been running this in production for several months now and it has been working well. |
|
Bump @garrettheel. We're still using this in production. I'd love to get back on the main codeline. |
garrettheel
left a comment
garrettheel
left a comment
There was a problem hiding this comment.
Thanks for the contribution, this looks really useful! Would you also be able to add some docs for this to the README?
graphite_beacon/alerts.py
Outdated
| """Schedule the next run of this callback.""" | ||
| if self.is_running: | ||
| now = datetime.now() | ||
| next = self.cron.get_next(datetime) |
There was a problem hiding this comment.
next shadows the python builtin, please use a different name
graphite_beacon/alerts.py
Outdated
| interval = options.get('interval', self.reactor.options['interval']) | ||
| time_window = options.get('time_window', None) | ||
|
|
||
| if is_cron(interval): |
There was a problem hiding this comment.
You don't set history_size in here which will break any uses of historical
graphite_beacon/alerts.py
Outdated
| time_window = options.get('time_window', None) | ||
|
|
||
| if is_cron(interval): | ||
| self.interval = interval |
There was a problem hiding this comment.
Can you check that this is a valid cron expression before proceeding here? It's best to fail early on bad input
graphite_beacon/alerts.py
Outdated
| from itertools import islice | ||
| from croniter import croniter | ||
| from datetime import datetime | ||
| from threading import Lock |
graphite_beacon/alerts.py
Outdated
| next = self.cron.get_next(datetime) | ||
| while next <= now: | ||
| next = self.cron.get_next(datetime) | ||
| LOGGER.debug("now: %s", now) |
There was a problem hiding this comment.
Please add context that this belongs to CronCallback
|
Changes Unknown when pulling 32e3c75 on jbrody1:develop into * on klen:develop*. |
|
Changes Unknown when pulling f36744c on jbrody1:develop into * on klen:develop*. |
|
Thanks for the comments @garrettheel. I've implemented the changes you requested. As you can see, there was an issue with implementing historical values for cron expressions. The possible solutions were:
I went with #1 to minimize the surface area of the change on syntax/API. However, this has the side effect of potentially using fewer data points if there are large gaps in the cron schedule. I think #3 might be functionally better, though it puts more burden on the user. Please take a look, and let me know if you have a preference. Thanks, |
|
|
||
| def is_running(self): | ||
| """Is running.""" | ||
| return self.is_running |
There was a problem hiding this comment.
Don't use the same name for the function and the variable here
| if self.is_running: | ||
| now = datetime.now() | ||
| next_time = self.cron.get_next(datetime) | ||
| while next_time <= now: |
There was a problem hiding this comment.
Do you think it's worth having a limit on how far this will search before erroring/failing early?
|
Ideally I'd like to see (2) but (3) would be fine for now also. Can you leave all of the history changes out of this PR and address them in a separate one? Let's stick to size-based and figure out a way to add time-based in a non-breaking way later. I'm happy with you to error with "Unsupported" if you don't want to implement history for cron intervals right now (just be sure to document it). Otherwise this is really great and pretty much ready to merge! |
4 participants
I am using graphite-beacon to monitor a production service that sees varying load patterns throughout the day and by day of week. I've added the ability to use cron syntax to schedule checks, rather than a fixed interval. This allows me to have alerts with certain thresholds for weekdays and work hours, with different thresholds for off-peak hours.