ARCHBOM-1490: fix custom monitoring accumulate (#58)

* fix custom monitoring accumulate

The accumulate method wasn't accumulating, and the unit
test wasn't working as expected. Also, adds a custom
attribute in case of failures.

ARCHBOM-1490

Author: robrap

CHANGELOG.rst

@@ -15,6 +15,11 @@ Change Log
 Unreleased
 ~~~~~~~~~~
 
+[3.7.4] - 2020-08-29
+~~~~~~~~~~~~~~~~~~~~
+
+* Fix to custom monitoring accumulate to actually accumulate rather than overwrite.
+
 [3.7.3] - 2020-08-12
 ~~~~~~~~~~~~~~~~~~~~
 

edx_django_utils/__init__.py

@@ -2,7 +2,7 @@
 EdX utilities for Django Application development..
 """
 
-__version__ = "3.7.3"
+__version__ = "3.7.4"
 
 default_app_config = (
     "edx_django_utils.apps.EdxDjangoUtilsConfig"

edx_django_utils/monitoring/middleware.py

@@ -60,8 +60,21 @@ def accumulate_metric(cls, name, value):
         Accumulate a custom metric (name and value) in the metrics cache.
         """
         metrics_cache = cls._get_metrics_cache()
-        metrics_cache.setdefault(name, 0)
-        metrics_cache.set(name, value)
+        cached_response = metrics_cache.get_cached_response(name)
+        if cached_response.is_found:
+            try:
+                accumulated_value = value + cached_response.value
+            except TypeError:
+                _set_custom_attribute(
+                    'error_adding_accumulated_metric',
+                    'name={}, new_value={}, cached_value={}'.format(
+                        name, repr(value), repr(cached_response.value)
+                    )
+                )
+                return
+        else:
+            accumulated_value = value
+        metrics_cache.set(name, accumulated_value)
 
     @classmethod
     def _batch_report(cls):
@@ -72,7 +85,7 @@ def _batch_report(cls):
             return
         metrics_cache = cls._get_metrics_cache()
         for key, value in metrics_cache.data.items():
-            newrelic.agent.add_custom_parameter(key, value)
+            _set_custom_attribute(key, value)
 
     # Whether or not there was an exception, report any custom NR metrics that
     # may have been collected.
@@ -91,6 +104,16 @@ def process_exception(self, request, exception):
         self._batch_report()
 
 
+def _set_custom_attribute(key, value):
+    """
+    Sets monitoring custom attribute.
+
+    Note: Can't use public method in ``utils.py`` due to circular reference.
+    """
+    if newrelic:  # pragma: no cover
+        newrelic.agent.add_custom_parameter(key, value)
+
+
 class MonitoringMemoryMiddleware(MiddlewareMixin):
     """
     Middleware for monitoring memory usage.

edx_django_utils/monitoring/tests/test_monitoring.py

@@ -5,20 +5,24 @@
 from mock import call, patch
 
 from edx_django_utils import monitoring
+from edx_django_utils.cache import RequestCache
 from edx_django_utils.monitoring.middleware import MonitoringCustomMetricsMiddleware, MonitoringMemoryMiddleware
 
 
 class TestMonitoringCustomMetrics(TestCase):
     """
     Test the monitoring_utils middleware and helpers
     """
+    def setUp(self):
+        super(TestMonitoringCustomMetrics, self).setUp()
+        RequestCache.clear_all_namespaces()
 
     @patch('newrelic.agent')
     @override_settings(MIDDLEWARE=[
         'edx_django_utils.cache.middleware.RequestCacheMiddleware',
         'edx_django_utils.monitoring.middleware.MonitoringCustomMetricsMiddleware',
     ])
-    def test_custom_metrics_with_new_relic(self, mock_newrelic_agent):
+    def test_accumulate_and_increment(self, mock_newrelic_agent):
         """
         Test normal usage of collecting custom metrics and reporting to New Relic
         """
@@ -48,7 +52,39 @@ def test_custom_metrics_with_new_relic(self, mock_newrelic_agent):
 
         # Assert call args to newrelic.agent.add_custom_parameter().  Due to
         # the nature of python dicts, call order is undefined.
-        mock_newrelic_agent.add_custom_parameter.has_calls(nr_agent_calls_expected, any_order=True)
+        mock_newrelic_agent.add_custom_parameter.assert_has_calls(nr_agent_calls_expected, any_order=True)
+
+    @patch('newrelic.agent')
+    @override_settings(MIDDLEWARE=[
+        'edx_django_utils.cache.middleware.RequestCacheMiddleware',
+        'edx_django_utils.monitoring.middleware.MonitoringCustomMetricsMiddleware',
+    ])
+    def test_accumulate_with_illegal_value(self, mock_newrelic_agent):
+        """
+        Test monitoring accumulate with illegal value that can't be added.
+        """
+        monitoring.accumulate('hello', None)
+        monitoring.accumulate('hello', 10)
+
+        # based on the metric data above, we expect the following calls to newrelic:
+        nr_agent_calls_expected = [
+            call('hello', None),
+            call('error_adding_accumulated_metric', 'name=hello, new_value=10, cached_value=None'),
+        ]
+
+        # fake a response to trigger metrics reporting
+        MonitoringCustomMetricsMiddleware().process_response(
+            'fake request',
+            'fake response',
+        )
+
+        # Assert call counts to newrelic.agent.add_custom_parameter()
+        expected_call_count = len(nr_agent_calls_expected)
+        measured_call_count = mock_newrelic_agent.add_custom_parameter.call_count
+        self.assertEqual(expected_call_count, measured_call_count)
+
+        # Assert call args to newrelic.agent.add_custom_parameter().
+        mock_newrelic_agent.add_custom_parameter.assert_has_calls(nr_agent_calls_expected, any_order=True)
 
     @override_settings(MIDDLEWARE=[
         'edx_django_utils.cache.middleware.RequestCacheMiddleware',