diff --git a/lms/djangoapps/monitoring/README.rst b/lms/djangoapps/monitoring/README.rst new file mode 100644 index 0000000000..e8936c5571 --- /dev/null +++ b/lms/djangoapps/monitoring/README.rst @@ -0,0 +1,9 @@ +This directory contains utilities for adding a code_owner custom metric for help with split-ownership of the LMS. + +For details on the decision to implement the code_owner custom metric, see: +lms/djangoapps/monitoring/docs/decisions/0001-monitoring-by-code-owner.rst + +Originally, this directory contained the ``CodeOwnerMetricMiddleware``, but that has since been moved to: +https://github.com/edx/edx-django-utils/tree/master/edx_django_utils/monitoring/code_owner + +This directory continues to contain scripts that can help generate the appropriate ownership mappings for the LMS. diff --git a/lms/djangoapps/monitoring/apps.py b/lms/djangoapps/monitoring/apps.py deleted file mode 100644 index 1ae13624c4..0000000000 --- a/lms/djangoapps/monitoring/apps.py +++ /dev/null @@ -1,11 +0,0 @@ -""" -Configuration for the monitoring Django application. -""" -from django.apps import AppConfig - - -class MonitoringConfig(AppConfig): - """ - Configuration class for the monitoring Django application. - """ - name = 'monitoring' diff --git a/lms/djangoapps/monitoring/middleware.py b/lms/djangoapps/monitoring/middleware.py deleted file mode 100644 index 05bca4c956..0000000000 --- a/lms/djangoapps/monitoring/middleware.py +++ /dev/null @@ -1,49 +0,0 @@ -""" -Middleware for monitoring the LMS -""" -import logging -from django.urls import Resolver404, resolve -from edx_django_utils.monitoring import set_custom_metric - -from .utils import get_code_owner_from_module, is_code_owner_mappings_configured - -log = logging.getLogger(__name__) - - -class CodeOwnerMetricMiddleware: - """ - Django middleware object to set custom metrics for the owner of each view. - - Custom metrics set: - - code_owner: The owning team mapped to the current view. - - code_owner_mapping_error: If there are any errors when trying to perform the mapping. - - view_func_module: The __module__ of the view_func, which can be used to - find missing mappings. - - """ - def __init__(self, get_response): - self.get_response = get_response - - def __call__(self, request): - self._set_owner_metrics_for_request(request) - response = self.get_response(request) - return response - - def _set_owner_metrics_for_request(self, request): - """ - Uses the request path to find the view_func and then sets code owner metrics based on the view. - """ - if not is_code_owner_mappings_configured(): - return - - try: - view_func, _, _ = resolve(request.path) - view_func_module = view_func.__module__ - set_custom_metric('view_func_module', view_func_module) - code_owner = get_code_owner_from_module(view_func_module) - if code_owner: - set_custom_metric('code_owner', code_owner) - except Resolver404: - set_custom_metric('code_owner_mapping_error', "Couldn't resolve view for request path {}".format(request.path)) - except Exception as e: - set_custom_metric('code_owner_mapping_error', e) diff --git a/lms/djangoapps/monitoring/tests/mock_views.py b/lms/djangoapps/monitoring/tests/mock_views.py deleted file mode 100644 index dee7872a28..0000000000 --- a/lms/djangoapps/monitoring/tests/mock_views.py +++ /dev/null @@ -1,13 +0,0 @@ -""" -Mock views with a different module to enable testing of mapping -code_owner to modules. Trying to mock __module__ on a view was -getting too complex. -""" -from rest_framework.views import APIView - - -class MockViewTest(APIView): - """ - Mock view for use in testing. - """ - pass diff --git a/lms/djangoapps/monitoring/tests/test_middleware.py b/lms/djangoapps/monitoring/tests/test_middleware.py deleted file mode 100644 index 62cd907de9..0000000000 --- a/lms/djangoapps/monitoring/tests/test_middleware.py +++ /dev/null @@ -1,122 +0,0 @@ -""" -Tests for the LMS monitoring middleware -""" -import ddt -from django.conf.urls import url -from django.test import override_settings, RequestFactory -from django.urls import resolve -from mock import call, patch, Mock -from rest_framework.views import APIView -from unittest import TestCase -from unittest.mock import ANY - -from lms.djangoapps.monitoring.middleware import CodeOwnerMetricMiddleware -from lms.djangoapps.monitoring.tests.mock_views import MockViewTest -from lms.djangoapps.monitoring.utils import _process_code_owner_mappings - - -class MockMiddlewareViewTest(APIView): - pass - - -urlpatterns = [ - url(r'^middleware-test/$', MockMiddlewareViewTest.as_view()), - url(r'^test/$', MockViewTest.as_view()), -] - - -@ddt.ddt -class CodeOwnerMetricMiddlewareTests(TestCase): - """ - Tests for the LMS monitoring utility functions - """ - urls = 'lms.djangoapps.monitoring.tests.test_middleware.test_urls' - - def setUp(self): - super().setUp() - self.mock_get_response = Mock() - self.middleware = CodeOwnerMetricMiddleware(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/': 'test_middleware', - '/test/': 'lms.djangoapps.monitoring.tests.mock_views', - } - - @override_settings( - CODE_OWNER_MAPPINGS={'team-red': ['lms.djangoapps.monitoring.tests.mock_views']}, - ROOT_URLCONF=__name__, - ) - @patch('lms.djangoapps.monitoring.middleware.set_custom_metric') - @ddt.data( - ('/middleware-test/', None), - ('/test/', 'team-red'), - ) - @ddt.unpack - def test_code_owner_mapping_hits_and_misses( - self, request_path, expected_owner, mock_set_custom_metric - ): - with patch('lms.djangoapps.monitoring.utils._PATH_TO_CODE_OWNER_MAPPINGS', _process_code_owner_mappings()): - request = RequestFactory().get(request_path) - self.middleware(request) - view_func, _, _ = resolve(request_path) - expected_view_func_module = self._REQUEST_PATH_TO_MODULE_PATH[request_path] - self._assert_code_owner_custom_metrics( - expected_view_func_module, mock_set_custom_metric, expected_code_owner=expected_owner - ) - - @patch('lms.djangoapps.monitoring.middleware.set_custom_metric') - def test_code_owner_no_mappings(self, mock_set_custom_metric): - request = RequestFactory().get('/test/') - self.middleware(request) - mock_set_custom_metric.assert_not_called() - - @override_settings( - CODE_OWNER_MAPPINGS={'team-red': ['lms.djangoapps.monitoring.tests.mock_views']}, - ) - @patch('lms.djangoapps.monitoring.middleware.set_custom_metric') - def test_no_resolver_for_request_path(self, mock_set_custom_metric): - with patch('lms.djangoapps.monitoring.utils._PATH_TO_CODE_OWNER_MAPPINGS', _process_code_owner_mappings()): - request = RequestFactory().get('/bad/path/') - self.middleware(request) - self._assert_code_owner_custom_metrics( - None, mock_set_custom_metric, has_error=True - ) - - @override_settings( - CODE_OWNER_MAPPINGS=['invalid_setting_as_list'], - ROOT_URLCONF=__name__, - ) - @patch('lms.djangoapps.monitoring.middleware.set_custom_metric') - def test_load_config_with_invalid_dict(self, mock_set_custom_metric): - with patch('lms.djangoapps.monitoring.utils._PATH_TO_CODE_OWNER_MAPPINGS', _process_code_owner_mappings()): - request = RequestFactory().get('/test/') - self.middleware(request) - expected_view_func_module = self._REQUEST_PATH_TO_MODULE_PATH['/test/'] - self._assert_code_owner_custom_metrics( - expected_view_func_module, mock_set_custom_metric, has_error=True - ) - - def _assert_code_owner_custom_metrics( - self, view_func_module, mock_set_custom_metric, expected_code_owner=None, has_error=False, - process_view_func=None, - ): - call_list = [] - if view_func_module: - call_list.append(call('view_func_module', view_func_module)) - if expected_code_owner: - call_list.append(call('code_owner', expected_code_owner)) - if has_error: - call_list.append(call('code_owner_mapping_error', ANY)) - mock_set_custom_metric.assert_has_calls(call_list) - self.assertEqual( - len(mock_set_custom_metric.call_args_list), len(call_list), - 'Expected calls {} vs actual calls {}'.format(call_list, mock_set_custom_metric.call_args_list) - ) diff --git a/lms/djangoapps/monitoring/tests/test_monitoring_utils.py b/lms/djangoapps/monitoring/tests/test_monitoring_utils.py deleted file mode 100644 index fa4cb9caf9..0000000000 --- a/lms/djangoapps/monitoring/tests/test_monitoring_utils.py +++ /dev/null @@ -1,93 +0,0 @@ -""" -Tests for the LMS monitoring middleware - -Note: File named test_monitoring_utils.py instead of test_utils.py because of an error -in Jenkins with test collection and a conflict with another test_utils.py file. - -""" -import ddt -import timeit -from django.test import override_settings -from mock import call, patch, Mock -from unittest import TestCase - -from lms.djangoapps.monitoring.utils import ( - _process_code_owner_mappings, - get_code_owner_from_module, - is_code_owner_mappings_configured, -) - - -@ddt.ddt -class MonitoringUtilsTests(TestCase): - """ - Tests for the LMS monitoring utility functions - """ - @override_settings(CODE_OWNER_MAPPINGS=None) - def test_is_config_loaded_with_no_config(self): - with patch('lms.djangoapps.monitoring.utils._PATH_TO_CODE_OWNER_MAPPINGS', _process_code_owner_mappings()): - self.assertFalse(is_code_owner_mappings_configured(), "Mappings should not be configured.") - - @override_settings(CODE_OWNER_MAPPINGS={'team-red': ['openedx.core.djangoapps.xblock']}) - def test_is_config_loaded_with_valid_dict(self): - with patch('lms.djangoapps.monitoring.utils._PATH_TO_CODE_OWNER_MAPPINGS', _process_code_owner_mappings()): - self.assertTrue(is_code_owner_mappings_configured(), "Mappings should be configured.") - - @override_settings(CODE_OWNER_MAPPINGS=['invalid_setting_as_list']) - def test_is_config_loaded_with_invalid_dict(self): - with patch('lms.djangoapps.monitoring.utils._PATH_TO_CODE_OWNER_MAPPINGS', _process_code_owner_mappings()): - self.assertTrue(is_code_owner_mappings_configured(), "Although invalid, mappings should be configured.") - - @override_settings(CODE_OWNER_MAPPINGS={ - 'team-red': [ - 'openedx.core.djangoapps.xblock', - 'lms.djangoapps.grades', - ], - 'team-blue': [ - 'common.djangoapps.xblock_django', - ], - }) - @ddt.data( - ('xbl', None), - ('xblock_2', None), - ('xblock', 'team-red'), - ('openedx.core.djangoapps', None), - ('openedx.core.djangoapps.xblock', 'team-red'), - ('openedx.core.djangoapps.xblock.views', 'team-red'), - ('grades', 'team-red'), - ('lms.djangoapps.grades', 'team-red'), - ('xblock_django', 'team-blue'), - ('common.djangoapps.xblock_django', 'team-blue'), - ) - @ddt.unpack - def test_code_owner_mapping_hits_and_misses(self, module, expected_owner): - with patch('lms.djangoapps.monitoring.utils._PATH_TO_CODE_OWNER_MAPPINGS', _process_code_owner_mappings()): - actual_owner = get_code_owner_from_module(module) - self.assertEqual(expected_owner, actual_owner) - - @override_settings(CODE_OWNER_MAPPINGS=['invalid_setting_as_list']) - def test_load_config_with_invalid_dict(self): - with patch('lms.djangoapps.monitoring.utils._PATH_TO_CODE_OWNER_MAPPINGS', _process_code_owner_mappings()): - self.assertTrue(is_code_owner_mappings_configured(), "Although invalid, mappings should be configured.") - with self.assertRaises(AssertionError): - get_code_owner_from_module('xblock') - - def test_mapping_performance(self): - code_owner_mappings = { - 'team-red': [] - } - # create a long list of mappings that are nearly identical - for n in range(1, 200): - path = 'openedx.core.djangoapps.{}'.format(n) - code_owner_mappings['team-red'].append(path) - with override_settings(CODE_OWNER_MAPPINGS=code_owner_mappings): - with patch( - 'lms.djangoapps.monitoring.utils._PATH_TO_CODE_OWNER_MAPPINGS', _process_code_owner_mappings() - ): - call_iterations = 100 - time = timeit.timeit( - # test a module name that matches nearly to the end, but doesn't actually match - lambda: get_code_owner_from_module('openedx.core.djangoapps.XXX.views'), number=call_iterations - ) - average_time = time / call_iterations - self.assertTrue(average_time < 0.0005, 'Mapping takes {}s which is too slow.'.format(average_time)) diff --git a/lms/djangoapps/monitoring/utils.py b/lms/djangoapps/monitoring/utils.py deleted file mode 100644 index 536b92c205..0000000000 --- a/lms/djangoapps/monitoring/utils.py +++ /dev/null @@ -1,114 +0,0 @@ -""" -Utilities for monitoring the LMS -""" -import logging -import re -from django.conf import settings - -log = logging.getLogger(__name__) - - -def get_code_owner_from_module(module): - """ - Attempts lookup of code_owner based on a code module, - finding the most specific match. If no match, returns None. - - For example, if the module were 'openedx.features.discounts.views', - this lookup would match on 'openedx.features.discounts' before - 'openedx.features', because the former is more specific. - - """ - assert _PATH_TO_CODE_OWNER_MAPPINGS != _INVALID_CODE_OWNER_MAPPING,\ - 'CODE_OWNER_MAPPINGS django setting set with invalid configuration. See logs for details.' - - module_parts = module.split('.') - # To make the most specific match, start with the max number of parts - for number_of_parts in range(len(module_parts), 0, -1): - partial_path = '.'.join(module_parts[0:number_of_parts]) - if partial_path in _PATH_TO_CODE_OWNER_MAPPINGS: - code_owner = _PATH_TO_CODE_OWNER_MAPPINGS[partial_path] - return code_owner - return None - - -def is_code_owner_mappings_configured(): - """ - Returs True if code owner mappings were configured, and False otherwise. - """ - return bool(_PATH_TO_CODE_OWNER_MAPPINGS) - - -def _process_code_owner_mappings(): - """ - Processes the CODE_OWNER_MAPPINGS Django Setting and returns a dict optimized - for efficient lookup by path. - - Returns: - (dict): optimized dict for success processing, None if there are no - configured mappings, or _INVALID_CODE_OWNER_MAPPING if there is an - error processing the setting. - - Example CODE_OWNER_MAPPINGS Django Setting:: - - CODE_OWNER_MAPPINGS = { - 'team-red': [ - 'xblock_django', - 'openedx.core.djangoapps.xblock', - ], - 'team-blue': [ - 'badges', - ], - } - - Example return value:: - - { - 'xblock_django': 'team-red', - 'openedx.core.djangoapps.xblock': 'team-red', - 'badges': 'team-blue', - } - - """ - _CODE_OWNER_MAPPINGS = getattr(settings, 'CODE_OWNER_MAPPINGS', None) - if not _CODE_OWNER_MAPPINGS: - return None - - try: - path_to_code_owner_mappings = {} - for code_owner in _CODE_OWNER_MAPPINGS: - path_list = _CODE_OWNER_MAPPINGS[code_owner] - for path in path_list: - path_to_code_owner_mappings[path] = code_owner - optional_module_prefix_match = _OPTIONAL_MODULE_PREFIX_PATTERN.match(path) - # if path has an optional prefix, also add the module name without the prefix - if optional_module_prefix_match: - path_without_prefix = path[optional_module_prefix_match.end():] - path_to_code_owner_mappings[path_without_prefix] = code_owner - - return path_to_code_owner_mappings - except Exception as e: - log.exception('Error processing code_owner_mappings. {}'.format(e)) - # errors should be unlikely due do scripting the setting values. - # this will trigger an error custom metric that can be alerted on. - return _INVALID_CODE_OWNER_MAPPING - -# .. toggle_name: CODE_OWNER_MAPPINGS -# .. toggle_implementation: DjangoSetting -# .. toggle_default: None -# .. toggle_description: Used to set monitoring custom metrics for owner. Dict with keys of code owner and value as list of dotted path module names owned by code owner. -# .. toggle_category: monitoring -# .. toggle_use_cases: open_edx -# .. toggle_creation_date: 2020-05-29 -# .. toggle_expiration_date: None -# .. toggle_tickets: None -# .. toggle_status: supported -# .. toggle_warnings: None -# Set to settings.CODE_OWNER_MAPPINGS during processing for simpler testing -_CODE_OWNER_MAPPINGS = None - -# The following module prefixes are optional in a view's reported module: -# 'common.djangoapps.', 'lms.djangoapps.', 'openedx.core.djangoapps.' -_OPTIONAL_MODULE_PREFIX_PATTERN = re.compile(r'^(lms|common|openedx\.core)\.djangoapps\.') -_INVALID_CODE_OWNER_MAPPING = 'invalid-code-owner-mapping' -# lookup table for code owner given a module path -_PATH_TO_CODE_OWNER_MAPPINGS = _process_code_owner_mappings() diff --git a/lms/envs/common.py b/lms/envs/common.py index 332e712f4b..e32e009f2a 100644 --- a/lms/envs/common.py +++ b/lms/envs/common.py @@ -1499,7 +1499,7 @@ MIDDLEWARE = [ 'edx_django_utils.monitoring.middleware.MonitoringCustomMetricsMiddleware', # Generate code ownership metrics. Keep this immediately after RequestCacheMiddleware. - 'lms.djangoapps.monitoring.middleware.CodeOwnerMetricMiddleware', + 'edx_django_utils.monitoring.code_owner.middleware.CodeOwnerMetricMiddleware', # Cookie monitoring 'openedx.core.lib.request_utils.CookieMetricsMiddleware', @@ -2561,9 +2561,6 @@ INSTALLED_APPS = [ 'openedx.core.djangoapps.schedules', 'rest_framework_jwt', - # Monitoring utilities for LMS - 'lms.djangoapps.monitoring.apps.MonitoringConfig', - # Learning Sequence Navigation 'openedx.core.djangoapps.content.learning_sequences.apps.LearningSequencesConfig',