[req-changes] Updated retrying mechanism

README.rst

@@ -1625,6 +1625,7 @@ This sub-app enables collection of following measurements:
 - OpenWISP Version
 - Enabled OpenWISP modules and their versions
 - OS details
+- Whether the event is related to a new installation or an upgrade
 
 We collect data on OpenWISP usage to gauge user engagement, satisfaction,
 and upgrade patterns. This informs our development decisions, ensuring

openwisp_utils/measurements/tasks.py

@@ -1,14 +1,13 @@
 import logging
-import time
 
-import requests
 from celery import shared_task
 from openwisp_utils.admin_theme.system_info import (
     get_enabled_openwisp_modules,
     get_openwisp_version,
     get_os_details,
 )
 
+from ..utils import retryable_request
 from .models import OpenwispVersion
 from .utils import _get_events, get_openwisp_module_events, get_os_detail_events
 
@@ -19,35 +18,24 @@
 
 
 def post_clean_insights_events(events):
-    tries = 0
-    while tries < MAX_TRIES:
-        try:
-            response = requests.post(
-                CLEAN_INSIGHTS_URL,
-                json={
-                    'idsite': 1,
-                    'events': events,
-                },
-            )
-            assert response.status_code == 204
-            return
-        except Exception as error:
-            tries += 1
-            if isinstance(error, AssertionError):
-                message = f'HTTP {response.status_code} Response'
-                if response.status_code in (400, 404):
-                    # Retrying sending data would result in the
-                    # same error.
-                    break
-            else:
-                message = str(error)
-            logger.warning(
-                f'Error posting clean insights events: {message}. Retrying in 5 seconds.'
-            )
-            time.sleep(5)
-    logger.error(
-        f'Maximum tries reach to upload Clean Insights measurements. Error: {message}'
-    )
+    try:
+        response = retryable_request(
+            'post',
+            url=CLEAN_INSIGHTS_URL,
+            json={
+                'idsite': 5,
+                'events': events,
+            },
+        )
+        assert response.status_code == 204
+    except Exception as error:
+        if isinstance(error, AssertionError):
+            message = f'HTTP {response.status_code} Response'
+        else:
+            message = str(error)
+        logger.error(
+            f'Maximum tries reached to upload Clean Insights measurements. Error: {message}'
+        )
 
 
 @shared_task

openwisp_utils/measurements/tests/test_models.py

@@ -6,6 +6,7 @@
 from django.db import migrations
 from django.test import TestCase, override_settings
 from freezegun import freeze_time
+from urllib3.response import HTTPResponse
 
 from .. import tasks
 from ..models import OpenwispVersion
@@ -167,31 +168,33 @@ def test_post_clean_insights_events_400_response(
     ):
         bad_response = requests.Response()
         bad_response.status_code = 400
-        with patch.object(requests, 'post', return_value=bad_response) as mocked_post:
+        with patch.object(
+            requests.Session, 'post', return_value=bad_response
+        ) as mocked_post:
             tasks.send_clean_insights_measurements.delay()
         mocked_post.assert_called_once()
         mocked_warning.assert_not_called()
         mocked_error.assert_called_with(
-            'Maximum tries reach to upload Clean Insights measurements. Error: HTTP 400 Response'
+            'Maximum tries reached to upload Clean Insights measurements.'
+            ' Error: HTTP 400 Response'
         )
 
-    @patch('time.sleep')
-    @patch('logging.Logger.warning')
+    @patch('urllib3.util.retry.Retry.sleep')
+    @patch(
+        'urllib3.connectionpool.HTTPConnection.getresponse',
+        return_value=HTTPResponse(status=500, version='1.1'),
+    )
     @patch('logging.Logger.error')
     def test_post_clean_insights_events_500_response(
-        self, mocked_error, mocked_warning, *args
+        self, mocked_error, mocked_getResponse, *args
     ):
