Georg Fritzsche [:gfritzsche] Assignee
	Description • 9 years ago

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

	Georg Fritzsche [:gfritzsche] Assignee
	Comment 2 • 9 years ago

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

	Georg Fritzsche [:gfritzsche] Assignee
	Comment 3 • 9 years ago

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

	David Teller [:Yoric] - still alive but not very active
	Comment 4 • 9 years ago

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?

	David Teller [:Yoric] - still alive but not very active
	Comment 5 • 9 years ago

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.

	Georg Fritzsche [:gfritzsche] Assignee
	Comment 7 • 9 years ago

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

This should be on RB.

	Georg Fritzsche [:gfritzsche] Assignee
	Comment 8 • 9 years ago

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

This should be on RB.

	Georg Fritzsche [:gfritzsche] Assignee
	Comment 9 • 9 years ago

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

	David Teller [:Yoric] - still alive but not very active
	Comment 10 • 9 years ago

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

	Georg Fritzsche [:gfritzsche] Assignee
	Comment 11 • 9 years ago

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.

	Georg Fritzsche [:gfritzsche] Assignee
	Comment 12 • 9 years ago

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

	Georg Fritzsche [:gfritzsche] Assignee
	Comment 13 • 9 years ago

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

	Pulsebot
	Comment 14 • 9 years ago

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

	Carsten Book [:Tomcat]
	Comment 15 • 9 years ago

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