Merge pull request #118 from reddit/add_measured_rollout_support

Support Measured Rollouts and retire "owner" field

Author: mrlevitas

docs/requirements.txt

@@ -4,7 +4,7 @@ docutils==0.16
 Jinja2==2.11.2
 MarkupSafe==1.1.1
 pydocstyle==5.1.1
-reddit-decider==1.7.0
+reddit-decider==1.11.0
 reddit-edgecontext==1.0.0a3
 Sphinx==3.4.0
 sphinx-autodoc-typehints==1.11.1

reddit_decider/__init__.py

@@ -73,8 +73,9 @@ class ExperimentConfig:
     bucket_val: str
     start_ts: int
     stop_ts: int
-    owner: str
+    owner: None = None
     emit_event: Optional[bool] = None
+    measured: Optional[bool] = None
 
 
 class DeciderContext:
@@ -236,7 +237,6 @@ def _send_expose(self, event: str, exposure_fields: dict) -> None:
                 bucket_val,
                 start_ts,
                 stop_ts,
-                owner,
             ) = event.split("::::")
         except ValueError:
             logger.warning(
@@ -251,7 +251,6 @@ def _send_expose(self, event: str, exposure_fields: dict) -> None:
             bucket_val=bucket_val,
             start_ts=self._cast_to_int(start_ts),
             stop_ts=self._cast_to_int(stop_ts),
-            owner=owner,
         )
 
         event_fields = {**event_fields, **{bucket_val: bucketing_value}}
@@ -279,7 +278,6 @@ def _send_expose_if_holdout(self, event: str, exposure_fields: dict) -> None:
                 bucket_val,
                 start_ts,
                 stop_ts,
-                owner,
             ) = event.split("::::")
         except ValueError:
             logger.warning(
@@ -299,7 +297,6 @@ def _send_expose_if_holdout(self, event: str, exposure_fields: dict) -> None:
                 bucket_val=bucket_val,
                 start_ts=self._cast_to_int(start_ts),
                 stop_ts=self._cast_to_int(stop_ts),
-                owner=owner,
             )
 
             event_fields = {**event_fields, **{bucket_val: bucketing_value}}
@@ -432,7 +429,6 @@ def expose(
             bucket_val=feature.bucket_val,
             start_ts=feature.start_ts,
             stop_ts=feature.stop_ts,
-            owner=feature.owner,
         )
 
         self._event_logger.log(
@@ -920,8 +916,8 @@ def get_experiment(self, experiment_name: str) -> Optional[ExperimentConfig]:
             bucket_val=feature.bucket_val,
             start_ts=feature.start_ts,
             stop_ts=feature.stop_ts,
-            owner=feature.owner,
             emit_event=feature.emit_event,
+            measured=feature.measured,
         )
 
 

requirements.txt

@@ -1,7 +1,7 @@
 -r requirements-transitive.txt
 baseplate>=2.0.0a1,<3.0
 black==21.4b2
-reddit-decider==1.7.0
+reddit-decider==1.11.0
 flake8==3.9.1
 mypy==0.790
 prometheus-client>=0.12.0,<1.0

setup.py

@@ -19,7 +19,7 @@
     install_requires=[
         "baseplate>=2.0.0a1,<3.0",
         "reddit-edgecontext>=1.0.0a3,<2.0",
-        "reddit-decider~=1.7.0",
+        "reddit-decider~=1.11.0",
         "typing_extensions>=3.10.0.0,<5.0",
     ],
     package_data={"reddit_experiments": ["py.typed"], "reddit_decider": ["py.typed"]},

tests/decider_tests.py

