Closed Bug 1113675 Opened 9 years ago Closed 9 years ago

On FHR AsyncShutdownTimeout, submit which provider is hanging in collection

Categories

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

defect
Not set
normal

Tracking

(firefox35 fixed, firefox36 fixed, firefox37 fixed)

RESOLVED FIXED
Firefox 37
Tracking Status
firefox35 --- fixed
firefox36 --- fixed
firefox37 --- fixed

People

(Reporter: gfritzsche, Assigned: gfritzsche)

References

Details

Attachments

(1 file)

Fixing collection hangs is on our list (bug 1030270), but in the meantime we should get AsyncShutdownTimeout data on which provider hung during collection.
Assignee: nobody → gfritzsche
Can you review this Benjamin? I'm not sure who's still around.
Attachment #8540831 - Flags: review?(benjamin)
Attachment #8540831 - Flags: review?(gps)
Attachment #8540831 - Flags: review?(benjamin)
Attachment #8540831 - Flags: review?
Status: NEW → ASSIGNED
Comment on attachment 8540831 [details] [diff] [review]
Add FHR provider collection diagnost

I don't think I should. I'm not familiar with this code.
Attachment #8540831 - Flags: review?
Comment on attachment 8540831 [details] [diff] [review]
Add FHR provider collection diagnost

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

This is better than nothing. But I really think the implementation is... too simple.

The collection code does provider collection concurrently. The code waiting on providers iterates through and "finishes" each one in turn. I think it might be better to make the "providers in collection" a set. You could also register multiple promise handlers on the collection promise so that the set is updated immediately after collection has finished, without having to wait for the iteration in _handleCollectionPromises. The benefits of this would be more complete data reporting. As is implemented, we could have multiple providers hanging and we won't know. We'd play a very frustrating game of whack-a-mole to identify and fix problems. I favor collecting all the data now. I don't think it is that much more effort to implement.

::: services/metrics/providermanager.jsm
@@ +434,5 @@
>     *
>     * Returns a Promise that will be fulfilled once all data providers have
>     * provided their constant data. A side-effect of this promise fulfillment
>     * is that the manager is populated with the obtained collection results.
>     * The resolved value to the promise is this `ProviderManager` instance.

Please document the @param

@@ +526,3 @@
>          try {
>            yield promise;
>            this._log.debug("Provider collected successfully: " + name);

Should we |providerDiagnostic(null)| here?
Attachment #8540831 - Flags: review?(gps) → review+
Ok, improving this (and the init & shutdown diagnostic) to report a set of providers we wait on sounds good, but not like something i'd want to target for aurora & beta uplift.
(In reply to Gregory Szorc [:gps] from comment #3)
> @@ +526,3 @@
> >          try {
> >            yield promise;
> >            this._log.debug("Provider collected successfully: " + name);
> 
> Should we |providerDiagnostic(null)| here?

Hm, i don't think that adds much more info here.
Comment on attachment 8540831 [details] [diff] [review]
Add FHR provider collection diagnost

Approval Request Comment
[Feature/regressing bug #]: Last of the diagnostic patch series around bug 1035290 et al that we want in beta.
[User impact if declined]: No diagnostics for FHR providers hanging in collection.
[Describe test coverage new/current, TBPL]: Automated test-coverage over most FHR behaviors.
[Risks and why]: Low-risk, just adding simple AsyncShutdownTimeout diagnostics.
[String/UUID change made/needed]: None.
Attachment #8540831 - Flags: approval-mozilla-beta?
Attachment #8540831 - Flags: approval-mozilla-aurora?
Yuck, follow-up: actually add the diagnostic to the AsyncShutdownTimeout state:
https://hg.mozilla.org/integration/mozilla-inbound/rev/af9239757205
Keywords: leave-open
Keywords: leave-open
Blocks: 1115385
https://hg.mozilla.org/mozilla-central/rev/fce824c4c646
https://hg.mozilla.org/mozilla-central/rev/af9239757205
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 37
Attachment #8540831 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8540831 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Product: Firefox Health Report → Firefox Health Report Graveyard
You need to log in before you can comment on or make changes to this bug.