From 4d30876d0690f1fdf25567e0ec0600b01f304a7c Mon Sep 17 00:00:00 2001 From: Robert Raposa Date: Tue, 9 Jun 2020 09:59:37 -0400 Subject: [PATCH] ARCHBOM-1263: refactor ownership mapping from middleware (#24175) - move ownership mapping code to separate module - simplify `__module__` mocking. ARCHBOM-1263 --- lms/djangoapps/monitoring/middleware.py | 115 +----------------- .../monitoring/tests/test_middleware.py | 73 +++-------- .../monitoring/tests/test_monitoring_utils.py | 93 ++++++++++++++ lms/djangoapps/monitoring/utils.py | 114 +++++++++++++++++ 4 files changed, 230 insertions(+), 165 deletions(-) create mode 100644 lms/djangoapps/monitoring/tests/test_monitoring_utils.py create mode 100644 lms/djangoapps/monitoring/utils.py diff --git a/lms/djangoapps/monitoring/middleware.py b/lms/djangoapps/monitoring/middleware.py index 342db5b7f4..6db6f09ca4 100644 --- a/lms/djangoapps/monitoring/middleware.py +++ b/lms/djangoapps/monitoring/middleware.py @@ -2,10 +2,10 @@ Middleware for monitoring the LMS """ import logging -import re -from django.conf import settings 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__) @@ -31,119 +31,14 @@ class CodeOwnerMetricMiddleware: """ Set custom metric for the code_owner of the view if configured to do so. """ - if not _PATH_TO_CODE_OWNER_MAPPINGS: + if not is_code_owner_mappings_configured(): return try: - view_func_module = _get_view_func_module(view_func) + view_func_module = view_func.__module__ set_custom_metric('view_func_module', view_func_module) - code_owner = self._find_code_owner(view_func_module) + code_owner = get_code_owner_from_module(view_func_module) if code_owner: set_custom_metric('code_owner', code_owner) except Exception as e: set_custom_metric('code_owner_mapping_error', e) - - def _find_code_owner(self, view_func_module): - """ - Attempts lookup of code_owner based on the view_func's __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. - - """ - if _PATH_TO_CODE_OWNER_MAPPINGS is _INVALID_CODE_OWNER_MAPPING: - raise Exception('CODE_OWNER_MAPPINGS django setting set with invalid configuration. See logs for details.') - - view_func_module_parts = view_func_module.split('.') - # To make the most specific match, start with the max number of parts - for number_of_parts in range(len(view_func_module_parts), 0, -1): - partial_path = '.'.join(view_func_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 _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 func_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() - - -def _get_view_func_module(view_func): - """ - Returns the view_func's __module__. Enables simpler testing via mocking. - """ - return view_func.__module__ diff --git a/lms/djangoapps/monitoring/tests/test_middleware.py b/lms/djangoapps/monitoring/tests/test_middleware.py index c795c65453..ceb612a7e4 100644 --- a/lms/djangoapps/monitoring/tests/test_middleware.py +++ b/lms/djangoapps/monitoring/tests/test_middleware.py @@ -8,15 +8,21 @@ from mock import call, patch, Mock from unittest import TestCase from unittest.mock import ANY -from lms.djangoapps.monitoring.middleware import _process_code_owner_mappings, CodeOwnerMetricMiddleware +from lms.djangoapps.monitoring.middleware import CodeOwnerMetricMiddleware +from lms.djangoapps.monitoring.utils import _process_code_owner_mappings -def _mock_get_view_func_module(view_func): +class MockWithMockModule(): """ - Enables mocking/overriding a private function that normally gets the view_func.__module__ - because it was too difficult to mock the __module__ method. + Mock object with a mocked __module__ method. """ - return view_func.mock_module + def __init__(self, mocked_module_name): + self._mocked_module_name = mocked_module_name + + def __getattribute__(self, name): + if name == '__module__': + return self._mocked_module_name + return super().__getattribute__(name) @ddt.ddt @@ -40,32 +46,25 @@ class CodeOwnerMetricMiddlewareTests(TestCase): @override_settings(CODE_OWNER_MAPPINGS={ 'team-red': [ 'openedx.core.djangoapps.xblock', - 'lms.djangoapps.grades', ], 'team-blue': [ 'common.djangoapps.xblock_django', ], }) @patch('lms.djangoapps.monitoring.middleware.set_custom_metric') - @patch('lms.djangoapps.monitoring.middleware._get_view_func_module', side_effect=_mock_get_view_func_module) @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, view_func_module, expected_owner, mock_get_view_func_module, mock_set_custom_metric + self, view_func_module, expected_owner, mock_set_custom_metric ): - with patch('lms.djangoapps.monitoring.middleware._PATH_TO_CODE_OWNER_MAPPINGS', _process_code_owner_mappings()): - mock_view_func = self._create_view_func_mock(view_func_module) + with patch('lms.djangoapps.monitoring.utils._PATH_TO_CODE_OWNER_MAPPINGS', _process_code_owner_mappings()): + mock_view_func = MockWithMockModule(view_func_module) self.middleware.process_view(None, mock_view_func, None, None) self._assert_code_owner_custom_metrics( view_func_module, mock_set_custom_metric, expected_code_owner=expected_owner @@ -73,48 +72,20 @@ class CodeOwnerMetricMiddlewareTests(TestCase): @patch('lms.djangoapps.monitoring.middleware.set_custom_metric') def test_code_owner_no_mappings(self, mock_set_custom_metric): - mock_view_func = self._create_view_func_mock('xblock') + mock_view_func = MockWithMockModule('xblock') self.middleware.process_view(None, mock_view_func, None, None) mock_set_custom_metric.assert_not_called() @override_settings(CODE_OWNER_MAPPINGS=['invalid_setting_as_list']) @patch('lms.djangoapps.monitoring.middleware.set_custom_metric') - @patch('lms.djangoapps.monitoring.middleware._get_view_func_module', side_effect=_mock_get_view_func_module) - def test_load_config_with_invalid_dict(self, mock_get_view_func_module, mock_set_custom_metric): - with patch('lms.djangoapps.monitoring.middleware._PATH_TO_CODE_OWNER_MAPPINGS', _process_code_owner_mappings()): - mock_view_func = self._create_view_func_mock('xblock') + 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()): + mock_view_func = MockWithMockModule('xblock') self.middleware.process_view(None, mock_view_func, None, None) self._assert_code_owner_custom_metrics( 'xblock', mock_set_custom_metric, has_error=True ) - @patch('lms.djangoapps.monitoring.middleware.set_custom_metric') - @patch('lms.djangoapps.monitoring.middleware._get_view_func_module', side_effect=_mock_get_view_func_module) - def test_mapping_performance(self, mock_get_view_func_module, mock_set_custom_metric): - 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.middleware._PATH_TO_CODE_OWNER_MAPPINGS', _process_code_owner_mappings() - ): - # test a module that matches nearly to the end, but doesn't actually match - mock_view_func = self._create_view_func_mock('openedx.core.djangoapps.XXX.views') - call_iterations = 100 - time = timeit.timeit( - lambda: self.middleware.process_view(None, mock_view_func, None, None), number=call_iterations - ) - average_time = time / call_iterations - self.assertTrue(average_time < 0.0005, 'Mapping takes {}s which is too slow.'.format(average_time)) - expected_calls = [] - for n in range(1, call_iterations): - expected_calls.append(call('view_func_module', 'openedx.core.djangoapps.XXX.views')) - mock_set_custom_metric.assert_has_calls(expected_calls) - def _assert_code_owner_custom_metrics( self, view_func_module, mock_set_custom_metric, expected_code_owner=None, has_error=False, ): @@ -125,11 +96,3 @@ class CodeOwnerMetricMiddlewareTests(TestCase): if has_error: call_list.append(call('code_owner_mapping_error', ANY)) mock_set_custom_metric.assert_has_calls(call_list) - - def _create_view_func_mock(self, module): - """ - Mock view function where __module__ returns 'test_middleware' - """ - view_func = Mock() - view_func.mock_module = module - return view_func diff --git a/lms/djangoapps/monitoring/tests/test_monitoring_utils.py b/lms/djangoapps/monitoring/tests/test_monitoring_utils.py new file mode 100644 index 0000000000..fa4cb9caf9 --- /dev/null +++ b/lms/djangoapps/monitoring/tests/test_monitoring_utils.py @@ -0,0 +1,93 @@ +""" +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 new file mode 100644 index 0000000000..536b92c205 --- /dev/null +++ b/lms/djangoapps/monitoring/utils.py @@ -0,0 +1,114 @@ +""" +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()