From 2f18d15bdcb3321a2540142e7e138ed8425aa663 Mon Sep 17 00:00:00 2001 From: Robert Raposa Date: Wed, 4 Dec 2024 17:26:48 -0500 Subject: [PATCH] feat!: switch code owner middleware to signals Initial rollout of moving code_owner monitoring code from edx-django-utils proved that the new CodeOwnerMonitoringMiddleware was not added high enough in the middleware stack. Instead, we have added signals to edx-django-utils's MonitoringSupportMiddleware, and this datadog_monitoring plugin now listens to those signals in order to automatically add code_owner custom span tags for django requests. BREAKING CHANGE: Removes CodeOwnerMonitoringMiddleware. See https://github.com/edx/edx-arch-experiments/issues/784 --- CHANGELOG.rst | 6 + edx_arch_experiments/__init__.py | 2 +- .../code_owner/middleware.py | 89 ----------- .../datadog_monitoring/code_owner/utils.py | 57 +++++++ ..._code_owner_custom_attribute_to_an_ida.rst | 18 +-- .../datadog_monitoring/signals/handlers.py | 32 +++- .../tests/code_owner/test_middleware.py | 151 ------------------ .../tests/code_owner/test_utils.py | 76 ++++++++- .../tests/signals/test_handlers.py | 18 ++- 9 files changed, 190 insertions(+), 259 deletions(-) delete mode 100644 edx_arch_experiments/datadog_monitoring/code_owner/middleware.py delete mode 100644 edx_arch_experiments/datadog_monitoring/tests/code_owner/test_middleware.py diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 3394747..abcf5a3 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -17,6 +17,12 @@ Added ----- * Adds search script datadog_search.py, for searching Datadog monitors and dashboards. +[6.0.0] - 2024-11-05 +~~~~~~~~~~~~~~~~~~~~ +Removed +------- +- Removes CodeOwnerMonitoringMiddleware, in favor of using new signals semt from edx-django-utils's MonitoringSupportMiddleware. + [5.1.0] - 2024-11-21 ~~~~~~~~~~~~~~~~~~~~ Added diff --git a/edx_arch_experiments/__init__.py b/edx_arch_experiments/__init__.py index 5a61fdb..6a4f275 100644 --- a/edx_arch_experiments/__init__.py +++ b/edx_arch_experiments/__init__.py @@ -2,4 +2,4 @@ A plugin to include applications under development by the architecture team at 2U. """ -__version__ = '5.1.0' +__version__ = '6.0.0' diff --git a/edx_arch_experiments/datadog_monitoring/code_owner/middleware.py b/edx_arch_experiments/datadog_monitoring/code_owner/middleware.py deleted file mode 100644 index f679959..0000000 --- a/edx_arch_experiments/datadog_monitoring/code_owner/middleware.py +++ /dev/null @@ -1,89 +0,0 @@ -""" -Middleware for code_owner_2 custom attribute -""" -import logging - -from django.urls import resolve -from edx_django_utils.monitoring import set_custom_attribute - -from .utils import get_code_owner_from_module, is_code_owner_mappings_configured, set_code_owner_custom_attributes - -log = logging.getLogger(__name__) - - -class CodeOwnerMonitoringMiddleware: - """ - Django middleware object to set custom attributes for the owner of each view. - - For instructions on usage, see: - https://github.com/edx/edx-arch-experiments/blob/master/edx_arch_experiments/datadog_monitoring/docs/how_tos/add_code_owner_custom_attribute_to_an_ida.rst - - Custom attributes set: - - code_owner_2: The owning team mapped to the current view. - - code_owner_2_module: The module found from the request or current transaction. - - code_owner_2_path_error: The error mapping by path, if code_owner_2 isn't found in other ways. - - """ - def __init__(self, get_response): - self.get_response = get_response - - def __call__(self, request): - response = self.get_response(request) - self._set_code_owner_attribute(request) - return response - - def process_exception(self, request, exception): # pylint: disable=W0613 - self._set_code_owner_attribute(request) - - def _set_code_owner_attribute(self, request): - """ - Sets the code_owner_2 custom attribute for the request. - """ - code_owner = None - module = self._get_module_from_request(request) - if module: - code_owner = get_code_owner_from_module(module) - - if code_owner: - set_code_owner_custom_attributes(code_owner) - - def _get_module_from_request(self, request): - """ - Get the module from the request path or the current transaction. - - Side-effects: - Sets code_owner_2_module custom attribute, used to determine code_owner_2. - If module was not found, may set code_owner_2_path_error custom attribute - if applicable. - - Returns: - str: module name or None if not found - - """ - if not is_code_owner_mappings_configured(): - return None - - module, path_error = self._get_module_from_request_path(request) - if module: - set_custom_attribute('code_owner_2_module', module) - return module - - # monitor errors if module was not found - if path_error: - set_custom_attribute('code_owner_2_path_error', path_error) - return None - - def _get_module_from_request_path(self, request): - """ - Uses the request path to get the view_func module. - - Returns: - (str, str): (module, error_message), where at least one of these should be None - - """ - try: - view_func, _, _ = resolve(request.path) - module = view_func.__module__ - return module, None - except Exception as e: # pragma: no cover, pylint: disable=broad-exception-caught - return None, str(e) diff --git a/edx_arch_experiments/datadog_monitoring/code_owner/utils.py b/edx_arch_experiments/datadog_monitoring/code_owner/utils.py index ecff3af..0304123 100644 --- a/edx_arch_experiments/datadog_monitoring/code_owner/utils.py +++ b/edx_arch_experiments/datadog_monitoring/code_owner/utils.py @@ -5,6 +5,7 @@ import re from django.conf import settings +from django.urls import resolve from edx_django_utils.monitoring import set_custom_attribute log = logging.getLogger(__name__) @@ -147,6 +148,62 @@ def set_code_owner_custom_attributes(code_owner): set_custom_attribute('code_owner_2_squad', squad) +def set_code_owner_attribute(request): + """ + Sets the code_owner_2 custom attribute for the request. + """ + code_owner = None + module = _get_module_from_request(request) + if module: + code_owner = get_code_owner_from_module(module) + + if code_owner: + set_code_owner_custom_attributes(code_owner) + + +def _get_module_from_request(request): + """ + Get the module from the request path or the current transaction. + + Side-effects: + Sets code_owner_2_module custom attribute, used to determine code_owner_2. + If module was not found, may set code_owner_2_path_error custom attribute + if applicable. + + Returns: + str: module name or None if not found + + """ + if not is_code_owner_mappings_configured(): + return None + + module, path_error = _get_module_from_request_path(request) + if module: + set_custom_attribute('code_owner_2_module', module) + return module + + # monitor errors if module was not found + if path_error: + set_custom_attribute('code_owner_2_path_error', path_error) + return None + + +def _get_module_from_request_path(request): + """ + Uses the request path to get the view_func module. + + Returns: + (str, str): (module, error_message), where at least one of these should be None + + """ + try: + view_func, _, _ = resolve(request.path) + module = view_func.__module__ + return module, None + except Exception as e: # pragma: no cover, pylint: disable=broad-exception-caught + return None, str(e) + + def clear_cached_mappings(): """ Clears the cached code owner mappings. Useful for testing. diff --git a/edx_arch_experiments/datadog_monitoring/docs/how_tos/add_code_owner_custom_attribute_to_an_ida.rst b/edx_arch_experiments/datadog_monitoring/docs/how_tos/add_code_owner_custom_attribute_to_an_ida.rst index a9872c2..ca1dab9 100644 --- a/edx_arch_experiments/datadog_monitoring/docs/how_tos/add_code_owner_custom_attribute_to_an_ida.rst +++ b/edx_arch_experiments/datadog_monitoring/docs/how_tos/add_code_owner_custom_attribute_to_an_ida.rst @@ -23,25 +23,19 @@ If you want to learn more about custom span tags in general, see `Enhanced Monit .. _ADR on monitoring by code owner: https://github.com/openedx/edx-platform/blob/master/lms/djangoapps/monitoring/docs/decisions/0001-monitoring-by-code-owner.rst .. _Enhanced Monitoring and Custom Attributes: https://edx.readthedocs.io/projects/edx-django-utils/en/latest/monitoring/how_tos/using_custom_attributes.html -Setting up the Middleware -------------------------- +What gets code_owner span tags +------------------------------ -You simply need to add ``edx_arch_experiments.datadog_monitoring.code_owner.middleware.CodeOwnerMonitoringMiddleware`` to get code owner span tags on Django requests. +Simply by installing the datadog_monitoring plugin, code owner span tags will automatically be added for: -Handling celery tasks ---------------------- - -For celery tasks, this plugin will automatically detect and add code owner span tags to any span with ``operation_name:celery.run``. - -This is accomplished by receiving signals from celery's worker_process_init for each process, and then adding a custom Datadog span processor to add the span tags as appropriate. +* ``operation_name:django.request``: tags are added by using edx-django-utils monitoring signals, which are sent by its MonitoringSupportMiddleware. +* ``operation_name:celery.run``: tags are added using celery's ``worker_process_init`` signal, and then adding a custom Datadog span processor to add the span tags as appropriate. Configuring your app settings ----------------------------- Once the Middleware is made available, simply set the Django Settings ``CODE_OWNER_MAPPINGS`` and ``CODE_OWNER_THEMES`` appropriately. -The following example shows how you can include an optional config for a catch-all using ``'*'``. Although you might expect this example to use Python, it is intentionally illustrated in YAML because the catch-all requires special care in YAML. - :: # YAML format of example CODE_OWNER_MAPPINGS @@ -50,7 +44,7 @@ The following example shows how you can include an optional config for a catch-a - xblock_django - openedx.core.djangoapps.xblock theme-x-team-blue: - - '*' # IMPORTANT: you must surround * with quotes in yml + - lms # YAML format of example CODE_OWNER_THEMES CODE_OWNER_THEMES: diff --git a/edx_arch_experiments/datadog_monitoring/signals/handlers.py b/edx_arch_experiments/datadog_monitoring/signals/handlers.py index 04b5ab3..77f8c7a 100644 --- a/edx_arch_experiments/datadog_monitoring/signals/handlers.py +++ b/edx_arch_experiments/datadog_monitoring/signals/handlers.py @@ -5,13 +5,43 @@ from celery.signals import worker_process_init from django.dispatch import receiver +from edx_django_utils.monitoring.signals import ( + monitoring_support_process_exception, + monitoring_support_process_request, + monitoring_support_process_response, +) from edx_arch_experiments.datadog_monitoring.code_owner.datadog import CeleryCodeOwnerSpanProcessor +from edx_arch_experiments.datadog_monitoring.code_owner.utils import set_code_owner_attribute log = logging.getLogger(__name__) -@receiver(worker_process_init) +@receiver(monitoring_support_process_response, dispatch_uid=f"datadog_monitoring_support_process_response") +def datadog_monitoring_support_process_response(sender, **kwargs): + """ + Adds datadog monitoring at monitoring process response time. + """ + if 'request' in kwargs: + set_code_owner_attribute(kwargs['request']) + else: + log.warning('monitoring_support_process_response sent without ' + 'expected parameter: request.') + + +@receiver(monitoring_support_process_exception, dispatch_uid=f"datadog_monitoring_support_process_exception") +def datadog_monitoring_support_process_exception(sender, **kwargs): + """ + Adds datadog monitoring at monitoring process exception time. + """ + if 'request' in kwargs: + set_code_owner_attribute(kwargs['request']) + else: + log.warning('monitoring_support_process_exception sent without ' + 'expected parameter: request.') + + +@receiver(worker_process_init, dispatch_uid=f"datadog_span_processor_worker_process_init") def init_worker_process(sender, **kwargs): """ Adds a Datadog span processor to each worker process. diff --git a/edx_arch_experiments/datadog_monitoring/tests/code_owner/test_middleware.py b/edx_arch_experiments/datadog_monitoring/tests/code_owner/test_middleware.py deleted file mode 100644 index eb4313d..0000000 --- a/edx_arch_experiments/datadog_monitoring/tests/code_owner/test_middleware.py +++ /dev/null @@ -1,151 +0,0 @@ -""" -Tests for the code_owner monitoring middleware -""" -from unittest import TestCase -from unittest.mock import ANY, MagicMock, Mock, call, patch - -import ddt -from django.test import RequestFactory, override_settings -from django.urls import re_path -from django.views.generic import View - -from edx_arch_experiments.datadog_monitoring.code_owner.middleware import CodeOwnerMonitoringMiddleware -from edx_arch_experiments.datadog_monitoring.code_owner.utils import clear_cached_mappings - -from .mock_views import MockViewTest - - -class MockMiddlewareViewTest(View): - pass - - -urlpatterns = [ - re_path(r'^middleware-test/$', MockMiddlewareViewTest.as_view()), - re_path(r'^test/$', MockViewTest.as_view()), -] - -SET_CUSTOM_ATTRIBUTE_MOCK = MagicMock() - - -# Enables the same mock to be used from different modules, using -# patch with new_callable=get_set_custom_attribute_mock -def get_set_custom_attribute_mock(): - return SET_CUSTOM_ATTRIBUTE_MOCK - - -@ddt.ddt -class CodeOwnerMetricMiddlewareTests(TestCase): - """ - Tests for the code_owner monitoring utility functions - """ - urls = 'lms.djangoapps.monitoring.tests.test_middleware.test_urls' - - def setUp(self): - super().setUp() - clear_cached_mappings() - SET_CUSTOM_ATTRIBUTE_MOCK.reset_mock() - self.mock_get_response = Mock() - self.middleware = CodeOwnerMonitoringMiddleware(self.mock_get_response) - - def test_init(self): - self.assertEqual(self.middleware.get_response, self.mock_get_response) - - def test_request_call(self): - self.mock_get_response.return_value = 'test-response' - request = Mock() - self.assertEqual(self.middleware(request), 'test-response') - - _REQUEST_PATH_TO_MODULE_PATH = { - '/middleware-test/': 'edx_arch_experiments.datadog_monitoring.tests.code_owner.test_middleware', - '/test/': 'edx_arch_experiments.datadog_monitoring.tests.code_owner.mock_views', - } - - @override_settings( - CODE_OWNER_MAPPINGS={'team-red': ['edx_arch_experiments.datadog_monitoring.tests.code_owner.mock_views']}, - CODE_OWNER_THEMES={'team': ['team-red']}, - ROOT_URLCONF=__name__, - ) - @patch( - 'edx_arch_experiments.datadog_monitoring.code_owner.middleware.set_custom_attribute', - new_callable=get_set_custom_attribute_mock - ) - @patch( - 'edx_arch_experiments.datadog_monitoring.code_owner.utils.set_custom_attribute', - new_callable=get_set_custom_attribute_mock - ) - @ddt.data( - ('/middleware-test/', None), - ('/test/', 'team-red'), - ) - @ddt.unpack - def test_code_owner_path_mapping_hits_and_misses( - self, request_path, expected_owner, mock_set_custom_attribute, _ - ): - request = RequestFactory().get(request_path) - self.middleware(request) - expected_path_module = self._REQUEST_PATH_TO_MODULE_PATH[request_path] - self._assert_code_owner_custom_attributes( - mock_set_custom_attribute, expected_code_owner=expected_owner, path_module=expected_path_module, - check_theme_and_squad=True - ) - - mock_set_custom_attribute.reset_mock() - self.middleware.process_exception(request, None) - self._assert_code_owner_custom_attributes( - mock_set_custom_attribute, expected_code_owner=expected_owner, path_module=expected_path_module, - check_theme_and_squad=True - ) - - @override_settings( - ROOT_URLCONF=__name__, - ) - @patch('edx_arch_experiments.datadog_monitoring.code_owner.middleware.set_custom_attribute') - def test_code_owner_no_mappings(self, mock_set_custom_attribute): - request = RequestFactory().get('/test/') - self.middleware(request) - mock_set_custom_attribute.assert_not_called() - - @override_settings( - CODE_OWNER_MAPPINGS={'team-red': ['lms.djangoapps.monitoring.tests.mock_views']}, - ) - @patch( - 'edx_arch_experiments.datadog_monitoring.code_owner.middleware.set_custom_attribute', - new_callable=get_set_custom_attribute_mock - ) - def test_no_resolver_for_path(self, mock_set_custom_attribute): - request = RequestFactory().get('/bad/path/') - self.middleware(request) - self._assert_code_owner_custom_attributes( - mock_set_custom_attribute, has_path_error=True - ) - - @override_settings( - CODE_OWNER_MAPPINGS=['invalid_setting_as_list'], - ROOT_URLCONF=__name__, - ) - def test_load_config_with_invalid_dict(self): - request = RequestFactory().get('/test/') - with self.assertRaises(TypeError): - self.middleware(request) - - def _assert_code_owner_custom_attributes( - self, mock_set_custom_attribute, expected_code_owner=None, - path_module=None, has_path_error=False, - check_theme_and_squad=False - ): # pylint: disable=too-many-positional-arguments - """ Performs a set of assertions around having set the proper custom attributes. """ - call_list = [] - if expected_code_owner: - call_list.append(call('code_owner_2', expected_code_owner)) - if check_theme_and_squad: - call_list.append(call('code_owner_2_theme', expected_code_owner.split('-')[0])) - call_list.append(call('code_owner_2_squad', expected_code_owner.split('-')[1])) - if path_module: - call_list.append(call('code_owner_2_module', path_module)) - if has_path_error: - call_list.append(call('code_owner_2_path_error', ANY)) - mock_set_custom_attribute.assert_has_calls(call_list, any_order=True) - self.assertEqual( - len(mock_set_custom_attribute.call_args_list), len(call_list), - f'Expected calls {call_list} vs actual calls {mock_set_custom_attribute.call_args_list}' - ) diff --git a/edx_arch_experiments/datadog_monitoring/tests/code_owner/test_utils.py b/edx_arch_experiments/datadog_monitoring/tests/code_owner/test_utils.py index be17856..a18f33d 100644 --- a/edx_arch_experiments/datadog_monitoring/tests/code_owner/test_utils.py +++ b/edx_arch_experiments/datadog_monitoring/tests/code_owner/test_utils.py @@ -3,17 +3,32 @@ """ import timeit from unittest import TestCase -from unittest.mock import call, patch +from unittest.mock import ANY, call, patch import ddt -from django.test import override_settings +from django.test import RequestFactory, override_settings +from django.urls import re_path +from django.views.generic import View from edx_arch_experiments.datadog_monitoring.code_owner.utils import ( clear_cached_mappings, get_code_owner_from_module, + set_code_owner_attribute, set_code_owner_attribute_from_module, ) +from .mock_views import MockViewTest + + +class MockUtilsViewTest(View): + pass + + +urlpatterns = [ + re_path(r'^utils-test/$', MockUtilsViewTest.as_view()), + re_path(r'^test/$', MockViewTest.as_view()), +] + @ddt.ddt class MonitoringUtilsTests(TestCase): @@ -91,7 +106,53 @@ def test_set_code_owner_attribute_from_module_success(self, mock_set_custom_attr set_code_owner_attribute_from_module(__name__) self._assert_set_custom_attribute(mock_set_custom_attribute, code_owner='team-red', module=__name__) - def _assert_set_custom_attribute(self, mock_set_custom_attribute, code_owner, module, check_theme_and_squad=False): + @override_settings( + CODE_OWNER_MAPPINGS={'team-red': ['edx_arch_experiments.datadog_monitoring.tests.code_owner.mock_views']}, + CODE_OWNER_THEMES={'team': ['team-red']}, + ROOT_URLCONF=__name__, + ) + @patch('edx_arch_experiments.datadog_monitoring.code_owner.utils.set_custom_attribute') + @ddt.data( + ('/utils-test/', None, 'edx_arch_experiments.datadog_monitoring.tests.code_owner.test_utils'), + ('/test/', 'team-red', 'edx_arch_experiments.datadog_monitoring.tests.code_owner.mock_views'), + ) + @ddt.unpack + def test_set_code_owner_attribute_from_request_success( + self, request_path, expected_owner, expected_module, mock_set_custom_attribute + ): + request = RequestFactory().get(request_path) + set_code_owner_attribute(request) + self._assert_set_custom_attribute( + mock_set_custom_attribute, code_owner=expected_owner, module=expected_module, + check_theme_and_squad=True + ) + + @override_settings( + CODE_OWNER_MAPPINGS={'team-red': ['lms.djangoapps.monitoring.tests.mock_views']}, + ) + @patch( + 'edx_arch_experiments.datadog_monitoring.code_owner.utils.set_custom_attribute', + ) + def test_set_code_owner_attribute_from_request_no_resolver_for_path(self, mock_set_custom_attribute): + request = RequestFactory().get('/bad/path/') + set_code_owner_attribute(request) + self._assert_set_custom_attribute( + mock_set_custom_attribute, has_path_error=True + ) + + @override_settings( + ROOT_URLCONF=__name__, + ) + @patch('edx_arch_experiments.datadog_monitoring.code_owner.utils.set_custom_attribute') + def test_set_code_owner_attribute_from_request_no_mappings(self, mock_set_custom_attribute): + request = RequestFactory().get('/test/') + set_code_owner_attribute(request) + mock_set_custom_attribute.assert_not_called() + + def _assert_set_custom_attribute( + self, mock_set_custom_attribute, code_owner=None, module=None, + check_theme_and_squad=False, has_path_error=False + ): # pylint: disable=too-many-positional-arguments """ Helper to assert that the proper set_custom_metric calls were made. """ @@ -101,5 +162,12 @@ def _assert_set_custom_attribute(self, mock_set_custom_attribute, code_owner, mo if check_theme_and_squad: call_list.append(call('code_owner_2_theme', code_owner.split('-')[0])) call_list.append(call('code_owner_2_squad', code_owner.split('-')[1])) - call_list.append(call('code_owner_2_module', module)) + if module: + call_list.append(call('code_owner_2_module', module)) + if has_path_error: + call_list.append(call('code_owner_2_path_error', ANY)) mock_set_custom_attribute.assert_has_calls(call_list, any_order=True) + self.assertEqual( + len(mock_set_custom_attribute.call_args_list), len(call_list), + f'Expected calls {call_list} vs actual calls {mock_set_custom_attribute.call_args_list}' + ) diff --git a/edx_arch_experiments/datadog_monitoring/tests/signals/test_handlers.py b/edx_arch_experiments/datadog_monitoring/tests/signals/test_handlers.py index c0816a7..f723671 100644 --- a/edx_arch_experiments/datadog_monitoring/tests/signals/test_handlers.py +++ b/edx_arch_experiments/datadog_monitoring/tests/signals/test_handlers.py @@ -1,10 +1,16 @@ """ Tests for celery signal handler. """ +from unittest.mock import patch + from ddtrace import tracer from django.test import TestCase -from edx_arch_experiments.datadog_monitoring.signals.handlers import init_worker_process +from edx_arch_experiments.datadog_monitoring.signals.handlers import ( + datadog_monitoring_support_process_exception, + datadog_monitoring_support_process_response, + init_worker_process, +) class TestHandlers(TestCase): @@ -17,6 +23,16 @@ def setUp(self): sp for sp in tracer._span_processors if type(sp).__name__ != 'CeleryCodeOwnerSpanProcessor' ] + @patch('edx_arch_experiments.datadog_monitoring.signals.handlers.set_code_owner_attribute') + def test_datadog_monitoring_support_process_response(self, mock_set_code_owner_attribute): + datadog_monitoring_support_process_response(sender=None, request='fake request') + mock_set_code_owner_attribute.assert_called_once_with('fake request') + + @patch('edx_arch_experiments.datadog_monitoring.signals.handlers.set_code_owner_attribute') + def test_datadog_monitoring_support_process_exception(self, mock_set_code_owner_attribute): + datadog_monitoring_support_process_exception(sender=None, request='fake request') + mock_set_code_owner_attribute.assert_called_once_with('fake request') + def test_init_worker_process(self): def get_processor_list(): # pylint: disable=protected-access