update waffle flag custom metrics (#24270)

The previous version of this code used the Django Setting
ENABLE_WAFFLE_FLAG_METRIC to determine whether to add a single
metric with a dict of details about all flags. Due to
NewRelic's 256 character limit on the metric value, this was
getting truncated.

This new version instead uses the Django Setting
WAFFLE_FLAG_CUSTOM_METRICS, a list of waffle flag names to
instrument.

The name of each custom metric will match the name of the flag.
The value of the custom metric 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.

ARCHBOM-132

openedx/core/djangoapps/waffle_utils/__init__.py

@@ -299,26 +299,19 @@ class WaffleFlagNamespace(six.with_metaclass(ABCMeta, WaffleNamespace)):
 
     def _set_waffle_flag_metric(self, name, value):
         """
-        If ENABLE_WAFFLE_FLAG_METRIC is True, adds name/value to cached values
-        and sets metric if the value changed.
+        For any flag name in _WAFFLE_FLAG_CUSTOM_METRIC_SET, add name/value
+        to cached values and set custom metric if the value changed.
 
-        Metric flag values could be False, True, or Both. The value Both would
-        mean that the flag had both a True and False value at different times
-        through the transaction. This is most likely due to having a
-        check_before_waffle_callback, as is the case with CourseWaffleFlag.
+        The name of the custom metric will match the name of the flag.
+        The value of the custom metric could be False, True, or Both.
 
-        Example metric value::
-
-            "{'another.course.flag': 'False', 'some.flag': 'False', 'some.course.flag': 'Both'}"
-
-        Warning: NewRelic does not recommend large custom metric values due to
-        the potential performance impact on the agent, so you may just want to
-        enable when researching usage of a particular flag. Metric values longer
-        than 255 are truncated.
+          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.
 
         """
-        is_waffle_flag_metric_enabled = getattr(settings, _ENABLE_WAFFLE_FLAG_METRIC, False)
-        if not is_waffle_flag_metric_enabled:
+        if name not in _WAFFLE_FLAG_CUSTOM_METRIC_SET:
             return
 
         flag_metric_data = self._get_request_cache().setdefault('flag_metric', {})
@@ -332,20 +325,32 @@ class WaffleFlagNamespace(six.with_metaclass(ABCMeta, WaffleNamespace)):
                 is_value_change = True
 
         if is_value_change:
-            set_custom_metric('waffle_flags', str(flag_metric_data))
+            set_custom_metric(name, flag_metric_data[name])
+
+
+def _get_waffle_flag_custom_metrics_set():
+    """
+    Returns a set based on the Django setting WAFFLE_FLAG_CUSTOM_METRICS (list).
+    """
+    waffle_flag_custom_metrics = getattr(settings, _WAFFLE_FLAG_CUSTOM_METRICS, None)
+    waffle_flag_custom_metrics = waffle_flag_custom_metrics if waffle_flag_custom_metrics else []
+    return set(waffle_flag_custom_metrics)
 
     # .. toggle_name: ENABLE_WAFFLE_FLAG_METRIC
     # .. toggle_implementation: DjangoSetting
     # .. toggle_default: False
-    # .. toggle_description: Set to True to enable a custom metric with waffle flag values (True, False, Both).
+    # .. toggle_description: A list of waffle flag to track with custom metrics having values of (True, False, or Both).
     # .. toggle_category: monitoring
-    # .. toggle_use_cases: opt_out
+    # .. toggle_use_cases: opt_in
     # .. toggle_creation_date: 2020-06-17
     # .. toggle_expiration_date: None
     # .. toggle_tickets: None
     # .. toggle_status: supported
-    # .. toggle_warnings: Metric of flags could be large and heavier than typical metrics.
-_ENABLE_WAFFLE_FLAG_METRIC = 'ENABLE_WAFFLE_FLAG_METRIC'
+    # .. toggle_warnings: Intent is for temporary research (1 day - several weeks) of a flag's usage.
+_WAFFLE_FLAG_CUSTOM_METRICS = 'WAFFLE_FLAG_CUSTOM_METRICS'
+
+# set of waffle flags that should be instrumented with custom metrics
+_WAFFLE_FLAG_CUSTOM_METRIC_SET = _get_waffle_flag_custom_metrics_set()
 
 
 class WaffleFlag(object):

openedx/core/djangoapps/waffle_utils/tests/test_init.py

@@ -12,7 +12,13 @@ from mock import call, patch
 from opaque_keys.edx.keys import CourseKey
 from waffle.testutils import override_flag
 
-from .. import CourseWaffleFlag, WaffleFlagNamespace, WaffleSwitchNamespace, WaffleSwitch
+from .. import (
+    _get_waffle_flag_custom_metrics_set,
+    CourseWaffleFlag,
+    WaffleFlagNamespace,
+    WaffleSwitchNamespace,
+    WaffleSwitch,
+)
 from ..models import WaffleFlagCourseOverrideModel
 
 
@@ -25,6 +31,8 @@ class TestCourseWaffleFlag(TestCase):
     NAMESPACE_NAME = "test_namespace"
     FLAG_NAME = "test_flag"
     NAMESPACED_FLAG_NAME = NAMESPACE_NAME + "." + FLAG_NAME
+    FLAG_2_NAME = "test_flag_2"
+    NAMESPACED_FLAG_2_NAME = NAMESPACE_NAME + "." + FLAG_2_NAME
 
     TEST_COURSE_KEY = CourseKey.from_string("edX/DemoX/Demo_Course")
     TEST_COURSE_2_KEY = CourseKey.from_string("edX/DemoX/Demo_Course_2")
@@ -38,7 +46,7 @@ class TestCourseWaffleFlag(TestCase):
         crum.set_current_request(request)
         RequestCache.clear_all_namespaces()
 
-    @override_settings(ENABLE_WAFFLE_FLAG_METRIC=True)
+    @override_settings(WAFFLE_FLAG_CUSTOM_METRICS=[NAMESPACED_FLAG_NAME])
     @patch('openedx.core.djangoapps.waffle_utils.set_custom_metric')
     @ddt.data(
         {'course_override': WaffleFlagCourseOverrideModel.ALL_CHOICES.on, 'waffle_enabled': False, 'result': True},
@@ -51,36 +59,40 @@ class TestCourseWaffleFlag(TestCase):
         Tests various combinations of a flag being set in waffle and overridden
         for a course.
         """
-        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
-                )
-
-        self._assert_waffle_flag_metric(mock_set_custom_metric, expected_flag_value=str(data['result']))
-        mock_set_custom_metric.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)
+        with patch(
+            'openedx.core.djangoapps.waffle_utils._WAFFLE_FLAG_CUSTOM_METRIC_SET',
+            _get_waffle_flag_custom_metrics_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
+                    )
+
+            self._assert_waffle_flag_metric(mock_set_custom_metric, expected_flag_value=str(data['result']))
+            mock_set_custom_metric.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)
 
         expected_flag_value = None if second_value == data['result'] else 'Both'
         self._assert_waffle_flag_metric(mock_set_custom_metric, expected_flag_value=expected_flag_value)
 
-    @override_settings(ENABLE_WAFFLE_FLAG_METRIC=True)
+    @override_settings(WAFFLE_FLAG_CUSTOM_METRICS=[NAMESPACED_FLAG_NAME])
     @patch('openedx.core.djangoapps.waffle_utils.set_custom_metric')
     @ddt.data(
         {'flag_undefined_default': None, 'result': False},
@@ -97,19 +109,23 @@ class TestCourseWaffleFlag(TestCase):
             flag_undefined_default=data['flag_undefined_default']
         )
 
-        with patch.object(
-            WaffleFlagCourseOverrideModel,
-            'override_value',
-            return_value=WaffleFlagCourseOverrideModel.ALL_CHOICES.unset
+        with patch(
+            'openedx.core.djangoapps.waffle_utils._WAFFLE_FLAG_CUSTOM_METRIC_SET',
+            _get_waffle_flag_custom_metrics_set(),
         ):
-            # check twice to test that the result is properly cached
-            self.assertEqual(test_course_flag.is_enabled(self.TEST_COURSE_KEY), data['result'])
-            self.assertEqual(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
-            )
+            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), data['result'])
+                self.assertEqual(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
+                )
 
         self._assert_waffle_flag_metric(mock_set_custom_metric, expected_flag_value=str(data['result']))
 
@@ -130,16 +146,28 @@ class TestCourseWaffleFlag(TestCase):
         )
         self.assertEqual(test_course_flag.is_enabled(self.TEST_COURSE_KEY), data['result'])
 
+    @ddt.data(
+        {'expected_count': 0, 'waffle_flag_metric_setting': None},
+        {'expected_count': 1, 'waffle_flag_metric_setting': [NAMESPACED_FLAG_NAME]},
+        {'expected_count': 2, 'waffle_flag_metric_setting': [NAMESPACED_FLAG_NAME, NAMESPACED_FLAG_2_NAME]},
+    )
     @patch('openedx.core.djangoapps.waffle_utils.set_custom_metric')
-    def test_waffle_flag_metric_disabled(self, mock_set_custom_metric):
-        test_course_flag = CourseWaffleFlag(self.TEST_NAMESPACE, self.FLAG_NAME)
-        test_course_flag.is_enabled(self.TEST_COURSE_KEY)
-        self.assertEqual(mock_set_custom_metric.call_count, 0)
+    def test_waffle_flag_metric_for_various_settings(self, data, mock_set_custom_metric):
+        with override_settings(WAFFLE_FLAG_CUSTOM_METRICS=data['waffle_flag_metric_setting']):
+            with patch(
+                'openedx.core.djangoapps.waffle_utils._WAFFLE_FLAG_CUSTOM_METRIC_SET',
+                _get_waffle_flag_custom_metrics_set(),
+            ):
+                test_course_flag = CourseWaffleFlag(self.TEST_NAMESPACE, self.FLAG_NAME)
+                test_course_flag.is_enabled(self.TEST_COURSE_KEY)
+                test_course_flag_2 = CourseWaffleFlag(self.TEST_NAMESPACE, self.FLAG_2_NAME)
+                test_course_flag_2.is_enabled(self.TEST_COURSE_KEY)
+
+        self.assertEqual(mock_set_custom_metric.call_count, data['expected_count'])
 
     def _assert_waffle_flag_metric(self, mock_set_custom_metric, expected_flag_value=None):
         if expected_flag_value:
-            expected_metric_value = str({self.NAMESPACED_FLAG_NAME: expected_flag_value})
-            expected_calls = [call('waffle_flags', expected_metric_value)]
+            expected_calls = [call(self.NAMESPACED_FLAG_NAME, expected_flag_value)]
             mock_set_custom_metric.assert_has_calls(expected_calls)
             self.assertEqual(mock_set_custom_metric.call_count, 1)
         else: