Merge pull request #118 from edx/robrap/code-owner-squad-and-theme

ARCHBOM-1759: code owner squad and theme and other cleanup

Author: robrap

CHANGELOG.rst

@@ -14,6 +14,23 @@ Change Log
 Unreleased
 ----------
 
+[4.0.0] = 2021-04-30
+
+Removed
+-------
+
+* Removed the old location of ``CodeOwnerMonitoringMiddleware``. It had moved in a past commit. Although technically a breaking change, all references in the Open edX platform have already been updated to point to the new location.
+
+Added
+-----
+
+* Added new ``code_owner_theme`` and ``code_owner_squad`` custom attributes. This is useful in cases where the ``code_owner`` combines a theme and squad name, because monitoring can instead reference ``code_owner_squad`` to be resilient to theme name updates. For the decision doc, see edx_django_utils/monitoring/docs/decisions/0004-code-owner-theme-and-squad.rst.
+
+Updated
+-------
+
+* Misconfigurations of CODE_OWNER_MAPPINGS will now fail fast, rather than just logging. Although technically a breaking change, if CODE_OWNER_MAPPINGS is in use, it is probably correctly configured and this change should be a no-op.
+
 [3.16.0] - 2021-03-24
 ---------------------
 

edx_django_utils/__init__.py

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

edx_django_utils/monitoring/code_owner/middleware.py

@@ -0,0 +1,30 @@
+Code Owner Theme and Squad
+==========================
+
+Status
+------
+
+Accepted
+
+Context
+-------
+
+As detailed in the `Monitoring by Code Owner ADR`_, we added a ``code_owner`` custom attribute for monitoring by code owner. The value for this attribute had the format 'theme-squad'.
+
+The problems with this configuration is that for theme name changes, or when squads transfer themes, any monitoring referencing the full name would also need to be updated.
+
+.. _Monitoring by Code Owner ADR: https://github.com/edx/edx-platform/blob/master/lms/djangoapps/monitoring/docs/decisions/0001-monitoring-by-code-owner.rst
+
+Decision
+--------
+
+We will add a ``code_owner_squad`` custom attribute. Monitoring will now be able to refer to ``code_owner_squad`` with a value of 'squad', and will be unaffected by theme name changes.
+
+Additionally, we are adding ``code_owner_theme`` for similar convenience if there is a need for theme-based monitoring.
+
+We will leave the original ``code_owner`` custom attribute for backward compatability, and for cases where theme and squad are not used.
+
+Consequences
+------------
+
+* Theme name changes may now result in a one time fix for those that were relying on ``code_owner``. Monitoring can switch from ``code_owner`` to ``code_owner_squad``, rather than a change for every theme update in the future.

edx_django_utils/monitoring/docs/decisions/0004-code-owner-theme-and-squad.rst

@@ -5,14 +5,20 @@ Add Code_Owner Custom Attribute to an IDA
    :local:
    :depth: 2
 
-What is the code_owner custom attribute?
-----------------------------------------
+What are the code owner custom attributes?
+------------------------------------------
 
-The code_owner custom attribute can be used to create custom dashboards and alerts for monitoring the things that you own. It was originally introduced for the LMS, as is described in this `ADR on monitoring by code owner`_.
+The code owner custom attributes can be used to create custom dashboards and alerts for monitoring the things that you own. It was originally introduced for the LMS, as is described in this `ADR on monitoring by code owner`_.
+
+The code owner custom attributes consist of:
+
+* code_owner: The owner name. When themes and squads are used, this will be the theme and squad names joined by a hyphen.
+* code_owner_theme: The theme name of the owner.
+* code_owner_squad: The squad name of the owner. Use this to avoid issues when theme name changes.
 
 You can now easily add this same attribute to any IDA so that your dashboards and alerts can work across multiple IDAs at once.
 
-If you want to know about custom attributes in general, see: using_custom_attributes.rst.
+If you want to know about custom attributes in general, see :doc:`using_custom_attributes`.
 
 .. _ADR on monitoring by code owner: https://github.com/edx/edx-platform/blob/master/lms/djangoapps/monitoring/docs/decisions/0001-monitoring-by-code-owner.rst
 
@@ -24,7 +30,7 @@ You simply need to add ``edx_django_utils.monitoring.CodeOwnerMonitoringMiddlewa
 Handling celery tasks
 ---------------------
 
-Celery tasks require use of a special decorator to set the ``code_owner`` custom attribute because no middleware will be run.
+Celery tasks require use of a special decorator to set the ``code_owner`` custom attributes because no middleware will be run.
 
 Here is an example::
 
