[change] Consider multiple metrics before marking as critical #566

- Added data collected check which checks whether monitoring data
  is being received, flagged as critical metric
- Set status to critical only when all the critical metrics are unhealthy:
  By default both ping and data collected checks must be failing for
  devices to be flagged as critical, otherwise they will be flagged as
  problem
- This change also avoids the recovery detection mechanism to
  be triggered unnecessarily
- The recovery detection mechanism is not triggered anymore
  by checksum requests

Closes #566

---------

Co-authored-by: Federico Capoano <f.capoano@openwisp.io>

Author: pandafy

docs/user/checks.rst

@@ -35,6 +35,16 @@ configuration status of a device changes, this ensures the check reacts
 quickly to events happening in the network and informs the user promptly
 if there's anything that is not working as intended.
 
+.. _monitoring_data_collected_check:
+
+Monitoring Data Collected
+-------------------------
+
+This check ensures that the server is receiving metrics from network
+devices in a timely manner. You may choose to disable auto creation of
+this check by using the setting
+:ref:`openwisp_monitoring_auto_data_collected_check`.
+
 .. _iperf3_check:
 
 Iperf3

docs/user/device-health-status.rst

@@ -25,17 +25,18 @@ One of the metrics has a value which is not in the expected range (the
 threshold value set in the alert settings has been crossed).
 
 Example: CPU usage should be less than 90% but current value is at 95%.
+Or, the device is not reachable by ping check (ping is a critical metric),
+but the device is sending monitoring metrics.
 
 ``CRITICAL``
 ------------
 
-One of the metrics defined in
-``OPENWISP_MONITORING_CRITICAL_DEVICE_METRICS`` has a value which is not
-in the expected range (the threshold value set in the alert settings has
-been crossed).
+All of the metrics defined in
+:ref:`openwisp_monitoring_critical_device_metrics` have values outside the
+expected range.
 
-Example: ping is by default a critical metric which is expected to be
-always 1 (reachable).
+Example: Both :ref:`ping_check` and :ref:`monitoring_data_collected_check`
+are failing for the device.
 
 ``DEACTIVATED``
 ---------------

docs/user/settings.rst

@@ -619,19 +619,20 @@ Automatically created charts.
 ``OPENWISP_MONITORING_CRITICAL_DEVICE_METRICS``
 -----------------------------------------------
 
-============ ================================================
+============ ==================================================================
 **type**:    ``list`` of ``dict`` objects
-**default**: ``[{'key': 'ping', 'field_name': 'reachable'}]``
-============ ================================================
+**default**: .. code-block::
 
-Device metrics that are considered critical:
+                 [
+                     {"key": "ping", "field_name": "reachable"},
+                     {"key": "data_collected", "field_name": "data_collected"},
+                 ]
+============ ==================================================================
 
-when a value crosses the boundary defined in the "threshold value" field
-of the alert settings related to one of these metric types, the health
-status of the device related to the metric moves into ``CRITICAL``.
+Device metrics that are considered critical.
 
-By default, if devices are not reachable by pings they are flagged as
-``CRITICAL``.
+When all critical metrics have values outside their expected range, the
+device's health status changes to ``CRITICAL``.
 
 .. _openwisp_monitoring_health_status_labels:
 
@@ -754,6 +755,34 @@ This setting allows you to configure the config check interval used by
 :ref:`config_applied <config_applied_check>`. By default it is set to 5
 minutes.
 
+.. _openwisp_monitoring_auto_data_collected_check:
+
+``OPENWISP_MONITORING_AUTO_DATA_COLLECTED_CHECK``
+-------------------------------------------------
+
+============ ========
+**type**:    ``bool``
+**default**: ``True``
+============ ========
+
+This setting allows you to choose whether :ref:`monitoring data collected
+<monitoring_data_collected_check>` checks should be created automatically
+for newly registered devices. It's enabled by default.
+
+.. _openwisp_monitoring_data_collected_check_interval:
+
+``OPENWISP_MONITORING_DATA_COLLECTED_CHECK_INTERVAL``
+-----------------------------------------------------
+
+============ =======
+**type**:    ``int``
+**default**: ``60``
+============ =======
+
+This setting allows you to configure the data collected check interval
+used by :ref:`data collected <monitoring_data_collected_check>`. By
+default it is set to 60 minutes.
+
 .. _openwisp_monitoring_auto_wifi_clients_check:
 
 ``OPENWISP_MONITORING_AUTO_WIFI_CLIENTS_CHECK``

openwisp_monitoring/check/apps.py

@@ -4,7 +4,6 @@
 from swapper import load_model
 
 from openwisp_monitoring.check import checks  # noqa
-from openwisp_monitoring.check import settings as app_settings
 
 
 class CheckConfig(AppConfig):
