From b721e186f3f5511f070546bd0d61c56bc3d80a22 Mon Sep 17 00:00:00 2001 From: Robert Raposa Date: Wed, 10 Jun 2020 10:36:13 -0400 Subject: [PATCH] ARCHBOM-1263: enhance code owner middleware (#24184) * conservative move to process_request * added temp_view_func_compare metric to be extra conservative ARCHBOM-1263 --- lms/djangoapps/monitoring/middleware.py | 30 +++- lms/djangoapps/monitoring/tests/mock_views.py | 13 ++ .../monitoring/tests/test_middleware.py | 137 +++++++++++++----- lms/envs/common.py | 4 +- 4 files changed, 147 insertions(+), 37 deletions(-) create mode 100644 lms/djangoapps/monitoring/tests/mock_views.py diff --git a/lms/djangoapps/monitoring/middleware.py b/lms/djangoapps/monitoring/middleware.py index 6db6f09ca4..4e425a8cb2 100644 --- a/lms/djangoapps/monitoring/middleware.py +++ b/lms/djangoapps/monitoring/middleware.py @@ -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) diff --git a/lms/djangoapps/monitoring/tests/mock_views.py b/lms/djangoapps/monitoring/tests/mock_views.py new file mode 100644 index 0000000000..dee7872a28 --- /dev/null +++ b/lms/djangoapps/monitoring/tests/mock_views.py @@ -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 diff --git a/lms/djangoapps/monitoring/tests/test_middleware.py b/lms/djangoapps/monitoring/tests/test_middleware.py index ceb612a7e4..587d359516 100644 --- a/lms/djangoapps/monitoring/tests/test_middleware.py +++ b/lms/djangoapps/monitoring/tests/test_middleware.py @@ -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) + ) diff --git a/lms/envs/common.py b/lms/envs/common.py index 3ff65c7b06..291d779ab0 100644 --- a/lms/envs/common.py +++ b/lms/envs/common.py @@ -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',