Add support for preference experiments that set multiple preferences
Categories
(Firefox :: Normandy Client, enhancement)
Tracking
()
| Tracking | Status | |
|---|---|---|
| firefox68 | --- | fixed |
People
(Reporter: glasserc, Assigned: glasserc)
References
Details
Attachments
(5 files)
Currently, a preference-experiment action is limited to controlling a single preference. There is a demand for experiments that set many different preferences at once. This demand has been satisfied previously using the Multipreffer addon. Having support for this feature directly in the Normandy client would dramatically simplify this workflow.
|
Assignee | |
Comment 1•5 years ago
|
||
This is part 1 of the required changes. This just addresses the
storage mechanism and the tests. There are still some call sites
outstanding which use the experiment data directly. These will have to
be adjusted, probably as part of this patch, but I wanted to gather
feedback first.
Once all the uses are updated, we can convert the start() method to
have a signature that allows for multiple preferences, and adjust the
existing PreferenceExperiment action to use it in the more modern way,
and then finally add a new MultiPreferenceExperiment action that can
support a schema which allows for multiple preferences.
Things I'd especially like feedback on:
-
Is the new storage schema sane?
-
Is the overall approach taken in the migration code reasonable? Is
tracking version number like this acceptable?
|
||
Updated•5 years ago
|
|
|
Assignee | |
Comment 2•5 years ago
|
||
Move startObserver to take a preferences object and store a list of
observers for each experiment. While we're here, rename the methods to
startObservers, stopObservers, hasObservers.
Also perform some drive-by cleanups in the tests: add a name to one
test, drop some unused arguments to some other tests.
Depends on D29293
|
|
Assignee | |
Comment 3•5 years ago
|
||
Add a little bit to some existing tests to cover this new
functionality.
Depends on D29871
|
|
Assignee | |
Comment 4•5 years ago
|
||
The existing, single-preference action format is supported by a new
SinglePreferenceExperimentAction, which converts single-preference
actions into multiple-preference actions. We keep the wire format name
"preference-experiment" for SinglePreferenceExperimentAction for now,
but perhaps one day we can move that to "single-preference-experiment".
Depends on D29872
|
|
Assignee | |
Comment 5•5 years ago
|
||
This is another intermediate step in the process. I've updated all the "plumbing" to assume experiments can have multiple preferences. I took a "shortcut" here -- rather than duplicate a majority of the preference-experiment action in both single and multiple preference types, I created a SinglePreferenceExperimentAction which is a subclass of the (now multiple) PreferenceExperimentAction. I didn't add much in the way of tests for this new SinglePreferenceExperimentAction class because it doesn't really do that much. I'm curious what you think about this :mythmon -- is the testing anemic? Is the approach clever, or too clever?
Still outstanding: I need to add user-facing description fields to the action (as in https://phabricator.services.mozilla.com/D28158#change-Ke6naAGg0rSt) and use it in the about:studies code.
|
|
Assignee | |
Comment 6•5 years ago
|
||
|
|
Assignee | |
Comment 7•5 years ago
|
||
Display these when available instead of generating one.
We play some games here to let SinglePreferenceExperiment continue to
validate according to the PreferenceExperiment schema. This is kind of
ugly. Another approach might be to move the about-studies code that
generates a description. I was hesitant to do this because it would
mean losing the formatting.
Depends on D29873
|
|
Assignee | |
Comment 8•5 years ago
|
||
Another question I had was whether it would be a good investment to convert SinglePreferenceExperimentAction from using inheritance to using composition -- managing a PreferenceExperimentAction explicitly as a property. So far I am sticking with inheritance but if someone feels strongly in favor of composition, then I'd be OK with implementing that too. One advantage it would have is that the use of the PreferenceExperimentAction schema could be "automatic", i.e. handled just by calling run() explicitly.
|
||
Comment 9•5 years ago
|
||
I think that since both actions are already inheriting from BaseAction, adding composition to the mix will probably only make this harder. Specifically, many methods inherited from BaseAction have side effects in the form of Telemetry, and I think that composing to re use the other action would cause duplicate telemetry. This could of course be worked around, but it doesn't seem like an obvious win to me. I'm happy to keep it the way it currently is.
|
|
Assignee | |
Comment 10•5 years ago
|
||
I rebased this onto relatively recent mozilla-central. Try build running at https://treeherder.mozilla.org/#/jobs?repo=try&revision=67e3e61da7255b85116ae51da10c25cc12577949
|
||
Comment 11•5 years ago
|
||
Pushed by eglassercamp@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/137380671f01 Migrate PreferenceExperiments to allow for multiple prefs r=mythmon,leplatrem https://hg.mozilla.org/integration/autoland/rev/da4380fe5ce1 Migrate PreferenceExperiments observers r=mythmon https://hg.mozilla.org/integration/autoland/rev/454e86015872 PreferenceExperiments.start can take multiple prefs r=mythmon https://hg.mozilla.org/integration/autoland/rev/0de43d5275d1 PreferenceExperimentAction supports multiple prefs r=mythmon https://hg.mozilla.org/integration/autoland/rev/31cf332c9b21 Add userFacingName and userFacingDescription to schema r=mythmon
|
||
Comment 12•5 years ago
|
||
| bugherder | ||
https://hg.mozilla.org/mozilla-central/rev/137380671f01
https://hg.mozilla.org/mozilla-central/rev/da4380fe5ce1
https://hg.mozilla.org/mozilla-central/rev/454e86015872
https://hg.mozilla.org/mozilla-central/rev/0de43d5275d1
https://hg.mozilla.org/mozilla-central/rev/31cf332c9b21
Description
•