Merge pull request #73 from edx/robrap/ARCHBOM-1260-code-owner-decorator

ARCHBOM-1260: enable code_owner for celery tasks

Author: robrap

CHANGELOG.rst

@@ -15,6 +15,21 @@ Change Log
 Unreleased
 ~~~~~~~~~~
 
+[3.12.0] - 2020-11-17
+~~~~~~~~~~~~~~~~~~~~~
+
+Added
+_____
+
+* Added set_code_owner_attribute decorator for use with celery tasks.
+* Added set_code_owner_attribute_from_module as an alternative to the decorator.
+
+Updated
+_______
+
+* Cleaned up some of the code owner middleware code. In doing so, renamed custom attribute code_owner_path_module to code_owner_module. This may affect monitoring dashboards. Also slightly changed when error custom attributes are set.
+
+
 [3.11.0] - 2020-10-31
 ~~~~~~~~~~~~~~~~~~~~~
 

edx_django_utils/__init__.py

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

edx_django_utils/monitoring/__init__.py

@@ -4,7 +4,11 @@
 See README.rst for details.
 """
 from .internal.code_owner.middleware import CodeOwnerMonitoringMiddleware
-from .internal.code_owner.utils import get_code_owner_from_module
+from .internal.code_owner.utils import (
+    get_code_owner_from_module,
+    set_code_owner_attribute,
+    set_code_owner_attribute_from_module
+)
 from .internal.middleware import CachedCustomMonitoringMiddleware, MonitoringMemoryMiddleware
 from .internal.transactions import (
     function_trace,

edx_django_utils/monitoring/docs/how_tos/add_code_owner_custom_attribute_to_an_ida.rst

@@ -21,6 +21,25 @@ Setting up the Middleware
 
 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.
 
+Handling celery tasks
+---------------------
+
+Celery tasks require use of a special decorator to set the ``code_owner`` custom attribute because no middleware will be run.
+
+Here is an example::
+
+  @task()
+  @set_code_owner_attribute
+  def example_task():
+      ...
+
+If the task is not compatible with additional decorators, you can use the following alternative::
+
+  @task()
+  def example_task():
+      set_code_owner_attribute_from_module(__name__)
+      ...
+
 Configuring your app settings
 -----------------------------
 

edx_django_utils/monitoring/internal/code_owner/middleware.py

@@ -4,10 +4,11 @@
 import logging
 
 from django.urls import resolve
+from django.urls.exceptions import Resolver404
 
 from ..transactions import get_current_transaction
 from ..utils import set_custom_attribute
-from .utils import get_code_owner_from_module, is_code_owner_mappings_configured
+from .utils import _get_catch_all_code_owner, get_code_owner_from_module, is_code_owner_mappings_configured
 
 log = logging.getLogger(__name__)
 
@@ -21,10 +22,8 @@ class CodeOwnerMonitoringMiddleware:
 
     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_module: The module found from the request or current transaction.
     - 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.
@@ -43,105 +42,91 @@ def process_exception(self, request, exception):
 
     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.
-
+        Sets the code_owner custom attribute for the request.
         """
-        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()
+        code_owner = None
+        module = self._get_module_from_request(request)
+        if module:
+            code_owner = get_code_owner_from_module(module)
+        if not code_owner:
+            code_owner = _get_catch_all_code_owner()
+
         if code_owner:
             set_custom_attribute('code_owner', code_owner)
-            return
 
-        # only report errors if code_owner couldn't be found, including catch-all
+    def _get_module_from_request(self, request):
+        """
+        Get the module from the request path or the current transaction.
+
+        Side-effects:
+            Sets code_owner_module custom attribute, used to determine code_owner.
+            If module was not found, may set code_owner_path_error and/or
+                code_owner_transaction_error custom attributes if applicable.
+
+        Returns:
+            str: module name or None if not found
+
+        """
+        if not is_code_owner_mappings_configured():
+            return None
+
+        module, path_error = self._get_module_from_request_path(request)
+        if module:
+            set_custom_attribute('code_owner_module', module)
+            return module
+
+        module, transaction_error = self._get_module_from_current_transaction()
+        if module:
+            set_custom_attribute('code_owner_module', module)
+            return module
+
+        # monitor errors if module was not found
         if path_error:
             set_custom_attribute('code_owner_path_error', path_error)
         if transaction_error:
             set_custom_attribute('code_owner_transaction_error', transaction_error)
+        return None
 
-    def _set_code_owner_attribute_from_path(self, request):
+    def _get_module_from_request_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
+        Uses the request path to get the view_func module.
 
         Returns:
