From 3ccd76dd75a4f07dba2f8a6b5eba8db075bea0e4 Mon Sep 17 00:00:00 2001 From: Ian Fijolek Date: Mon, 9 Apr 2018 17:07:32 -0700 Subject: [PATCH] Fix issue with alerting on first alert An issue with the failure function caused the first alert after reaching the minimum `alert_after` threshold to not notify unless using exponential backoff. New tests were added to reproduce the error and then it was fixed. Fixes #2 --- minitor/main.py | 101 ++++++++++++++++++++++++------------------ tests/first_test.py | 6 --- tests/monitor_test.py | 94 +++++++++++++++++++++++++++++++++++++++ 3 files changed, 153 insertions(+), 48 deletions(-) delete mode 100644 tests/first_test.py create mode 100644 tests/monitor_test.py diff --git a/minitor/main.py b/minitor/main.py index 5d6f265..89e8b03 100644 --- a/minitor/main.py +++ b/minitor/main.py @@ -20,6 +20,50 @@ def read_yaml(path): return yamlenv.load(contents) +def validate_monitor_settings(settings): + """Validates that settings for a Monitor are valid + + Note: Cannot yet validate the Alerts exist from within this class. + That will be done by Minitor later + """ + name = settings.get('name') + if not name: + raise InvalidMonitorException('Invalid name for monitor') + if not settings.get('command'): + raise InvalidMonitorException( + 'Invalid command for monitor {}'.format(name) + ) + + type_assertions = ( + ('check_interval', int), + ('alert_after', int), + ('alert_every', int), + ) + + for key, val_type in type_assertions: + val = settings.get(key) + if not isinstance(val, val_type): + raise InvalidMonitorException( + 'Invalid type on {}: {}. Expected {} and found {}'.format( + name, key, val_type.__name__, type(val).__name__ + ) + ) + + non_zero = ( + 'check_interval', + 'alert_after', + 'alert_every', + ) + + for key in non_zero: + if settings.get(key) == 0: + raise InvalidMonitorException( + 'Invalid value for {}: {}. Value cannot be 0'.format( + name, key + ) + ) + + class InvalidAlertException(Exception): pass @@ -44,7 +88,7 @@ class Monitor(object): 'alert_every': -1, } settings.update(config) - self.validate_settings(settings) + validate_monitor_settings(settings) self.name = settings['name'] self.command = settings['command'] @@ -54,38 +98,9 @@ class Monitor(object): self.alert_every = settings.get('alert_every') self.last_check = None - self.failure_count = 0 + self.total_failure_count = 0 self.alert_count = 0 - def validate_settings(self, settings): - """Validates that settings for this Monitor are valid - - Note: Cannot yet validate the Alerts exist from within this class. - That will be done by Minitor later - """ - name = settings.get('name') - if not name: - raise InvalidMonitorException('Invalid name for monitor') - if not settings.get('command'): - raise InvalidMonitorException( - 'Invalid command for monitor {}'.format(name) - ) - - type_assertions = ( - ('check_interval', int), - ('alert_after', int), - ('alert_every', int), - ) - - for key, val_type in type_assertions: - val = settings.get(key) - if not isinstance(val, val_type): - raise InvalidMonitorException( - 'Invalid type on {} {}. Expected {} and found {}'.format( - name, key, val_type.__name__, type(val).__name__ - ) - ) - def should_check(self): """Determines if this Monitor should run it's check command""" if not self.last_check: @@ -111,25 +126,27 @@ class Monitor(object): def success(self): """Handles success tasks""" - self.failure_count = 0 + self.total_failure_count = 0 self.alert_count = 0 def failure(self): """Handles failure tasks and possibly raises MinitorAlert""" - self.failure_count += 1 - if self.failure_count < self.alert_after: + self.total_failure_count += 1 + # Ensure we've hit the minimum number of failures to alert + if self.total_failure_count < self.alert_after: return - if self.alert_every >= 0: - failure_interval = (self.failure_count % self.alert_every) == 0 + + failure_count = (self.total_failure_count - self.alert_after) + if self.alert_every > 0: + # Otherwise, we should check against our alert_every + should_alert = (failure_count % self.alert_every) == 0 else: - failure_interval = ( - (self.failure_count - self.alert_after) >= - (2 ** self.alert_count) - ) - if failure_interval: + should_alert = (failure_count >= (2 ** self.alert_count) - 1) + + if should_alert: self.alert_count += 1 raise MinitorAlert('{} check has failed {} times'.format( - self.name, self.failure_count + self.name, self.total_failure_count )) diff --git a/tests/first_test.py b/tests/first_test.py deleted file mode 100644 index 08c066c..0000000 --- a/tests/first_test.py +++ /dev/null @@ -1,6 +0,0 @@ -class TestMain(object): - def test_something(self): - pass - - def test_something_else(self): - assert False diff --git a/tests/monitor_test.py b/tests/monitor_test.py new file mode 100644 index 0000000..7b49483 --- /dev/null +++ b/tests/monitor_test.py @@ -0,0 +1,94 @@ +import pytest + +from minitor.main import InvalidMonitorException +from minitor.main import MinitorAlert +from minitor.main import Monitor +from minitor.main import validate_monitor_settings + + +class TestMonitor(object): + + @pytest.fixture + def monitor(self): + return Monitor({ + 'name': 'SampleMonitor', + 'command': ['echo', 'foo'], + }) + + @pytest.mark.parametrize('settings', [ + {'alert_after': 0}, + {'alert_every': 0}, + {'check_interval': 0}, + {'alert_after': 'invalid'}, + {'alert_every': 'invalid'}, + {'check_interval': 'invalid'}, + ]) + def test_monitor_invalid_configuration(self, settings): + with pytest.raises(InvalidMonitorException): + validate_monitor_settings(settings) + + @pytest.mark.parametrize( + 'alert_after', + [1, 20], + ids=lambda arg: 'alert_after({})'.format(arg), + ) + @pytest.mark.parametrize( + 'alert_every', + [-1, 1, 2, 1440], + ids=lambda arg: 'alert_every({})'.format(arg), + ) + def test_monitor_alert_after(self, monitor, alert_after, alert_every): + monitor.alert_after = alert_after + monitor.alert_every = alert_every + + # fail a bunch of times before the final failure + for _ in range(alert_after - 1): + monitor.failure() + + # this time should raise an alert + with pytest.raises(MinitorAlert): + monitor.failure() + + @pytest.mark.parametrize( + 'alert_after', + [1, 20], + ids=lambda arg: 'alert_after({})'.format(arg), + ) + @pytest.mark.parametrize( + 'alert_every', + [1, 2, 1440], + ids=lambda arg: 'alert_every({})'.format(arg), + ) + def test_monitor_alert_every(self, monitor, alert_after, alert_every): + monitor.alert_after = alert_after + monitor.alert_every = alert_every + + # fail a bunch of times before the final failure + for _ in range(alert_after - 1): + monitor.failure() + + # this time should raise an alert + with pytest.raises(MinitorAlert): + monitor.failure() + + # fail a bunch more times until the next alert + for _ in range(alert_every - 1): + monitor.failure() + + # this failure should alert now + with pytest.raises(MinitorAlert): + monitor.failure() + + def test_monitor_alert_every_exponential(self, monitor): + monitor.alert_after = 1 + monitor.alert_every = -1 + + failure_count = 16 + expect_failures_on = {1, 2, 4, 8, 16} + + for i in range(failure_count): + if i + 1 in expect_failures_on: + with pytest.raises(MinitorAlert): + monitor.failure() + else: + monitor.failure()