1156359 - Telemetry redesign: Move send logic to TelemetrySend.jsm

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect)

Description

[Georg Fritzsche [:gfritzsche]]

Per: https://docs.google.com/document/d/1lKRfNl5aJ0W2flOp67Y5O3WDz2dSSz7a6r23DQ89poU/edit?pli=1

Comment 1

[Georg Fritzsche [:gfritzsche]]

Attachment: Part 1: Add PromiseUtils.allResolvedOrRejected()

Comment 2

[Georg Fritzsche [:gfritzsche]]

Attachment: Part 2: Move send logic from TelemetryController to TelemetrySend

Also move over triggering sending of pending pings on "idle-daily" from TelemetrySession.

Comment 3

[Georg Fritzsche [:gfritzsche]]

This is still missing porting the "pending ping" queue (or just rewriting it directly).

Comment 4

[David Teller [:Yoric] - still alive but not very active]

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?

Comment 5

[David Teller [:Yoric] - still alive but not very active]

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.

Comment 6

[Georg Fritzsche [:gfritzsche]]

Attachment: Part 3: Let TelemetrySend manage the pending ping queue

Comment 7

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8610438 [details] [diff] [review]
Part 1: Add PromiseUtils.allResolvedOrRejected()

This should be on RB.

Comment 8

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8610439 [details] [diff] [review]
Part 2: Move send logic from TelemetryController to TelemetrySend

This should be on RB.

Comment 9

[Georg Fritzsche [:gfritzsche]]

(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.

Comment 10

[David Teller [:Yoric] - still alive but not very active]

I don't have time to finish review right now, so here's a first batch. I'll try and finish by Monday.

Comment 11

[Georg Fritzsche [:gfritzsche]]

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.

Comment 12

[Georg Fritzsche [:gfritzsche]]

Reviews finished on RB:
https://reviewboard.mozilla.org/r/9431/
https://reviewboard.mozilla.org/r/9825/

Pushed to try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1a490836e7c5

Comment 13

[Georg Fritzsche [:gfritzsche]]

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

Comment 14

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/8b1a1ddd1721
https://hg.mozilla.org/integration/fx-team/rev/653299500d3c

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8b1a1ddd1721
https://hg.mozilla.org/mozilla-central/rev/653299500d3c