Closed Bug 763526 Opened 10 years ago Closed 10 years ago

reverse order of telemetry ping sending

Categories

(Toolkit :: Telemetry, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla16

People

(Reporter: froydnj, Assigned: froydnj)

References

Details

Attachments

(5 files)

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.
Attachment #632668 - Flags: review?(taras.mozilla)
Attachment #632669 - Flags: review?(taras.mozilla)
Make it part of the iteration logic, rather than hidden away in some handler somewhere.
Attachment #632670 - Flags: review?(taras.mozilla)
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)
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+
(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.
Attachment #632669 - Flags: review?(taras.mozilla) → review+
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.
Attachment #632670 - Flags: review?(taras.mozilla) → review+
Attachment #632671 - Flags: review?(taras.mozilla) → review+
Attachment #632672 - Flags: review?(taras.mozilla) → review+
You need to log in before you can comment on or make changes to this bug.