Andal.LND/edx-platform
6
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

ARCHBOM-1263: refactor ownership mapping from middleware (#24175)

Browse Source 
- move ownership mapping code to separate module
- simplify `__module__` mocking.

ARCHBOM-1263
This commit is contained in:
Robert Raposa
2020-06-09 09:59:37 -04:00
committed by GitHub
parent 03032d4979
commit 4d30876d06
4 changed files with 230 additions and 165 deletions
Download Patch File Download Diff File Expand all files Collapse all files

115
lms/djangoapps/monitoring/middleware.py
View File

@@ -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__

73
lms/djangoapps/monitoring/tests/test_middleware.py
View File

@@ -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

93
lms/djangoapps/monitoring/tests/test_monitoring_utils.py Normal file
View File

@@ -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))

114
lms/djangoapps/monitoring/utils.py Normal file
View File

@@ -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()