Telemetry redesign: Move send logic to TelemetrySend.jsm

RESOLVED FIXED in Firefox 41

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: gfritzsche, Assigned: gfritzsche)

Tracking

Trunk
mozilla41
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox39 unaffected, firefox40 wontfix, firefox41 fixed)

Details

(Whiteboard: [b5] [unifiedTelemetry])

Attachments

(3 obsolete attachments)

Assignee

Updated

4 years ago
Blocks: 1156360
Assignee

Updated

4 years ago
Blocks: 1156712
Assignee

Updated

4 years ago
Assignee: nobody → gfritzsche
Assignee

Updated

4 years ago
Status: NEW → ASSIGNED
Assignee

Comment 1

4 years ago
Attachment #8610438 - Flags: review?(dteller)
Assignee

Comment 2

4 years ago
Also move over triggering sending of pending pings on "idle-daily" from TelemetrySession.
Attachment #8610439 - Flags: review?(dteller)
Assignee

Comment 3

4 years ago
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)
Assignee

Comment 7

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

This should be on RB.
Attachment #8610438 - Attachment is obsolete: true
Assignee

Comment 8

4 years ago
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
Assignee

Comment 9

4 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.
I don't have time to finish review right now, so here's a first batch. I'll try and finish by Monday.
Assignee

Comment 11

4 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.
Attachment #8612799 - Attachment is obsolete: true
Attachment #8612799 - Flags: review?(dteller)
Assignee

Comment 13

4 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
https://hg.mozilla.org/mozilla-central/rev/8b1a1ddd1721
https://hg.mozilla.org/mozilla-central/rev/653299500d3c
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Assignee

Updated

4 years ago
Depends on: 1173310
Assignee

Updated

4 years ago
Whiteboard: [uplift]
Assignee

Updated

4 years ago
Whiteboard: [uplift] → [uplift2]
Assignee

Updated

4 years ago
No longer depends on: 1156358
Assignee

Updated

4 years ago
Blocks: 1176006
Assignee

Updated

4 years ago
Whiteboard: [uplift2] → [b5] [unifiedTelemetry] [uplift2]
Assignee

Updated

4 years ago
Whiteboard: [b5] [unifiedTelemetry] [uplift2] → [b5] [unifiedTelemetry]
You need to log in before you can comment on or make changes to this bug.