reverse order of telemetry ping sending

RESOLVED FIXED in mozilla16

Status

()

RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: froydnj, Assigned: froydnj)

Tracking

unspecified
mozilla16
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments)

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.
Created attachment 632668 [details] [diff] [review]
part 1 - abort sending after first failed ping
Attachment #632668 - Flags: review?(taras.mozilla)
Created attachment 632669 [details] [diff] [review]
part 2 - record success/fail for every telemetry ping
Attachment #632669 - Flags: review?(taras.mozilla)
Created attachment 632670 [details] [diff] [review]
part 3 - move telemetry-test-xhr-complete notification

Make it part of the iteration logic, rather than hidden away in some handler somewhere.
Attachment #632670 - Flags: review?(taras.mozilla)
Created attachment 632671 [details] [diff] [review]
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.
Attachment #632671 - Flags: review?(taras.mozilla)
Created attachment 632672 [details] [diff] [review]
part 5 - send current session pings before persisted pings
Attachment #632672 - Flags: review?(taras.mozilla)

Comment 6

6 years ago
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 :(
Attachment #632668 - Flags: review?(taras.mozilla) → review+

Comment 7

6 years ago
(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.
(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.

Updated

6 years ago
Attachment #632669 - Flags: review?(taras.mozilla) → review+

Comment 9

6 years ago
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.

Updated

6 years ago
Attachment #632670 - Flags: review?(taras.mozilla) → review+

Updated

6 years ago
Attachment #632671 - Flags: review?(taras.mozilla) → review+

Updated

6 years ago
Attachment #632672 - Flags: review?(taras.mozilla) → review+
You need to log in before you can comment on or make changes to this bug.