841568 - collectMeasurements() only called at upload time

Status: RESOLVED FIXED

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

Description

[Gregory Szorc [:gps]]

Currently pull-based metrics are only collected at upload time. This means that if a user has opted out of FHR (data collection still occurs if they are opted out - only upload is disabled) that not all data will be pulled into the FHR database. This means that the value of about:healthreport is diminished for users that have opted out.

I'm asserting this is a beta blocker. Since code changes will be contained to FHR JS, I'm guessing we can uplift this to Aurora without much issue.

Comment 1

[Richard Newman [:rnewman]]

Also necessary for Android: "upload" should really be "document generation", with upload being a separate phase (that will differ between platforms).

Comment 2

[Gregory Szorc [:gps]]

We could consider using nsIUpdateTimerManager to handle scheduling of collection. https://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/nsIUpdateTimerManager.idl

Comment 3

[Gregory Szorc [:gps]]

Blair granted permission on IRC to use nsIUpdateTimerManager despite its name. Apparently there is precedent.

Comment 4

[Gregory Szorc [:gps]]

Attachment: Install timer for daily collection, v1

This is somewhat hacky and fragile. More robust solutions were not chosen because this code will be refactored for Android and I didn't think the effort was worth it.

I wish I could deploy a more robust test. However, the timer manager does not easily allow itself to be tested. Perhaps one day somebody will write a testing-only module to help with testing installed timers. See https://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/test_timermanager/unit/test_0010_timermanager.js for what it would take.

Comment 5

[Richard Newman [:rnewman]]

Comment on attachment 724243 [details] [diff] [review]
Install timer for daily collection, v1

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

Movin'.

::: services/healthreport/healthreporter.jsm
@@ +220,5 @@
> +      // the timer ID.
> +      let timerName = this._branch.replace(".", "-", "g") + "lastDailyCollection";
> +      let tm = Cc["@mozilla.org/updates/timer-manager;1"]
> +                 .getService(Ci.nsIUpdateTimerManager);
> +      tm.registerTimer(timerName, this.collectMeasurements.bind(this),

Is there any chance that either of these will fail?

It seems sensible to wrap this in a try block.

Comment 6

[Gregory Szorc [:gps]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7190b9dd7f51

with a try block.

Comment 7

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/7190b9dd7f51

Comment 8

[Gregory Szorc [:gps]]

Comment on attachment 724243 [details] [diff] [review]
Install timer for daily collection, v1

[Approval Request Comment]
Bug caused by (feature/regressing bug #): FHR initial landing.
User impact if declined: FHR won't collect data unless submission is enabled.
Testing completed (on m-c, etc.): It has baked for a few days. There is test coverage.
Risk to taking this patch (and alternatives if risky): Low risk from a technical standpoint.
String or UUID changes made by this patch: None

Unlike the uplift requests I just filed, this one is more important. A P1 launch requirement for FHR is that FHR collect data even if upload is disabled. This is to allow people to utilize the benefits of about:healthreport while not periodically submitting data to Mozilla. This is an important adherence to Mozilla's principle of users being in control of their data. This should have made the initial code drop of FHR but it fell through the cracks.

Comment 9

[Gregory Szorc [:gps]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/41e28777f350