Merge pull request #70 from edx/robrap/ARCHBOM-1584-add-monitoring-internal

ARCHBOM-1584: refactor monitoring app

Author: robrap

CHANGELOG.rst

@@ -15,6 +15,42 @@ Change Log
 Unreleased
 ~~~~~~~~~~
 
+[3.11.0] - 2020-10-31
+~~~~~~~~~~~~~~~~~~~~~
+
+Added
+_____
+
+* Added ADR 0004-public-api-and-app-organization.rst to explain a new app organization, which makes use of the public API more consistent.
+
+Updated
+_______
+
+* Applied the new app organization described in th ADR to the monitoring Django app.
+* Moved CachedCustomMonitoringMiddleware, CodeOwnerMonitoringMiddleware, and MonitoringMemoryMiddleware to the public API.
+
+Deprecated
+__________
+
+* Deprecated the old locations of CachedCustomMonitoringMiddleware, CodeOwnerMonitoringMiddleware, and MonitoringMemoryMiddleware.
+* Deprecated various methods from modules that were always meant to be used from the public API.
+
+  * accumulate
+  * increment
+  * set_custom_attribute
+  * set_custom_attributes_for_course_key
+
+* Added additional custom attributes for deprecated classes and methods to make them safer to retire.
+
+.. note::
+
+  Some method implementations that were available in the public API were moved without adding a deprecated equivalent. These were not found when searching, so hopefully they are only used via the public API, which did not change. This includes functions in ``transactions.py`` and ``code_owner/utils.py``.
+
+Removed
+_______
+
+* Removed the middleware ordering checks. This is not a typical Django feature and it is painful when refactoring.
+
 [3.10.0] - 2020-10-28
 ~~~~~~~~~~~~~~~~~~~~~
 

docs/decisions/0004-public-api-and-app-organization.rst

@@ -0,0 +1,40 @@
+Public API and App Organization
+===============================
+
+Status
+------
+
+Accepted
+
+Context
+-------
+
+The original apps in the library (e.g. ``cache`` and ``monitoring``) exposed a public API via ``__init__.py``.
+
+There are several problems with the original organization of the app code:
+
+* It was easy to forget to add new code to the public API, or ignore this requirement. For example, the Middleware didn't follow the same process.
+* It was easy for a user of the library to mistakenly use code from a different module in the app, rather than through the public API.
+
+Decision
+--------
+
+All implementation code should be moved to an ``internal`` module.
+
+using monitoring as an example, the public API would be exposed as follows in ``edx_django_utils/monitoring/__init__.py``::
+
+    from .internal.somemodule import ...
+
+The benefits of this setup include:
+
+* A clear designation of what is part of the public API.
+* The ability to refactor the implementation without changing the API.
+* A clear reminder to developers adding new code that it needs to be exposed if it is public.
+* A clear reminder to developers using the library not to use code from the internal implementation.
+
+Consequences
+------------
+
+Whenever a new class or function is added to the edx_django_utils public API, it should be implemented in the Django app's ``internal`` module and explicitly imported in its ``__init__.py`` module.
+
+Additionally, some existing apps will need to be refactored.

edx_django_utils/__init__.py

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

edx_django_utils/cache/middleware.py

@@ -3,8 +3,6 @@
 """
 from django.utils.deprecation import MiddlewareMixin
 
-from edx_django_utils.private_utils import _check_middleware_dependencies
-
 from . import RequestCache, TieredCache
 
 
@@ -36,16 +34,6 @@ class TieredCacheMiddleware(MiddlewareMixin):
     """
     Middleware to store whether or not to force django cache misses.
     """
-    def __init__(self, get_response=None):
-        super(TieredCacheMiddleware, self).__init__(get_response=get_response)
-        # checks proper dependency order as well.
-        _check_middleware_dependencies(self, required_middleware=[
-            'edx_django_utils.cache.middleware.RequestCacheMiddleware',
-            # Some Authentication Middleware also needs to be in between,
-            # but don't want to hard-code that dependency.
-            'edx_django_utils.cache.middleware.TieredCacheMiddleware',
-        ])
-
     def process_request(self, request):
         """
         Stores whether or not FORCE_CACHE_MISS_PARAM was supplied in the

edx_django_utils/cache/tests/test_middleware.py

@@ -2,7 +2,7 @@
 """
 Tests for the RequestCacheMiddleware.
 """
-from django.test import RequestFactory, TestCase, override_settings
+from django.test import RequestFactory, TestCase
 from mock import MagicMock
 
 from edx_django_utils.cache import middleware
@@ -15,7 +15,6 @@
 
 class TestRequestCacheMiddleware(TestCase):  # pylint: disable=missing-class-docstring
 
-    @override_settings(MIDDLEWARE=['edx_django_utils.cache.middleware.RequestCacheMiddleware'])
     def setUp(self):
         super(TestRequestCacheMiddleware, self).setUp()
         self.middleware = middleware.RequestCacheMiddleware()
@@ -56,10 +55,6 @@ def _dirty_request_cache(self):
 
 class TestTieredCacheMiddleware(TestCase):  # pylint: disable=missing-class-docstring
 
