From 2119b7ba8a20655449a25095b5255adb14ec10b7 Mon Sep 17 00:00:00 2001 From: Robert Raposa Date: Wed, 4 Dec 2024 12:48:03 -0500 Subject: [PATCH] feat: add monitoring signals for plugins Adds signals monitoring_support_process_request, monitoring_support_process_response, and monitoring_support_process_exception to the MonitoringSupportMiddleware to enable plugging in monitoring capabilities. --- CHANGELOG.rst | 6 ++ edx_django_utils/__init__.py | 2 +- edx_django_utils/monitoring/README.rst | 16 +++- edx_django_utils/monitoring/__init__.py | 5 +- .../monitoring/internal/middleware.py | 30 +++++++- edx_django_utils/monitoring/signals.py | 17 +++++ ...nitoring.py => test_monitoring_support.py} | 74 +++++++++++++++++-- 7 files changed, 131 insertions(+), 19 deletions(-) create mode 100644 edx_django_utils/monitoring/signals.py rename edx_django_utils/monitoring/tests/{test_custom_monitoring.py => test_monitoring_support.py} (70%) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 0d5c6528..1ad4c890 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -11,6 +11,12 @@ Change Log .. There should always be an "Unreleased" section for changes pending release. +7.1.0 - 2024-12-05 +------------------ +Added +~~~~~ +* Added signals monitoring_support_process_request, monitoring_support_process_response, and monitoring_support_process_exception to the MonitoringSupportMiddleware to enable plugging in monitoring capabilities. + 7.0.1 - 2024-11-21 ------------------ Fixed diff --git a/edx_django_utils/__init__.py b/edx_django_utils/__init__.py index 7eaec0c2..a2e76089 100644 --- a/edx_django_utils/__init__.py +++ b/edx_django_utils/__init__.py @@ -2,7 +2,7 @@ EdX utilities for Django Application development.. """ -__version__ = "7.0.1" +__version__ = "7.1.0" default_app_config = ( "edx_django_utils.apps.EdxDjangoUtilsConfig" diff --git a/edx_django_utils/monitoring/README.rst b/edx_django_utils/monitoring/README.rst index ae4dc036..26879c9d 100644 --- a/edx_django_utils/monitoring/README.rst +++ b/edx_django_utils/monitoring/README.rst @@ -78,18 +78,26 @@ Here is how you add the middleware: 'edx_django_utils.cache.middleware.RequestCacheMiddleware', # Add monitoring middleware immediately after RequestCacheMiddleware + 'edx_django_utils.monitoring.MonitoringSupportMiddleware', 'edx_django_utils.monitoring.DeploymentMonitoringMiddleware', 'edx_django_utils.monitoring.CookieMonitoringMiddleware', 'edx_django_utils.monitoring.CodeOwnerMonitoringMiddleware', - 'edx_django_utils.monitoring.CachedCustomMonitoringMiddleware', 'edx_django_utils.monitoring.FrontendMonitoringMiddleware', 'edx_django_utils.monitoring.MonitoringMemoryMiddleware', ) -Cached Custom Monitoring Middleware ------------------------------------ +Monitoring Support Middleware and Monitoring Plugins +---------------------------------------------------- -The middleware ``CachedCustomMonitoringMiddleware`` is required to allow certain utility methods, like ``accumulate`` and ``increment``, to work appropriately. +The middleware ``MonitoringSupportMiddleware`` provides a number of monitoring capabilities: + +* It enables plugging in outside monitoring capabilities, by sending the signals ``monitoring_support_process_request``, ``monitoring_support_process_response``, and ``monitoring_support_process_exception``. These can be useful because this middleware should be available in all Open edX services, and should appear early enough in the list of middleware to monitor most requests, even those that respond early from another middleware. +* It allows certain utility methods, like ``accumulate`` and ``increment``, to work appropriately. +* It adds error span tags to the root span. + +In order to use the monitoring signals, import them as follows:: + + from edx_django_utils.monitoring.signals import monitoring_support_process_response Code Owner Custom Attribute --------------------------- diff --git a/edx_django_utils/monitoring/__init__.py b/edx_django_utils/monitoring/__init__.py index 9537725a..7c199d89 100644 --- a/edx_django_utils/monitoring/__init__.py +++ b/edx_django_utils/monitoring/__init__.py @@ -1,7 +1,8 @@ """ -Metrics utilities public api +Monitoring utilities public api -See README.rst for details. +Does not include signals.py, which is also part of the public api. +See README.rst for additional details. """ from .internal.backends import DatadogBackend, NewRelicBackend, OpenTelemetryBackend, TelemetryBackend from .internal.code_owner.middleware import CodeOwnerMonitoringMiddleware diff --git a/edx_django_utils/monitoring/internal/middleware.py b/edx_django_utils/monitoring/internal/middleware.py index 1ecbdae3..c9271cd9 100644 --- a/edx_django_utils/monitoring/internal/middleware.py +++ b/edx_django_utils/monitoring/internal/middleware.py @@ -21,6 +21,11 @@ from edx_django_utils.cache import RequestCache from edx_django_utils.logging import encrypt_for_log +from edx_django_utils.monitoring.signals import ( + monitoring_support_process_exception, + monitoring_support_process_request, + monitoring_support_process_response +) from .backends import configured_backends @@ -74,10 +79,13 @@ class MonitoringSupportMiddleware(MiddlewareMixin): """ Middleware to support monitoring. - 1. Middleware batch reports cached custom attributes at the end of a request. - 2. Middleware adds error span tags to the root span. + 1. Send process request, response, and exception signals to enable + plugins to add custom monitoring. + 2. Middleware batch reports cached custom attributes at the end of a request. + 3. Middleware adds error span tags to the root span. - Make sure to add below the request cache in MIDDLEWARE. + Make sure to add below the request cache in MIDDLEWARE, but as early + in the stack of middleware as possible. This middleware will only call on the telemetry collector if there are any attributes to report for this request, so it will not incur any processing overhead for @@ -140,13 +148,24 @@ def _tag_root_span_with_error(self, exception): for backend in configured_backends(): backend.tag_root_span_with_error(exception) - # Whether or not there was an exception, report any custom NR attributes that + # Whether or not there was an exception, report any custom attributes that # may have been collected. + def process_request(self, request): + """ + Django middleware handler to process a request + """ + monitoring_support_process_request.send_robust( + sender=self.__class__, request=request + ) + def process_response(self, request, response): """ Django middleware handler to process a response """ + monitoring_support_process_response.send_robust( + sender=self.__class__, request=request, response=response + ) self._batch_report() return response @@ -154,6 +173,9 @@ def process_exception(self, request, exception): """ Django middleware handler to process an exception """ + monitoring_support_process_exception.send_robust( + sender=self.__class__, request=request, exception=exception + ) self._batch_report() self._tag_root_span_with_error(exception) diff --git a/edx_django_utils/monitoring/signals.py b/edx_django_utils/monitoring/signals.py new file mode 100644 index 00000000..21b85486 --- /dev/null +++ b/edx_django_utils/monitoring/signals.py @@ -0,0 +1,17 @@ +""" +Signals defined to support monitoring plugins. + +Placing signals in this file (vs alternatives like /internal, or +signals/signals.py) provides a good public api for these signals. +""" +import django.dispatch + +# The MonitoringSupportMiddleware sends these signals to enable plugging in +# outside monitoring capabilities. This is a useful capability because the +# MonitoringSupportMiddleware should be available in all Open edX services, +# and should be one of the first middleware (after RequestCacheMiddleware), +# allowing it access to monitor most requests (even those that fail or +# respond early in other middleware). +monitoring_support_process_request = django.dispatch.Signal() +monitoring_support_process_response = django.dispatch.Signal() +monitoring_support_process_exception = django.dispatch.Signal() diff --git a/edx_django_utils/monitoring/tests/test_custom_monitoring.py b/edx_django_utils/monitoring/tests/test_monitoring_support.py similarity index 70% rename from edx_django_utils/monitoring/tests/test_custom_monitoring.py rename to edx_django_utils/monitoring/tests/test_monitoring_support.py index c5aaf345..d6e0e0ff 100644 --- a/edx_django_utils/monitoring/tests/test_custom_monitoring.py +++ b/edx_django_utils/monitoring/tests/test_monitoring_support.py @@ -1,15 +1,21 @@ """ -Tests for CachedCustomMonitoringMiddleware and associated utilities. +Tests for MonitoringSupportMiddleware and associated utilities. Note: See test_middleware.py for the rest of the middleware tests. """ -from unittest.mock import Mock, call, patch +from contextlib import contextmanager +from unittest.mock import ANY, Mock, call, patch import ddt from django.test import TestCase, override_settings from edx_django_utils.cache import RequestCache from edx_django_utils.monitoring import MonitoringSupportMiddleware, accumulate, increment +from edx_django_utils.monitoring.signals import ( + monitoring_support_process_exception, + monitoring_support_process_request, + monitoring_support_process_response +) from ..middleware import CachedCustomMonitoringMiddleware as DeprecatedCachedCustomMonitoringMiddleware from ..middleware import MonitoringCustomMetricsMiddleware as DeprecatedMonitoringCustomMetricsMiddleware @@ -20,13 +26,13 @@ @ddt.ddt -class TestCustomMonitoringMiddleware(TestCase): +class TestMonitoringSupportMiddleware(TestCase): """ Test the monitoring_utils middleware and helpers """ def setUp(self): super().setUp() - self.mock_response = Mock() + self.mock_get_response = Mock() RequestCache.clear_all_namespaces() @patch('newrelic.agent') @@ -57,7 +63,7 @@ def test_accumulate_and_increment( ] # fake a response to trigger attributes reporting - middleware_method = getattr(cached_monitoring_middleware_class(self.mock_response), middleware_method_name) + middleware_method = getattr(cached_monitoring_middleware_class(self.mock_get_response), middleware_method_name) middleware_method( 'fake request', 'fake response', @@ -96,9 +102,9 @@ def test_accumulate_with_illegal_value( call('error_adding_accumulated_metric', 'name=hello, new_value=10, cached_value=None'), ] - self.mock_response = Mock() + self.mock_get_response = Mock() # fake a response to trigger metrics reporting - cached_monitoring_middleware_class(self.mock_response).process_response( + cached_monitoring_middleware_class(self.mock_get_response).process_response( 'fake request', 'fake response', ) @@ -113,6 +119,58 @@ def test_accumulate_with_illegal_value( # 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) + @contextmanager + def catch_signal(self, signal): + """ + Catch django signal and return the mocked handler. + """ + handler = Mock() + signal.connect(handler) + yield handler + signal.disconnect(handler) + + def test_process_request_signal(self): + """ + Test middleware sends process request signal. + """ + with self.catch_signal(monitoring_support_process_request) as handler: + MonitoringSupportMiddleware(self.mock_get_response).process_request( + 'fake request' + ) + + handler.assert_called_once_with( + signal=ANY, sender=MonitoringSupportMiddleware, request='fake request' + ) + + def test_process_response_signal(self): + """ + Test middleware sends process response signal. + """ + with self.catch_signal(monitoring_support_process_response) as handler: + MonitoringSupportMiddleware(self.mock_get_response).process_response( + 'fake request', 'fake response' + ) + + handler.assert_called_once_with( + signal=ANY, sender=MonitoringSupportMiddleware, + request='fake request', response='fake response' + ) + + def test_process_exception_signal(self): + """ + Test middleware sends process exception signal. + """ + fake_exception = Exception() + with self.catch_signal(monitoring_support_process_exception) as handler: + MonitoringSupportMiddleware(self.mock_get_response).process_exception( + 'fake request', fake_exception + ) + + handler.assert_called_once_with( + signal=ANY, sender=MonitoringSupportMiddleware, + request='fake request', exception=fake_exception + ) + @patch('ddtrace.Tracer.current_root_span') def test_error_tagging(self, mock_get_root_span): # Ensure that we continue to support tagging exceptions in MonitoringSupportMiddleware. @@ -121,7 +179,7 @@ def test_error_tagging(self, mock_get_root_span): mock_root_span = Mock() mock_get_root_span.return_value = mock_root_span with override_settings(OPENEDX_TELEMETRY=['edx_django_utils.monitoring.DatadogBackend']): - MonitoringSupportMiddleware(self.mock_response).process_exception( + MonitoringSupportMiddleware(self.mock_get_response).process_exception( 'fake request', fake_exception ) mock_root_span.set_exc_info.assert_called_with(