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)
Firefox Health Report Graveyard
Client: Desktop
Tracking
(firefox35 fixed, firefox36 fixed, firefox37 fixed)
RESOLVED
FIXED
Firefox 37
People
(Reporter: gfritzsche, Assigned: gfritzsche)
References
Details
Attachments
(1 file)
6.80 KB,
patch
|
gps
:
review+
Sylvestre
:
approval-mozilla-aurora+
lsblakk
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
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 | ||
Updated•9 years ago
|
Assignee: nobody → gfritzsche
Assignee | ||
Comment 1•9 years ago
|
||
Can you review this Benjamin? I'm not sure who's still around.
Attachment #8540831 -
Flags: review?(benjamin)
Assignee | ||
Updated•9 years ago
|
Attachment #8540831 -
Flags: review?(gps)
Attachment #8540831 -
Flags: review?(benjamin)
Attachment #8540831 -
Flags: review?
Assignee | ||
Updated•9 years ago
|
Status: NEW → ASSIGNED
Comment 2•9 years ago
|
||
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 3•9 years ago
|
||
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+
Assignee | ||
Comment 4•9 years ago
|
||
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.
Assignee | ||
Comment 5•9 years ago
|
||
(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.
Assignee | ||
Comment 6•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/fce824c4c646
Assignee | ||
Comment 7•9 years ago
|
||
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?
Assignee | ||
Comment 8•9 years ago
|
||
Yuck, follow-up: actually add the diagnostic to the AsyncShutdownTimeout state: https://hg.mozilla.org/integration/mozilla-inbound/rev/af9239757205
Assignee | ||
Updated•9 years ago
|
Keywords: leave-open
Assignee | ||
Updated•9 years ago
|
Keywords: leave-open
Comment 9•9 years ago
|
||
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
Updated•9 years ago
|
Updated•9 years ago
|
Attachment #8540831 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 10•9 years ago
|
||
Folded the follow-up in with the original patch. https://hg.mozilla.org/releases/mozilla-aurora/rev/63eda68642df
Updated•9 years ago
|
Attachment #8540831 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Updated•6 years 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.
Description
•