763526 - reverse order of telemetry ping sending

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect)

Description

[Nathan Froyd [:froydnj]]

From bug 748517 comment 14:

I feel we should reverse the ping flow. Send current ping first, followed by old pings. We can then abort on the first failed ping instead of hammering an upset/missing server as we do now. Should also record success/fail with every ping.

This could almost be done concurrently with resending telemetry pings (bug 748520), but can certainly be implemented separately.

Comment 1

[Nathan Froyd [:froydnj]]

Attachment: part 1 - abort sending after first failed ping

Comment 2

[Nathan Froyd [:froydnj]]

Attachment: part 2 - record success/fail for every telemetry ping

Comment 3

[Nathan Froyd [:froydnj]]

Attachment: part 3 - move telemetry-test-xhr-complete notification

Make it part of the iteration logic, rather than hidden away in some handler somewhere.

Comment 4

[Nathan Froyd [:froydnj]]

Attachment: part 4 - determine success directly from the event handler

We should need to query the channel; the kind of event handler called tells us all we need to know about success/fail.  Not strictly part of this patch, but a nice cleanup.

Comment 5

[Nathan Froyd [:froydnj]]

Attachment: part 5 - send current session pings before persisted pings

Comment 6

[(dormant account)]

Comment on attachment 632668 [details] [diff] [review]
part 1 - abort sending after first failed ping

+    function onSuccess() {
+      this.sendPingsFromIterator(server, i);
+    }

at first I thought this was horrible, but on second thought chaining iterators via callbacks is kinda cool.

+                onSuccess.bind(this), onError.bind(this));
I wish JS had a mode to do this by default :(

Comment 7

[(dormant account)]

(In reply to Taras Glek (:taras) from comment #6)
> at first I thought this was horrible, but on second thought chaining
> iterators via callbacks is kinda cool.

The reason I thought it was terrible is that there is no comment explaining the need for complexity, please add a comment describing the purpose of the function.

Comment 8

[Nathan Froyd [:froydnj]]

(In reply to Taras Glek (:taras) from comment #7)
> (In reply to Taras Glek (:taras) from comment #6)
> > at first I thought this was horrible, but on second thought chaining
> > iterators via callbacks is kinda cool.
> 
> The reason I thought it was terrible is that there is no comment explaining
> the need for complexity, please add a comment describing the purpose of the
> function.

I suppose it's not strictly necessary, and the callbacks, at least, could be moved into the event handlers for the ping request, if not the whole iteration logic.  I think it is slightly simpler this way, though a bit confusing at first; I had to write out a little diagram before I arrived at what I had now.  Will add a comment.

Comment 9

[(dormant account)]

Comment on attachment 632669 [details] [diff] [review]
part 2 - record success/fail for every telemetry ping

I wonder if these would be more appropriate in the onSuccess/onError handlers in patch 1.

Comment 10

[Nathan Froyd [:froydnj]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a43806df7029
https://hg.mozilla.org/integration/mozilla-inbound/rev/fa4aed87db0a
https://hg.mozilla.org/integration/mozilla-inbound/rev/7227e77dd8d4
https://hg.mozilla.org/integration/mozilla-inbound/rev/c570feae74ce
https://hg.mozilla.org/integration/mozilla-inbound/rev/15d0bf6d3d8e

Comment 11

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/a43806df7029
https://hg.mozilla.org/mozilla-central/rev/fa4aed87db0a
https://hg.mozilla.org/mozilla-central/rev/7227e77dd8d4
https://hg.mozilla.org/mozilla-central/rev/c570feae74ce
https://hg.mozilla.org/mozilla-central/rev/15d0bf6d3d8e