@@ -16,37 +15,10 @@ def ready(self):
         self._connect_signals()
 
     def _connect_signals(self):
-        if app_settings.AUTO_PING:
-            from .base.models import auto_ping_receiver
+        Check = load_model('check', 'Check')
 
-            post_save.connect(
-                auto_ping_receiver,
-                sender=load_model('config', 'Device'),
-                dispatch_uid='auto_ping',
-            )
-
-        if app_settings.AUTO_CONFIG_CHECK:
-            from .base.models import auto_config_check_receiver
-
-            post_save.connect(
-                auto_config_check_receiver,
-                sender=load_model('config', 'Device'),
-                dispatch_uid='auto_config_check',
-            )
-        if app_settings.AUTO_IPERF3:
-            from .base.models import auto_iperf3_check_receiver
-
-            post_save.connect(
-                auto_iperf3_check_receiver,
-                sender=load_model('config', 'Device'),
-                dispatch_uid='auto_iperf3_check',
-            )
-
-        if app_settings.AUTO_WIFI_CLIENTS_CHECK:
-            from .base.models import auto_wifi_clients_check_receiver
-
-            post_save.connect(
-                auto_wifi_clients_check_receiver,
-                sender=load_model('config', 'Device'),
-                dispatch_uid='auto_wifi_clients_check',
-            )
+        post_save.connect(
+            Check.auto_create_check_receiver,
+            sender=load_model('config', 'Device'),
+            dispatch_uid='auto_create_check',
+        )

openwisp_monitoring/check/base/models.py

@@ -9,12 +9,7 @@
 from jsonfield import JSONField
 
 from openwisp_monitoring.check import settings as app_settings
-from openwisp_monitoring.check.tasks import (
-    auto_create_config_check,
-    auto_create_iperf3_check,
-    auto_create_ping,
-    auto_create_wifi_clients_check,
-)
+from openwisp_monitoring.check.tasks import auto_create_check
 from openwisp_utils.base import TimeStampedEditableModel
 
 from ...utils import transaction_on_commit
@@ -108,74 +103,54 @@ def perform_check_delayed(self, duration=0):
 
         perform_check.apply_async(args=[self.id], countdown=duration)
 
