Merge pull request #68 from edx/robrap/ARCHBOM-1527-update-code-owner…

…-mapping

ARCHBOM-1527: update code owner mapping

CHANGELOG.rst

@@ -15,6 +15,17 @@ Change Log
 Unreleased
 ~~~~~~~~~~
 
+[3.9.0] - 2020-10-21
+~~~~~~~~~~~~~~~~~~~~
+
+Updated
+_______
+
+* Exposed existing get_code_owner_from_module via the public api.
+* Fixed get_code_owner_from_module to not require a call to is_code_owner_mappings_configured beforehand.
+* Set the existing code_owner_path_module custom attribute, even for cases where the transaction name was used, rather than the view module.
+* Refactor code owner setting processing.
+
 [3.8.0] - 2020-08-31
 ~~~~~~~~~~~~~~~~~~~~
 

edx_django_utils/__init__.py

@@ -2,7 +2,7 @@
 EdX utilities for Django Application development..
 """
 
-__version__ = "3.8.0"
+__version__ = "3.9.0"
 
 default_app_config = (
     "edx_django_utils.apps.EdxDjangoUtilsConfig"

edx_django_utils/monitoring/__init__.py

@@ -3,6 +3,7 @@
 
 See README.rst for details.
 """
+from .code_owner.utils import get_code_owner_from_module
 from .transactions import function_trace, get_current_transaction, ignore_transaction, set_monitoring_transaction_name
 # "set_custom_metric*" methods are deprecated
 from .utils import (

edx_django_utils/monitoring/code_owner/__init__.py

@@ -0,0 +1,6 @@
+"""
+This directory should only be used internally.
+
+Its public API is exposed in the top-level monitoring __init__.py.
+See its README.rst for details.
+"""

edx_django_utils/monitoring/code_owner/middleware.py

@@ -117,15 +117,17 @@ def _set_code_owner_attribute_from_current_transaction(self, request):
 
         """
         if not is_code_owner_mappings_configured():
-            return None, None
+            # ensure we don't set code ownership custom attributes if not configured to do so
+            return None, None  # pragma: no cover
 
         try:
             # Example: openedx.core.djangoapps.contentserver.middleware:StaticContentServer
             transaction_name = get_current_transaction().name
             if not transaction_name:
                 return None, 'No current transaction name found.'
-            set_custom_attribute('code_owner_transaction_name', transaction_name)
             module_name = transaction_name.split(':')[0]
+            set_custom_attribute('code_owner_transaction_name', transaction_name)
+            set_custom_attribute('code_owner_path_module', module_name)
             code_owner = get_code_owner_from_module(module_name)
             return code_owner, None
         except Exception as e:  # pylint: disable=broad-except
@@ -139,9 +141,6 @@ def _set_code_owner_attribute_catch_all(self):
             (str): code_owner or None if no catch-all configured.
 
         """
-        if not is_code_owner_mappings_configured():
-            return None
-
         try:
             code_owner = get_code_owner_from_module('*')
             return code_owner

edx_django_utils/monitoring/code_owner/tests/test_middleware.py

@@ -12,7 +12,7 @@
 
 from edx_django_utils.monitoring.code_owner.middleware import CodeOwnerMonitoringMiddleware
 from edx_django_utils.monitoring.code_owner.tests.mock_views import MockViewTest
-from edx_django_utils.monitoring.code_owner.utils import _process_code_owner_mappings
+from edx_django_utils.monitoring.code_owner.utils import clear_cached_mappings
 
 
 class MockMiddlewareViewTest(View):
@@ -34,6 +34,7 @@ class CodeOwnerMetricMiddlewareTests(TestCase):
 
     def setUp(self):
         super().setUp()
+        clear_cached_mappings()
         self.mock_get_response = Mock()
         self.middleware = CodeOwnerMonitoringMiddleware(self.mock_get_response)
 
@@ -63,22 +64,18 @@ def test_request_call(self):
     def test_code_owner_path_mapping_hits_and_misses(
         self, request_path, expected_owner, mock_set_custom_attribute
     ):
-        with patch(
-                'edx_django_utils.monitoring.code_owner.utils._PATH_TO_CODE_OWNER_MAPPINGS',
-                _process_code_owner_mappings()
-        ):
-            request = RequestFactory().get(request_path)
-            self.middleware(request)
-            expected_path_module = self._REQUEST_PATH_TO_MODULE_PATH[request_path]
-            self._assert_code_owner_custom_attributes(
-                mock_set_custom_attribute, expected_code_owner=expected_owner, path_module=expected_path_module
-            )
+        request = RequestFactory().get(request_path)
+        self.middleware(request)
+        expected_path_module = self._REQUEST_PATH_TO_MODULE_PATH[request_path]
+        self._assert_code_owner_custom_attributes(
+            mock_set_custom_attribute, expected_code_owner=expected_owner, path_module=expected_path_module
+        )
 
-            mock_set_custom_attribute.reset_mock()
-            self.middleware.process_exception(request, None)
-            self._assert_code_owner_custom_attributes(
-                mock_set_custom_attribute, expected_code_owner=expected_owner, path_module=expected_path_module
-            )
+        mock_set_custom_attribute.reset_mock()
+        self.middleware.process_exception(request, None)
+        self._assert_code_owner_custom_attributes(
+            mock_set_custom_attribute, expected_code_owner=expected_owner, path_module=expected_path_module
+        )
 
     @override_settings(
         CODE_OWNER_MAPPINGS={
@@ -96,16 +93,12 @@ def test_code_owner_path_mapping_hits_and_misses(
     def test_code_owner_path_mapping_with_catch_all(
         self, request_path, expected_owner, mock_set_custom_attribute
     ):
-        with patch(
-                'edx_django_utils.monitoring.code_owner.utils._PATH_TO_CODE_OWNER_MAPPINGS',
-                _process_code_owner_mappings()
-        ):
-            request = RequestFactory().get(request_path)
-            self.middleware(request)
-            expected_path_module = self._REQUEST_PATH_TO_MODULE_PATH[request_path]
-            self._assert_code_owner_custom_attributes(
-                mock_set_custom_attribute, expected_code_owner=expected_owner, path_module=expected_path_module
-            )
+        request = RequestFactory().get(request_path)
+        self.middleware(request)
+        expected_path_module = self._REQUEST_PATH_TO_MODULE_PATH[request_path]
+        self._assert_code_owner_custom_attributes(
+            mock_set_custom_attribute, expected_code_owner=expected_owner, path_module=expected_path_module
+        )
 
     @override_settings(
         CODE_OWNER_MAPPINGS={'team-red': ['edx_django_utils.monitoring.code_owner.tests.mock_views']},
@@ -114,29 +107,35 @@ def test_code_owner_path_mapping_with_catch_all(
     @patch('edx_django_utils.monitoring.code_owner.middleware.set_custom_attribute')
     @patch('newrelic.agent')
     @ddt.data(
-        ('edx_django_utils.monitoring.code_owner.tests.test_middleware:MockMiddlewareViewTest', None),
-        ('edx_django_utils.monitoring.code_owner.tests.mock_views:MockViewTest', 'team-red'),
+        (
+            'edx_django_utils.monitoring.code_owner.tests.test_middleware',
+            'edx_django_utils.monitoring.code_owner.tests.test_middleware:MockMiddlewareViewTest',
+            None
+        ),
+        (
+            'edx_django_utils.monitoring.code_owner.tests.mock_views',
+            'edx_django_utils.monitoring.code_owner.tests.mock_views:MockViewTest',
+            'team-red'
+        ),
     )
     @ddt.unpack
     def test_code_owner_transaction_mapping_hits_and_misses(
-        self, transaction_name, expected_owner, mock_newrelic_agent, mock_set_custom_attribute
+        self, path_module, transaction_name, expected_owner, mock_newrelic_agent, mock_set_custom_attribute
     ):
         mock_newrelic_agent.current_transaction().name = transaction_name
-        with patch(
-                'edx_django_utils.monitoring.code_owner.utils._PATH_TO_CODE_OWNER_MAPPINGS',
-                _process_code_owner_mappings()
-        ):
-            request = RequestFactory().get('/bad/path/')
-            self.middleware(request)
-            self._assert_code_owner_custom_attributes(
-                mock_set_custom_attribute, expected_code_owner=expected_owner, transaction_name=transaction_name
-            )
+        request = RequestFactory().get('/bad/path/')
+        self.middleware(request)
+        self._assert_code_owner_custom_attributes(
+            mock_set_custom_attribute, expected_code_owner=expected_owner, path_module=path_module,
+            transaction_name=transaction_name
+        )
 
-            mock_set_custom_attribute.reset_mock()
-            self.middleware.process_exception(request, None)
-            self._assert_code_owner_custom_attributes(
-                mock_set_custom_attribute, expected_code_owner=expected_owner, transaction_name=transaction_name
-            )
+        mock_set_custom_attribute.reset_mock()
+        self.middleware.process_exception(request, None)
+        self._assert_code_owner_custom_attributes(
+            mock_set_custom_attribute, expected_code_owner=expected_owner, path_module=path_module,
+            transaction_name=transaction_name
+        )
 
     @override_settings(
         CODE_OWNER_MAPPINGS={
@@ -148,23 +147,28 @@ def test_code_owner_transaction_mapping_hits_and_misses(
     @patch('edx_django_utils.monitoring.code_owner.middleware.set_custom_attribute')
     @patch('newrelic.agent')
     @ddt.data(
-        ('edx_django_utils.monitoring.code_owner.tests.test_middleware:MockMiddlewareViewTest', 'team-blue'),
-        ('edx_django_utils.monitoring.code_owner.tests.mock_views:MockViewTest', 'team-red'),
+        (
+            'edx_django_utils.monitoring.code_owner.tests.test_middleware',
+            'edx_django_utils.monitoring.code_owner.tests.test_middleware:MockMiddlewareViewTest',
+            'team-blue'
+        ),
+        (
+            'edx_django_utils.monitoring.code_owner.tests.mock_views',
+            'edx_django_utils.monitoring.code_owner.tests.mock_views:MockViewTest',
+            'team-red'
+        ),
     )
     @ddt.unpack
-    def test_code_owner_transaction_mapping__with_catch_all(
-        self, transaction_name, expected_owner, mock_newrelic_agent, mock_set_custom_attribute
+    def test_code_owner_transaction_mapping_with_catch_all(
+        self, path_module, transaction_name, expected_owner, mock_newrelic_agent, mock_set_custom_attribute
     ):
         mock_newrelic_agent.current_transaction().name = transaction_name
-        with patch(
-                'edx_django_utils.monitoring.code_owner.utils._PATH_TO_CODE_OWNER_MAPPINGS',
-                _process_code_owner_mappings()
-        ):
-            request = RequestFactory().get('/bad/path/')
-            self.middleware(request)
-            self._assert_code_owner_custom_attributes(
-                mock_set_custom_attribute, expected_code_owner=expected_owner, transaction_name=transaction_name
-            )
+        request = RequestFactory().get('/bad/path/')
+        self.middleware(request)
+        self._assert_code_owner_custom_attributes(
+            mock_set_custom_attribute, expected_code_owner=expected_owner, path_module=path_module,
+            transaction_name=transaction_name
+        )
 
     @override_settings(
         CODE_OWNER_MAPPINGS={'team-red': ['edx_django_utils.monitoring.code_owner.tests.mock_views']},
@@ -174,67 +178,56 @@ def test_code_owner_transaction_mapping__with_catch_all(
     @patch('newrelic.agent')
     def test_code_owner_transaction_mapping_error(self, mock_newrelic_agent, mock_set_custom_attribute):
         mock_newrelic_agent.current_transaction = Mock(side_effect=Exception('forced exception'))
-        with patch(
-                'edx_django_utils.monitoring.code_owner.utils._PATH_TO_CODE_OWNER_MAPPINGS',
-                _process_code_owner_mappings()
-        ):
-            request = RequestFactory().get('/bad/path/')
-            self.middleware(request)
-            self._assert_code_owner_custom_attributes(
-                mock_set_custom_attribute, has_path_error=True, has_transaction_error=True
-            )
+        request = RequestFactory().get('/bad/path/')
+        self.middleware(request)
+        self._assert_code_owner_custom_attributes(
+            mock_set_custom_attribute, has_path_error=True, has_transaction_error=True
+        )
 
     @patch('edx_django_utils.monitoring.code_owner.middleware.set_custom_attribute')
     def test_code_owner_no_mappings(self, mock_set_custom_attribute):
         request = RequestFactory().get('/test/')
         self.middleware(request)
         mock_set_custom_attribute.assert_not_called()
 
+    @patch('edx_django_utils.monitoring.code_owner.middleware.set_custom_attribute')
+    def test_code_owner_transaction_no_mappings(self, mock_set_custom_attribute):
+        request = RequestFactory().get('/bad/path/')
+        self.middleware(request)
+        mock_set_custom_attribute.assert_not_called()
+
     @override_settings(
         CODE_OWNER_MAPPINGS={'team-red': ['lms.djangoapps.monitoring.tests.mock_views']},
     )
     @patch('edx_django_utils.monitoring.code_owner.middleware.set_custom_attribute')
     def test_no_resolver_for_path_and_no_transaction(self, mock_set_custom_attribute):
-        with patch(
-                'edx_django_utils.monitoring.code_owner.utils._PATH_TO_CODE_OWNER_MAPPINGS',
-                _process_code_owner_mappings()
-        ):
-            request = RequestFactory().get('/bad/path/')
-            self.middleware(request)
-            self._assert_code_owner_custom_attributes(
-                mock_set_custom_attribute, has_path_error=True, has_transaction_error=True
-            )
+        request = RequestFactory().get('/bad/path/')
+        self.middleware(request)
+        self._assert_code_owner_custom_attributes(
+            mock_set_custom_attribute, has_path_error=True, has_transaction_error=True
+        )
 
     @override_settings(
         CODE_OWNER_MAPPINGS={'team-red': ['*']},
     )
     @patch('edx_django_utils.monitoring.code_owner.middleware.set_custom_attribute')
     def test_catch_all_instead_of_errors(self, mock_set_custom_attribute):
-        with patch(
-                'edx_django_utils.monitoring.code_owner.utils._PATH_TO_CODE_OWNER_MAPPINGS',
-                _process_code_owner_mappings()
-        ):
-            request = RequestFactory().get('/bad/path/')
-            self.middleware(request)
-            self._assert_code_owner_custom_attributes(mock_set_custom_attribute, expected_code_owner='team-red')
+        request = RequestFactory().get('/bad/path/')
+        self.middleware(request)
+        self._assert_code_owner_custom_attributes(mock_set_custom_attribute, expected_code_owner='team-red')
 
     @override_settings(
         CODE_OWNER_MAPPINGS=['invalid_setting_as_list'],
         ROOT_URLCONF=__name__,
     )
     @patch('edx_django_utils.monitoring.code_owner.middleware.set_custom_attribute')
     def test_load_config_with_invalid_dict(self, mock_set_custom_attribute):
-        with patch(
-                'edx_django_utils.monitoring.code_owner.utils._PATH_TO_CODE_OWNER_MAPPINGS',
-                _process_code_owner_mappings()
-        ):
-            request = RequestFactory().get('/test/')
-            self.middleware(request)
-            expected_path_module = self._REQUEST_PATH_TO_MODULE_PATH['/test/']
-            self._assert_code_owner_custom_attributes(
-                mock_set_custom_attribute, path_module=expected_path_module,
-                has_path_error=True, has_transaction_error=True
-            )
+        request = RequestFactory().get('/test/')
+        self.middleware(request)
+        expected_path_module = self._REQUEST_PATH_TO_MODULE_PATH['/test/']
+        self._assert_code_owner_custom_attributes(
+            mock_set_custom_attribute, path_module=expected_path_module,
+        )
 
     def _assert_code_owner_custom_attributes(self, mock_set_custom_attribute, expected_code_owner=None,
                                              path_module=None, has_path_error=False,