987225 - Telemetry experiments: Combine pending tasks into a serial task

Status: RESOLVED FIXED

(Firefox Health Report Graveyard :: Client: Desktop, defect)

Comment 1

[Benjamin Smedberg]

Attachment: telex-singletask

You really need to review the -w version of this patch, but I can't get reviewboard to work.

Comment 2

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8396092 [details] [diff] [review]
telex-singletask

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

This is definitely better and easier to argue about, thanks.
Mostly smaller comments/questions following.

::: browser/experiments/Experiments.jsm
@@ +272,5 @@
> +  // The _main task handles all other actions:
> +  // * refreshing the manifest off the network (if _refresh)
> +  // * disabling/enabling experiments
> +  // * saving the cache (if _dirty)
> +  this._runningTask = null;

Nit: _runningTask sounds like a boolean, maybe _mainTask or _runTask?

@@ +331,5 @@
>        gPrefsTelemetry.ignore(PREF_TELEMETRY_PRERELEASE, this._telemetryStatusChanged, this);
>  
>        if (this._timer) {
>          this._timer.clear();
> +        this._timer = null;

You don't need to null out the timer here or later, clear() already takes care of that:
http://hg.mozilla.org/mozilla-central/annotate/5c0673441fc8/services/common/utils.js#l216

@@ +492,5 @@
> +
> +  _main: function*() {
> +    do {
> +      if (this._loadTask) {
> +        yield this._loadTask;

Checking on this, you can actually just |yield null| so we don't need the branching here.

@@ +526,5 @@
> +
> +      let data = JSON.parse(responseText);
> +      this._updateExperiments(data);
> +    } catch (e) {
> +      gLogger.error("Experiments::_loadManifest - failure to fetch/parse manifest (continuing anyway)" + e);

Add some separation/space between the message and the error here?

@@ +691,5 @@
>    _populateFromCache: function (data) {
>      gLogger.trace("Experiments::populateFromCache() - data: " + JSON.stringify(data));
>  
> +    // If the user has a newer cache version than we can understand, we fail
> +    // hard; no experiments should be active in this older client.

Might potentially be "newer client"/"older cache version" too? (e.g. switching from Nightly to Release)

@@ +786,2 @@
>  
> +    this._terminateCurrent = userDisabled ? TELEMETRY_LOG.TERMINATION.USERDISABLED : TELEMETRY_LOG.TERMINATION.FROM_API;

_terminateCurrent is overloaded and used both as a flag and to carry the reason.
Maybe _terminateCurrent should be _terminateReason to convey what's actually stored in it?

@@ +826,5 @@
>            } catch (e) {
>              gLogger.error(e);
>              // On failure try the next experiment.
>              activeExperiment = null;
> +            this._dirty = true;

I think we should also set _dirty when it didn't fail as details on the entry can have changed (e.g. the description/name).

::: browser/experiments/test/xpcshell/test_api.js
@@ +166,5 @@
>    gTimerScheduleOffset = -1;
>    defineNow(gPolicy, now);
>  
>    yield experiments.updateManifest();
> +  dump("updateManifest done\n");

Left-over.

@@ +207,5 @@
>    Assert.equal(gTimerScheduleOffset, startDate2 - now,
>                 "Experiment re-evaluation should have been scheduled correctly.");
>  
>    // Trigger update, clock set for experiment 2 to start.
> +  // Use _run() to wait for "synchonous" re-evaluation

Why drop our coverage for the notify() path?

Comment 3

[Benjamin Smedberg]

Attachment: telex-singletask

Felipe, not sure how much you want to review this or delegate to gfritzsche based on his prior f+.

Comment 4

[Benjamin Smedberg]

> > +    // If the user has a newer cache version than we can understand, we fail
> > +    // hard; no experiments should be active in this older client.
> 
> Might potentially be "newer client"/"older cache version" too? (e.g.
> switching from Nightly to Release)

Newer clients ought to be able to import/upgrade older cache versions.

Comment 5

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8396432 [details] [diff] [review]
telex-singletask

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

::: browser/experiments/Experiments.jsm
@@ +798,5 @@
> +    if (activeExperiment) {
> +      this._pendingUninstall = activeExperiment._addonId;
> +      try {
> +        let wasStopped;
> +        if (this._terminateReason) {

Cross-checking my rebases i noticed that you don't seem to actually reset _terminateReason if this path hit.
It looks like this will always do a second iteration in _main() and only null it then.

Comment 6

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8396432 [details] [diff] [review]
telex-singletask

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

I'm happy with gfritzsche doing the final review. Just two notes:

::: browser/experiments/Experiments.jsm
@@ +652,3 @@
>     * Save the experiment data on disk. Returns a promise that
>     * is resolved when the operation is done.
>     */

Some of the comments are incorrect now as this function doesn't return a promise by itself, but it's meant to be run as a task from Task.spawn.
Can we have a brief comment on all functions that are not meant to be called directly?

@@ +685,5 @@
>    _populateFromCache: function (data) {
>      gLogger.trace("Experiments::populateFromCache() - data: " + JSON.stringify(data));
>  
> +    // If the user has a newer cache version than we can understand, we fail
> +    // hard; no experiments should be active in this older client.

Does it mean though that after a version downgrade, an experiment can still be re-activated after retrieving the manifest again? I imagine we want to support version downgrades if there are still experiments targeting that version

Comment 7

[Benjamin Smedberg]

In general I don't expect the cache format to change, so I don't expect version downgrades to matter. In the event we do change the cache format, the following behavior would apply:

* Once the profile has upgraded, older clients won't run the experiment system at all. The experiment list in the addon manager will be empty
* New clients will continue to run applicable experiments while their conditions matched

I'll go through the doccomments and make sure that everything is up to date.

Comment 8

[Benjamin Smedberg]

Attachment: telex-singletask rev. 3

Comment 9

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8396502 [details] [diff] [review]
telex-singletask rev. 3

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

This looks good to me.

I'd personally love to unnest _evaluateExperiments() as it is becoming harder to follow, but we can do that later.

Comment 10

[Benjamin Smedberg]

https://hg.mozilla.org/integration/fx-team/rev/12d30d5980b1

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/12d30d5980b1