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

ARCHBOM-1263: enhance code owner middleware (#24184)

Browse Source 
* conservative move to process_request
* added temp_view_func_compare metric to be extra conservative

ARCHBOM-1263
This commit is contained in:
Robert Raposa
2020-06-10 10:36:13 -04:00
committed by GitHub
parent 44b0d832a0
commit b721e186f3
4 changed files with 147 additions and 37 deletions
Download Patch File Download Diff File Expand all files Collapse all files

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

@@ -2,6 +2,8 @@
Middleware for monitoring the LMS
"""
import logging
from django.urls import Resolver404, resolve
from edx_django_utils.cache import DEFAULT_REQUEST_CACHE
from edx_django_utils.monitoring import set_custom_metric
from .utils import get_code_owner_from_module, is_code_owner_mappings_configured
@@ -24,21 +26,47 @@ class CodeOwnerMetricMiddleware:
self.get_response = get_response
def __call__(self, request):
self._set_owner_metrics_for_request(request)
response = self.get_response(request)
return response
def process_view(self, request, view_func, view_args, view_kwargs):
self._set_view_func_compare_metric(view_func)
_VIEW_FUNC_MODULE_METRIC_CACHE_KEY = '{}.view_func_module_metric'.format(__file__)
def _set_owner_metrics_for_request(self, request):
"""
Set custom metric for the code_owner of the view if configured to do so.
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__
DEFAULT_REQUEST_CACHE.set(self._VIEW_FUNC_MODULE_METRIC_CACHE_KEY, 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)
def _set_view_func_compare_metric(self, view_func):
"""
Set temporary metric to ensure that the view_func of `process_view` always matches
the one from using `resolve` on the request.
"""
try:
view_func_module = view_func.__module__
cached_response = DEFAULT_REQUEST_CACHE.get_cached_response(self._VIEW_FUNC_MODULE_METRIC_CACHE_KEY)
if cached_response.is_found:
view_func_compare = 'success' if view_func_module == cached_response.value else view_func_module
else:
view_func_compare = 'missing'
set_custom_metric('temp_view_func_compare', view_func_compare)
except Exception as e:
set_custom_metric('temp_view_func_compare_error', e)

13
lms/djangoapps/monitoring/tests/mock_views.py Normal file
View File

@@ -0,0 +1,13 @@
"""
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

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

@@ -2,27 +2,28 @@
Tests for the LMS monitoring middleware
"""
import ddt
import timeit
from django.test import override_settings
from django.conf.urls import url
from django.test import override_settings, RequestFactory
from django.urls import resolve
from edx_django_utils.cache import RequestCache
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 MockWithMockModule():
"""
Mock object with a mocked __module__ method.
"""
def __init__(self, mocked_module_name):
self._mocked_module_name = mocked_module_name
class MockMiddlewareViewTest(APIView):
pass
def __getattribute__(self, name):
if name == '__module__':
return self._mocked_module_name
return super().__getattribute__(name)
urlpatterns = [
url(r'^middleware-test/$', MockMiddlewareViewTest.as_view()),
url(r'^test/$', MockViewTest.as_view()),
]
@ddt.ddt
@@ -30,8 +31,11 @@ class CodeOwnerMetricMiddlewareTests(TestCase):
"""
Tests for the LMS monitoring utility functions
"""
urls = 'lms.djangoapps.monitoring.tests.test_middleware.test_urls'
def setUp(self):
super().setUp()
RequestCache.clear_all_namespaces()
self.mock_get_response = Mock()
self.middleware = CodeOwnerMetricMiddleware(self.mock_get_response)
@@ -43,56 +47,119 @@ class CodeOwnerMetricMiddlewareTests(TestCase):
request = Mock()
self.assertEqual(self.middleware(request), 'test-response')
@override_settings(CODE_OWNER_MAPPINGS={
'team-red': [
'openedx.core.djangoapps.xblock',
],
'team-blue': [
'common.djangoapps.xblock_django',
],
})
_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(
('xblock', 'team-red'),
('openedx.core.djangoapps', None),
('openedx.core.djangoapps.xblock.views', 'team-red'),
('xblock_django', 'team-blue'),
('common.djangoapps.xblock_django', 'team-blue'),
('/middleware-test/', None),
('/test/', 'team-red'),
)
@ddt.unpack
def test_code_owner_mapping_hits_and_misses(
self, view_func_module, expected_owner, mock_set_custom_metric
self, request_path, expected_owner, mock_set_custom_metric
):
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)
request = RequestFactory().get(request_path)
self.middleware(request)
view_func, _, _ = resolve(request_path)
self.middleware.process_view(None, view_func, None, None)
expected_view_func_module = self._REQUEST_PATH_TO_MODULE_PATH[request_path]
self._assert_code_owner_custom_metrics(
view_func_module, mock_set_custom_metric, expected_code_owner=expected_owner
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):
mock_view_func = MockWithMockModule('xblock')
self.middleware.process_view(None, mock_view_func, None, None)
request = RequestFactory().get('/test/')
self.middleware(request)
mock_set_custom_metric.assert_not_called()
@override_settings(CODE_OWNER_MAPPINGS=['invalid_setting_as_list'])
@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()):
mock_view_func = MockWithMockModule('xblock')
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
)
@override_settings(
CODE_OWNER_MAPPINGS={'team-red': ['lms.djangoapps.monitoring.tests.mock_views']},
ROOT_URLCONF=__name__,
)
@patch('lms.djangoapps.monitoring.middleware.set_custom_metric')
def test_failed_compare_in_process_view(self, mock_set_custom_metric):
"""
Tests that the process_view gets the same function. This is to conservatively
test that the resolve method results in all the same metrics.
"""
class MockWithMockModule():
"""
Added inside function to enable quick clean-up when this is removed.
"""
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)
with patch('lms.djangoapps.monitoring.utils._PATH_TO_CODE_OWNER_MAPPINGS', _process_code_owner_mappings()):
request_path = '/test/'
expected_owner = 'team-red'
request = RequestFactory().get(request_path)
self.middleware(request)
expected_view_func_module = self._REQUEST_PATH_TO_MODULE_PATH[request_path]
mock_view_func = MockWithMockModule('different.module')
self.middleware.process_view(None, mock_view_func, None, None)
self._assert_code_owner_custom_metrics(
'xblock', mock_set_custom_metric, has_error=True
expected_view_func_module, mock_set_custom_metric, expected_code_owner=expected_owner,
process_view_func=mock_view_func,
)
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 = []
call_list.append(call('view_func_module', view_func_module))
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))
if view_func_module and not has_error:
if process_view_func:
call_list.append(call('temp_view_func_compare', process_view_func.__module__))
else:
call_list.append(call('temp_view_func_compare', 'success'))
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)
)

4
lms/envs/common.py
View File

@@ -1494,6 +1494,9 @@ MIDDLEWARE = [
'edx_django_utils.cache.middleware.RequestCacheMiddleware',
'edx_django_utils.monitoring.middleware.MonitoringCustomMetricsMiddleware',
# Generate code ownership metrics. Keep this immediately after RequestCacheMiddleware.
'lms.djangoapps.monitoring.middleware.CodeOwnerMetricMiddleware',
# Cookie monitoring
'openedx.core.lib.request_utils.CookieMetricsMiddleware',
@@ -1576,7 +1579,6 @@ MIDDLEWARE = [
# Outputs monitoring metrics for a request.
'edx_rest_framework_extensions.middleware.RequestMetricsMiddleware',
'lms.djangoapps.monitoring.middleware.CodeOwnerMetricMiddleware',
'edx_rest_framework_extensions.auth.jwt.middleware.EnsureJWTAuthSettingsMiddleware',