diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 2aea9c68..5b60c7b0 100644 --- a/CHANGELOG.rst +++ b/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 ~~~~~~~~~~~~~~~~~~~~~ diff --git a/edx_django_utils/__init__.py b/edx_django_utils/__init__.py index 3a6a53e8..6654226c 100644 --- a/edx_django_utils/__init__.py +++ b/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" diff --git a/edx_django_utils/monitoring/__init__.py b/edx_django_utils/monitoring/__init__.py index e9746f26..a929a1b2 100644 --- a/edx_django_utils/monitoring/__init__.py +++ b/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, diff --git a/edx_django_utils/monitoring/docs/how_tos/add_code_owner_custom_attribute_to_an_ida.rst b/edx_django_utils/monitoring/docs/how_tos/add_code_owner_custom_attribute_to_an_ida.rst index a8502939..d56ae89b 100644 --- a/edx_django_utils/monitoring/docs/how_tos/add_code_owner_custom_attribute_to_an_ida.rst +++ b/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 ----------------------------- diff --git a/edx_django_utils/monitoring/internal/code_owner/middleware.py b/edx_django_utils/monitoring/internal/code_owner/middleware.py index c324b2ea..916a8122 100644 --- a/edx_django_utils/monitoring/internal/code_owner/middleware.py +++ b/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 diff --git a/edx_django_utils/monitoring/internal/code_owner/utils.py b/edx_django_utils/monitoring/internal/code_owner/utils.py index 6b10c9ad..ac7f4675 100644 --- a/edx_django_utils/monitoring/internal/code_owner/utils.py +++ b/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\.') diff --git a/edx_django_utils/monitoring/tests/code_owner/test_middleware.py b/edx_django_utils/monitoring/tests/code_owner/test_middleware.py index b0ab69d8..79cdd049 100644 --- a/edx_django_utils/monitoring/tests/code_owner/test_middleware.py +++ b/edx_django_utils/monitoring/tests/code_owner/test_middleware.py @@ -212,10 +212,12 @@ def test_no_resolver_for_path_and_no_transaction(self, mock_set_custom_attribute CODE_OWNER_MAPPINGS={'team-red': ['*']}, ) @patch('edx_django_utils.monitoring.internal.code_owner.middleware.set_custom_attribute') - def test_catch_all_instead_of_errors(self, mock_set_custom_attribute): + def test_catch_all_with_errors(self, mock_set_custom_attribute): request = RequestFactory().get('/bad/path/') self.middleware(request) - self._assert_code_owner_custom_attributes(mock_set_custom_attribute, expected_code_owner='team-red') + self._assert_code_owner_custom_attributes( + mock_set_custom_attribute, has_path_error=True, has_transaction_error=True, expected_code_owner='team-red' + ) @override_settings( CODE_OWNER_MAPPINGS=['invalid_setting_as_list'], @@ -238,15 +240,21 @@ def _assert_code_owner_custom_attributes(self, mock_set_custom_attribute, expect if expected_code_owner: call_list.append(call('code_owner', expected_code_owner)) if path_module: - call_list.append(call('code_owner_path_module', path_module)) + call_list.append(call('code_owner_module', path_module)) if has_path_error: call_list.append(call('code_owner_path_error', ANY)) if transaction_name: call_list.append(call('code_owner_transaction_name', transaction_name)) if has_transaction_error: call_list.append(call('code_owner_transaction_error', ANY)) + # TODO: Remove this list filtering once the ``deprecated_broad_except_XXX`` custom attributes have been removed. + actual_filtered_call_list = [ + mock_call + for mock_call in mock_set_custom_attribute.call_args_list + if not mock_call[0][0].startswith('deprecated_') + ] mock_set_custom_attribute.assert_has_calls(call_list, any_order=True) self.assertEqual( - len(mock_set_custom_attribute.call_args_list), len(call_list), - 'Expected calls {} vs actual calls {}'.format(call_list, mock_set_custom_attribute.call_args_list) + len(actual_filtered_call_list), len(call_list), + 'Expected calls {} vs actual calls {}'.format(call_list, actual_filtered_call_list) ) diff --git a/edx_django_utils/monitoring/tests/code_owner/test_utils.py b/edx_django_utils/monitoring/tests/code_owner/test_utils.py index 38c2cc95..2e215a41 100644 --- a/edx_django_utils/monitoring/tests/code_owner/test_utils.py +++ b/edx_django_utils/monitoring/tests/code_owner/test_utils.py @@ -6,12 +6,24 @@ import ddt from django.test import override_settings -from mock import patch +from mock import call, patch -from edx_django_utils.monitoring import get_code_owner_from_module +from edx_django_utils.monitoring import ( + get_code_owner_from_module, + set_code_owner_attribute, + set_code_owner_attribute_from_module +) from edx_django_utils.monitoring.internal.code_owner.utils import clear_cached_mappings +@set_code_owner_attribute +def decorated_function(pass_through): + """ + For testing the set_code_owner_attribute decorator. + """ + return pass_through + + @ddt.ddt class MonitoringUtilsTests(TestCase): """ @@ -59,6 +71,9 @@ def test_code_owner_mapping_with_invalid_dict(self, mock_logger): def test_code_owner_mapping_with_no_settings(self): self.assertIsNone(get_code_owner_from_module('xblock')) + def test_code_owner_mapping_with_no_module(self): + self.assertIsNone(get_code_owner_from_module(None)) + def test_mapping_performance(self): code_owner_mappings = { 'team-red': [] @@ -75,3 +90,43 @@ def test_mapping_performance(self): ) average_time = time / call_iterations self.assertLess(average_time, 0.0005, 'Mapping takes {}s which is too slow.'.format(average_time)) + + @override_settings(CODE_OWNER_MAPPINGS={ + 'team-red': ['edx_django_utils.monitoring.tests.code_owner.test_utils'] + }) + @patch('edx_django_utils.monitoring.internal.code_owner.utils.set_custom_attribute') + def test_set_code_owner_attribute_success(self, mock_set_custom_attribute): + self.assertEqual(decorated_function('test'), 'test') + self._assert_set_custom_attribute(mock_set_custom_attribute, code_owner='team-red', module=__name__) + + @override_settings(CODE_OWNER_MAPPINGS={ + 'team-red': ['*'] + }) + @patch('edx_django_utils.monitoring.internal.code_owner.utils.set_custom_attribute') + def test_set_code_owner_attribute_catch_all(self, mock_set_custom_attribute): + self.assertEqual(decorated_function('test'), 'test') + self._assert_set_custom_attribute(mock_set_custom_attribute, code_owner='team-red', module=__name__) + + @patch('edx_django_utils.monitoring.internal.code_owner.utils.set_custom_attribute') + def test_set_code_owner_attribute_no_mappings(self, mock_set_custom_attribute): + self.assertEqual(decorated_function('test'), 'test') + self._assert_set_custom_attribute(mock_set_custom_attribute, code_owner=None, module=__name__) + + @override_settings(CODE_OWNER_MAPPINGS={ + 'team-red': ['edx_django_utils.monitoring.tests.code_owner.test_utils'] + }) + @patch('edx_django_utils.monitoring.internal.code_owner.utils.set_custom_attribute') + def test_set_code_owner_attribute_from_module_success(self, mock_set_custom_attribute): + set_code_owner_attribute_from_module(__name__) + self._assert_set_custom_attribute(mock_set_custom_attribute, code_owner='team-red', module=__name__) + + def _assert_set_custom_attribute(self, mock_set_custom_attribute, code_owner, module): + """ + Helper to assert that the proper set_custom_metric calls were made. + """ + call_list = [] + if code_owner: + call_list.append(call('code_owner', code_owner)) + if module: + call_list.append(call('code_owner_module', module)) + mock_set_custom_attribute.assert_has_calls(call_list, any_order=True)