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

[feature] Added check for WiFi Clients #623

Open

pandafy wants to merge 18 commits into master from wifi-client-check

Member

pandafy commented Dec 11, 2024 •

edited by nemesifier

Loading

Checklist

I have read the OpenWISP Contributing Guidelines.
I have manually tested the changes proposed in this pull request.
I have written new test cases for new code and/or updated existing tests for changes to existing code.
I have updated the documentation.

Description of Changes

An alert is sent when the connected clients are less than configured threshold
An alert is sent when the connected clients are more than configured threshold

pandafy force-pushed the wifi-client-check branch from 085383d to 958bd6d Compare

December 12, 2024 16:24

pandafy marked this pull request as ready for review

December 13, 2024 15:46

nemesifier requested changes

View reviewed changes

Member

nemesifier left a comment

The ordering in the alert settings tab seems alphabetical, so calling the two checks Max WiFi Clients and Min WiFi Clients causes the memory check to sit in between, which is bad ux. I think we should rename these to "WiFi Clients (Maximum)" and "WiFi Clients (Minimum)".

docs/user/checks.rst Outdated

    
              .. _wifi_client_check:

              WiFi Client

Member

nemesifier Jan 2, 2025

Suggested change

      
            WiFi Client
          
            WiFi Clients (Maximum & Minimum)

docs/user/checks.rst Outdated

    
              This check sends alerts based on the total number of WiFi Clients

              connected to a device. It sends two types of alerts:

              - **Maximum WiFi Users**: When the total number of WiFi clients connected

Member

nemesifier Jan 2, 2025

Suggested change

      
            - **Maximum WiFi Users**: When the total number of WiFi clients connected
          
            - **Maximum WiFi Clients**: When the total number of WiFi clients connected

I recommend consistency with the wording to avoid confusing readers.

docs/user/checks.rst Outdated

    
                provides valuable insights into the network's performance, signaling

                when a specific access point is overwhelmed by an excessive number of

                WiFi users.

              - **Minimum WiFi Users**: When the total number of WiFi clients connected

Member

nemesifier Jan 2, 2025

Suggested change

      
            - **Minimum WiFi Users**: When the total number of WiFi clients connected
          
            - **Minimum WiFi Clients**: When the total number of WiFi clients connected

Same here.

openwisp_monitoring/check/settings.py Outdated

@@ @@ -8,6 +8,7 @@ @@
                       ('openwisp_monitoring.check.classes.Ping', 'Ping'),
                       ('openwisp_monitoring.check.classes.ConfigApplied', 'Configuration Applied'),
                       ('openwisp_monitoring.check.classes.Iperf3', 'Iperf3'),
+                      ('openwisp_monitoring.check.classes.WifiClient', 'Wifi Client'),

Member

nemesifier Jan 2, 2025

Suggested change

      
                    ('openwisp_monitoring.check.classes.WifiClient', 'Wifi Client'),
          
                    ('openwisp_monitoring.check.classes.WifiClients', 'Wifi Clients'),

docs/user/settings.rst Outdated

               than the total iperf3 check time, i.e. greater than the TCP + UDP test
               time. By default, it is set to **600 seconds (10 mins)**.
+              .. _openwisp_monitoring_auto_wifi_client_check:
+              ``OPENWISP_MONITORING_AUTO_WIFI_CLIENT_CHECK``

Member

nemesifier Jan 2, 2025

I think we should use "clients" (plural) instead of client in all references to this feature.

openwisp_monitoring/monitoring/configuration.py Outdated

