Closed Bug 1547034 Opened 6 months ago Closed 5 months ago

Add support for preference experiments that set multiple preferences

Categories

(Firefox :: Normandy Client, enhancement)

67 Branch
enhancement
Not set

Tracking

()

RESOLVED FIXED
Firefox 68
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.

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?

Attachment #9061537 - Attachment description: Bug 1547034: Migrate PreferenceExperiments to allow for multiple prefs f?mythmon → Bug 1547034: Migrate PreferenceExperiments to allow for multiple prefs

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

Add a little bit to some existing tests to cover this new
functionality.

Depends on D29871

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

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.

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

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.

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.

I rebased this onto relatively recent mozilla-central. Try build running at https://treeherder.mozilla.org/#/jobs?repo=try&revision=67e3e61da7255b85116ae51da10c25cc12577949

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
Regressions: 1553198
You need to log in before you can comment on or make changes to this bug.