Closed
Bug 1169691
Opened 9 years ago
Closed 9 years ago
Make TelemetryArchive return accurate results immediately after calling TelemetryController.submitExternalPing
Categories
(Toolkit :: Telemetry, defect)
Toolkit
Telemetry
Tracking
()
RESOLVED
FIXED
mozilla41
People
(Reporter: benjamin, Assigned: benjamin)
References
Details
(Whiteboard: [uplift])
Attachments
(1 file)
39 bytes,
text/x-review-board-request
|
gfritzsche
:
review+
Sylvestre
:
approval-mozilla-aurora+
|
Details |
Especially for testing, but also for about:telemetry and about:healthreport, it would be useful for TelemetryArchive to return correct results as soon as TelemetryController.submitExternalPing has been called. This allows us to write correctness tests that don't need to wait on the promise returned by submitExternalPing, which is often unavailable to the test. This may also help with QA if testers do an action that submits an external ping and refresh about:telemetry immediately after to check the results.
Assignee | ||
Comment 1•9 years ago
|
||
Bug 1169691 - Make TelemetryStorage wait for pings that have been submitted for archiving but haven't finished submitting to disk, so that tests can reliably fetch submitted ping lists, r=gfritzsche
Attachment #8612965 -
Flags: review?(gfritzsche)
Comment 2•9 years ago
|
||
Comment on attachment 8612965 [details] MozReview Request: Bug 1169691 - Make TelemetryStorage wait for pings that have been submitted for archiving but haven't finished submitting to disk, so that tests can reliably fetch submitted ping lists, r?gfritzsche https://reviewboard.mozilla.org/r/9677/#review8449 ::: toolkit/components/telemetry/TelemetryStorage.jsm:642 (Diff revision 1) > - return this._archiveScanningTask; > + return yield this._archiveScanningTask; Why the yield here? Returning a task should be fine. ::: toolkit/components/telemetry/TelemetryStorage.jsm:120 (Diff revision 1) > + archivingPings.add(promise); This implementation part should go into TelemetryStorageImpl.saveArchivedPing(). Then we can make archivingPings a property of TelemetryStorageImpl. ::: toolkit/components/telemetry/TelemetryStorage.jsm:75 (Diff revision 1) > +let archivingPings = new Set(); "A set ..." Lets not make this a global, but a property of TelemetryStorageImpl (FWIW i'm removing other globals too in bug 1156359). ::: toolkit/components/telemetry/TelemetryStorage.jsm:653 (Diff revision 1) > let clear = pingList => { We're not using this anymore, let's remove it. ::: toolkit/components/telemetry/TelemetryStorage.jsm:659 (Diff revision 1) > - return this._archiveScanningTask; > + return yield this._archiveScanningTask = this._scanArchive(); I don't think the yield is needed. Can we do this a bit more readable in two statements?
Attachment #8612965 -
Flags: review?(gfritzsche)
Assignee | ||
Comment 3•9 years ago
|
||
Comment on attachment 8612965 [details] MozReview Request: Bug 1169691 - Make TelemetryStorage wait for pings that have been submitted for archiving but haven't finished submitting to disk, so that tests can reliably fetch submitted ping lists, r?gfritzsche Bug 1169691 - Make TelemetryStorage wait for pings that have been submitted for archiving but haven't finished submitting to disk, so that tests can reliably fetch submitted ping lists, r=gfritzsche
Attachment #8612965 -
Flags: review?(gfritzsche)
Comment 4•9 years ago
|
||
Comment on attachment 8612965 [details] MozReview Request: Bug 1169691 - Make TelemetryStorage wait for pings that have been submitted for archiving but haven't finished submitting to disk, so that tests can reliably fetch submitted ping lists, r?gfritzsche https://reviewboard.mozilla.org/r/9677/#review8655 ::: toolkit/components/telemetry/TelemetryStorage.jsm:428 (Diff revisions 1 - 2) > + _archivingPings: new Set(), I think this could be named better: `_archivingPingsPromises`, `_activePingArchiving`, ... ::: toolkit/components/telemetry/TelemetryStorage.jsm:475 (Diff revisions 1 - 2) > + saveArchivedPingTask: Task.async(function*(ping) { We prefix internal functions with `_` in this file. ::: toolkit/components/telemetry/TelemetryStorage.jsm:648 (Diff revisions 1 - 2) > - yield Promise.all(archivingPings); > + yield Promise.all(this._archivingPings); Promise.all() will reject as soon as any of the promises passed rejects. This is breaking the expected behavior here. I think we need a general solution like PromiseUtils.allResolvedOrRejected, but haven't had the time yet to come up with a solution that makes both me and David happy.
Attachment #8612965 -
Flags: review?(gfritzsche)
Comment 5•9 years ago
|
||
https://reviewboard.mozilla.org/r/9677/#review8657 > Promise.all() will reject as soon as any of the promises passed rejects. > This is breaking the expected behavior here. > > I think we need a general solution like PromiseUtils.allResolvedOrRejected, but haven't had the time yet to come up with a solution that makes both me and David happy. FWIW, context here: https://reviewboard.mozilla.org/r/9431/diff/1/?file=155473#file155473line243
Assignee | ||
Updated•9 years ago
|
Attachment #8612965 -
Attachment description: MozReview Request: Bug 1169691 - Make TelemetryStorage wait for pings that have been submitted for archiving but haven't finished submitting to disk, so that tests can reliably fetch submitted ping lists, r=gfritzsche → MozReview Request: Bug 1169691 - Make TelemetryStorage wait for pings that have been submitted for archiving but haven't finished submitting to disk, so that tests can reliably fetch submitted ping lists, r?gfritzsche
Attachment #8612965 -
Flags: review?(gfritzsche)
Assignee | ||
Comment 6•9 years ago
|
||
Comment on attachment 8612965 [details] MozReview Request: Bug 1169691 - Make TelemetryStorage wait for pings that have been submitted for archiving but haven't finished submitting to disk, so that tests can reliably fetch submitted ping lists, r?gfritzsche Bug 1169691 - Make TelemetryStorage wait for pings that have been submitted for archiving but haven't finished submitting to disk, so that tests can reliably fetch submitted ping lists, r?gfritzsche
Updated•9 years ago
|
Attachment #8612965 -
Flags: review?(gfritzsche) → review+
Comment 7•9 years ago
|
||
Comment on attachment 8612965 [details] MozReview Request: Bug 1169691 - Make TelemetryStorage wait for pings that have been submitted for archiving but haven't finished submitting to disk, so that tests can reliably fetch submitted ping lists, r?gfritzsche https://reviewboard.mozilla.org/r/9677/#review8683 ::: toolkit/components/telemetry/TelemetryStorage.jsm:87 (Diff revisions 2 - 3) > +function _waitForAll(it) { Misunderstanding here: We use the _ prefix only for private member functions / attributes in these modules.
https://hg.mozilla.org/mozilla-central/rev/628698b10e04
Assignee: nobody → benjamin
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox41:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Updated•9 years ago
|
status-firefox40:
--- → affected
Whiteboard: [uplift]
Comment 10•9 years ago
|
||
Comment on attachment 8612965 [details] MozReview Request: Bug 1169691 - Make TelemetryStorage wait for pings that have been submitted for archiving but haven't finished submitting to disk, so that tests can reliably fetch submitted ping lists, r?gfritzsche Approval Request Comment [Feature/regressing bug #]: Unified Telemetry, https://wiki.mozilla.org/Unified_Telemetry This is part of the first (main) batch of uplifts to 40 to enable shipping on that train, see bug 1120356, comment 2. [User impact if declined]: Data & measurement insight projects delayed or blocked with direct impact on projects depending on this. [Describe test coverage new/current, TreeHerder]: We have good automation coverage of the feature. We also had manual tests of the main tasks as well as confirmation of correct behavior on Nightly for the patches here. [Risks and why]: Low-risk - these patches are rather isolated to Telemetry and have been on Nightly for a while with no bad reports. We intend to track on-going data quality and other issues during the 40 aurora & beta and flip the new behavior off when it doesn't meet the requirements. [String/UUID change made/needed]: The only string changes were to the about:telemetry page. We decided that we can live with missing translations on that page for a cycle as that page is not exactly user-facing.
Attachment #8612965 -
Flags: approval-mozilla-aurora?
Comment 11•9 years ago
|
||
Comment on attachment 8612965 [details] MozReview Request: Bug 1169691 - Make TelemetryStorage wait for pings that have been submitted for archiving but haven't finished submitting to disk, so that tests can reliably fetch submitted ping lists, r?gfritzsche Unified telemetry is an important new feature. It is blocking some other projects. Taking it.
Attachment #8612965 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in
before you can comment on or make changes to this bug.
Description
•