Closed Bug 1293318 Opened 3 years ago Closed 3 years ago

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

Categories

(Toolkit :: Telemetry, defect, P1)

defect
Points:
2

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox51 --- affected
firefox52 --- fixed

People

(Reporter: Dexter, Assigned: Dexter)

References

Details

(Whiteboard: [measurement:client])

Attachments

(1 file, 4 obsolete files)

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
Priority: -- → P3
Whiteboard: [measurement:client]
Assignee: nobody → gfritzsche
Priority: P3 → P1
Duplicate of this bug: 1302812
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.
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.
Points: --- → 2
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?
Attachment #8794248 - Flags: review?(alessio.placitelli) → review+
(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.
Attachment #8794248 - Attachment is obsolete: true
Attachment #8795194 - Flags: review?(alessio.placitelli)
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.
Attachment #8795194 - Flags: review?(alessio.placitelli) → review+
Pushed by georg.fritzsche@googlemail.com:
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
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
Flags: needinfo?(gfritzsche)
Untaking temporarily, will be picked back up soon.
Assignee: gfritzsche → nobody
Flags: needinfo?(gfritzsche)
Assignee: nobody → alessio.placitelli
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.
Attachment #8795185 - Attachment is obsolete: true
Attachment #8804225 - Flags: review?(gfritzsche)
Just rebased.
Attachment #8804226 - Flags: review+
Attachment #8795194 - Attachment is obsolete: true
Attachment #8804226 - Attachment is patch: true
Attachment #8804225 - Attachment is patch: true
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.
Attachment #8804226 - Attachment is obsolete: true
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
Attachment #8804225 - Flags: review?(gfritzsche) → review+
Status: NEW → ASSIGNED
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0dba8df81286
Always throttle environment changes and limit throttling to main pings. r=dexter
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/0dba8df81286
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.