From d512e900c175eba4ffb9e705c498b5e10da12cdd Mon Sep 17 00:00:00 2001 From: Gagan Deep Date: Thu, 14 Mar 2024 23:07:52 +0530 Subject: [PATCH] [req-changes] Use a separate metric for storing total registered users --- docs/source/user/radius_monitoring.rst | 4 +- .../integrations/monitoring/apps.py | 18 +- .../integrations/monitoring/configuration.py | 31 +-- .../integrations/monitoring/tasks.py | 179 ++++++++++++++---- .../monitoring/tests/test_metrics.py | 127 ------------- tests/openwisp2/settings.py | 43 +++-- 6 files changed, 187 insertions(+), 215 deletions(-) diff --git a/docs/source/user/radius_monitoring.rst b/docs/source/user/radius_monitoring.rst index 5305ea05..e171679e 100644 --- a/docs/source/user/radius_monitoring.rst +++ b/docs/source/user/radius_monitoring.rst @@ -19,11 +19,11 @@ RADIUS metrics This chart shows number of users signed up using different registration methods. -2. Cumulative user registrations +2. Total user registrations ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ .. image:: /images/total-user-registration-chart.png - :alt: Cumulative user registration chart + :alt: Total user registration chart This chart shows total users registered using different registration methods in the system on a given date. diff --git a/openwisp_radius/integrations/monitoring/apps.py b/openwisp_radius/integrations/monitoring/apps.py index 0f1ab96f..c1ad0be1 100644 --- a/openwisp_radius/integrations/monitoring/apps.py +++ b/openwisp_radius/integrations/monitoring/apps.py @@ -110,26 +110,10 @@ def register_radius_metrics(self): _register_chart_configuration_choice(chart_key, chart_config) def connect_signal_receivers(self): - from .receivers import ( - post_save_organizationuser, - post_save_radiusaccounting, - post_save_registereduser, - ) + from .receivers import post_save_radiusaccounting - OrganizationUser = load_model('openwisp_users', 'OrganizationUser') - RegisteredUser = load_model('openwisp_radius', 'RegisteredUser') RadiusAccounting = load_model('openwisp_radius', 'RadiusAccounting') - post_save.connect( - post_save_organizationuser, - sender=OrganizationUser, - dispatch_uid='post_save_organizationuser_user_signup_metric', - ) - post_save.connect( - post_save_registereduser, - sender=RegisteredUser, - dispatch_uid='post_save_registereduser_user_signup_metric', - ) post_save.connect( post_save_radiusaccounting, sender=RadiusAccounting, diff --git a/openwisp_radius/integrations/monitoring/configuration.py b/openwisp_radius/integrations/monitoring/configuration.py index 3d8ae4d5..1e6ceec4 100644 --- a/openwisp_radius/integrations/monitoring/configuration.py +++ b/openwisp_radius/integrations/monitoring/configuration.py @@ -28,11 +28,12 @@ 'description': _('Daily user registration grouped by registration method'), 'summary_labels': user_signups_chart_summary_labels, 'order': 240, + 'filter__all__': True, 'unit': '', 'calculate_total': True, 'query': { 'influxdb': ( - "SELECT COUNT(DISTINCT(user_id)) FROM " + "SELECT SUM(count) FROM " " {key} WHERE time >= '{time}' {end_date} {organization_id}" " GROUP BY time(1d), method" ) @@ -53,18 +54,16 @@ ], } -cumulative_user_singups_chart_config = deepcopy(user_singups_chart_config) -cumulative_user_singups_chart_config['query']['influxdb'] = ( - "SELECT CUMULATIVE_SUM(COUNT(DISTINCT(user_id))) FROM " +total_user_singups_chart_config = deepcopy(user_singups_chart_config) +total_user_singups_chart_config['query']['influxdb'] = ( + "SELECT LAST(count) FROM " " {key} WHERE time >= '{time}' {end_date} {organization_id}" " GROUP BY time(1d), method" ) -cumulative_user_singups_chart_config['summary_query'] = { - 'influxdb': user_singups_chart_config['query']['influxdb'] -} -cumulative_user_singups_chart_config['title'] = _('Total Registered Users') -cumulative_user_singups_chart_config['label'] = _('Total Registered Users') -cumulative_user_singups_chart_config['order'] = 241 +total_user_singups_chart_config['title'] = _('Total Registered Users') +total_user_singups_chart_config['label'] = _('Total Registered Users') +total_user_singups_chart_config['filter__all__'] = True +total_user_singups_chart_config['order'] = 241 RADIUS_METRICS = { @@ -72,10 +71,18 @@ 'label': _('User Registration'), 'name': 'User Registration', 'key': 'user_signups', - 'field_name': 'user_id', + 'field_name': 'count', 'charts': { 'user_signups': user_singups_chart_config, - 'tot_user_signups': cumulative_user_singups_chart_config, + }, + }, + 'tot_user_signups': { + 'label': _('Total User Registration'), + 'name': 'Total User Registration', + 'key': 'tot_user_signups', + 'field_name': 'count', + 'charts': { + 'tot_user_signups': total_user_singups_chart_config, }, }, 'radius_acc': { diff --git a/openwisp_radius/integrations/monitoring/tasks.py b/openwisp_radius/integrations/monitoring/tasks.py index 79f8eb29..6f570662 100644 --- a/openwisp_radius/integrations/monitoring/tasks.py +++ b/openwisp_radius/integrations/monitoring/tasks.py @@ -2,15 +2,20 @@ import logging from celery import shared_task +from django.contrib.auth import get_user_model from django.contrib.contenttypes.models import ContentType +from django.db.models import Count +from django.utils import timezone from swapper import load_model Metric = load_model('monitoring', 'Metric') Chart = load_model('monitoring', 'Chart') RegisteredUser = load_model('openwisp_radius', 'RegisteredUser') +RadiusAccounting = load_model('openwisp_radius', 'RadiusAccounting') OrganizationUser = load_model('openwisp_users', 'OrganizationUser') Device = load_model('config', 'Device') DeviceLocation = load_model('geo', 'Location') +User = get_user_model() logger = logging.getLogger(__name__) @@ -27,57 +32,149 @@ def clean_registration_method(method): return method -@shared_task -def post_save_registereduser(user_id, registration_method): +def _get_user_signup_metric(organization_id, registration_method): + metric, _ = Metric._get_or_create( + configuration='user_signups', + name='User SignUps', + key='user_signups', + object_id=None, + content_type=None, + extra_tags={ + 'organization_id': str(organization_id), + 'method': registration_method, + }, + ) + return metric + + +def _get_total_user_signup_metric(organization_id, registration_method): + metric, _ = Metric._get_or_create( + configuration='tot_user_signups', + name='Total User SignUps', + key='tot_user_signups', + object_id=None, + content_type=None, + extra_tags={ + 'organization_id': str(organization_id), + 'method': registration_method, + }, + ) + return metric + + +def _write_user_signup_metric_for_all(metric_key): metric_data = [] - org_query = OrganizationUser.objects.filter(user_id=user_id).values_list( - 'organization_id', flat=True + start_time = timezone.now() - timezone.timedelta(hours=1) + end_time = timezone.now() + if metric_key == 'user_signups': + get_metric_func = _get_user_signup_metric + else: + get_metric_func = _get_total_user_signup_metric + # Get the total number of registered users + registered_user_query = RegisteredUser.objects + if metric_key == 'user_signups': + registered_user_query = registered_user_query.filter( + user__date_joined__gt=start_time, + user__date_joined__lte=end_time, + ) + total_registered_users = dict( + registered_user_query.values_list('method').annotate( + count=Count('user', distinct=True) + ) ) - if not org_query: - logger.warning( - f'"{user_id}" is not a member of any organization.' - ' Skipping user_signup metric writing!' + # Some manually created users, like superuser may not have a + # RegisteredUser object. We would could them with "unspecified" method + users_without_registereduser_query = User.objects.filter( + registered_user__isnull=True + ) + if metric_key == 'user_signups': + users_without_registereduser_query = users_without_registereduser_query.filter( + date_joined__gt=start_time, + date_joined__lte=end_time, ) - return - registration_method = clean_registration_method(registration_method) - for org_id in org_query: - metric, _ = Metric._get_or_create( - configuration='user_signups', - name='User SignUps', - key='user_signups', - object_id=None, - content_type=None, - extra_tags={ - 'organization_id': str(org_id), - 'method': registration_method, - }, + users_without_registereduser = users_without_registereduser_query.count() + + # Add the number of users which do not have a related RegisteredUser + # to the number of users which registered using "unspecified" method. + try: + total_registered_users[''] = ( + total_registered_users[''] + users_without_registereduser ) - metric_data.append((metric, {'value': sha1_hash(str(user_id))})) + except KeyError: + total_registered_users[''] = users_without_registereduser + + for method, count in total_registered_users.items(): + method = clean_registration_method(method) + metric = get_metric_func(organization_id='__all__', registration_method=method) + metric_data.append((metric, {'value': count})) Metric.batch_write(metric_data) -@shared_task -def post_save_organizationuser(user_id, organization_id): - try: - registration_method = ( - RegisteredUser.objects.only('method').get(user_id=user_id).method +def _write_user_signup_metrics_for_orgs(metric_key): + metric_data = [] + start_time = timezone.now() - timezone.timedelta(hours=1) + end_time = timezone.now() + if metric_key == 'user_signups': + get_metric_func = _get_user_signup_metric + else: + get_metric_func = _get_total_user_signup_metric + + # Get the registration data for the past hour. + # The query returns a tuple of organization_id, registration_method and + # count of users who registered with that organization and method. + registered_users_query = RegisteredUser.objects + if metric_key == 'user_signups': + registered_users_query = registered_users_query.filter( + user__openwisp_users_organizationuser__created__gt=start_time, + user__openwisp_users_organizationuser__created__lte=end_time, ) - except RegisteredUser.DoesNotExist: - logger.warning( - f'RegisteredUser object not found for "{user_id}".' - ' Skipping user_signup metric writing!' + registered_users = registered_users_query.values_list( + 'user__openwisp_users_organizationuser__organization_id', 'method' + ).annotate(count=Count('user_id', distinct=True)) + + # There could be users which were manually created (e.g. superuser) + # which do not have related RegisteredUser object. Add the count + # of such users with the "unspecified" method. + users_without_registereduser_query = OrganizationUser.objects.filter( + user__registered_user__isnull=True + ) + if metric_key == 'user_signups': + users_without_registereduser_query = users_without_registereduser_query.filter( + created__gt=start_time, created__lte=end_time + ) + users_without_registereduser = dict( + users_without_registereduser_query.values_list('organization_id').annotate( + count=Count('user_id', distinct=True) ) - return - registration_method = clean_registration_method(registration_method) - metric, _ = Metric._get_or_create( - configuration='user_signups', - name='User SignUps', - key='user_signups', - object_id=None, - content_type=None, - extra_tags={'organization_id': organization_id, 'method': registration_method}, ) - metric.write(sha1_hash(str(user_id))) + + for org_id, registration_method, count in registered_users: + registration_method = clean_registration_method(registration_method) + if registration_method == 'unspecified': + count += users_without_registereduser.get(org_id, 0) + metric = get_metric_func( + organization_id=org_id, registration_method=registration_method + ) + metric_data.append((metric, {'value': count})) + Metric.batch_write(metric_data) + + +@shared_task +def write_user_registration_metrics(): + """ + This task is expected to be executed hourly. + + This task writes user registration metrics to the InfluxDB. + It writes to the following metrics: + - User Signups: This shows the number of new users who + have registered using different methods + - Total User Signups: This shows the total number of + users registered using different methods + """ + _write_user_signup_metric_for_all(metric_key='user_signups') + _write_user_signup_metric_for_all(metric_key='tot_user_signups') + _write_user_signup_metrics_for_orgs(metric_key='user_signups') + _write_user_signup_metrics_for_orgs(metric_key='tot_user_signups') @shared_task diff --git a/openwisp_radius/integrations/monitoring/tests/test_metrics.py b/openwisp_radius/integrations/monitoring/tests/test_metrics.py index bf48260b..96e5e934 100644 --- a/openwisp_radius/integrations/monitoring/tests/test_metrics.py +++ b/openwisp_radius/integrations/monitoring/tests/test_metrics.py @@ -27,105 +27,6 @@ def _create_registered_user(self, **kwargs): reg_user.save() return reg_user - @patch('logging.Logger.warning') - def test_post_save_registered_user(self, *args): - user = self._create_user() - org1 = self._create_org(name='org1') - org2 = self._create_org(name='org2') - self._create_org_user(user=user, organization=org1) - self._create_org_user(user=user, organization=org2) - self._create_registered_user(user=user) - self.assertEqual( - self.metric_model.objects.filter( - configuration='user_signups', - name='User SignUps', - key='user_signups', - object_id=None, - content_type=None, - ).count(), - 1, - ) - metric = self.metric_model.objects.filter(key='user_signups').first() - points = metric.read() - # The query performs DISTINCT on the username field, - # therefore the read() method returns only one point. - self.assertEqual(len(points), 1) - # Assert username field is hashed - self.assertNotEqual(points[0]['user_id'], str(user.id)) - - @patch('openwisp_monitoring.monitoring.models.Metric.write') - @patch('logging.Logger.warning') - @patch('openwisp_monitoring.monitoring.models.Metric.batch_write') - def test_post_save_registered_user_edge_cases( - self, mocked_metric_batch_write, mocked_logger, *args - ): - user = self._create_user() - - with self.subTest('Test organization user does not exist'): - reg_user = self._create_registered_user(user=user) - mocked_logger.assert_called_once_with( - f'"{user.id}" is not a member of any organization.' - ' Skipping user_signup metric writing!' - ) - mocked_metric_batch_write.assert_not_called() - - self._create_org_user(user=user) - - with self.subTest('Test saving an existing RegisteredUser object'): - with patch(f'{TASK_PATH}.post_save_registereduser.delay') as mocked_task: - reg_user.is_verified = True - reg_user.full_clean() - reg_user.save() - mocked_task.assert_not_called() - mocked_metric_batch_write.assert_not_called() - - @patch('logging.Logger.warning') - def test_post_save_organization_user(self, *args): - user = self._create_user() - self._create_registered_user(user=user, method='') - self._create_org_user(user=user) - self.assertEqual( - self.metric_model.objects.filter( - configuration='user_signups', - name='User SignUps', - key='user_signups', - object_id=None, - content_type=None, - ).count(), - 1, - ) - metric = self.metric_model.objects.filter(key='user_signups').first() - points = metric.read() - self.assertEqual(len(points), 1) - # Assert username field is hashed - self.assertNotEqual(points[0]['user_id'], str(user.id)) - - @patch('openwisp_monitoring.monitoring.models.Metric.batch_write') - @patch('logging.Logger.warning') - @patch('openwisp_monitoring.monitoring.models.Metric.write') - def test_post_save_organization_user_edge_cases( - self, mocked_metric_write, mocked_logger, *args - ): - user = self._create_user() - - with self.subTest('Test registered user does not exist'): - org_user = self._create_org_user(user=user) - mocked_logger.assert_called_once_with( - f'RegisteredUser object not found for "{user.id}".' - ' Skipping user_signup metric writing!' - ) - mocked_metric_write.assert_not_called() - - self._create_registered_user(user=user) - - with self.subTest('Test saving an existing RegisteredUser object'): - with patch(f'{TASK_PATH}.post_save_organizationuser.delay') as mocked_task: - org_user.is_admin = True - org_user.full_clean() - org_user.save() - mocked_task.assert_not_called() - mocked_metric_write.assert_not_called() - @patch('logging.Logger.warning') def test_post_save_radiusaccounting(self, *args): user = self._create_user() @@ -256,31 +157,3 @@ def test_post_save_radius_accounting(self, mocked_logger): f' and organization "{self.default_org.id}".' ' The metric will be written without a related object!' ) - - @patch('openwisp_monitoring.monitoring.models.Metric.batch_write') - @patch('logging.Logger.warning') - def test_post_save_radiusaccounting_edge_cases( - self, mocked_logger, mocked_metric_write, *args - ): - options = _RADACCT.copy() - options['called_station_id'] = '00:00:00:00:00:00' - options['unique_id'] = '117' - with self.subTest('Test session is not closed'): - with patch(f'{TASK_PATH}.post_save_registereduser.delay') as mocked_task: - rad_acc = self._create_radius_accounting(**options) - self.assertEqual(rad_acc.stop_time, None) - mocked_task.assert_not_called() - mocked_metric_write.assert_not_called() - - user = self._create_user() - options['username'] = user.username - options['unique_id'] = '118' - options['stop_time'] = options['start_time'] - - with self.subTest('Test RegisteredUser object does not exist'): - rad_acc = self._create_radius_accounting(**options) - self.assertNotEqual(rad_acc.stop_time, None) - mocked_logger.assert_called_once_with( - f'RegisteredUser object not found for "{user.username}".' - ' Skipping radius_acc metric writing!' - ) diff --git a/tests/openwisp2/settings.py b/tests/openwisp2/settings.py index 09db1843..dfd917a4 100644 --- a/tests/openwisp2/settings.py +++ b/tests/openwisp2/settings.py @@ -1,5 +1,6 @@ import os import sys +from datetime import timedelta from celery.schedules import crontab @@ -171,22 +172,22 @@ }, } -if not TESTING and SHELL: - LOGGING['loggers'] = { - 'django.db': { - 'level': 'DEBUG', - 'handlers': ['console'], - 'propagate': False, - }, - '': { - # this sets root level logger to log debug and higher level - # logs to console. All other loggers inherit settings from - # root level logger. - 'handlers': ['console', 'django.server'], - 'level': 'DEBUG', - 'propagate': False, - }, - } +# if not TESTING and SHELL: +# LOGGING['loggers'] = { +# 'django.db': { +# 'level': 'DEBUG', +# 'handlers': ['console'], +# 'propagate': False, +# }, +# '': { +# # this sets root level logger to log debug and higher level +# # logs to console. All other loggers inherit settings from +# # root level logger. +# 'handlers': ['console', 'django.server'], +# 'level': 'DEBUG', +# 'propagate': False, +# }, +# } # WARNING: for development only! AUTH_PASSWORD_VALIDATORS = [] @@ -381,6 +382,16 @@ } } DATABASES['default']['ENGINE'] = 'openwisp_utils.db.backends.spatialite' + CELERY_BEAT_SCHEDULE.update( + { + 'write_user_registration_metrics': { + 'task': 'openwisp_radius.integrations.monitoring.tasks.write_user_registration_metrics', + 'schedule': timedelta(hours=1), + 'args': None, + 'relative': True, + } + } + ) if os.environ.get('SAMPLE_APP', False): INSTALLED_APPS.remove('openwisp_radius')