-            (str, str): (code_owner, error_message), where at least one of these should be None
+            (str, str): (module, 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
+            module = view_func.__module__
+            return module, None
+        # TODO: Replace ImportError with ModuleNotFoundError when Python 3.5 support is dropped.
+        except (ImportError, Resolver404) as e:
+            return None, str(e)
+        except Exception as e:  # pylint: disable=broad-except; #pragma: no cover
+            # will remove broad exceptions after ensuring all proper cases are covered
+            set_custom_attribute('deprecated_broad_except__get_module_from_request_path', e.__class__)
             return None, str(e)
 
-    def _set_code_owner_attribute_from_current_transaction(self, request):
+    def _get_module_from_current_transaction(self):
         """
-        Uses the current transaction name to set the code owner attribute.
+        Uses the current transaction to get the module.
 
         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
+            (str, str): (module, 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]
+            module = 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
+            return module, None
         except Exception as e:  # pylint: disable=broad-except
+            # will remove broad exceptions after ensuring all proper cases are covered
+            set_custom_attribute('deprecated_broad_except___get_module_from_current_transaction', e.__class__)
             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

edx_django_utils/monitoring/internal/code_owner/utils.py

@@ -3,9 +3,12 @@
 """
 import logging
 import re
+from functools import wraps
 
 from django.conf import settings
 
+from ..utils import set_custom_attribute
+
 log = logging.getLogger(__name__)
 
 
@@ -22,6 +25,9 @@ def get_code_owner_from_module(module):
     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
 
     """
+    if not module:
+        return None
+
     code_owner_mappings = get_code_owner_mappings()
     if code_owner_mappings is None:
         return None
@@ -97,12 +103,80 @@ def get_code_owner_mappings():
                     path_without_prefix = path[optional_module_prefix_match.end():]
                     path_to_code_owner_mapping[path_without_prefix] = code_owner
     except Exception as e:  # pylint: disable=broad-except
+        # will remove broad exceptions after ensuring all proper cases are covered
+        set_custom_attribute('deprecated_broad_except_get_code_owner_mappings', e.__class__)
         log.exception('Error processing code_owner_mappings. {}'.format(e))
 
     _PATH_TO_CODE_OWNER_MAPPINGS = path_to_code_owner_mapping
     return _PATH_TO_CODE_OWNER_MAPPINGS
 
 
+def _get_catch_all_code_owner():
+    """
+    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 as e:  # pylint: disable=broad-except; #pragma: no cover
+        # will remove broad exceptions after ensuring all proper cases are covered
+        set_custom_attribute('deprecated_broad_except___get_module_from_current_transaction', e.__class__)
+        return None
+
+
+def set_code_owner_attribute_from_module(module):
+    """
+    Updates the code_owner and code_owner_module custom attributes.
+
+    Celery tasks or other non-web functions do not use middleware, so we need
+        an alternative way to set the code_owner custom attribute.
+
+    Note: These settings will be overridden by the CodeOwnerMonitoringMiddleware.
+        This method can't be used to override web functions at this time.
+
+    Usage::
+
+        set_code_owner_attribute_from_module(__name__)
+
+    """
+    set_custom_attribute('code_owner_module', module)
+    code_owner = get_code_owner_from_module(module)
+    if not code_owner:
+        code_owner = _get_catch_all_code_owner()
+
+    if code_owner:
+        set_custom_attribute('code_owner', code_owner)
+
+
+def set_code_owner_attribute(wrapped_function):
+    """
+    Decorator to set the code_owner and code_owner_module custom attributes.
+
+    Celery tasks or other non-web functions do not use middleware, so we need
+        an alternative way to set the code_owner custom attribute.
+
+    Usage::
+
+        @task()
+        @set_code_owner_attribute
+        def example_task():
+            ...
+
+    Note: If the decorator can't be used for some reason, just call
+        ``set_code_owner_attribute_from_module`` directly.
+
+    """
+    @wraps(wrapped_function)
+    def new_function(*args, **kwargs):
+        set_code_owner_attribute_from_module(wrapped_function.__module__)
+        return wrapped_function(*args, **kwargs)
+    return new_function
+
+
 def clear_cached_mappings():
     """
     Clears the cached path to code owner mappings. Useful for testing.
@@ -111,6 +185,6 @@ def clear_cached_mappings():
     _PATH_TO_CODE_OWNER_MAPPINGS = None
 
 
-# TODO: Remove this LMS specific configuration by replacing with a Django Setting named
-#    CODE_OWNER_OPTIONAL_MODULE_PREFIXES that takes a list of module prefixes (without the final period).
+# TODO: Retire this once edx-platform import_shims is no longer used.
+#   See https://github.com/edx/edx-platform/tree/854502b560bda74ef898501bb2a95ce238cf794c/import_shims
 _OPTIONAL_MODULE_PREFIX_PATTERN = re.compile(r'^(lms|common|openedx\.core)\.djangoapps\.')