@@ -500,7 +500,7 @@ def assert_exposure_event_fields(
         cfg = self.exp_base_config[experiment_name]
         self.assertEqual(getattr(event_fields["experiment"], "id"), cfg["id"])
         self.assertEqual(getattr(event_fields["experiment"], "name"), cfg["name"])
-        self.assertEqual(getattr(event_fields["experiment"], "owner"), cfg["owner"])
+        self.assertEqual(getattr(event_fields["experiment"], "owner"), None)
         self.assertEqual(getattr(event_fields["experiment"], "version"), cfg["version"])
         self.assertEqual(getattr(event_fields["experiment"], "bucket_val"), bucket_val)
 
@@ -520,7 +520,7 @@ def assert_minimal_exposure_event_fields(
         cfg = self.exp_base_config[experiment_name]
         self.assertEqual(getattr(event_fields["experiment"], "id"), cfg["id"])
         self.assertEqual(getattr(event_fields["experiment"], "name"), cfg["name"])
-        self.assertEqual(getattr(event_fields["experiment"], "owner"), cfg["owner"])
+        self.assertEqual(getattr(event_fields["experiment"], "owner"), None)
         self.assertEqual(getattr(event_fields["experiment"], "version"), cfg["version"])
         self.assertEqual(getattr(event_fields["experiment"], "bucket_val"), bucket_val)
 
@@ -573,7 +573,7 @@ def test_none_returned_on_get_variant_call_with_bad_id(self):
                 self.assertEqual(self.event_logger.log.call_count, 0)
 
                 assert any(
-                    "Partially loaded Decider: 1 features failed to load: {'test': 'Manifest parsing error: invalid type: string \"1\", expected u32'}"
+                    'Partially loaded Decider: 1 feature(s) failed to load: {"test": ParsingError(Error("invalid type: string \\"1\\", expected u32", line: 0, column: 0))'
                     in x.getMessage()
                     for x in captured.records
                 )
@@ -1508,6 +1508,81 @@ def test_get_variant_with_disabled_exp(self):
             # exposure assertions
             self.assertEqual(self.event_logger.log.call_count, 0)
 
+    def test_range_variant_emit_event_override(self):
+        cfg = {
+            "feature_rollout_100": {
+                "id": 9110,
+                "name": "feature_rollout_100",
+                "enabled": True,
+                "version": "1",
+                "start_ts": 1522306800,
+                "stop_ts": 32533405261,
+                "owner": "[email protected]",
+                "type": "feature_rollout",
+                "emit_event": False,
+                "experiment": {
+                    "variants": [
+                        {"name": "enabled", "size": 1.0, "range_end": 1.0, "range_start": 0.0}
+                    ],
+                    "experiment_version": 1,
+                    "shuffle_version": 0,
+                    "bucket_val": "user_id",
+                    "log_bucketing": False,
+                },
+            },
+            "measured_rollout_100": {
+                "id": 9119,
+                "name": "measured_rollout_100",
+                "enabled": True,
+                "version": "1",
+                "start_ts": 1522306800,
+                "stop_ts": 32533405261,
+                "owner": "[email protected]",
+                "type": "range_variant",
+                "emit_event": False,
+                "measured": True,
+                "experiment": {
+                    "variants": [
+                        {
+                            "name": "enabled",
+                            "size": 1.0,
+                            "range_end": 1.0,
+                            "range_start": 0.0,
+                            "emit_event_override": True,
+                        }
+                    ],
+                    "experiment_version": 1,
+                    "shuffle_version": 0,
+                    "bucket_val": "user_id",
+                    "log_bucketing": False,
+                },
+            },
+        }
+
+        with create_temp_config_file(cfg) as f:
+            decider = setup_decider(f, self.dc, self.mock_span, self.event_logger)
+
+            self.assertEqual(self.event_logger.log.call_count, 0)
+            variant = decider.get_variant(experiment_name="feature_rollout_100")
+            self.assertEqual(variant, "enabled")
+
+            # FR does NOT emit exposure
+            self.assertEqual(self.event_logger.log.call_count, 0)
+
+            # FR is NOT "measured"
+            fr_cfg = decider.get_experiment(experiment_name="feature_rollout_100")
+            self.assertEqual(fr_cfg.measured, False)
+
+            variant = decider.get_variant(experiment_name="measured_rollout_100")
+            self.assertEqual(variant, "enabled")
+
+            # Measured Rollout DOES emit exposure (due to RV "emit_event_override" field)
+            self.assertEqual(self.event_logger.log.call_count, 1)
+
+            # MR IS "measured"
+            mr_cfg = decider.get_experiment(experiment_name="measured_rollout_100")
+            self.assertEqual(mr_cfg.measured, True)
+
     def test_get_experiment(self):
         with create_temp_config_file(self.exp_base_config) as f:
             decider = setup_decider(f, self.dc, self.mock_span, self.event_logger)
@@ -1521,7 +1596,6 @@ def test_get_experiment(self):
             self.assertEqual(experiment.bucket_val, cfg["experiment"]["bucket_val"])
             self.assertEqual(experiment.start_ts, cfg["start_ts"])
             self.assertEqual(experiment.stop_ts, cfg["stop_ts"])
-            self.assertEqual(experiment.owner, cfg["owner"])
             self.assertEqual(experiment.emit_event, True)
 
     def test_get_variant_without_expose_with_HG_as_control_1_and_child_returns_none_does_expose(