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

Field content value condition broken #1245

Closed
bernd opened this issue Jun 12, 2015 · 3 comments
Closed

Field content value condition broken #1245

bernd opened this issue Jun 12, 2015 · 3 comments
Assignees
Labels
Milestone

Comments

@bernd
Copy link
Member

@bernd bernd commented Jun 12, 2015

The field content value condition for alarm callbacks is somewhat broken.

wgkvrced1ssaaaaasuvork5cyii

It does not let the user specify a time range for the condition, but hardcodes it to alert_check_interval * 60.

With a field content value condition configured like the following and an alert_check_interval of 60 (the default), the user gets an alert email every minute for one hour if only one message with that field arrived in the time range.

k9t9ssppbsqaaaabjru5erkjggg

So the time range needs to be configurable like with the other alert conditions. see comments below

@bernd bernd added the bug label Jun 12, 2015
@bernd bernd added this to the 1.1.3 milestone Jun 12, 2015
@dennisoelkers dennisoelkers self-assigned this Jun 12, 2015
@dennisoelkers
Copy link
Member

@dennisoelkers dennisoelkers commented Jun 12, 2015

I think it makes sense that you are not specifying a time range for this condition, as it is different from the other conditions as in that it's not working on an aggregation of the messages in a time range (count for MessageCoundCondition and min/max/avg/stddev for FieldValueAlertCondition) but responding to single messages. This means, that we should make a search going back to the last time we have checked and search all messages in between for the occurence of a message with field with this content. Therefore, the idea was to just do a relative time range search for the configured alert checking interval.

The '* 60' is the issue here, which probably resulted from the time range in the other alert conditions being specified in minutes while the alert_check_interval config parameter being specified in seconds.

@bernd
Copy link
Member Author

@bernd bernd commented Jun 12, 2015

Good point!

dennisoelkers added a commit that referenced this issue Jun 12, 2015
This was incorrectly multiplied by 60 before, probably because in the
other alert conditions the time range was specified in minutes while
the alert check interval which is used here is specified in seconds.

This resulted in the time range always going back to the last n minutes
instead of the last n seconds,

Fixes #1245
@kroepke
Copy link
Member

@kroepke kroepke commented Jun 12, 2015

I would like to keep the change for 1.1 as simple as possible, though.

Thus, I think the most logical change is what commit 8eebc14 is doing.

Let's maybe consider saving the last-run-time for each alert for 1.2 or beyond?

dennisoelkers added a commit that referenced this issue Jun 12, 2015
Test for #1245.
dennisoelkers added a commit that referenced this issue Jun 12, 2015
Test for #1245.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants