1293318 - TelemetryEnvironment should honour the throttling interval at the beginning of the session

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect, P1)

Description

[Alessio Placitelli [:Dexter]]

In bug 1292682 it was found that pref changes on the first run on a fresh profile generate a premature subsession split.

This is happening because we're picking a change to |browser.cache.disk.capacity| that's happening right after Telemetry inits, due to a special case at the beginning of the session [1].

To make subsession splits more robust, and avoid this (and potentially other) bogus very short subsession, we should always wait for CHANGE_THROTTLE_INTERVAL_MS (5 minutes) for environment changes.

[1] - https://dxr.mozilla.org/mozilla-central/rev/763fe887c37cee5fcfe0f00e94fdffc84a41ea1c/toolkit/components/telemetry/TelemetryEnvironment.jsm#1416

Comment 2

[Benjamin Smedberg]

I'm a bit concerned that we shouldn't be doing this throttling in TelemetryEnvironment.jsm but in the controller. It's important that other code which uses the environment (e.g. the crash service) to get more immediate updates.

Comment 3

[Georg Fritzsche [:gfritzsche]]

There are only two consumers for the environment change notifications - the crash service (crash ping) and TelemetrySession (main ping).

The crash service is already affected right now by the throttling, but the startup throttling would throw it off a lot more.
I'll see about moving the throttling over to TelemetrySession.

Comment 4

[Georg Fritzsche [:gfritzsche]]

Attachment: Always throttle environment changes and limit throttling to main pings

Comment 5

[Alessio Placitelli [:Dexter]]

Comment on attachment 8794248 [details] [diff] [review]
Always throttle environment changes and limit throttling to main pings

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

This looks good with the |const| issue from test_TelemetrySession.js cleared up.

If you think it's we can get rid of the fakeNow/futureDate calls from test_TelemetryEnvironment.js and you don't want to do that in this bug, feel free to move that part to another bug.

::: toolkit/components/telemetry/tests/unit/test_TelemetryEnvironment.js
@@ -1280,5 @@
>    // Uninstall the valid addon.
>    yield AddonManagerTesting.uninstallAddonByID(ADDON_ID);
>  });
>  
> -add_task(function* test_changeThrottling() {

Given that we're removing the throttling logic from test_TelemetryEnvironment, maybe we should also remove all the fakeNow/futureDate juggling from this test. I don't think they are needed any more, correct?

::: toolkit/components/telemetry/tests/unit/test_TelemetrySession.js
@@ +512,5 @@
>  
>    let now = new Date(2020, 1, 1, 12, 0, 0);
>    let expectedDate = new Date(2020, 1, 1, 0, 0, 0);
>    fakeNow(now);
> +  const gMonotonicNow = fakeMonotonicNow(5000);

Is the const a leftover?

Comment 6

[Georg Fritzsche [:gfritzsche]]

(In reply to Alessio Placitelli [:Dexter] from comment #5)
> ::: toolkit/components/telemetry/tests/unit/test_TelemetryEnvironment.js
> @@ -1280,5 @@
> >    // Uninstall the valid addon.
> >    yield AddonManagerTesting.uninstallAddonByID(ADDON_ID);
> >  });
> >  
> > -add_task(function* test_changeThrottling() {
> 
> Given that we're removing the throttling logic from
> test_TelemetryEnvironment, maybe we should also remove all the
> fakeNow/futureDate juggling from this test. I don't think they are needed
> any more, correct?

Good catch, cleaning that up.

Comment 7

[Georg Fritzsche [:gfritzsche]]

Attachment: Always throttle environment changes and limit throttling to main pings

Comment 8

[Georg Fritzsche [:gfritzsche]]

Attachment: Bonus: Modernize test_TelemetryEnvironment.js

Comment 9

[Alessio Placitelli [:Dexter]]

Comment on attachment 8795194 [details] [diff] [review]
Bonus: Modernize test_TelemetryEnvironment.js

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

This looks good (and thanks for doing that!) with the comment below cleared.

::: toolkit/components/telemetry/tests/unit/test_TelemetryEnvironment.js
@@ +67,5 @@
>  
>  // system add-ons are enabled at startup, so record date when the test starts
>  const SYSTEM_ADDON_INSTALL_DATE = Date.now();
>  
> +function isRejected(promise) {

This doesn't seem to be used anywhere in the file.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/21487ad720b3
Always throttle environment changes and limit throttling to main pings. r=dexter
https://hg.mozilla.org/integration/fx-team/rev/e289c0bdae84
Bonus: Modernize test_TelemetryEnvironment.js. r=dexter

Comment 11

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for failing new test test_TelemetrySession.js in test_TelemetrySession.js at least on Android 4.2 x86 opt:

https://hg.mozilla.org/integration/fx-team/rev/b0d4f06aabd64fe182c1f145d70c90681d91028f
https://hg.mozilla.org/integration/fx-team/rev/6099228b4131d3ce3ecd6fa5772a2d2431f532a0

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=fx-team&revision=e289c0bdae843b7262108b3c53460ae1622530e9
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=11726351&repo=fx-team

[task 2016-09-27T12:05:43.364016Z] 12:05:43     INFO -  TEST-START | toolkit/components/telemetry/tests/unit/test_TelemetrySession.js
[task 2016-09-27T12:06:26.383851Z] 12:06:26  WARNING -  TEST-UNEXPECTED-FAIL | toolkit/components/telemetry/tests/unit/test_TelemetrySession.js | xpcshell return code: 0
...
[task 2016-09-27T12:06:26.720966Z] 12:06:26     INFO -  TEST-PASS | toolkit/components/telemetry/tests/unit/test_TelemetrySession.js | test_changeThrottling - [test_changeThrottling : 1904] 1 == 1
[task 2016-09-27T12:06:26.721387Z] 12:06:26     INFO -  TEST-PASS | toolkit/components/telemetry/tests/unit/test_TelemetrySession.js | test_changeThrottling - [test_changeThrottling : 1911] 1 == 1
[task 2016-09-27T12:06:26.721784Z] 12:06:26  WARNING -  TEST-UNEXPECTED-FAIL | toolkit/components/telemetry/tests/unit/test_TelemetrySession.js | test_changeThrottling - [test_changeThrottling : 1917] 1 == 2
[task 2016-09-27T12:06:26.722048Z] 12:06:26     INFO -      test_TelemetrySession.js:test_changeThrottling:1917
[task 2016-09-27T12:06:26.722381Z] 12:06:26     INFO -      _run_next_test@/mnt/sdcard/tests/xpc/head.js:1566:9
[task 2016-09-27T12:06:26.722527Z] 12:06:26     INFO -      do_execute_soon/<.run@/mnt/sdcard/tests/xpc/head.js:713:9
[task 2016-09-27T12:06:26.722834Z] 12:06:26     INFO -      _do_main@/mnt/sdcard/tests/xpc/head.js:210:5
[task 2016-09-27T12:06:26.723142Z] 12:06:26     INFO -      _execute_test@/mnt/sdcard/tests/xpc/head.js:545:5
[task 2016-09-27T12:06:26.723437Z] 12:06:26     INFO -      @-e:1:1

Comment 12

[Georg Fritzsche [:gfritzsche]]

Untaking temporarily, will be picked back up soon.

Comment 13

[Alessio Placitelli [:Dexter]]

Attachment: Always throttle environment changes and limit throttling to main pings

I've rebased the patch against the latest m-c. The only change here is that I'm skipping |test_changeThrottling| on Android since we don't support subsessions there. Before this patch it wasn't getting executed due to the exclusion in the xpcshell.ini file. However, test_TelemetrySession.js is and that's why we were failing.

Comment 14

[Alessio Placitelli [:Dexter]]

Attachment: Bonus: Modernize test_TelemetryEnvironment.js

Just rebased.

Comment 15

[Alessio Placitelli [:Dexter]]

Comment on attachment 8804226 [details] [diff] [review]
Bonus: Modernize test_TelemetryEnvironment.js

Whoops, turns out this was already done in another bug, so this patch is no longer needed.

Comment 16

[Alessio Placitelli [:Dexter]]

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5979d774d278

Comment 17

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8804225 [details] [diff] [review]
Always throttle environment changes and limit throttling to main pings

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

rs=me

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0dba8df81286
Always throttle environment changes and limit throttling to main pings. r=dexter

Comment 19

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/0dba8df81286