Closed Bug 846083 Opened 7 years ago Closed 7 years ago

Submit an FHR payload if initialization has failed

Categories

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

defect

Tracking

(firefox20 unaffected, firefox21 fixed)

RESOLVED FIXED
Firefox 22
Tracking Status
firefox20 --- unaffected
firefox21 --- fixed

People

(Reporter: gps, Assigned: gps)

Details

Attachments

(1 file)

Currently, if FHR fails to initialize we don't do much. The HealthReporter instance is sitting around but it has no storage and collector. Since the instance isn't initialized, the request to upload is essentially a no-op and no ping is seen. If you figure that init errors are likely to recur, this would manifest as an FHR drop-off over time.

I think it would be a nice feature if we still managed to submit an FHR payload even if initialization failed. The payload would contain an error message so we can help triage what kinds of issues people are having and where we need rigor in the initialization sequence.
This will likely be much easier once a document handler entity comes into existence. We can just generate a stub document if init fails and hand it off to the document handler.
Priority: -- → P2
I believe this should do it.

This patches really exposes the need for a document handler! The big delta in the middle is due to indentation. At some point we should factor out payload generation into helper functions!
Assignee: nobody → gps
Status: NEW → ASSIGNED
Attachment #725560 - Flags: review?(rnewman)
Comment on attachment 725560 [details] [diff] [review]
Submit payload on init failure, v1

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

I'm happy enough with this.

::: services/healthreport/healthreporter.jsm
@@ +710,1 @@
>      }

This method is getting big :/

@@ +1083,5 @@
> +    // backoff. We should arguably not do this. However, reporting
> +    // startup errors is important. And, they should not occur with much
> +    // frequency in the wild. So, it shouldn't be too big of a deal.
> +    if (!inShutdown && this._policy.ensureNotifyResponse(new Date()) &&
> +        this._policy.healthReportUploadEnabled) {

Three lines, plz.
Attachment #725560 - Flags: review?(rnewman) → review+
https://hg.mozilla.org/mozilla-central/rev/74b3075c2d94
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [fixed in services]
Target Milestone: --- → mozilla22
Comment on attachment 725560 [details] [diff] [review]
Submit payload on init failure, v1

[Approval Request Comment]
Bug caused by (feature/regressing bug #): FHR
User impact if declined: FHR errors during initialization will not be reported.
Testing completed (on m-c, etc.): It's been on the tree for a few days. There is test coverage.
Risk to taking this patch (and alternatives if risky): Low risk.
String or UUID changes made by this patch: None

This patch enables FHR to self-report errors that occur on startup. We have reason to believe that a non-trivial amount of clients may experience errors on startup that are not yet captured by FHR's error reporting mechanism. We want these errors reported so we can identify and fix issues sooner.

If we fail to uplift this patch, FHR init errors may not be identified until 22 has a sufficient user base. As a result, we may find ourselves uplifting patches to beta instead of aurora since error discovery was delayed by a release.
Attachment #725560 - Flags: approval-mozilla-aurora?
Comment on attachment 725560 [details] [diff] [review]
Submit payload on init failure, v1

Patch helps self report start-up errors for FHR, catching these will help us identify/fix issues sooner on our end .Its low risk & has test's .
Attachment #725560 - 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
Product: Firefox Health Report → Firefox Health Report Graveyard
You need to log in before you can comment on or make changes to this bug.