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.

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

Jump to bottom

Individualized run_every period for each rule #1128

Closed
wants to merge 5 commits into from
Closed

Individualized run_every period for each rule #1128

wants to merge 5 commits into from

Conversation

mircopolo
Copy link
Contributor

Changes to allow rules to have individualized run_every periods. Still very much in beta needs much review for thread safety etc, plus figure out how to hand reloaded rules who's name have changed etc..

Main changes are as follows

  1. Update config code to load run_every from rule_file.yaml if present
  2. Replace certain EA instance attributes with thread_local storage (counters and es_clients)
  3. Split run_all into separate functions for sending pending alerts, reloading config and executing a single rule instance.
  4. Added APScheduler to schedule each rule in its own job, two further background threads for pending alerts and config_reloads.

This PR should work very nicely with the cron syntax changes in #1114, but at the moment just using a straight up interval scheduling.

@mircopolo
Copy link
Contributor Author

Should address #497, #644

Qmando
Qmando reviewed Jul 28, 2017
View reviewed changes
Copy link
Member

@Qmando Qmando left a comment

Choose a reason for hiding this comment

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

Looks mostly good to me. Sorry for the long wait on reviewing this. I'll probably try fixing merge conflicts and running this for a while to see if any errors occur.

If that looks good, I can try fixing the tests too.

elastalert/elastalert.py
@@ -575,7 +574,7 @@ def run_query(self, rule, start=None, end=None, scroll=False):
rule_inst.add_data(data)

try:
if rule.get('scroll_id') and self.num_hits < self.total_hits:
if rule.get('scroll_id') and self.thread_data.num_hits < self.total_hits:
Copy link
Member

Choose a reason for hiding this comment

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

I think total_hits need to be in thread_data too.

elastalert/elastalert.py
@@ -875,7 +876,9 @@ def init_rule(self, new_rule, new=True):
if prop not in rule:
continue
new_rule[prop] = rule[prop]


self.scheduler.add_job(self.handle_rule_execution, 'interval', args=[new_rule] ,seconds=new_rule['run_every'].total_seconds(),id=new_rule['name'])
Copy link
Member

Choose a reason for hiding this comment

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

Style: Add spaces after commas

Also, lets add max_instances=1, which should prevent overlapping runs if execution time goes over run_every

elastalert/elastalert.py

def handle_rule_execution(self, rule):
self.thread_data.alerts_sent = 0
next_run = datetime.datetime.utcnow() + self.run_every
Copy link
Member

Choose a reason for hiding this comment

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

rule['run_every'] ?

@kvalev
Copy link
Contributor

kvalev commented Jul 31, 2017

I can also help out with the validation when you think that the pull request is ready to be tested.

@Qmando Qmando mentioned this pull request Aug 22, 2017
Closed
@Qmando
Copy link
Member

Qmando commented Aug 22, 2017

So you know, I haven't given up on this PR. I've made some modifcations to it and opened a new pull request, #1298

@Qmando Qmando closed this Aug 22, 2017
@mircopolo
Copy link
Contributor Author

Thanks @Qmando , I actually missed the notification for the code review so didn't realise it had been looked at recently. I had meant to come back to complete this at some point but been pretty busy for the last while, but looks like you have everything under control.

I'll try and pull down the other new PR to help with some testing if I can.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Qmando Qmando Qmando left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mircopolo @kvalev @Qmando