Closed
Bug 763526
Opened 13 years ago
Closed 13 years ago
reverse order of telemetry ping sending
Categories
(Toolkit :: Telemetry, defect)
Toolkit
Telemetry
Tracking
()
RESOLVED
FIXED
mozilla16
People
(Reporter: froydnj, Assigned: froydnj)
References
Details
Attachments
(5 files)
2.91 KB,
patch
|
taras.mozilla
:
review+
|
Details | Diff | Splinter Review |
3.34 KB,
patch
|
taras.mozilla
:
review+
|
Details | Diff | Splinter Review |
3.90 KB,
patch
|
taras.mozilla
:
review+
|
Details | Diff | Splinter Review |
2.02 KB,
patch
|
taras.mozilla
:
review+
|
Details | Diff | Splinter Review |
5.41 KB,
patch
|
taras.mozilla
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•13 years ago
|
||
Attachment #632668 -
Flags: review?(taras.mozilla)
Assignee | ||
Comment 2•13 years ago
|
||
Attachment #632669 -
Flags: review?(taras.mozilla)
Assignee | ||
Comment 3•13 years ago
|
||
Make it part of the iteration logic, rather than hidden away in some handler somewhere.
Attachment #632670 -
Flags: review?(taras.mozilla)
Assignee | ||
Comment 4•13 years ago
|
||
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)
Assignee | ||
Comment 5•13 years ago
|
||
Attachment #632672 -
Flags: review?(taras.mozilla)
Comment 6•13 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•13 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.
Assignee | ||
Comment 8•13 years ago
|
||
(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•13 years ago
|
Attachment #632669 -
Flags: review?(taras.mozilla) → review+
Comment 9•13 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•13 years ago
|
Attachment #632670 -
Flags: review?(taras.mozilla) → review+
Updated•13 years ago
|
Attachment #632671 -
Flags: review?(taras.mozilla) → review+
Updated•13 years ago
|
Attachment #632672 -
Flags: review?(taras.mozilla) → review+
Assignee | ||
Comment 10•13 years ago
|
||
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
Assignee: nobody → nfroyd
Status: NEW → ASSIGNED
Comment 11•13 years ago
|
||
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
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla16
You need to log in
before you can comment on or make changes to this bug.
Description
•