-        bad_response = requests.Response()
-        bad_response.status_code = 500
-        with patch.object(requests, 'post', return_value=bad_response) as mocked_post:
-            tasks.send_clean_insights_measurements.delay()
-        self.assertEqual(len(mocked_post.mock_calls), 3)
-        self.assertEqual(len(mocked_warning.mock_calls), 3)
-        mocked_warning.assert_called_with(
-            'Error posting clean insights events: HTTP 500 Response. Retrying in 5 seconds.'
-        )
+        tasks.send_clean_insights_measurements.delay()
+        self.assertEqual(len(mocked_getResponse.mock_calls), 4)
         mocked_error.assert_called_with(
-            'Maximum tries reach to upload Clean Insights measurements. Error: HTTP 500 Response'
+            'Maximum tries reached to upload Clean Insights measurements.'
+            ' Error: HTTPSConnectionPool(host=\'analytics.openwisp.io\', port=443):'
+            ' Max retries exceeded with url: /cleaninsights.php (Caused by ResponseError'
+            '(\'too many 500 error responses\'))'
         )
 
     @patch('time.sleep')
@@ -203,32 +206,30 @@ def test_post_clean_insights_events_204_response(
         bad_response = requests.Response()
         bad_response.status_code = 204
         with patch.object(
-            tasks.requests, 'post', return_value=bad_response
+            requests.Session, 'post', return_value=bad_response
         ) as mocked_post:
             tasks.send_clean_insights_measurements.delay()
         self.assertEqual(len(mocked_post.mock_calls), 1)
         mocked_warning.assert_not_called()
         mocked_error.assert_not_called()
 
-    @patch('time.sleep')
-    @patch('logging.Logger.warning')
-    @patch('logging.Logger.error')
+    @patch('urllib3.util.retry.Retry.sleep')
     @patch(
-        'requests.post',
-        side_effect=requests.ConnectionError('Error connecting to the server'),
+        'urllib3.connectionpool.HTTPConnectionPool._get_conn',
+        side_effect=OSError,
     )
+    @patch('logging.Logger.error')
     def test_post_clean_insights_events_connection_error(
-        self, mocked_post, mocked_error, mocked_warning, *args
+        self, mocked_error, mocked_get_conn, *args
     ):
         tasks.send_clean_insights_measurements.delay()
-        self.assertEqual(len(mocked_post.mock_calls), 3)
-        self.assertEqual(len(mocked_warning.mock_calls), 3)
-        mocked_warning.assert_called_with(
-            'Error posting clean insights events: Error connecting to the server. Retrying in 5 seconds.'
-        )
         mocked_error.assert_called_with(
-            'Maximum tries reach to upload Clean Insights measurements. Error: Error connecting to the server'
+            'Maximum tries reached to upload Clean Insights measurements.'
+            ' Error: HTTPSConnectionPool(host=\'analytics.openwisp.io\', port=443):'
+            ' Max retries exceeded with url: /cleaninsights.php'
+            ' (Caused by ProtocolError(\'Connection aborted.\', OSError()))'
         )
+        self.assertEqual(mocked_get_conn.call_count, 4)
 
     @patch.object(tasks.send_clean_insights_measurements, 'delay')
     def test_post_migrate_receiver(self, mocked_task, *args):

openwisp_utils/utils.py

@@ -1,8 +1,11 @@
 from collections import OrderedDict
 from copy import deepcopy
 
+import requests
 from django.conf import settings
 from django.utils.crypto import get_random_string
+from requests.adapters import HTTPAdapter
+from urllib3.util.retry import Retry
 
 
 class SortedOrderedDict(OrderedDict):
@@ -62,3 +65,17 @@ def print_color(string, color_name, end='\n'):
     }
     color = color_dict.get(color_name, '0')
     print(f'\033[{color}m{string}\033[0m', end=end)
+
+
+def retryable_request(method, timeout=(4, 8), max_retries=3, **kwargs):
+    request_session = requests.Session()
+    retries = Retry(
+        total=max_retries,
+        backoff_factor=1,
+        status_forcelist=[429, 500, 502, 503, 504],
+        allowed_methods=['DELETE', 'GET', 'HEAD', 'OPTIONS', 'PUT', 'TRACE', 'POST'],
+    )
+    request_session.mount('https://', HTTPAdapter(max_retries=retries))
+    request_session.mount('http://', HTTPAdapter(max_retries=retries))
+    request_method = getattr(request_session, method)
+    return request_method(timeout=timeout, **kwargs)