-    @override_settings(MIDDLEWARE=[
-        'edx_django_utils.cache.middleware.RequestCacheMiddleware',
-        'edx_django_utils.cache.middleware.TieredCacheMiddleware'
-    ])
     def setUp(self):
         super(TestTieredCacheMiddleware, self).setUp()
         self.middleware = middleware.TieredCacheMiddleware()
@@ -90,11 +85,6 @@ def test_process_request_force_cache_miss_non_staff(self):
 
         self.assertFalse(self.request_cache.get_cached_response(SHOULD_FORCE_CACHE_MISS_KEY).value)
 
-    @override_settings(MIDDLEWARE=['some.Middleware'])
-    def test_tiered_cache_missing_middleware(self):
-        with self.assertRaises(AssertionError):
-            middleware.TieredCacheMiddleware()
-
     def _mock_user(self, is_staff=True):
         mock_user = MagicMock()
         mock_user.is_active = True

edx_django_utils/monitoring/README.rst

@@ -22,9 +22,9 @@ Here is how you add the middleware:
         # Generate code ownership attributes. Keep this immediately after RequestCacheMiddleware.
         ...
         # Monitoring middleware must come after RequestCacheMiddleware
-        'edx_django_utils.monitoring.code_owner.middleware.CodeOwnerMonitoringMiddleware',
-        'edx_django_utils.monitoring.middleware.CachedCustomMonitoringMiddleware',
-        'edx_django_utils.monitoring.middleware.MonitoringMemoryMiddleware',
+        'edx_django_utils.monitoring.CodeOwnerMonitoringMiddleware',
+        'edx_django_utils.monitoring.CachedCustomMonitoringMiddleware',
+        'edx_django_utils.monitoring.MonitoringMemoryMiddleware',
     )
 
 Monitoring Memory Usage

edx_django_utils/monitoring/__init__.py

