1245544 - Telemetry experiments aren't expiring historical data properly

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect, P1)

Description

[Benjamin Smedberg]

I just checked about:support and discovered that the experiments section still has data about the nightly branching test:

Invisible test of the experiment branching system.	[e-mail]	An experiment using branches just to test whether branches get saved correctly.	false	1410111105504		0

That date is 2014-09-07T17:31:45.504Z which is well in the past: this data should have expired after 180 days.

Comment 1

[Georg Fritzsche [:gfritzsche]]

I'll look into it.

Comment 2

[Alessio Placitelli [:Dexter]]

Attachment: bug1245544.patch

This patch addresses two issues:

- Experiment info loaded from the cache was not being expired.
- |_lastChangedDate| (used by the expiration mechanism) was being refreshed loading from the cache, making it impossible to expire old experiment data. This is now set to |_endDate|, if that's available.

The patch also adds test coverage:

- It checks that terminated experiments data is properly expired after 180 date.
- It checks that the cache is properly updated.

Comment 3

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8725736 [details] [diff] [review]
bug1245544.patch

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

::: browser/experiments/test/xpcshell/test_cache.js
@@ +239,5 @@
>    experimentListData[0].active = false;
>    experimentListData[0].endDate = now.getTime();
>    checkExperimentListsEqual(experimentListData, list);
>    checkExperimentSerializations(experiments._experiments.values());
>  

So, the cache is not just about history.
We have 3 cases here i think:
(1) Historic experiments that ran on this profile
(2) Experiment that is still running now
(3) Experiment entries in the manifest that are cached locally and never ran

I would suggest that we use a separate test function to explicitly test through those different scenarios.

(1) is historic data and expiry should be fixed by your patch.
(2) is active data and needs to stay around.
(3) should be removed from cache when they are removed from the manifest (this is cached data, not really history).

Comment 4

[Alessio Placitelli [:Dexter]]

Attachment: bug1245544.patch

Thank you for the review. This patch adds a new test that goes through the scenarios you mentioned.

Comment 5

[Alessio Placitelli [:Dexter]]

Attachment: bug1245544_bonus.patch

Bonus: indentation fix for Experiments.jsm (tabs were being used instead of spaces).

Comment 6

[Alessio Placitelli [:Dexter]]

I've done some manual QA on this fix using a cache file with an expired experiment:

- Without this patch, running Nightly shows the experiment data both in about:support and about:addons
- With this patch, Nightly shows no experiment data in about:support nor in about:addons (the Experiments section doesn't show up, as it's empty).

Comment 7

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8726709 [details] [diff] [review]
bug1245544.patch

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

::: browser/experiments/test/xpcshell/test_cache.js
@@ +80,5 @@
>    }
>  }
>  
> +function validateCache(cachedExperiments, experimentIds) {
> +  let cachedExperimentIds = [...cachedExperiments].map(e => e.id);

Nit: It would be more consistent to expect |cachedExperiments| to be an array here and do the conversion at the call site.

@@ +84,5 @@
> +  let cachedExperimentIds = [...cachedExperiments].map(e => e.id);
> +  Assert.equal(cachedExperimentIds.length, experimentIds.length,
> +               "The number of cached experiments does not match with the provided list");
> +  for (let id of experimentIds) {
> +    Assert.ok(cachedExperimentIds.includes(id), "The cache must contain the experiment with id " + id);

Nit: Sounds like cachedExperimentIds should be a set?

@@ +321,5 @@
> +
> +  let now = null;
> +  let experiments = null;
> +
> +  let setDataAndRestartExperiments = new Task.async(function* (newDate) {

setDate... ?

@@ +328,5 @@
> +
> +    yield promiseRestartManager();
> +    experiments = new Experiments.Experiments(gPolicy);
> +    yield experiments._run();
> +  }.bind(this));

Nit: No need for .bind(this) here.

@@ +340,5 @@
> +  let list = yield experiments.getExperiments();
> +  Assert.equal(list.length, 0, "Experiment list should be empty.");
> +
> +  // Re-init, clock set for experiment 1 to start...
> +  yield setDataAndRestartExperiments(startDates[0]);

We could as well assert that the experiments list is still empty here.

@@ +353,5 @@
> +
> +  experimentListData[1].active = false;
> +  experimentListData[1].endDate = now.getTime();
> +  checkExperimentListsEqual(experimentListData.slice(1), list);
> +  validateCache(experiments._experiments.values(), [ EXPERIMENT1_ID, EXPERIMENT2_ID, EXPERIMENT3_ID ]);

Nit: No whitespaces around the array contents, ditto below.

@@ +369,5 @@
> +  checkExperimentListsEqual(experimentListData, list);
> +
> +  // Move the clock in the future, so that the cache for the first experiment expires
> +  // and the second experiment is still running.
> +  yield setDataAndRestartExperiments(futureDate(endDates[0], 181 * MS_IN_ONE_DAY));

It's a bit confusing to base those on:
* startDates[0]
* endDates[0]
* startDates[1]
* now endDates[0] ?

Basing this off startDates[1] and commenting appropriately seems less confusing.

@@ +372,5 @@
> +  // and the second experiment is still running.
> +  yield setDataAndRestartExperiments(futureDate(endDates[0], 181 * MS_IN_ONE_DAY));
> +  validateCache(experiments._experiments.values(), [ EXPERIMENT2_ID, EXPERIMENT3_ID ]);
> +
> +  // The first experiment should be expired and not in the cache, it ended more than

"it ended more than ..."?

@@ +390,5 @@
> +  // Test that experiments that are cached locally but never ran are removed from cache
> +  // when they are removed from the manifest (this is cached data, not really history).
> +  gManifestObject["experiments"] = gManifestObject["experiments"].slice(1, 1);
> +  yield experiments.updateManifest();
> +  validateCache(experiments._experiments.values(), [ EXPERIMENT2_ID ]);

.keys() has the experiment ids, no need to map later.
validateCache([...experiments._experiments.keys()], ...

Comment 8

[Alessio Placitelli [:Dexter]]

Attachment: bug1245544.patch

Comment 9

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8726829 [details] [diff] [review]
bug1245544.patch

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

Thanks.

Comment 10

[Alessio Placitelli [:Dexter]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=28d4538abc19

Comment 11

[Alessio Placitelli [:Dexter]]

https://hg.mozilla.org/integration/fx-team/rev/eb559f626161d47fb4c27cdbcddb212698b52d53
Bug 1245544 - Telemetry experiments aren't expiring historical data properly. r=gfritzsche

https://hg.mozilla.org/integration/fx-team/rev/fe1b8cb74332185af638d0cfe0565726042e66ea
Bug 1245544 - Fix the indentation in Experiments.jsm. r=gfritzsche

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/eb559f626161
https://hg.mozilla.org/mozilla-central/rev/fe1b8cb74332