Support reporting active experiments in FHR

RESOLVED FIXED in Firefox 43

Status

Android Background Services
Firefox Health Report Service
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: mfinkle, Assigned: mfinkle)

Tracking

unspecified
Firefox 43

Firefox Tracking Flags

(firefox43 fixed)

Details

Attachments

(5 attachments, 2 obsolete attachments)

The new A/B testing work will start using experiment cohorts for testing new features. In order to determine if one experiment variation is better than another, we need to track some metrics. One of those metrics is retention, which is currently tracked in Fennec using FHR. In order to correlate experiments with retention, we need to track active experiments in the FR payload, much like we do with active add-ons.

An array of strings should be good enough to provide correlation data.

If this could be done in Unified Telemetry, we should consider that instead of FHR. Mostly saying that because Unified Telemetry is the future.
In Telemetry we would track this in the environment data, prior art there would e.g. be |activeExperiment| (http://bit.ly/1hYNyOh)

Desktop FHR already has a "org.mozilla.experiments" provider.
(In reply to Georg Fritzsche [:gfritzsche] from comment #1)
> In Telemetry we would track this in the environment data, prior art there
> would e.g. be |activeExperiment| (http://bit.ly/1hYNyOh)
> 
> Desktop FHR already has a "org.mozilla.experiments" provider.

Georg - This is kinda awesome. It almost handles our need. It looks like this provide only tracks the last active experiment. We expect more than one experiment to be active at a time. Do you think I could rev the version to v3 and add an array of active experiments? Something like:

"org.mozilla.experiments.info": {
  "_v": 3,
  "allActive": ["some.experiment.id", "another.experiment.id"]
}

https://gecko.readthedocs.org/en/latest/services/healthreport/healthreport/dataformat.html#org-mozilla-experiments-info
Sure, we could just bump that to v3, make it suit your needs on mobile and document the differences to desktop etc.
Note that this is currently implemented desktop-only - rnewman would probably know where best to put it on mobile?

I'm not sure offhand what the best option is to store a daily list of strings here.
"org.mozilla.addons" uses the "store JSON strings in a daily-last-text field" hack, other dynamic fields are all key-value like "org.mozilla.searches" counts.
Flags: needinfo?(rnewman)
Android's FHR storage has more flexibility than desktop's, so we can store a discrete list of values that apply per day (recordDailyDiscrete), and each value can be int/String/JSON. We can also store keyed counts, as we do for searches.

The main open question will be figuring out when to pass the values to Java to write into health report storage. In the v3 FHR format almost every contextual value like this lives in the environment, so measurements are all written in response to user events. I don't want to rev the environment for this, so we would need to figure something else out.
Flags: needinfo?(rnewman)
I talked to Richard a little about this work. I started added "org.mozilla.experiments.active" as a first-class provider, but it's non-trivial. We also have thoughts about moving away from FHR to Unified Telemetry as well (bug 1183320). Richard had suggested a simple fix of treating the experiments as add-ons, which are already supported.

We were a bit worried about the post-scripting analysis and creating fake add-ons, but I think it's probably the least-worst way forward here.
Created attachment 8657600 [details] [diff] [review]
switchboard-fhr WIP

Here is my first cut. The patch adds a method to SwitchBoard so we can extract the list of active experiments. Then , we create a fake add-on for each active experiment. I do two things with the fake add-ons:

1. Create a name of the form "<experiment-name>@experiments.mozilla.org" which matches the form Telemetry Experiments use.
2. Set the "type" field to "experiment".

I did not add all the other fields that an add-on uses. I might need to, depending on how the client-side system reacts.

Currently, I am debugging why the code path for "HealthReport:RequestSnapshot" is not firing. Based on the logcat, it looks like the addOnDistributionReadyCallback is never being called.
Assignee: nobody → mark.finkle
We don't add a distribution hook if we successfully restore the ProfileInformationCache from the last run. The snapshot should only be requested if that's missing -- i.e., on first run, after Clear Data, or if the PIC data format has changed.

That's to avoid us tickling AddonManager and doing the message exchange/filter/JSONify/hash/env/DB dance unnecessarily.

In normal use we expect incremental Addons:Change (and friends) events to be sent proactively from Gecko, so we incrementally update the PIC and update the current environment.

What you're trying to do here is add the current set of SwitchBoard experiments to the add-on set. That's a two-parter: include them going forward, and get them included now.

The easiest way to do what you're trying to achieve:

1. Add experiments to the addons set when you receive a snapshot. That's what your patch does now. This only works for new profiles.

2. Bump the ProfileInformationCache version to trigger a new snapshot for existing profiles. That'll make your code work for existing profiles... once. See the comment in ProfileInformationCache.java:

  /*
   * FORMAT_VERSION history:
   *   -: No version number; implicit v1.
   *   1: Add versioning (Bug 878670).
   *   2: Bump to regenerate add-on set after landing Bug 900694 (Bug 901622).
   *   3: Add distribution, osLocale, appLocale.
   */
  public static final int FORMAT_VERSION = 3;


3. Proactively tell BHR about added and removed 'addons' as the SwitchBoard experiments change, not just in bulk in response to a snapshot event. That should be easy. See how BrowserHealthRecorder handles EVENT_ADDONS_UNINSTALLING and EVENT_ADDONS_CHANGE.
(In reply to Richard Newman [:rnewman] from comment #7)

Thanks for the guidance.

> The easiest way to do what you're trying to achieve:
> 
> 1. Add experiments to the addons set when you receive a snapshot. That's
> what your patch does now. This only works for new profiles.

Good to know.

> 2. Bump the ProfileInformationCache version to trigger a new snapshot for
> existing profiles. That'll make your code work for existing profiles...
> once. See the comment in ProfileInformationCache.java:

I suppose that's easy enough, unless supporting #3 means I remove the code altogether from the snapshot code path.

> 3. Proactively tell BHR about added and removed 'addons' as the SwitchBoard
> experiments change, not just in bulk in response to a snapshot event. That
> should be easy. See how BrowserHealthRecorder handles
> EVENT_ADDONS_UNINSTALLING and EVENT_ADDONS_CHANGE.

Easy in interacting with BHR, but hard when dealing with SwitchBoard, since it has no concept of "added" and "removed" experiments. I could build some kind of diffing system that compares the existing (if any) experiments to the new, incoming set retrieved from the server.

What kind of damage would I do the the PIC if I treated each startup as: remove all "@experiments.mozilla.org" addons, then add the current set back?

I suppose it might not be too much more work to add some sort of "diffing" to the code somewhere. I want to keep SwitchBoard decoupled so I don't think I'll impose FHR semantics to it. BHR seems the most likely candidate.
The main constraints:

* We want to avoid recalculating the environment on each launch.

* We want to avoid writing the PIC unnecessarily.


In both cases, that means two things:

* Recognizing when the set of experiments is the same as on the last launch (or the last time we computed the environment or changed the PIC).

* Processing the entire set of experiments at once, without recording an environment change for each.

That implies that remove-and-re-add isn't quite the way we should go, if only to avoid excessive disk writes and hashing.


There are two places we could track the current set of experiments:

* In BHR/the PIC itself, just as we do for locales -- see ProfileInformationCache.setAppLocale, which has a short-circuit block. That is, we'd ignore a new set of experiments if it's identical to the current set.

* In whatever code encapsulates SwitchBoard management (perhaps SwitchBoard itself, if it knows the current set of experiments and has well-defined times that the set changes). That is, we wouldn't poke BHR unless the set changes.


Both are pretty straightforward. The latter will be slightly more efficient and gives us a single place that manages experiments (perhaps useful if we switch to telemetry reporting?), but I don't know how we feel about touching that code.

That doesn't necessarily imply coupling SwitchBoard to BHR: we can do this through a LocalBroadcast ("ExperimentsDidChange" or whatever), for which either BHR or BrowserApp can listen.
Incoming set of patches that hope to implement all three points in comment 7
Created attachment 8657960 [details] [diff] [review]
switchboard-getactive v0.1

Adds SwitchBoard.getActiveExperiments, which returns a List<String> of active experiment names.
Attachment #8657600 - Attachment is obsolete: true
Attachment #8657960 - Flags: review?(rnewman)
Created attachment 8657961 [details] [diff] [review]
switchboard-broadcast v0.1

Patch adds a a simple LocalBroadcastManager broadcast after the configuration code path finishes. It currently does not send any status of success or failure. It only lets other code that the networking is finished.
Attachment #8657961 - Flags: review?(rnewman)
Created attachment 8657962 [details] [diff] [review]
profilecache-bump-version v0.1

Bumps the profile cache version. As Richard noted, this will force all existing profiles to be dropped and recalculated. Seems to work just fine.
Attachment #8657962 - Flags: review?(rnewman)
Created attachment 8657963 [details] [diff] [review]
bhr-switchboard-receiver v0.1

This patch uses the LocalBroadcastManager broadcast from SwitchBoard to start a simple-minded "diff" search using the current profile cache and the current active set of experiments. The end goal is to only remove experiment add-ons that are no longer in the active set and to add only new experiment add-ons that are not already in the profile.

This should achieve the requirements noted in comment 9.
Attachment #8657963 - Flags: review?(rnewman)
Created attachment 8657964 [details] [diff] [review]
bhr-switchboard-snapshot v0.1

This last patch handles the new profile snapshot.
Attachment #8657964 - Flags: review?(rnewman)
Comment on attachment 8657960 [details] [diff] [review]
switchboard-getactive v0.1

Review of attachment 8657960 [details] [diff] [review]:
-----------------------------------------------------------------

All of this code is awful, which I guess means it's copypasta'd. If it works, fine, but even better if you clean it up.

::: mobile/android/thirdparty/com/keepsafe/switchboard/SwitchBoard.java
@@ +298,5 @@
> +			return returnList;
> +		}
> +
> +		try {
> +			JSONObject experiments = (JSONObject) new JSONObject(config);

No need for this cast. (Also clean up the rest of this method if you can.)
Attachment #8657960 - Flags: review?(rnewman) → review+
Attachment #8657961 - Flags: review?(rnewman) → review+
Attachment #8657962 - Flags: review?(rnewman) → review+
Comment on attachment 8657963 [details] [diff] [review]
bhr-switchboard-receiver v0.1

Review of attachment 8657963 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with those changes.

::: mobile/android/base/health/BrowserHealthRecorder.java
@@ +198,5 @@
>  
>          this.state = State.CLOSED;
>          this.unregisterEventListeners();
> +        if (AppConstants.MOZ_SWITCHBOARD) {
> +            LocalBroadcastManager.getInstance(GeckoAppShell.getContext()).unregisterReceiver(this);

Add a `context` argument to close() and pass `GeckoApp.this` as the value from GeckoApp's call site.

@@ +494,5 @@
>  
> +                        // Listen for experiment changes.
> +                        if (AppConstants.MOZ_SWITCHBOARD) {
> +                            IntentFilter intentFilter = new IntentFilter(SwitchBoard.ACTION_CONFIG_FETCHED);
> +                            LocalBroadcastManager.getInstance(GeckoAppShell.getContext()).registerReceiver(self, intentFilter);

Pass `context` as a final argument into `initializeStorage` from its caller in `initialize`. Don't use GAS if you can avoid it.

@@ +729,5 @@
> +                // Create a list of add-ons(experiments) we need to remove.
> +                ArrayList<String> removeFromProfile = new ArrayList<String>();
> +
> +                // Loop over the current profile set of add-ons, and figure out
> +                // which add-ons(experiments) are new and which need to be removed.

Nit: space before "(".

@@ +735,5 @@
> +                Iterator<?> keys = addons.keys();
> +                while (keys.hasNext()) {
> +                    String addon = (String) keys.next();
> +                    if (addon.endsWith("@experiments.mozilla.org")) {
> +                        if (addToProfile.contains(addon) == false) {

Invert the clauses (to make this == true), then remove the == true.
Attachment #8657963 - Flags: review?(rnewman) → review+
Attachment #8657964 - Flags: review?(rnewman) → review+
(In reply to Richard Newman [:rnewman] from comment #17)
> Comment on attachment 8657963 [details] [diff] [review]
> bhr-switchboard-receiver v0.1
> 
> Review of attachment 8657963 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ with those changes.
> 
> ::: mobile/android/base/health/BrowserHealthRecorder.java
> @@ +198,5 @@
> >  
> >          this.state = State.CLOSED;
> >          this.unregisterEventListeners();
> > +        if (AppConstants.MOZ_SWITCHBOARD) {
> > +            LocalBroadcastManager.getInstance(GeckoAppShell.getContext()).unregisterReceiver(this);
> 
> Add a `context` argument to close() and pass `GeckoApp.this` as the value
> from GeckoApp's call site.

This would work OK.

IntentFilter(SwitchBoard.ACTION_CONFIG_FETCHED);
> > +                            LocalBroadcastManager.getInstance(GeckoAppShell.getContext()).registerReceiver(self, intentFilter);
> 
> Pass `context` as a final argument into `initializeStorage` from its caller
> in `initialize`. Don't use GAS if you can avoid it.

initializeStorage is called in two other places and I don't have ready access to a Context in one of them. I really want to avoid GAS, but I don't think I can. If I can't remove GAS from initializeStorage, should I keep it in close()? I like the symmetry.

> > +                        if (addToProfile.contains(addon) == false) {
> 
> Invert the clauses (to make this == true), then remove the == true.

Done
I should have NI'd about the GAS.getContext issue above.
Flags: needinfo?(rnewman)
(In reply to Mark Finkle (:mfinkle) from comment #18)

> initializeStorage is called in two other places and I don't have ready
> access to a Context in one of them. I really want to avoid GAS, but I don't
> think I can. If I can't remove GAS from initializeStorage, should I keep it
> in close()? I like the symmetry.

I'd normally be in favor of either doing the chaining necessary to avoid entirely, or symmetry.

In this case, though: if we can't add the chaining I'd be inclined to take non-symmetry -- if the call sites of initializeStorage change for another reason, I prefer to be standing on the shortest path to get rid of GAS. One fewer GAS use is worth it.

I don't feel super strongly; if you do, so be it.
Status: NEW → ASSIGNED
Flags: needinfo?(rnewman)
Created attachment 8659288 [details] [diff] [review]
bhr-switchboard-receiver v0.2

Changes:
1. I had to update HealthRecorder interface and the Stub for the close(Context context) change
2. GeckoApp passes the context to close(...)
3. BHR now only uses the receiver if we are reloading the profile cache. If we are building a new cache, the SNAPSHOT code will handle adding the experiments to the profile.
4. I had to add this.onEnvironmentChanged() calls to the onReceive handler when adding or removing experiments. I put the call in the for loop for simplicity. Hopefully that won't cause environment thrashing.
Attachment #8657963 - Attachment is obsolete: true
Attachment #8659288 - Flags: review?(rnewman)
Comment on attachment 8659288 [details] [diff] [review]
bhr-switchboard-receiver v0.2

Review of attachment 8659288 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/health/BrowserHealthRecorder.java
@@ +716,5 @@
> +        switch (action) {
> +            case SwitchBoard.ACTION_CONFIG_FETCHED:
> +                Log.d(LOG_TAG, "Handle the new experiments.");
> +                // Get the list of active experiments.
> +                List<String> experiments = SwitchBoard.getActiveExperiments(GeckoAppShell.getContext());

Use `context` here.

@@ +745,5 @@
> +                        }
> +                    }
> +                }
> +
> +                // Add the newly active experiments into the profile.

Right here, if both addToProfile and removeFromProfile are empty, bail out.

@@ +752,5 @@
> +                        // Create a dummy JSON object for the experiment.
> +                        JSONObject fakeAddon = new JSONObject();
> +                        fakeAddon.put("type", "experiment");
> +                        this.onAddonChanged(fakeName, fakeAddon);
> +                        this.onEnvironmentChanged();

Call this only once, after doing all the additions and removals (line 768). This is the expensive part.
Comment on attachment 8659288 [details] [diff] [review]
bhr-switchboard-receiver v0.2

r+ if those changes go smoothly!
Attachment #8659288 - Flags: review?(rnewman) → review+
You need to log in before you can comment on or make changes to this bug.