@@ -3,14 +3,15 @@
 
 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 (
-    accumulate,
-    increment,
-    set_custom_attribute,
-    set_custom_attributes_for_course_key,
-    set_custom_metric,
-    set_custom_metrics_for_course_key
+from .internal.code_owner.middleware import CodeOwnerMonitoringMiddleware
+from .internal.code_owner.utils import get_code_owner_from_module
+from .internal.middleware import CachedCustomMonitoringMiddleware, MonitoringMemoryMiddleware
+from .internal.transactions import (
+    function_trace,
+    get_current_transaction,
+    ignore_transaction,
+    set_monitoring_transaction_name
 )
+from .internal.utils import accumulate, increment, set_custom_attribute, set_custom_attributes_for_course_key
+# "set_custom_metric*" methods are deprecated
+from .utils import set_custom_metric, set_custom_metrics_for_course_key

edx_django_utils/monitoring/code_owner/middleware.py

@@ -1,158 +1,36 @@
 """
-Middleware for code_owner custom attribute
-"""
-import logging
-import warnings
+Deprecated Middleware for backward-compatibility.
 
-from django.urls import resolve
+IMPORTANT: No new classes should be added to this file.
+TODO: Remove this file once these classes are no longer used.
 
-from edx_django_utils.monitoring import get_current_transaction, set_custom_attribute
-
-from .utils import get_code_owner_from_module, is_code_owner_mappings_configured
+"""
+import warnings
 
-log = logging.getLogger(__name__)
+from edx_django_utils.monitoring.internal.code_owner.middleware import \
+    CodeOwnerMonitoringMiddleware as InternalCodeOwnerMonitoringMiddleware
+from edx_django_utils.monitoring.internal.utils import set_custom_attribute
 
 
-class CodeOwnerMonitoringMiddleware:
+class CodeOwnerMonitoringMiddleware(InternalCodeOwnerMonitoringMiddleware):
     """
-    Django middleware object to set custom attributes for the owner of each view.
-
-    For instructions on usage, see:
-    https://github.com/edx/edx-django-utils/blob/master/edx_django_utils/monitoring/docs/how_tos/add_code_owner_custom_attribute_to_an_ida.rst
-
-    Custom attributes set:
-    - code_owner: The owning team mapped to the current view.
-    - code_owner_mapping_error: If there are any errors when trying to perform the mapping.
-    - code_owner_path_error: The error mapping by path, if code_owner isn't found in other ways.
-    - code_owner_path_module: The __module__ of the view_func which was used to try to map to code_owner.
-        This can be used to find missing mappings.
-    - code_owner_transaction_error: The error mapping by transaction, if code_owner isn't found in other ways.
-    - code_owner_transaction_name: The current transaction name used to try to map to code_owner.
-        This can be used to find missing mappings.
-
+    Deprecated class for handling middleware. Class has been moved to public API.
     """
-    def __init__(self, get_response):
-        self.get_response = get_response
-
-    def __call__(self, request):
-        response = self.get_response(request)
-        self._set_code_owner_attribute(request)
-        return response
-
-    def process_exception(self, request, exception):
-        self._set_code_owner_attribute(request)
-
-    def _set_code_owner_attribute(self, request):
-        """
-        Sets the code_owner custom attribute, as well as several supporting custom attributes.
-
-        See CodeOwnerMonitoringMiddleware docstring for a complete list of attributes.
-
-        """
-        code_owner, path_error = self._set_code_owner_attribute_from_path(request)
-        if code_owner:
-            set_custom_attribute('code_owner', code_owner)
-            return
-        if not path_error:
-            # module found, but mapping wasn't configured
-            code_owner = self._set_code_owner_attribute_catch_all()
-            if code_owner:
-                set_custom_attribute('code_owner', code_owner)
-            return
-
-        code_owner, transaction_error = self._set_code_owner_attribute_from_current_transaction(request)
-        if code_owner:
-            set_custom_attribute('code_owner', code_owner)
-            return
-        if not transaction_error:
-            # transaction name found, but mapping wasn't configured
-            code_owner = self._set_code_owner_attribute_catch_all()
-            if code_owner:
-                set_custom_attribute('code_owner', code_owner)
-            return
-
-        code_owner = self._set_code_owner_attribute_catch_all()
-        if code_owner:
-            set_custom_attribute('code_owner', code_owner)
-            return
-
-        # only report errors if code_owner couldn't be found, including catch-all
-        if path_error:
-            set_custom_attribute('code_owner_path_error', path_error)
-        if transaction_error:
-            set_custom_attribute('code_owner_transaction_error', transaction_error)
-
-    def _set_code_owner_attribute_from_path(self, request):
-        """
-        Uses the request path to find the view_func and then sets code owner attributes based on the view.
-
-        Side-effects:
-            Sets code_owner_path_module custom attribute, used to determine code_owner
-
-        Returns:
-            (str, str): (code_owner, error_message), where at least one of these should be None
-
-        """
-        if not is_code_owner_mappings_configured():
-            return None, None
-
-        try:
-            view_func, _, _ = resolve(request.path)
-            path_module = view_func.__module__
-            set_custom_attribute('code_owner_path_module', path_module)
-            code_owner = get_code_owner_from_module(path_module)
-            return code_owner, None
-        except Exception as e:  # pylint: disable=broad-except
-            return None, str(e)
-
-    def _set_code_owner_attribute_from_current_transaction(self, request):
-        """
-        Uses the current transaction name to set the code owner attribute.
-
-        Side-effects:
-            Sets code_owner_transaction_name custom attribute, used to determine code_owner
-
-        Returns:
-            (str, str): (code_owner, error_message), where at least one of these should be None
-
-        """
-        if not is_code_owner_mappings_configured():
-            # 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.'
-            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
-            return None, str(e)
-
-    def _set_code_owner_attribute_catch_all(self):
-        """
-        If the catch-all module "*" is configured, return the code_owner.
-
-        Returns:
-            (str): code_owner or None if no catch-all configured.
-
-        """
-        try:
-            code_owner = get_code_owner_from_module('*')
-            return code_owner
-        except Exception:  # pylint: disable=broad-except; #pragma: no cover
-            return None
+    def __init__(self, *args, **kwargs):
+        super().__init__(*args, **kwargs)
+        msg = "Use 'edx_django_utils.monitoring.CodeOwnerMonitoringMiddleware' in place of " \
+              "'edx_django_utils.monitoring.code_owner.middleware.CodeOwnerMonitoringMiddleware'."
+        warnings.warn(msg, DeprecationWarning)
+        set_custom_attribute('deprecated_code_owner_middleware', 'CodeOwnerMonitoringMiddleware')
 
 
-class CodeOwnerMetricMiddleware(CodeOwnerMonitoringMiddleware):
+class CodeOwnerMetricMiddleware(InternalCodeOwnerMonitoringMiddleware):
     """
     Deprecated class for handling middleware. Class has been renamed to CodeOwnerMonitoringMiddleware.
     """
     def __init__(self, *args, **kwargs):  # pragma: no cover
-        super(CodeOwnerMetricMiddleware, self).__init__(*args, **kwargs)
-        msg = "Use 'CodeOwnerMonitoringMiddleware' in place of 'CodeOwnerMetricMiddleware'."
+        super().__init__(*args, **kwargs)
+        msg = "Use 'edx_django_utils.monitoring.CodeOwnerMonitoringMiddleware' in place of " \
+              "'edx_django_utils.monitoring.code_owner.middleware.CodeOwnerMetricMiddleware'."
         warnings.warn(msg, DeprecationWarning)
+        set_custom_attribute('deprecated_code_owner_middleware', 'CodeOwnerMetricMiddleware')

edx_django_utils/monitoring/docs/how_tos/add_code_owner_custom_attribute_to_an_ida.rst

@@ -19,7 +19,7 @@ If you want to know about custom attributes in general, see: using_custom_attrib
 Setting up the Middleware
 -------------------------
 
-You simply need to add ``edx_django_utils.monitoring.code_owner.middleware.CodeOwnerMonitoringMiddleware`` as described in the README to make this functionality available. Then it is ready to be configured.
+You simply need to add ``edx_django_utils.monitoring.CodeOwnerMonitoringMiddleware`` as described in the README to make this functionality available. Then it is ready to be configured.
 
 Configuring your app settings
 -----------------------------