From e0e23d914c1ba9746c9e820eb1a5e255eb26fa73 Mon Sep 17 00:00:00 2001 From: Aryamanz29 Date: Fri, 5 Aug 2022 03:22:55 +0530 Subject: [PATCH] [requested-changes] Removed alert_on_related field - Removed alert_on_related feature from this PR. --- README.rst | 7 -- openwisp_monitoring/check/classes/iperf.py | 9 -- openwisp_monitoring/monitoring/base/models.py | 31 +---- .../monitoring/configuration.py | 32 ----- .../monitoring/tests/__init__.py | 9 -- .../tests/test_monitoring_notifications.py | 112 ------------------ 6 files changed, 1 insertion(+), 199 deletions(-) diff --git a/README.rst b/README.rst index 88d9536ee..85d1300c1 100644 --- a/README.rst +++ b/README.rst @@ -1949,13 +1949,6 @@ The ``AlertSettings`` of ``ping`` metric will by default use ``threshold`` and ` defined in the ``alert_settings`` key. You can always override them and define your own custom values via the *admin*. -You can also use ``alert_on_related_field`` key in metric configuration -which allows ``AlertSettings`` to use ``related_field`` as value to check ``threshold`` instead -of default ``field_name`` key. A real world example of this can be seen in -`Iperf metric configuration -`_, -Where we used ``jitter`` (related_field) for alerts. - **Note**: It will raise ``ImproperlyConfigured`` exception if a metric configuration is already registered with same name (not to be confused with verbose_name). diff --git a/openwisp_monitoring/check/classes/iperf.py b/openwisp_monitoring/check/classes/iperf.py index 29be28320..dfed79401 100644 --- a/openwisp_monitoring/check/classes/iperf.py +++ b/openwisp_monitoring/check/classes/iperf.py @@ -341,7 +341,6 @@ def _get_metric(self): """ metric, created = self._get_or_create_metric() if created: - self._create_alert_settings(metric) self._create_charts(metric) return metric @@ -361,11 +360,3 @@ def _create_charts(self, metric): chart = Chart(metric=metric, configuration=chart) chart.full_clean() chart.save() - - def _create_alert_settings(self, metric): - """ - Creates iperf alertsettings - """ - alert_settings = AlertSettings(metric=metric) - alert_settings.full_clean() - alert_settings.save() diff --git a/openwisp_monitoring/monitoring/base/models.py b/openwisp_monitoring/monitoring/base/models.py index 384428ba3..2806cf4e8 100644 --- a/openwisp_monitoring/monitoring/base/models.py +++ b/openwisp_monitoring/monitoring/base/models.py @@ -355,21 +355,6 @@ def write( 'send_alert': send_alert, } options['metric_pk'] = self.pk - - # check if alert_on_related_field is present in metric configuration - if 'alert_on_related_field' in self.config_dict: - related_field = self.config_dict['alert_on_related_field'] - if not extra_values: - raise ValueError( - 'write() missing positional argument: "extra_values" required for alert on related field' - ) - if related_field not in extra_values.keys(): - raise ValueError( - f'"{key}" is not defined for alert_on_related_field in metric configuration' - ) - options['check_threshold_kwargs'].update( - {'value': extra_values[related_field]} - ) timeseries_write.delay(name=self.key, values=values, **options) def read(self, **kwargs): @@ -768,11 +753,6 @@ def _is_crossed_by(self, current_value, time=None, retention_policy=None): return value_crossed # tolerance is set, we must go back in time # to ensure the threshold is trepassed for enough time - # check if alert_on_related_field is present in metric configuration - if 'alert_on_related_field' in self.metric.config_dict: - alert_on_related_field = [self.metric.config_dict['alert_on_related_field']] - else: - alert_on_related_field = [] if time is None: # retrieves latest measurements, ordered by most recent first points = self.metric.read( @@ -780,7 +760,6 @@ def _is_crossed_by(self, current_value, time=None, retention_policy=None): limit=None, order='-time', retention_policy=retention_policy, - extra_fields=alert_on_related_field, ) # store a list with the results results = [value_crossed] @@ -792,15 +771,7 @@ def _is_crossed_by(self, current_value, time=None, retention_policy=None): continue utc_time = utc.localize(datetime.utcfromtimestamp(point['time'])) # did this point cross the threshold? Append to result list - # check if alert_on_related_field is present in metric configuration - if 'alert_on_related_field' in self.metric.config_dict: - results.append( - self._value_crossed( - point[self.metric.config_dict['alert_on_related_field']] - ) - ) - else: - results.append(self._value_crossed(point[self.metric.field_name])) + results.append(self._value_crossed(point[self.metric.field_name])) # tolerance is trepassed if self._time_crossed(utc_time): # if the latest results are consistent, the metric being diff --git a/openwisp_monitoring/monitoring/configuration.py b/openwisp_monitoring/monitoring/configuration.py index 91c7e0538..d081fcdc3 100644 --- a/openwisp_monitoring/monitoring/configuration.py +++ b/openwisp_monitoring/monitoring/configuration.py @@ -561,7 +561,6 @@ def _get_access_tech(): 'lost_packets', 'lost_percent', ], - 'alert_on_related_field': 'jitter', 'charts': { 'bandwidth': { 'type': 'scatter', @@ -658,33 +657,6 @@ def _get_access_tech(): 'colors': [DEFAULT_COLORS[3]], }, }, - 'alert_settings': {'operator': '>', 'threshold': 5, 'tolerance': 0}, - 'notification': { - 'problem': { - 'verbose_name': 'Iperf PROBLEM', - 'verb': _('iperf test jitter is greater than normal value'), - 'level': 'warning', - 'email_subject': _( - '[{site.name}] PROBLEM: {notification.target} {notification.verb}' - ), - 'message': _( - 'The device [{notification.target}]({notification.target_link}) ' - '{notification.verb}.' - ), - }, - 'recovery': { - 'verbose_name': 'Iperf RECOVERY', - 'verb': _('iperf test jitter now back to normal'), - 'level': 'info', - 'email_subject': _( - '[{site.name}] RECOVERY: {notification.target} {notification.verb}' - ), - 'message': _( - 'The device [{notification.target}]({notification.target_link}) ' - '{notification.verb}.' - ), - }, - }, }, } @@ -698,10 +670,6 @@ def _validate_metric_configuration(metric_config): assert 'name' in metric_config assert 'key' in metric_config assert 'field_name' in metric_config - if 'alert_on_related_field' in metric_config: - assert ( - metric_config['alert_on_related_field'] in metric_config['related_fields'] - ) def _validate_chart_configuration(chart_config): diff --git a/openwisp_monitoring/monitoring/tests/__init__.py b/openwisp_monitoring/monitoring/tests/__init__.py index bff66abc5..83ccfd5b1 100644 --- a/openwisp_monitoring/monitoring/tests/__init__.py +++ b/openwisp_monitoring/monitoring/tests/__init__.py @@ -51,15 +51,6 @@ 'label': 'Test Metric', 'notification': test_notification, }, - 'test_alert_on_rf': { - 'name': 'test_alert_related', - 'key': '{key}', - 'field_name': '{field_name}', - 'label': 'Test alert related', - 'notification': test_notification, - 'related_fields': ['test_related_1', 'test_related_2', 'test_related_3'], - 'alert_on_related_field': 'test_related_2', - }, 'top_fields_mean': { 'name': 'top_fields_mean_test', 'key': '{key}', diff --git a/openwisp_monitoring/monitoring/tests/test_monitoring_notifications.py b/openwisp_monitoring/monitoring/tests/test_monitoring_notifications.py index f5cba1de8..81ffac53e 100644 --- a/openwisp_monitoring/monitoring/tests/test_monitoring_notifications.py +++ b/openwisp_monitoring/monitoring/tests/test_monitoring_notifications.py @@ -402,118 +402,6 @@ def test_alerts_disabled(self): self.assertEqual(d.monitoring.status, 'problem') self.assertEqual(Notification.objects.count(), 0) - def test_alert_on_related_field(self): - admin = self._create_admin() - m = self._create_general_metric(configuration='test_alert_on_rf') - self._create_alert_settings( - metric=m, custom_operator='>', custom_threshold=30, custom_tolerance=0 - ) - - with self.subTest('Test notification for metric without related field'): - with self.assertRaises(ValueError) as err: - m.write(1) - self.assertEqual( - str(err.exception), - 'write() missing positional argument: "extra_values" required for alert on related field', - ) - m.refresh_from_db() - self.assertEqual(m.is_healthy, True) - self.assertEqual(m.is_healthy_tolerant, True) - self.assertEqual(Notification.objects.count(), 0) - - with self.subTest('Test notification for metric on different related field'): - with self.assertRaises(ValueError) as err: - m.write(10, extra_values={'test_related_3': 40}) - self.assertEqual( - str(err.exception), - '"test_related_3" is not defined for alert_on_related_field in metric configuration', - ) - m.refresh_from_db() - self.assertEqual(m.is_healthy, True) - self.assertEqual(m.is_healthy_tolerant, True) - self.assertEqual(Notification.objects.count(), 0) - - with self.subTest('Test notification for metric with multiple related fields'): - m.write(10, extra_values={'test_related_2': 40, 'test_related_3': 20}) - m.refresh_from_db() - self.assertEqual(m.is_healthy, False) - self.assertEqual(m.is_healthy_tolerant, False) - self.assertEqual(Notification.objects.count(), 1) - n = notification_queryset.first() - self.assertEqual(n.recipient, admin) - self.assertEqual(n.actor, m) - self.assertEqual(n.action_object, m.alertsettings) - self.assertEqual(n.level, 'warning') - - with self.subTest( - 'Test notification for metric exceeding related field alert settings' - ): - m.write(10, extra_values={'test_related_2': 40}) - m.refresh_from_db() - self.assertEqual(m.is_healthy, False) - self.assertEqual(m.is_healthy_tolerant, False) - self.assertEqual(Notification.objects.count(), 1) - n = notification_queryset.first() - self.assertEqual(n.recipient, admin) - self.assertEqual(n.actor, m) - self.assertEqual(n.action_object, m.alertsettings) - self.assertEqual(n.level, 'warning') - - with self.subTest( - 'Test no double alarm for metric exceeding related field alert settings' - ): - m.write(20, extra_values={'test_related_2': 35}) - m.refresh_from_db() - self.assertEqual(m.is_healthy, False) - self.assertEqual(m.is_healthy_tolerant, False) - self.assertEqual(Notification.objects.count(), 1) - - with self.subTest( - 'Test notification for metric falling behind related field alert settings' - ): - m.write(30, extra_values={'test_related_2': 25}) - m.refresh_from_db() - self.assertEqual(m.is_healthy, True) - self.assertEqual(m.is_healthy_tolerant, True) - self.assertEqual(Notification.objects.count(), 2) - n = notification_queryset.last() - self.assertEqual(n.recipient, admin) - self.assertEqual(n.actor, m) - self.assertEqual(n.action_object, m.alertsettings) - self.assertEqual(n.level, 'info') - - def test_general_check_threshold_with_alert_on_rf_crossed_deferred(self): - admin = self._create_admin() - m = self._create_general_metric(configuration='test_alert_on_rf') - self._create_alert_settings( - metric=m, custom_operator='>', custom_threshold=30, custom_tolerance=1 - ) - m.write(10, time=ten_minutes_ago, extra_values={'test_related_2': 35}) - m.refresh_from_db() - self.assertEqual(m.is_healthy, False) - self.assertEqual(m.is_healthy_tolerant, False) - self.assertEqual(Notification.objects.count(), 1) - n = notification_queryset.first() - self.assertEqual(n.recipient, admin) - self.assertEqual(n.actor, m) - self.assertEqual(n.action_object, m.alertsettings) - self.assertEqual(n.level, 'warning') - - def test_general_check_threshold_with_alert_on_rf_deferred_not_crossed(self): - self._create_admin() - m = self._create_general_metric(configuration='test_alert_on_rf') - self._create_alert_settings( - metric=m, custom_operator='>', custom_threshold=30, custom_tolerance=1 - ) - m.write(10, extra_values={'test_related_2': 32}) - self.assertEqual(m.is_healthy, True) - self.assertEqual(m.is_healthy_tolerant, True) - self.assertEqual(Notification.objects.count(), 0) - m.write(20, extra_values={'test_related_2': 35}) - self.assertEqual(m.is_healthy, True) - self.assertEqual(m.is_healthy_tolerant, True) - self.assertEqual(Notification.objects.count(), 0) - class TestTransactionMonitoringNotifications(DeviceMonitoringTransactionTestcase): device_model = Device