+                          },
+                      },
+                  },
+                  'min_wifi_clients': {

Member

nemesifier Jan 2, 2025

Suggested change

      
                'min_wifi_clients': {
          
                'wifi_clients_min': {

openwisp_monitoring/monitoring/configuration.py Outdated

+                      },
+                  },
+                  'min_wifi_clients': {
+                      'label': _('Min WiFi Clients'),

Member

nemesifier Jan 2, 2025

similar change here as for max

openwisp_monitoring/monitoring/configuration.py Outdated

+                  'min_wifi_clients': {
+                      'label': _('Min WiFi Clients'),
+                      'name': '{name}',
+                      'key': 'min_wifi_clients',

Member

nemesifier Jan 2, 2025

similar change here as for max

openwisp_monitoring/monitoring/configuration.py Outdated

+                      'name': '{name}',
+                      'key': 'min_wifi_clients',
+                      'field_name': 'clients',
+                      'alert_settings': {'operator': '<', 'threshold': 0, 'tolerance': 240},

Member

nemesifier Jan 2, 2025

Suggested change

      
                    'alert_settings': {'operator': '<', 'threshold': 0, 'tolerance': 240},
          
                    'alert_settings': {'operator': '<', 'threshold': 1, 'tolerance': 1440},

Shouldn't this be less than 1? Would less than zero trigger at all?

I would set a tolerance of 3 days by default (4320 minutes), which would indicate an AP hasn't been used for 3 days in a row, which would account for a full weekend plus festive day, a common occurrence in many countries.

openwisp_monitoring/monitoring/configuration.py Outdated

+                      'name': '{name}',
+                      'key': 'max_wifi_clients',
+                      'field_name': 'clients',
+                      'alert_settings': {'operator': '>', 'threshold': 40, 'tolerance': 60},

Member

nemesifier Jan 2, 2025

Suggested change

      
                    'alert_settings': {'operator': '>', 'threshold': 40, 'tolerance': 60},
          
                    'alert_settings': {'operator': '>', 'threshold': 50, 'tolerance': 120},

pandafy force-pushed the wifi-client-check branch 3 times, most recently from b5cdd83 to f5c0b05 Compare

January 8, 2025 14:23

nemesifier requested changes

View reviewed changes

openwisp_monitoring/check/classes/wifi_clients.py Outdated

+                  def check(self, store=True):
+                      values = timeseries_db.read(
+                          key='wifi_clients',
+                          fields='COUNT(DISTINCT(clients))',

Member

nemesifier Jan 8, 2025

As discussed, please move the influxdb query language details to the timeseries DB backend.

openwisp_monitoring/check/classes/wifi_clients.py Outdated

+                              'content_type': self.related_object._meta.label_lower,
+                              'object_id': str(self.related_object.pk),
+                          },
+                          since="'{}'".format(

Member

nemesifier Jan 8, 2025

same here

pandafy commented

View reviewed changes

openwisp_monitoring/monitoring/base/models.py

    
            @@ -962,7 +962,7 @@ def _is_crossed_by(self, current_value, time=None, retention_policy=None):
          
                      if time is None:

                          # retrieves latest measurements, ordered by most recent first

                          points = self.metric.read(

                              since=f'{self._tolerance_search_range}m',

                              since=timezone.now() - timedelta(minutes=self._tolerance_search_range),

Member Author

pandafy Jan 9, 2025

Before this change, the code was iterating though points that was beyond the tolerance search range because of the broken InfluxQL query (time >= 15m).

pandafy force-pushed the wifi-client-check branch from f7ecaac to a1fa913 Compare

January 21, 2025 12:40

nemesifier reviewed

View reviewed changes

Member

nemesifier left a comment

LGTM! Testing it and waiting for more feedback.

nemesifier reviewed

View reviewed changes

Member

nemesifier left a comment

Feedback from testing:

As you had anticipated previously in a discussion we had, setting devices in PROBLEM state as soon as the check detects the configured minimum threshold of wifi clients is confusing for users, I think we could change the check to take into account the points of the a specified time range (eg: 3 days by default), we may need a new setting for this as using the tolerance wouldn't work, because this logic needs to be introduced in the checkitself.
I have seen alerts for devices which have been offline for a few days, can we look into ways of avoiding the alert to trigger if the device is in critical state already (it's redundant)?

pandafy added 13 commits

January 28, 2025 17:01


          [feature] Added check for WiFi Clients

2bcb4d9


          [docs] Added docs

36cfe74


          [chores] Fixed reference

e4be79b


          [req-changes] Changed "Client" > "Clients"

c16f2c2


          [skip ci][docs] Use single quotes

b6ab994


          [fix] Fixed time clause of InfluxQL query

179cbd2


          [revert][docs] Use single quotes

eb6b66b

This reverts commit d68559c.


          [fix] Improved Influxdb client's read method

ddf6c4d


          [fix] Fixed tolerance for min WiFi Clients alert

ccaea8d


          [req-change] Allowed skipping check for time range

be57f52


          [req-changes] Modified logic for snoozing WiFi Clients check

c976fb6


          [chores] Added check for OPENWISP_MONITORING_WIFI_CLIENTS_CHECK_SNOOZ…

1ef1505

…E_SCHEDULE


          [req-change] Changed evaluation criteria for WifiClients checks

0fc683e

- The check counts the number of connected clients for the period
  defined in OPENWISP_MONITORING_WIFI_CLIENTS_MIN_CHECK_INTERVAL
  and OPENWISP_MONITORING_WIFI_CLIENTS_MAX_CHECK_INTERVAL
- Don't send alerts for devices that have "critical" health status

pandafy force-pushed the wifi-client-check branch from a653668 to 0fc683e Compare

January 28, 2025 11:31

pandafy added 3 commits

January 28, 2025 17:16


          [chores] Added WifiClients to the celertybeat schedule

d5b98b1


          [change] Don't execute test for devices in critical health status

cfb0792


          [req-changes] Fixed WifiClient check for sending alerts

9e50ba8

nemesifier requested changes

View reviewed changes

docs/user/settings.rst Outdated

+              <wifi_clients_check>` to monitor the maximum number of connected clients.
+              This determines how far back the check looks when evaluating the maximum
+              client threshold.

Member

nemesifier Jan 28, 2025

I think this sentence is not really human friendly.

I think we should explain that we look for clients which have connected between now and the interval of minutes specified there of something like that.

docs/user/settings.rst Outdated

+              <wifi_clients_check>` to monitor the minimum number of connected clients.
+              This determines how far back the check looks when evaluating the minimum
+              clients threshold.

Member

nemesifier Jan 28, 2025

Same here and we can spend a few words on why this is a lot higher.


          [skip ci][req-changes] Updated docs

b596049

nemesifier reviewed

View reviewed changes

Member

nemesifier left a comment

Looking better, let's continue testing a bit more!


          Merge branch 'master' into wifi-client-check

f664010

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

Labels

None yet