Closed Bug 1156359 Opened 6 years ago Closed 6 years ago

Telemetry redesign: Move send logic to TelemetrySend.jsm

Categories

(Toolkit :: Telemetry, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox39 --- unaffected
firefox40 --- wontfix
firefox41 --- fixed

People

(Reporter: gfritzsche, Assigned: gfritzsche)

References

Details

(Whiteboard: [b5] [unifiedTelemetry])

Attachments

(3 obsolete files)

Blocks: 1156360
Blocks: 1156712
Assignee: nobody → gfritzsche
Status: NEW → ASSIGNED
Attachment #8610438 - Flags: review?(dteller)
Also move over triggering sending of pending pings on "idle-daily" from TelemetrySession.
Attachment #8610439 - Flags: review?(dteller)
This is still missing porting the "pending ping" queue (or just rewriting it directly).
Comment on attachment 8610438 [details] [diff] [review]
Part 1: Add PromiseUtils.allResolvedOrRejected()

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

I'm not a huge fan of this utility. Let's see how it's used in patch 2.

::: toolkit/modules/PromiseUtils.jsm
@@ +27,5 @@
> +   * @return {Promise} Resolved when all promises from the list are resolved or rejected.
> +   */
> +  allResolvedOrRejected: function(promiseList) {
> +    const dummy = () => {};
> +    const promises = [for (p of promiseList) p.catch(dummy)];

This will eat any error reporting, is that by design? So we won't be informed of any error.

Why not add a call to `Promise.all(promiseList)` to at least have the console warning in case we leave any error uncaught?
Attachment #8610438 - Flags: review?(dteller) → feedback+
Comment on attachment 8610439 [details] [diff] [review]
Part 2: Move send logic from TelemetryController to TelemetrySend

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

As you expected, reading the patch is tricky.
Could you resubmit it on MozReview?

::: toolkit/components/telemetry/TelemetryController.jsm
@@ +454,5 @@
>     * @param {Object}  [aOptions.overrideEnvironment=null] set to override the environment data.
>     * @returns {Promise} A promise that is resolved with the ping id once the ping is stored or sent.
>     */
>    submitExternalPing: function send(aType, aPayload, aOptions) {
> +    this._log.trace("submitExternalPing - type: " + aType + ", aOptions: " + JSON.stringify(aOptions));

Nit: this would be nicer with backquotes

this._log.trace(`submitExternalPing - type: ${aType}, options: ${JSON.stringify(aOptions)}`)

::: toolkit/components/telemetry/TelemetrySend.jsm
@@ +2,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +"use strict";
> +

Some module doc would be useful.
Attachment #8610439 - Flags: review?(dteller)
Comment on attachment 8610438 [details] [diff] [review]
Part 1: Add PromiseUtils.allResolvedOrRejected()

This should be on RB.
Attachment #8610438 - Attachment is obsolete: true
Comment on attachment 8610439 [details] [diff] [review]
Part 2: Move send logic from TelemetryController to TelemetrySend

This should be on RB.
Attachment #8610439 - Attachment is obsolete: true
(In reply to Georg Fritzsche [:gfritzsche] from comment #6)
> Created attachment 8612799 [details] [diff] [review]
> Part 3: Let TelemetrySend manage the pending ping queue

Note that this takes care of bug 1041616 too:
We now always save pending pings unless we can send them right away and don't keep them in memory.
When we send pending pings out we just load them from disk as needed.

That also affected some tests that depended on pending pings being discarded if the Telemetry modules weren't shutdown properly, so there is some test cleanup in here too.
I don't have time to finish review right now, so here's a first batch. I'll try and finish by Monday.
Comment on attachment 8612799 [details] [diff] [review]
Part 3: Let TelemetrySend manage the pending ping queue

I was able to move this over to RB.
Attachment #8612799 - Attachment is obsolete: true
Attachment #8612799 - Flags: review?(dteller)
test_TelemetrySendOldPings.js was disabled locally for debug builds and blew up on try.
Re-enabled it and fixed things:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a2390595c4e8
https://hg.mozilla.org/mozilla-central/rev/8b1a1ddd1721
https://hg.mozilla.org/mozilla-central/rev/653299500d3c
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Depends on: 1173310
Depends on: 1173709
Whiteboard: [uplift]
Whiteboard: [uplift] → [uplift2]
No longer depends on: 1156358
Blocks: 1176006
Whiteboard: [uplift2] → [b5] [unifiedTelemetry] [uplift2]
Whiteboard: [b5] [unifiedTelemetry] [uplift2] → [b5] [unifiedTelemetry]
You need to log in before you can comment on or make changes to this bug.