Andal.LND/edx-platform
6
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Refactor WaffleFlag and WaffleFlagNamespace internal API

Browse Source 
This simplifies the internals of the waffle flag classes in order to
better move them to edx-toggles later.
This commit is contained in:
Régis Behmo
2020-10-07 19:12:24 +02:00
parent 24cf0543f3
commit 474da0c5a5
3 changed files with 217 additions and 224 deletions
Download Patch File Download Diff File Expand all files Collapse all files

343
openedx/core/djangoapps/waffle_utils/__init__.py
View File

@@ -52,7 +52,7 @@ Also see ``WAFFLE_FLAG_CUSTOM_ATTRIBUTES`` and docstring for _set_waffle_flag_at
for temporarily instrumenting/monitoring waffle flag usage.
"""
from functools import lru_cache
import logging
import re
import traceback
@@ -61,7 +61,6 @@ from contextlib import contextmanager
from weakref import WeakSet
import crum
import six
from django.conf import settings
from edx_django_utils.monitoring import set_custom_attribute
from opaque_keys.edx.keys import CourseKey
@@ -76,29 +75,31 @@ from edx_toggles.toggles import WaffleSwitchNamespace as BaseWaffleSwitchNamespa
log = logging.getLogger(__name__)
class RequestCacheMixin:
@staticmethod
def _get_request_cache():
"""
Returns a request cache shared by all instances of this class.
"""
return get_request_cache("WaffleNamespace")
class WaffleSwitchNamespace(BaseWaffleSwitchNamespace):
"""
A waffle switch namespace class that implements request-based caching.
"""
class WaffleSwitchNamespace(BaseWaffleSwitchNamespace, RequestCacheMixin):
@property
def _cached_switches(self):
"""
Returns a dictionary of all namespaced switches in the request cache.
"""
return self._get_request_cache().setdefault("switches", {})
return _get_waffle_namespace_request_cache().setdefault("switches", {})
def _get_waffle_namespace_request_cache():
"""
Returns a request cache shared by all Waffle namespace objects.
"""
return get_request_cache("WaffleNamespace")
class WaffleSwitch(BaseWaffleSwitch):
NAMESPACE_CLASS = WaffleSwitchNamespace
class WaffleFlagNamespace(BaseNamespace, RequestCacheMixin):
class WaffleFlagNamespace(BaseNamespace):
"""
Provides a single namespace for a set of waffle flags.
@@ -111,20 +112,13 @@ class WaffleFlagNamespace(BaseNamespace, RequestCacheMixin):
"""
Returns a dictionary of all namespaced flags in the request cache.
"""
return self._get_request_cache().setdefault("flags", {})
return _get_waffle_namespace_request_cache().setdefault("flags", {})
def is_flag_active(self, flag_name, check_before_waffle_callback=None):
def is_flag_active(self, flag_name):
"""
Returns and caches whether the provided flag is active.
If the flag value is already cached in the request, it is returned.
If check_before_waffle_callback is supplied, it is called before
checking waffle.
If check_before_waffle_callback returns None, or if it is not supplied,
then waffle is used to check the flag.
Important: Caching for the check_before_waffle_callback must be handled
by the callback itself.
Note: A waffle flag's default is False if not defined. If you think you
need the default to be True, see the module docstring for
@@ -132,121 +126,50 @@ class WaffleFlagNamespace(BaseNamespace, RequestCacheMixin):
Arguments:
flag_name (String): The name of the flag to check.
check_before_waffle_callback (function): (Optional) A function that
will be checked before continuing on to waffle. If
check_before_waffle_callback(namespaced_flag_name) returns True
or False, it is returned. If it returns None, then waffle is
used.
"""
# validate arguments
namespaced_flag_name = self._namespaced_name(flag_name)
if check_before_waffle_callback:
value = check_before_waffle_callback(namespaced_flag_name)
if value is not None:
# Do not cache value for the callback, because the key might be different.
# The callback needs to handle its own caching if it wants it.
self._set_waffle_flag_attribute(namespaced_flag_name, value)
return value
value = self._cached_flags.get(namespaced_flag_name)
if value is not None:
self._set_waffle_flag_attribute(namespaced_flag_name, value)
return value
request = crum.get_current_request()
if not request:
log.warning(
u"%sFlag '%s' accessed without a request",
self.log_prefix,
namespaced_flag_name,
)
# Return the Flag's Everyone value if not in a request context.
# Note: this skips the cache as the value might be different
# in a normal request context. This case seems to occur when
# a page redirects to a 404, or for celery workers.
value = self._is_flag_active_for_everyone(namespaced_flag_name)
self._set_waffle_flag_attribute(namespaced_flag_name, value)
set_custom_attribute("warn_flag_no_request_return_value", value)
return value
value = flag_is_active(request, namespaced_flag_name)
self._cached_flags[namespaced_flag_name] = value
self._set_waffle_flag_attribute(namespaced_flag_name, value)
value = self._get_flag_active(namespaced_flag_name)
_set_waffle_flag_attribute(namespaced_flag_name, value)
return value
def _is_flag_active_for_everyone(self, namespaced_flag_name):
def _get_flag_active(self, namespaced_flag_name):
"""
Returns True if the waffle flag is configured as active for Everyone,
False otherwise.
Return the value of the flag activation without touching the _waffle_flag_attribute.
"""
# Import is placed here to avoid model import at project startup.
from waffle.models import Flag
# Check namespace cache
value = self._cached_flags.get(namespaced_flag_name)
if value is not None:
return value
try:
waffle_flag = Flag.objects.get(name=namespaced_flag_name)
return waffle_flag.everyone is True
except Flag.DoesNotExist:
return False
# Check in context of request
request = crum.get_current_request()
value = self._get_flag_active_request(namespaced_flag_name, request)
if value is not None:
return value
def _set_waffle_flag_attribute(self, name, value):
# Check value in the absence of any context
log.warning(
u"%sFlag '%s' accessed without a request",
self.log_prefix,
namespaced_flag_name,
)
# Return the Flag's Everyone value if not in a request context.
# Note: this skips the cache as the value might be different
# in a normal request context. This case seems to occur when
# a page redirects to a 404, or for celery workers.
value = is_flag_active_for_everyone(namespaced_flag_name)
set_custom_attribute("warn_flag_no_request_return_value", value)
return value
def _get_flag_active_request(self, namespaced_flag_name, request):
"""
For any flag name in _WAFFLE_FLAG_CUSTOM_ATTRIBUTE_SET, add name/value
to cached values and set custom attribute if the value changed.
The name of the custom attribute will have the prefix ``flag_`` and the
suffix will match the name of the flag.
The value of the custom attribute could be False, True, or Both.
The value Both would mean that the flag had both a True and False
value at different times during the transaction. This is most
likely due to having a check_before_waffle_callback, as is the
case with CourseWaffleFlag.
An example NewRelic query to see the values of a flag in different
environments, if your waffle flag was named ``my.waffle.flag`` might
look like::
SELECT count(*) FROM Transaction
WHERE flag_my.waffle.flag IS NOT NULL
FACET appName, flag_my.waffle.flag
Important: Remember to configure ``WAFFLE_FLAG_CUSTOM_ATTRIBUTES`` for
LMS, Studio and Workers in order to see waffle flag usage in all
edx-platform environments.
Get flag value in the context of the current request.
"""
if name not in _WAFFLE_FLAG_CUSTOM_ATTRIBUTE_SET:
return
flag_attribute_data = self._get_request_cache().setdefault("flag_attribute", {})
is_value_change = False
if name not in flag_attribute_data:
flag_attribute_data[name] = str(value)
is_value_change = True
else:
if flag_attribute_data[name] != str(value):
flag_attribute_data[name] = "Both"
is_value_change = True
if is_value_change:
attribute_name = "flag_{}".format(name)
set_custom_attribute(attribute_name, flag_attribute_data[name])
def _get_waffle_flag_custom_attributes_set():
"""
Returns a set based on the Django setting WAFFLE_FLAG_CUSTOM_ATTRIBUTES (list).
"""
waffle_flag_custom_attributes = getattr(
settings, _WAFFLE_FLAG_CUSTOM_ATTRIBUTES, None
)
waffle_flag_custom_attributes = (
waffle_flag_custom_attributes if waffle_flag_custom_attributes else []
)
return set(waffle_flag_custom_attributes)
if request:
value = flag_is_active(request, namespaced_flag_name)
self._cached_flags[namespaced_flag_name] = value
return value
return None
# .. toggle_name: WAFFLE_FLAG_CUSTOM_ATTRIBUTES
@@ -257,16 +180,86 @@ def _get_waffle_flag_custom_attributes_set():
# .. toggle_use_cases: opt_in
# .. toggle_creation_date: 2020-06-17
# .. toggle_warnings: Intent is for temporary research (1 day - several weeks) of a flag's usage.
_WAFFLE_FLAG_CUSTOM_ATTRIBUTES = "WAFFLE_FLAG_CUSTOM_ATTRIBUTES"
# set of waffle flags that should be instrumented with custom attributes
_WAFFLE_FLAG_CUSTOM_ATTRIBUTE_SET = _get_waffle_flag_custom_attributes_set()
@lru_cache(maxsize=None)
def _get_waffle_flag_custom_attributes_set():
attributes = getattr(settings, "WAFFLE_FLAG_CUSTOM_ATTRIBUTES", None) or []
return set(attributes)
class WaffleFlag(object):
def _set_waffle_flag_attribute(name, value):
"""
Represents a single waffle flag, using a cached waffle namespace.
For any flag name in settings.WAFFLE_FLAG_CUSTOM_ATTRIBUTES, add name/value
to cached values and set custom attribute if the value changed.
The name of the custom attribute will have the prefix ``flag_`` and the
suffix will match the name of the flag.
The value of the custom attribute could be False, True, or Both.
The value Both would mean that the flag had both a True and False
value at different times during the transaction. This is most
likely due to having a course override, as is the case with
CourseWaffleFlag.
An example NewRelic query to see the values of a flag in different
environments, if your waffle flag was named ``my.waffle.flag`` might
look like::
SELECT count(*) FROM Transaction
WHERE flag_my.waffle.flag IS NOT NULL
FACET appName, flag_my.waffle.flag
Important: Remember to configure ``WAFFLE_FLAG_CUSTOM_ATTRIBUTES`` for
LMS, Studio and Workers in order to see waffle flag usage in all
edx-platform environments.
"""
if name not in _get_waffle_flag_custom_attributes_set():
return
flag_attribute_data = _get_waffle_namespace_request_cache().setdefault(
"flag_attribute", {}
)
is_value_changed = True
if name not in flag_attribute_data:
# New flag
flag_attribute_data[name] = str(value)
else:
# Existing flag
if flag_attribute_data[name] == str(value):
# Same value
is_value_changed = False
else:
# New value
flag_attribute_data[name] = "Both"
if is_value_changed:
attribute_name = "flag_{}".format(name)
set_custom_attribute(attribute_name, flag_attribute_data[name])
def is_flag_active_for_everyone(namespaced_flag_name):
"""
Returns True if the waffle flag is configured as active for Everyone,
False otherwise.
"""
# Import is placed here to avoid model import at project startup.
from waffle.models import Flag
try:
waffle_flag = Flag.objects.get(name=namespaced_flag_name)
return waffle_flag.everyone is True
except Flag.DoesNotExist:
return False
class WaffleFlag:
"""
Waffle flag class that implements custom override method.
This class should be removed in favour of edx_toggles.toggles.WaffleFlag once we get rid of the WaffleFlagNamespace
class and the `override` method.
"""
NAMESPACE_CLASS = WaffleFlagNamespace
# use a WeakSet so these instances can be garbage collected if need be
_class_instances = WeakSet()
@@ -281,10 +274,9 @@ class WaffleFlag(object):
module_name (String): The name of the module where the flag is created. This should be ``__name__`` in most
cases.
"""
if isinstance(waffle_namespace, six.string_types):
waffle_namespace = WaffleFlagNamespace(name=waffle_namespace)
if isinstance(waffle_namespace, str):
waffle_namespace = self.NAMESPACE_CLASS(name=waffle_namespace)
self.waffle_namespace = waffle_namespace
self.waffle_namespace = waffle_namespace
self.flag_name = flag_name
self._module_name = module_name or ""
@@ -311,6 +303,7 @@ class WaffleFlag(object):
"""
Returns the fully namespaced flag name.
"""
# pylint: disable=protected-access
return self.waffle_namespace._namespaced_name(self.flag_name)
def is_enabled(self):
@@ -321,6 +314,9 @@ class WaffleFlag(object):
@contextmanager
def override(self, active=True):
"""
Shortcut method for `override_waffle_flag`.
"""
# TODO We can move this import to the top of the file once this code is
# not all contained within the __init__ module.
from openedx.core.djangoapps.waffle_utils.testutils import override_waffle_flag
@@ -334,47 +330,49 @@ class CourseWaffleFlag(WaffleFlag):
Represents a single waffle flag that can be forced on/off for a course.
Uses a cached waffle namespace.
Usage:
WAFFLE_FLAG_NAMESPACE = WaffleFlagNamespace(name='my_namespace')
SOME_COURSE_FLAG = CourseWaffleFlag(WAFFLE_FLAG_NAMESPACE, 'some_course_feature', __name__)
And then we can check this flag in code with::
SOME_COURSE_FLAG.is_enabled(course_key)
The Django Admin "waffle_utils" section can be used to configure a course override for this same flag (e.g.
my_namespace.some_course_feature).
"""
def _get_course_override_callback(self, course_key):
def _get_course_override_value(self, course_key):
"""
Returns a function to use as the check_before_waffle_callback.
Returns True/False if the flag was forced on or off for the provided course. Returns None if the flag was not
overridden.
Note: Has side effect of caching the override value.
Arguments:
course_key (CourseKey): The course to check for override before
checking waffle.
course_key (CourseKey): The course to check for override before checking waffle.
"""
# Import is placed here to avoid model import at project startup.
from .models import WaffleFlagCourseOverrideModel
def course_override_callback(namespaced_flag_name):
"""
Returns True/False if the flag was forced on or off for the provided
course. Returns None if the flag was not overridden.
cache_key = "{}.{}".format(self.namespaced_flag_name, str(course_key))
# pylint: disable=protected-access
course_override = self.waffle_namespace._cached_flags.get(cache_key)
Note: Has side effect of caching the override value.
if course_override is None:
course_override = WaffleFlagCourseOverrideModel.override_value(
self.namespaced_flag_name, course_key
)
# pylint: disable=protected-access
self.waffle_namespace._cached_flags[cache_key] = course_override
Arguments:
namespaced_flag_name (String): A namespaced version of the flag
to check.
"""
# Import is placed here to avoid model import at project startup.
from .models import WaffleFlagCourseOverrideModel
cache_key = u"{}.{}".format(namespaced_flag_name, six.text_type(course_key))
force_override = self.waffle_namespace._cached_flags.get(cache_key)
if force_override is None:
force_override = WaffleFlagCourseOverrideModel.override_value(
namespaced_flag_name, course_key
)
self.waffle_namespace._cached_flags[cache_key] = force_override
if force_override == WaffleFlagCourseOverrideModel.ALL_CHOICES.on:
return True
if force_override == WaffleFlagCourseOverrideModel.ALL_CHOICES.off:
return False
return None
return course_override_callback
if course_override == WaffleFlagCourseOverrideModel.ALL_CHOICES.on:
return True
if course_override == WaffleFlagCourseOverrideModel.ALL_CHOICES.off:
return False
return None
def is_enabled(self, course_key=None): # pylint: disable=arguments-differ
"""
@@ -391,7 +389,8 @@ class CourseWaffleFlag(WaffleFlag):
), "Provided course_key '{}' is not instance of CourseKey.".format(
course_key
)
return self.waffle_namespace.is_flag_active(
self.flag_name,
check_before_waffle_callback=self._get_course_override_callback(course_key),
)
is_enabled_for_course = self._get_course_override_value(course_key)
if is_enabled_for_course is not None:
_set_waffle_flag_attribute(self.namespaced_flag_name, is_enabled_for_course)
return is_enabled_for_course
return super().is_enabled()

95
openedx/core/djangoapps/waffle_utils/tests/test_init.py
View File

@@ -57,35 +57,33 @@ class TestCourseWaffleFlag(TestCase):
Tests various combinations of a flag being set in waffle and overridden
for a course.
"""
with patch(
'openedx.core.djangoapps.waffle_utils._WAFFLE_FLAG_CUSTOM_ATTRIBUTE_SET',
_get_waffle_flag_custom_attributes_set(),
):
with patch.object(WaffleFlagCourseOverrideModel, 'override_value', return_value=data['course_override']):
with override_flag(self.NAMESPACED_FLAG_NAME, active=data['waffle_enabled']):
# check twice to test that the result is properly cached
self.assertEqual(self.TEST_COURSE_FLAG.is_enabled(self.TEST_COURSE_KEY), data['result'])
self.assertEqual(self.TEST_COURSE_FLAG.is_enabled(self.TEST_COURSE_KEY), data['result'])
# result is cached, so override check should happen once
WaffleFlagCourseOverrideModel.override_value.assert_called_once_with(
self.NAMESPACED_FLAG_NAME,
self.TEST_COURSE_KEY
)
_get_waffle_flag_custom_attributes_set.cache_clear()
with patch.object(WaffleFlagCourseOverrideModel, 'override_value', return_value=data['course_override']):
with override_flag(self.NAMESPACED_FLAG_NAME, active=data['waffle_enabled']):
# check twice to test that the result is properly cached
self.assertEqual(self.TEST_COURSE_FLAG.is_enabled(self.TEST_COURSE_KEY), data['result'])
self.assertEqual(self.TEST_COURSE_FLAG.is_enabled(self.TEST_COURSE_KEY), data['result'])
# result is cached, so override check should happen once
# pylint: disable=no-member
WaffleFlagCourseOverrideModel.override_value.assert_called_once_with(
self.NAMESPACED_FLAG_NAME,
self.TEST_COURSE_KEY
)
self._assert_waffle_flag_attribute(mock_set_custom_attribute, expected_flag_value=str(data['result']))
mock_set_custom_attribute.reset_mock()
self._assert_waffle_flag_attribute(mock_set_custom_attribute, expected_flag_value=str(data['result']))
mock_set_custom_attribute.reset_mock()
# check flag for a second course
if data['course_override'] == WaffleFlagCourseOverrideModel.ALL_CHOICES.unset:
# When course override wasn't set for the first course, the second course will get the same
# cached value from waffle.
second_value = data['waffle_enabled']
self.assertEqual(self.TEST_COURSE_FLAG.is_enabled(self.TEST_COURSE_2_KEY), second_value)
else:
# When course override was set for the first course, it should not apply to the second
# course which should get the default value of False.
second_value = False
self.assertEqual(self.TEST_COURSE_FLAG.is_enabled(self.TEST_COURSE_2_KEY), second_value)
# check flag for a second course
if data['course_override'] == WaffleFlagCourseOverrideModel.ALL_CHOICES.unset:
# When course override wasn't set for the first course, the second course will get the same
# cached value from waffle.
second_value = data['waffle_enabled']
self.assertEqual(self.TEST_COURSE_FLAG.is_enabled(self.TEST_COURSE_2_KEY), second_value)
else:
# When course override was set for the first course, it should not apply to the second
# course which should get the default value of False.
second_value = False
self.assertEqual(self.TEST_COURSE_FLAG.is_enabled(self.TEST_COURSE_2_KEY), second_value)
expected_flag_value = None if second_value == data['result'] else 'Both'
self._assert_waffle_flag_attribute(mock_set_custom_attribute, expected_flag_value=expected_flag_value)
@@ -102,23 +100,21 @@ class TestCourseWaffleFlag(TestCase):
__name__,
)
with patch(
'openedx.core.djangoapps.waffle_utils._WAFFLE_FLAG_CUSTOM_ATTRIBUTE_SET',
_get_waffle_flag_custom_attributes_set(),
_get_waffle_flag_custom_attributes_set.cache_clear()
with patch.object(
WaffleFlagCourseOverrideModel,
'override_value',
return_value=WaffleFlagCourseOverrideModel.ALL_CHOICES.unset
):
with patch.object(
WaffleFlagCourseOverrideModel,
'override_value',
return_value=WaffleFlagCourseOverrideModel.ALL_CHOICES.unset
):
# check twice to test that the result is properly cached
self.assertEqual(test_course_flag.is_enabled(self.TEST_COURSE_KEY), False)
self.assertEqual(test_course_flag.is_enabled(self.TEST_COURSE_KEY), False)
# result is cached, so override check should happen once
WaffleFlagCourseOverrideModel.override_value.assert_called_once_with(
self.NAMESPACED_FLAG_NAME,
self.TEST_COURSE_KEY
)
# check twice to test that the result is properly cached
self.assertEqual(test_course_flag.is_enabled(self.TEST_COURSE_KEY), False)
self.assertEqual(test_course_flag.is_enabled(self.TEST_COURSE_KEY), False)
# result is cached, so override check should happen once
# pylint: disable=no-member
WaffleFlagCourseOverrideModel.override_value.assert_called_once_with(
self.NAMESPACED_FLAG_NAME,
self.TEST_COURSE_KEY
)
self._assert_waffle_flag_attribute(
mock_set_custom_attribute,
@@ -161,14 +157,11 @@ class TestCourseWaffleFlag(TestCase):
Test that custom attributes are recorded when waffle flag accessed.
"""
with override_settings(WAFFLE_FLAG_CUSTOM_ATTRIBUTES=data['waffle_flag_attribute_setting']):
with patch(
'openedx.core.djangoapps.waffle_utils._WAFFLE_FLAG_CUSTOM_ATTRIBUTE_SET',
_get_waffle_flag_custom_attributes_set(),
):
test_course_flag = CourseWaffleFlag(self.TEST_NAMESPACE, self.FLAG_NAME, __name__)
test_course_flag.is_enabled(self.TEST_COURSE_KEY)
test_course_flag_2 = CourseWaffleFlag(self.TEST_NAMESPACE, self.FLAG_2_NAME, __name__)
test_course_flag_2.is_enabled(self.TEST_COURSE_KEY)
_get_waffle_flag_custom_attributes_set.cache_clear()
test_course_flag = CourseWaffleFlag(self.TEST_NAMESPACE, self.FLAG_NAME, __name__)
test_course_flag.is_enabled(self.TEST_COURSE_KEY)
test_course_flag_2 = CourseWaffleFlag(self.TEST_NAMESPACE, self.FLAG_2_NAME, __name__)
test_course_flag_2.is_enabled(self.TEST_COURSE_KEY)
self.assertEqual(mock_set_custom_attribute.call_count, data['expected_count'])

3
openedx/features/course_experience/__init__.py
View File

@@ -8,7 +8,8 @@ from waffle import flag_is_active
from lms.djangoapps.experiments.flags import ExperimentWaffleFlag
from openedx.core.djangoapps.util.user_messages import UserMessageCollection
from openedx.core.djangoapps.waffle_utils import CourseWaffleFlag, WaffleFlag, WaffleFlagNamespace
from openedx.core.djangoapps.waffle_utils import WaffleFlag, WaffleFlagNamespace
from openedx.core.djangoapps.waffle_utils import CourseWaffleFlag
# Namespace for course experience waffle flags.
WAFFLE_FLAG_NAMESPACE = WaffleFlagNamespace(name='course_experience')