collectMeasurements() only called at upload time

RESOLVED FIXED in Firefox 21


6 years ago
8 months ago


(Reporter: gps, Assigned: gps)


Firefox 22
Dependency tree / graph

Firefox Tracking Flags

(firefox20 unaffected, firefox21 fixed)



(1 attachment)



6 years ago
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.
Also necessary for Android: "upload" should really be "document generation", with upload being a separate phase (that will differ between platforms).
Priority: -- → P1

Comment 2

6 years ago
We could consider using nsIUpdateTimerManager to handle scheduling of collection.

Comment 3

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

Comment 4

6 years ago
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 for what it would take.
Assignee: nobody → gps
Attachment #724243 - Flags: review?(rnewman)
Comment on attachment 724243 [details] [diff] [review]
Install timer for daily collection, v1

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


::: services/healthreport/healthreporter.jsm
@@ +220,5 @@
> +      // the timer ID.
> +      let timerName = this._branch.replace(".", "-", "g") + "lastDailyCollection";
> +      let tm = Cc[";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.
Attachment #724243 - Flags: review?(rnewman) → review+

Comment 6

6 years ago

with a try block.
Target Milestone: --- → mozilla22
Closed: 6 years ago
Resolution: --- → FIXED

Comment 8

6 years ago
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.
Attachment #724243 - Flags: approval-mozilla-aurora?
Attachment #724243 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Component: Metrics and Firefox Health Report → Client: Desktop
Flags: in-testsuite+
Product: Mozilla Services → Firefox Health Report
Target Milestone: mozilla22 → Firefox 22


8 months ago
Product: Firefox Health Report → Firefox Health Report Graveyard
You need to log in before you can comment on or make changes to this bug.