From b05948153af26560cff4de0dab9fa1316b3dee71 Mon Sep 17 00:00:00 2001 From: Kyle McCormick Date: Thu, 6 Aug 2020 16:23:00 -0400 Subject: [PATCH] Add course-unawareness option to ExperimentWaffleFlag For the Courseware MFE rollout experiment, we want users' default buckets to be consistent across course runs. ExperimentWaffleFlag advised that this could be done by calling `.is_enabled(...)` without a course_key argument; however, doing so breaks when uing the main_flag.BUCKET_NUM scheme to apply bucket rules for a specific set of users or courses. This commit explicitly adds course-unaware-bucketing via a new kwarg to ExperimentWaffleFlag.__init__ method. Furthermore, it fixes ExperimentWaffleFlag.is_enabled(course_key=None) to work as advertised, by means of calling .is_enabled_without_course_context on its subordinate flags. TNL-7405 --- lms/djangoapps/courseware/toggles.py | 4 +- lms/djangoapps/experiments/flags.py | 69 ++++++++---- .../experiments/tests/test_flags.py | 101 ++++++++++++++++++ .../core/djangoapps/waffle_utils/__init__.py | 2 +- 4 files changed, 156 insertions(+), 20 deletions(-) diff --git a/lms/djangoapps/courseware/toggles.py b/lms/djangoapps/courseware/toggles.py index c1039ca5dc..b0a425a081 100644 --- a/lms/djangoapps/courseware/toggles.py +++ b/lms/djangoapps/courseware/toggles.py @@ -21,7 +21,9 @@ WAFFLE_FLAG_NAMESPACE = WaffleFlagNamespace(name='courseware') # .. toggle_warnings: Also set settings.LEARNING_MICROFRONTEND_URL and ENABLE_COURSEWARE_MICROFRONTEND. # .. toggle_tickets: TNL-7000 # .. toggle_status: supported -REDIRECT_TO_COURSEWARE_MICROFRONTEND = ExperimentWaffleFlag(WAFFLE_FLAG_NAMESPACE, 'courseware_mfe') +REDIRECT_TO_COURSEWARE_MICROFRONTEND = ExperimentWaffleFlag( + WAFFLE_FLAG_NAMESPACE, 'courseware_mfe', use_course_aware_bucketing=False +) # Waffle flag to display a link for the new learner experience to course teams without redirecting students. # diff --git a/lms/djangoapps/experiments/flags.py b/lms/djangoapps/experiments/flags.py index 2922a5303d..b326b79b3c 100644 --- a/lms/djangoapps/experiments/flags.py +++ b/lms/djangoapps/experiments/flags.py @@ -30,8 +30,14 @@ class ExperimentWaffleFlag(CourseWaffleFlag): "main_flag.BUCKET_NUM" (e.g. "course_experience.animated_exy.0") to force users that pass the first main waffle check into a specific bucket experience. - If you pass this flag a course key, tracking calls to segment will be made per-course-run - (rather than one call overall) and will include the course key. + If a user is not forced into a specific bucket by one of the aforementioned smaller flags, + then they will be randomly assigned a default bucket based on a consistent hash of: + * (flag_name, course_key, username) if use_course_aware_bucketing=True, or + * (flag_name, username) if use_course_aware_bucketing=False. + + Note that you may call `.get_bucket` without a course_key, in which case: + * the smaller flags will be evaluated without course context, and + * the default bucket will be evaluated as if use_course_aware_bucketing=False. You can also control whether the experiment only affects future enrollments by setting an ExperimentKeyValue model object with a key of 'enrollment_start' to the date of the @@ -56,7 +62,15 @@ class ExperimentWaffleFlag(CourseWaffleFlag): ... """ - def __init__(self, waffle_namespace, flag_name, num_buckets=2, experiment_id=None, **kwargs): + def __init__( + self, + waffle_namespace, + flag_name, + num_buckets=2, + experiment_id=None, + use_course_aware_bucketing=True, + **kwargs + ): super().__init__(waffle_namespace, flag_name, **kwargs) self.num_buckets = num_buckets self.experiment_id = experiment_id @@ -64,6 +78,7 @@ class ExperimentWaffleFlag(CourseWaffleFlag): CourseWaffleFlag(waffle_namespace, '{}.{}'.format(flag_name, bucket)) for bucket in range(num_buckets) ] + self.use_course_aware_bucketing = use_course_aware_bucketing def _cache_bucket(self, key, value): request_cache = RequestCache('experiments') @@ -119,12 +134,7 @@ class ExperimentWaffleFlag(CourseWaffleFlag): # Keep some imports in here, because this class is commonly used at a module level, and we want to avoid # circular imports for any models. from experiments.models import ExperimentKeyValue - from lms.djangoapps.courseware.access import has_access - from lms.djangoapps.courseware.masquerade import ( - setup_masquerade, - is_masquerading, - get_specific_masquerading_user, - ) + from lms.djangoapps.courseware.masquerade import get_specific_masquerading_user request = get_current_request() if not request: @@ -141,8 +151,16 @@ class ExperimentWaffleFlag(CourseWaffleFlag): else: masquerading_as_specific_student = True - # Use course key in experiment name to separate caches and segment calls per-course-run - experiment_name = self.namespaced_flag_name + ('.{}'.format(course_key) if course_key else '') + # If a course key is passed in, include it in the experiment name + # in order to separate caches and analytics calls per course-run. + # If we are using course-aware bucketing, then also append that course key + # to `bucketing_group_name`, such that users can be hashed into different + # buckets for different course-runs. + experiment_name = bucketing_group_name = self.namespaced_flag_name + if course_key: + experiment_name += ".{}".format(course_key) + if course_key and self.use_course_aware_bucketing: + bucketing_group_name += ".{}".format(course_key) # Check if we have a cache for this request already request_cache = RequestCache('experiments') @@ -162,16 +180,28 @@ class ExperimentWaffleFlag(CourseWaffleFlag): if not self._is_enrollment_inside_date_bounds(values, user, course_key): return self._cache_bucket(experiment_name, 0) - bucket = stable_bucketing_hash_group(experiment_name, self.num_buckets, user.username) - - # Now check if the user is forced into a particular bucket, using our subordinate bucket flags + # Determine the user's bucket. + # First check if forced into a particular bucket, using our subordinate bucket flags. + # If not, calculate their default bucket using a consistent hash function. for i, bucket_flag in enumerate(self.bucket_flags): - if bucket_flag.is_enabled(course_key): + forced_into_bucket = ( + bucket_flag.is_enabled(course_key) if course_key + else bucket_flag.is_enabled_without_course_context() + ) + if forced_into_bucket: bucket = i break + else: + bucket = stable_bucketing_hash_group( + bucketing_group_name, self.num_buckets, user.username + ) session_key = 'tracked.{}'.format(experiment_name) - if track and hasattr(request, 'session') and session_key not in request.session and not masquerading_as_specific_student: + if ( + track and hasattr(request, 'session') and + session_key not in request.session and + not masquerading_as_specific_student + ): segment.track( user_id=user.id, event_name='edx.bi.experiment.user.bucketed', @@ -198,10 +228,13 @@ class ExperimentWaffleFlag(CourseWaffleFlag): return self.is_enabled() def is_experiment_on(self, course_key=None): - # If no course_key is supplied check the global flag irrespective of courses + """ + Return whether the overall experiment flag is enabled for this user. + + This disregards `.bucket_flags`. + """ if course_key is None: return super().is_enabled_without_course_context() - return super().is_enabled(course_key) @contextmanager diff --git a/lms/djangoapps/experiments/tests/test_flags.py b/lms/djangoapps/experiments/tests/test_flags.py index 3f1328df87..ff80fe694f 100644 --- a/lms/djangoapps/experiments/tests/test_flags.py +++ b/lms/djangoapps/experiments/tests/test_flags.py @@ -16,6 +16,7 @@ from experiments.factories import ExperimentKeyValueFactory from experiments.flags import ExperimentWaffleFlag from openedx.core.djangoapps.site_configuration.tests.factories import SiteFactory from openedx.core.djangoapps.waffle_utils import CourseWaffleFlag +from openedx.core.djangoapps.waffle_utils.models import WaffleFlagCourseOverrideModel from openedx.core.djangoapps.waffle_utils.testutils import override_waffle_flag from student.tests.factories import CourseEnrollmentFactory, UserFactory from xmodule.modulestore.tests.django_utils import SharedModuleStoreTestCase @@ -156,3 +157,103 @@ class ExperimentWaffleFlagTests(SharedModuleStoreTestCase): with self.flag.override(active=active, bucket=bucket_override): self.assertEqual(self.flag.get_bucket(), expected_bucket) self.assertEqual(self.flag.is_experiment_on(), active) + + +class ExperimentWaffleFlagCourseAwarenessTest(SharedModuleStoreTestCase): + """ + Tests for how course context awareness/unawareness interacts with the + ExperimentWaffleFlag class. + """ + course_aware_flag = ExperimentWaffleFlag( + 'exp', 'aware', num_buckets=20, use_course_aware_bucketing=True, + ) + course_aware_subflag = CourseWaffleFlag('exp', 'aware.1') + + course_unaware_flag = ExperimentWaffleFlag( + 'exp', 'unaware', num_buckets=20, use_course_aware_bucketing=False, + ) + course_unaware_subflag = CourseWaffleFlag('exp', 'unaware.1') + + course_key_1 = CourseKey.from_string("x/y/1") + course_key_2 = CourseKey.from_string("x/y/22") + course_key_3 = CourseKey.from_string("x/y/333") + + @classmethod + def setUpTestData(cls): + super().setUpTestData() + # Force all users into Bucket 1 for course at `course_key_1`. + WaffleFlagCourseOverrideModel.objects.create( + waffle_flag="exp.aware.1", course_id=cls.course_key_1, enabled=True + ) + WaffleFlagCourseOverrideModel.objects.create( + waffle_flag="exp.unaware.1", course_id=cls.course_key_1, enabled=True + ) + cls.user = UserFactory() + + def setUp(self): + super().setUp() + self.request = RequestFactory().request() + self.request.session = {} + self.request.site = SiteFactory() + self.request.user = self.user + self.addCleanup(set_current_request, None) + set_current_request(self.request) + self.addCleanup(RequestCache.clear_all_namespaces) + + # Enable all experiment waffle flags. + experiment_waffle_flag_patcher = patch.object( + ExperimentWaffleFlag, 'is_experiment_on', return_value=True + ) + experiment_waffle_flag_patcher.start() + self.addCleanup(experiment_waffle_flag_patcher.stop) + + # Use our custom fake `stable_bucketing_hash_group` implementation. + stable_bucket_patcher = patch( + 'experiments.flags.stable_bucketing_hash_group', self._mock_stable_bucket + ) + stable_bucket_patcher.start() + self.addCleanup(stable_bucket_patcher.stop) + + @staticmethod + def _mock_stable_bucket(group_name, *_args, **_kwargs): + """ + A fake version of `stable_bucketing_hash_group` that just returns + the length of `group_name`. + """ + return len(group_name) + + def test_course_aware_bucketing(self): + """ + Test behavior of an experiment flag configured wtih course-aware bucket hashing. + """ + + # Expect queries for Course 1 to be forced into Bucket 1 + # due to `course_aware_subflag`. + assert self.course_aware_flag.get_bucket(self.course_key_1) == 1 + + # Because we are using course-aware bucket hashing, different + # courses may default to different buckets. + # In the case of Courses 2 and 3 here, we expect two different buckets. + assert self.course_aware_flag.get_bucket(self.course_key_2) == 16 + assert self.course_aware_flag.get_bucket(self.course_key_3) == 17 + + # We can still query a course-aware flag outside of course context, + # which has its own default bucket. + assert self.course_aware_flag.get_bucket() == 9 + + def test_course_unaware_bucketing(self): + """ + Test behavior of an experiment flag configured wtih course-unaware bucket hashing. + """ + + # Expect queries for Course 1 to be forced into Bucket 1 + # due to `course_unaware_subflag`. + # This should happen in spite of the fact that *default* bucketing + # is unaware of courses. + assert self.course_unaware_flag.get_bucket(self.course_key_1) == 1 + + # Expect queries for Course 2, queries for Course 3, and queries outside + # the context of the course to all be hashed into the same default bucket. + assert self.course_unaware_flag.get_bucket(self.course_key_2) == 11 + assert self.course_unaware_flag.get_bucket(self.course_key_3) == 11 + assert self.course_unaware_flag.get_bucket() == 11 diff --git a/openedx/core/djangoapps/waffle_utils/__init__.py b/openedx/core/djangoapps/waffle_utils/__init__.py index 567ec2daf0..94be7ba332 100644 --- a/openedx/core/djangoapps/waffle_utils/__init__.py +++ b/openedx/core/djangoapps/waffle_utils/__init__.py @@ -477,7 +477,7 @@ class CourseWaffleFlag(WaffleFlag): """ return self._is_enabled() - def is_enabled(self, course_key=None): + def is_enabled(self, course_key): """ Returns whether or not the flag is enabled within the context of a given course.