@@ -47,23 +53,29 @@ An untested potential alternative to the decorator is documented in the `Code Ow
 Configuring your app settings
 -----------------------------
 
-Once the Middleware is made available, simply set the Django Setting ``CODE_OWNER_MAPPINGS`` appropriately.
+Once the Middleware is made available, simply set the Django Settings ``CODE_OWNER_MAPPINGS`` and ``CODE_OWNER_THEMES`` appropriately.
 
 The following example shows how you can include an optional config for a catch-all using ``'*'``. Although you might expect this example to use Python, it is intentionally illustrated in YAML because the catch-all requires special care in YAML.
 
 ::
 
     # YAML format of example CODE_OWNER_MAPPINGS
     CODE_OWNER_MAPPINGS:
-      team-red:
+      theme-x-team-red:
         - xblock_django
         - openedx.core.djangoapps.xblock
-      team-blue:
+      theme-x-team-blue:
       - '*'  # IMPORTANT: you must surround * with quotes in yml
 
+    # YAML format of example CODE_OWNER_THEMES
+    CODE_OWNER_THEMES:
+      theme-x:
+      - theme-x-team-red
+      - theme-x-team-blue
+
 How to find and fix code_owner mappings
 ---------------------------------------
 
-If you are missing the `code_owner` custom attribute on a particular Transaction or Error, or if `code_owner` is matching the catch-all, but you want to add a more specific mapping, you can use the other `code_owner supporting attributes`_ to determine what the appropriate mappings should be.
+If you are missing the `code_owner` custom attributes on a particular Transaction or Error, or if `code_owner` is matching the catch-all, but you want to add a more specific mapping, you can use the other `code_owner supporting attributes`_ to determine what the appropriate mappings should be.
 
 .. _code_owner supporting attributes: https://github.com/edx/edx-django-utils/blob/7db8301af21760f8bca188b3c6c95a8ae873baf7/edx_django_utils/monitoring/code_owner/middleware.py#L28-L34

edx_django_utils/monitoring/docs/how_tos/add_code_owner_custom_attribute_to_an_ida.rst

@@ -8,7 +8,12 @@
 
 from ..transactions import get_current_transaction
 from ..utils import set_custom_attribute
-from .utils import _get_catch_all_code_owner, 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,
+    set_code_owner_custom_attributes
+)
 
 log = logging.getLogger(__name__)
 
@@ -52,7 +57,7 @@ def _set_code_owner_attribute(self, request):
             code_owner = _get_catch_all_code_owner()
 
         if code_owner:
-            set_custom_attribute('code_owner', code_owner)
+            set_code_owner_custom_attributes(code_owner)
 
     def _get_module_from_request(self, request):
         """

edx_django_utils/monitoring/internal/code_owner/middleware.py

@@ -29,7 +29,7 @@ def get_code_owner_from_module(module):
         return None
 
     code_owner_mappings = get_code_owner_mappings()
-    if code_owner_mappings is None:
+    if not code_owner_mappings:
         return None
 
     module_parts = module.split('.')
@@ -88,9 +88,7 @@ def get_code_owner_mappings():
     # .. setting_description: Used for monitoring and reporting of ownership. Use a
     #      dict with keys of code owner name and value as a list of dotted path
     #      module names owned by the code owner.
-    code_owner_mappings = getattr(settings, 'CODE_OWNER_MAPPINGS', None)
-    if code_owner_mappings is None:
-        return None
+    code_owner_mappings = getattr(settings, 'CODE_OWNER_MAPPINGS', {})
 
     try:
         for code_owner in code_owner_mappings:
@@ -102,10 +100,9 @@ def get_code_owner_mappings():
                 if optional_module_prefix_match:
                     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))
+    except TypeError as e:
+        log.exception('Error processing CODE_OWNER_MAPPINGS. {}'.format(e))
+        raise e
 
     _PATH_TO_CODE_OWNER_MAPPINGS = path_to_code_owner_mapping
     return _PATH_TO_CODE_OWNER_MAPPINGS
@@ -149,7 +146,22 @@ def set_code_owner_attribute_from_module(module):
         code_owner = _get_catch_all_code_owner()
 
     if code_owner:
-        set_custom_attribute('code_owner', code_owner)
+        set_code_owner_custom_attributes(code_owner)
+
+
+def set_code_owner_custom_attributes(code_owner):
+    """
+    Sets custom metrics for code_owner, code_owner_theme, and code_owner_squad
+    """
+    if not code_owner:  # pragma: no cover
+        return
+    set_custom_attribute('code_owner', code_owner)
+    theme = _get_theme_from_code_owner(code_owner)
+    if theme:
+        set_custom_attribute('code_owner_theme', theme)
+    squad = _get_squad_from_code_owner(code_owner)
+    if squad:
+        set_custom_attribute('code_owner_squad', squad)
 
 
 def set_code_owner_attribute(wrapped_function):
@@ -182,12 +194,108 @@ def new_function(*args, **kwargs):
 
 def clear_cached_mappings():
     """
-    Clears the cached path to code owner mappings. Useful for testing.
+    Clears the cached code owner mappings. Useful for testing.
     """
     global _PATH_TO_CODE_OWNER_MAPPINGS
     _PATH_TO_CODE_OWNER_MAPPINGS = None
+    global _CODE_OWNER_TO_THEME_AND_SQUAD_MAPPINGS
+    _CODE_OWNER_TO_THEME_AND_SQUAD_MAPPINGS = None
 
 
 # TODO: Retire this once edx-platform import_shims is no longer used.
+#   Note: This should be ready for removal because import_shims has been removed.
 #   See https://github.com/edx/edx-platform/tree/854502b560bda74ef898501bb2a95ce238cf794c/import_shims
 _OPTIONAL_MODULE_PREFIX_PATTERN = re.compile(r'^(lms|common|openedx\.core)\.djangoapps\.')
+
+
+# Cached lookup table for code owner theme and squad given a code owner.
+# - Although code owner is "theme-squad", a hyphen may also be in the theme or squad name, so this ensures we get both
+#   correctly from config.
+# Do not access this directly, but instead use get_code_owner_theme_squad_mappings.
+_CODE_OWNER_TO_THEME_AND_SQUAD_MAPPINGS = None
+
+
+def get_code_owner_theme_squad_mappings():
+    """
+    Returns the contents of the CODE_OWNER_THEMES Django Setting, processed
+    for efficient lookup by path.
+
+    Returns:
+         (dict): dict mapping code owners to a dict containing the squad and theme, or
+            an empty dict if there are no configured mappings.
+
+    Example return value::
+
+        {
+            'theme-x-team-red': {
+                'theme': 'theme-x',
+                'squad': 'team-red',
+            },
+            'theme-x-team-blue': {
+                'theme': 'theme-x',
+                'squad': 'team-blue',
+            },
+        }
+
+    """
+    global _CODE_OWNER_TO_THEME_AND_SQUAD_MAPPINGS
+
+    # Return cached processed mappings if already processed
+    if _CODE_OWNER_TO_THEME_AND_SQUAD_MAPPINGS is not None:
+        return _CODE_OWNER_TO_THEME_AND_SQUAD_MAPPINGS
+
+    # Uses temporary variable to build mappings to avoid multi-threading issue with a partially
+    # processed map.  Worst case, it is processed more than once at start-up.
+    code_owner_to_theme_and_squad_mapping = {}
+
+    # .. setting_name: CODE_OWNER_THEMES
+    # .. setting_default: None
+    # .. setting_description: Used for monitoring and reporting of ownership. Use a
+    #      dict with keys of code owner themes and values as a list of code owner names
+    #      including theme and squad, separated with a hyphen.
+    code_owner_themes = getattr(settings, 'CODE_OWNER_THEMES', {})
+
+    try:
+        for theme in code_owner_themes:
+            code_owner_list = code_owner_themes[theme]
+            for code_owner in code_owner_list:
+                squad = code_owner.split(theme + '-', 1)[1]
+                code_owner_details = {
+                    'theme': theme,
+                    'squad': squad,
+                }
+                code_owner_to_theme_and_squad_mapping[code_owner] = code_owner_details
+    except TypeError as e:
+        log.exception('Error processing CODE_OWNER_THEMES setting. {}'.format(e))
+        raise e
+
+    _CODE_OWNER_TO_THEME_AND_SQUAD_MAPPINGS = code_owner_to_theme_and_squad_mapping
+    return _CODE_OWNER_TO_THEME_AND_SQUAD_MAPPINGS
+
+
+def _get_theme_from_code_owner(code_owner):
+    """
+    Returns theme for a code_owner (e.g. 'theme-my-squad' => 'theme')
+    """
+    mappings = get_code_owner_theme_squad_mappings()
+    if mappings is None:  # pragma: no cover
+        return None
+
+    if code_owner in mappings:
+        return mappings[code_owner]['theme']
+
+    return None
+
+
+def _get_squad_from_code_owner(code_owner):
+    """
+    Returns squad for a code_owner (e.g. 'theme-my-squad' => 'my-squad')
+    """
+    mappings = get_code_owner_theme_squad_mappings()
+    if mappings is None:  # pragma: no cover
+        return None
+
+    if code_owner in mappings:
+        return mappings[code_owner]['squad']
+
+    return None