Telemetry experiments aren't expiring historical data properly

RESOLVED FIXED in Firefox 48

Status

()

Toolkit
Telemetry
P1
normal
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: Benjamin Smedberg, Assigned: Dexter)

Tracking

unspecified
mozilla48
Points:
2

Firefox Tracking Flags

(firefox48 fixed)

Details

(Whiteboard: [measurement:client])

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

3 years ago
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.	experiment-branch-test-nightly@experiments.mozilla.org	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.
Flags: needinfo?(gfritzsche)
I'll look into it.
Flags: needinfo?(gfritzsche)
Priority: -- → P1
Whiteboard: [measurement:client]
Points: --- → 2
(Assignee)

Updated

2 years ago
Assignee: nobody → alessio.placitelli
(Assignee)

Comment 2

2 years ago
Created attachment 8725736 [details] [diff] [review]
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.
Attachment #8725736 - Flags: review?(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).
Attachment #8725736 - Flags: review?(gfritzsche) → feedback+
(Assignee)

Comment 4

2 years ago
Created attachment 8726709 [details] [diff] [review]
bug1245544.patch

Thank you for the review. This patch adds a new test that goes through the scenarios you mentioned.
Attachment #8725736 - Attachment is obsolete: true
Attachment #8726709 - Flags: review?(gfritzsche)
(Assignee)

Comment 5

2 years ago
Created attachment 8726710 [details] [diff] [review]
bug1245544_bonus.patch

Bonus: indentation fix for Experiments.jsm (tabs were being used instead of spaces).
Attachment #8726710 - Flags: review?(gfritzsche)
Attachment #8726710 - Flags: review?(gfritzsche) → review+
(Assignee)

Comment 6

2 years ago
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 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()], ...
Attachment #8726709 - Flags: review?(gfritzsche) → feedback+
(Assignee)

Comment 8

2 years ago
Created attachment 8726829 [details] [diff] [review]
bug1245544.patch
Attachment #8726709 - Attachment is obsolete: true
Attachment #8726829 - Flags: review?(gfritzsche)
Comment on attachment 8726829 [details] [diff] [review]
bug1245544.patch

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

Thanks.
Attachment #8726829 - Flags: review?(gfritzsche) → review+
(Assignee)

Comment 11

2 years ago
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

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/eb559f626161
https://hg.mozilla.org/mozilla-central/rev/fe1b8cb74332
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.