-
-def auto_ping_receiver(sender, instance, created, **kwargs):
-    """Implements OPENWISP_MONITORING_AUTO_PING.
-
-    The creation step is executed in the background.
-    """
-    # we need to skip this otherwise this task will be executed
-    # every time the configuration is requested via checksum
-    if not created:
-        return
-    transaction_on_commit(
-        lambda: auto_create_ping.delay(
-            model=sender.__name__.lower(),
-            app_label=sender._meta.app_label,
-            object_id=str(instance.pk),
+    @classmethod
+    def auto_create_check_receiver(cls, created, **kwargs):
+        if not created:
+            return
+        transaction_on_commit(lambda: _auto_check_receiver(created=created, **kwargs))
+
+
+def _auto_check_receiver(sender, instance, **kwargs):
+    model = sender.__name__.lower()
+    app_label = sender._meta.app_label
+    object_id = str(instance.pk)
+    if app_settings.AUTO_PING:
+        auto_create_check.delay(
+            model=model,
+            app_label=app_label,
+            object_id=object_id,
+            check_type='openwisp_monitoring.check.classes.Ping',
+            check_name='Ping',
         )
-    )
-
-
-def auto_config_check_receiver(sender, instance, created, **kwargs):
-    """Implements OPENWISP_MONITORING_AUTO_DEVICE_CONFIG_CHECK.
-
-    The creation step is executed in the background.
-    """
-    # we need to skip this otherwise this task will be executed
-    # every time the configuration is requested via checksum
-    if not created:
-        return
-    transaction_on_commit(
-        lambda: auto_create_config_check.delay(
-            model=sender.__name__.lower(),
-            app_label=sender._meta.app_label,
-            object_id=str(instance.pk),
+    if app_settings.AUTO_CONFIG_CHECK:
+        auto_create_check.delay(
+            model=model,
+            app_label=app_label,
+            object_id=object_id,
+            check_type='openwisp_monitoring.check.classes.ConfigApplied',
+            check_name='Configuration Applied',
         )
-    )
-
-
-def auto_iperf3_check_receiver(sender, instance, created, **kwargs):
-    """Implements OPENWISP_MONITORING_AUTO_IPERF3.
-
-    The creation step is executed in the background.
-    """
-    # we need to skip this otherwise this task will be executed
-    # every time the configuration is requested via checksum
-    if not created:
-        return
-    transaction_on_commit(
-        lambda: auto_create_iperf3_check.delay(
-            model=sender.__name__.lower(),
-            app_label=sender._meta.app_label,
-            object_id=str(instance.pk),
+    if app_settings.AUTO_IPERF3:
+        auto_create_check.delay(
+            model=model,
+            app_label=app_label,
+            object_id=object_id,
+            check_type='openwisp_monitoring.check.classes.Iperf3',
+            check_name='Iperf3',
         )
-    )
-
-
-def auto_wifi_clients_check_receiver(sender, instance, created, **kwargs):
-    """Implements OPENWISP_MONITORING_AUTO_WIFI_CLIENTS_CHECK.
-
-    The creation step is executed in the background.
-    """
-    # we need to skip this otherwise this task will be executed
-    # every time the configuration is requested via checksum
-    if not created:
-        return
-    transaction_on_commit(
-        lambda: auto_create_wifi_clients_check.delay(
-            model=sender.__name__.lower(),
-            app_label=sender._meta.app_label,
-            object_id=str(instance.pk),
+    if app_settings.AUTO_WIFI_CLIENTS_CHECK:
+        auto_create_check.delay(
+            model=model,
+            app_label=app_label,
+            object_id=object_id,
+            check_type='openwisp_monitoring.check.classes.WifiClients',
+            check_name='WiFi Clients',
+        )
+    if app_settings.AUTO_DATA_COLLECTED_CHECK:
+        auto_create_check.delay(
+            model=model,
+            app_label=app_label,
+            object_id=object_id,
+            check_type='openwisp_monitoring.check.classes.DataCollected',
+            check_name='Monitoring Data Collected',
         )
-    )

openwisp_monitoring/check/classes/__init__.py

@@ -1,4 +1,5 @@
 from .config_applied import ConfigApplied  # noqa
+from .data_collected import DataCollected  # noqa
 from .iperf3 import Iperf3  # noqa
 from .ping import Ping  # noqa
 from .wifi_clients import WifiClients  # noqa

openwisp_monitoring/check/classes/base.py

@@ -13,6 +13,19 @@ def __init__(self, check, params):
         self.related_object = check.content_object
         self.params = params
 
+    @classmethod
+    def get_related_metrics(cls):
+        """
+        Returns a tuple of metric names related to this check class.
+
+        The default implementation returns a tuple containing the lowercase
+        name of the class.
+
+        Returns:
+            tuple: A tuple of strings representing metric identifiers
+        """
+        return (cls.__name__.lower(),)
+
     def validate_instance(self):
         # check instance is of type device
         obj = self.related_object

openwisp_monitoring/check/classes/config_applied.py

@@ -11,6 +11,10 @@
 
 
 class ConfigApplied(BaseCheck):
+    @classmethod
+    def get_related_metrics(cls):
+        return ('config_applied',)
+
     def check(self, store=True):
         # If the device is down or does not have a config
         # do not run config applied check

openwisp_monitoring/check/classes/data_collected.py

@@ -0,0 +1,64 @@
+from django.utils import timezone
+from swapper import load_model
+
+from ...db import timeseries_db
+from ...device.utils import SHORT_RP
+from .. import settings as app_settings
+from .base import BaseCheck
+
+AlertSettings = load_model('monitoring', 'AlertSettings')
+Metric = load_model('monitoring', 'Metric')
+
+
+class DataCollected(BaseCheck):
+    @classmethod
+    def get_related_metrics(cls):
+        return ('data_collected',)
+
+    def check(self, store=True):
+        device_monitoring = self.related_object.monitoring
+        active_metrics = device_monitoring.get_active_metrics()
+        passive_metrics = device_monitoring.related_metrics.exclude(
+            configuration__in=active_metrics
+        ).values_list('key', flat=True)
+        if passive_metrics:
+            # Perform a query to the timeseries database to get the last
+            # written point for every passive metric. This is done to optimize
+            # the query instead of iterating over each metric and getting
+            # its last written point individually.
+            points = timeseries_db.read(
+                key=','.join(set(passive_metrics)),
+                fields='*',
+                tags={
+                    'content_type': self.related_object._meta.label_lower,
+                    'object_id': str(self.related_object.pk),
+                },
+                since=(
+                    timezone.localtime()
+                    - timezone.timedelta(
+                        minutes=app_settings.DATA_COLLECTED_CHECK_INTERVAL
+                    )
+                ),
+                limit=1,
+                order_by='-time',
+            )
+            result = int(len(points) > 0)
+        else:
+            result = 0
+        send_alert = device_monitoring.status != 'critical'
+        if store:
+            self._get_metric().write(
+                result, retention_policy=SHORT_RP, send_alert=send_alert
+            )
+        return {'data_collected': result}
+
+    def _get_metric(self):
+        metric, created = self._get_or_create_metric(configuration='data_collected')
+        if created:
+            self._create_alert_setting(metric)
+        return metric
+
+    def _create_alert_setting(self, metric):
+        alert_s = AlertSettings(metric=metric)
+        alert_s.full_clean()
+        alert_s.save()