The chromoting client logs connection statistics to the server.

The chromoting client logs connection statistics to the server.

BUG=106208
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=113868

[simonmorris, 2011-12-07 23:10:11]

PTAL

[Jamie, 2011-12-09 06:58:34]

http://codereview.chromium.org/8865005/diff/1/remoting/webapp/me2mom/debug_lo...
File remoting/webapp/me2mom/debug_log.js (right):

http://codereview.chromium.org/8865005/diff/1/remoting/webapp/me2mom/debug_lo...
remoting/webapp/me2mom/debug_log.js:697: if (event_name == 'session-state') {
This feels unnecessary to me. My understanding of the pretty-printing code is
that it does all this checking because the XML comes from somewhere we don't
control (or at least, that the web-app doesn't control). Isn't this stuff
generated elsewhere in the web-app? Would it be simpler to log it at source, and
skip pretty-printing it here altogether?

http://codereview.chromium.org/8865005/diff/1/remoting/webapp/me2mom/log_to_s...
File remoting/webapp/me2mom/log_to_server.js (right):

http://codereview.chromium.org/8865005/diff/1/remoting/webapp/me2mom/log_to_s...
remoting/webapp/me2mom/log_to_server.js:76: if
(this.statsAccumulator.getTimeSinceFirstValue() >= 60 * 1000) {
Optional: How would you feel about replacing this logic with a timer-based
approach? It feels like it would be a more natural way of handling this, since
we already use a timer to log the statistics, and it would simplify
StatsAccumulator a fair bit.

http://codereview.chromium.org/8865005/diff/1/remoting/webapp/me2mom/server_l...
File remoting/webapp/me2mom/server_log_entry.js (right):

http://codereview.chromium.org/8865005/diff/1/remoting/webapp/me2mom/server_l...
remoting/webapp/me2mom/server_log_entry.js:191: var nonZero = false;
Can't these just be entry.addStatsField, and remove the entry parameter?

http://codereview.chromium.org/8865005/diff/1/remoting/webapp/me2mom/stats_ac...
File remoting/webapp/me2mom/stats_accumulator.js (right):

http://codereview.chromium.org/8865005/diff/1/remoting/webapp/me2mom/stats_ac...
remoting/webapp/me2mom/stats_accumulator.js:108: };
I don't think a generic map function buys you anything here. Clearer just to
code calcMean in the obvious way.

[simonmorris, 2011-12-09 18:40:35]

http://codereview.chromium.org/8865005/diff/1/remoting/webapp/me2mom/debug_lo...
File remoting/webapp/me2mom/debug_log.js (right):

http://codereview.chromium.org/8865005/diff/1/remoting/webapp/me2mom/debug_lo...
remoting/webapp/me2mom/debug_log.js:697: if (event_name == 'session-state') {
On 2011/12/09 06:58:34, Jamie wrote:
> This feels unnecessary to me. My understanding of the pretty-printing code is
> that it does all this checking because the XML comes from somewhere we don't
> control (or at least, that the web-app doesn't control). Isn't this stuff
> generated elsewhere in the web-app? Would it be simpler to log it at source,
and
> skip pretty-printing it here altogether?

Done.

http://codereview.chromium.org/8865005/diff/1/remoting/webapp/me2mom/log_to_s...
File remoting/webapp/me2mom/log_to_server.js (right):

http://codereview.chromium.org/8865005/diff/1/remoting/webapp/me2mom/log_to_s...
remoting/webapp/me2mom/log_to_server.js:76: if
(this.statsAccumulator.getTimeSinceFirstValue() >= 60 * 1000) {
On 2011/12/09 06:58:34, Jamie wrote:
> Optional: How would you feel about replacing this logic with a timer-based
> approach? It feels like it would be a more natural way of handling this, since
> we already use a timer to log the statistics, and it would simplify
> StatsAccumulator a fair bit.

One problem is that LogToServer would then have to be careful not to send
stats when the plugin isn't generating any (eg when there's no connection).
LogToServer should never send stats unless it's given some, and coding it
this way guarantees that.

http://codereview.chromium.org/8865005/diff/1/remoting/webapp/me2mom/server_l...
File remoting/webapp/me2mom/server_log_entry.js (right):

http://codereview.chromium.org/8865005/diff/1/remoting/webapp/me2mom/server_l...
remoting/webapp/me2mom/server_log_entry.js:191: var nonZero = false;
On 2011/12/09 06:58:34, Jamie wrote:
> Can't these just be entry.addStatsField, and remove the entry parameter?

Done.

http://codereview.chromium.org/8865005/diff/1/remoting/webapp/me2mom/stats_ac...
File remoting/webapp/me2mom/stats_accumulator.js (right):

http://codereview.chromium.org/8865005/diff/1/remoting/webapp/me2mom/stats_ac...
remoting/webapp/me2mom/stats_accumulator.js:108: };
On 2011/12/09 06:58:34, Jamie wrote:
> I don't think a generic map function buys you anything here. Clearer just to
> code calcMean in the obvious way.

It doesn't buy me anything yet, but just returning the mean throws away
information about eg intermittent low latency. I think we might later add
calcMax, or even calcStandardDeviation, and then this way avoids some
repetition.

[simonmorris, 2011-12-09 18:48:25]

> 
> It doesn't buy me anything yet, but just returning the mean throws away
> information about eg intermittent low latency. I think we might later add
> calcMax, or even calcStandardDeviation, and then this way avoids some
> repetition.

I meant "intermittent _high_ latency".

[Jamie, 2011-12-09 19:47:56]

http://codereview.chromium.org/8865005/diff/7001/remoting/webapp/me2mom/clien...
File remoting/webapp/me2mom/client_session.js (right):

http://codereview.chromium.org/8865005/diff/7001/remoting/webapp/me2mom/clien...
remoting/webapp/me2mom/client_session.js:440: return dict;
Nit: Why change to this form of construction?

http://codereview.chromium.org/8865005/diff/7001/remoting/webapp/me2mom/debug...
File remoting/webapp/me2mom/debug_log.js (left):

http://codereview.chromium.org/8865005/diff/7001/remoting/webapp/me2mom/debug...
remoting/webapp/me2mom/debug_log.js:655: } else if (child.nodeName == 'gr:log')
{
Has this code really all gone, or does it still exist elsewhere?

http://codereview.chromium.org/8865005/diff/7001/remoting/webapp/me2mom/log_t...
File remoting/webapp/me2mom/log_to_server.js (right):

http://codereview.chromium.org/8865005/diff/7001/remoting/webapp/me2mom/log_t...
remoting/webapp/me2mom/log_to_server.js:91: this.statsAccumulator);
Directly invoking a method on the prototype seems very odd to me. Is this an
oversight, or am I missing something? I realize that it's done the same way
above, but I think that's wrong too :)

http://codereview.chromium.org/8865005/diff/7001/remoting/webapp/me2mom/log_t...
remoting/webapp/me2mom/log_to_server.js:191: };
This function looks like it's unused.

http://codereview.chromium.org/8865005/diff/7001/remoting/webapp/me2mom/serve...
File remoting/webapp/me2mom/server_log_entry.js (right):

http://codereview.chromium.org/8865005/diff/7001/remoting/webapp/me2mom/serve...
remoting/webapp/me2mom/server_log_entry.js:110:
remoting.ServerLogEntry.prototype.KEY_ROUNDTRIP_LATENCY_ = "roundtrip-latency";
Why not reuse the keys from ClientSession?

http://codereview.chromium.org/8865005/diff/7001/remoting/webapp/me2mom/serve...
remoting/webapp/me2mom/server_log_entry.js:168: }
It might be better to log these on a single line, since they're all closely
related.

http://codereview.chromium.org/8865005/diff/7001/remoting/webapp/me2mom/serve...
remoting/webapp/me2mom/server_log_entry.js:198:
remoting.ServerLogEntry.prototype.makeStats = function(statsAccumulator) {
I don't think this (or the previous method) belongs in the prototype; they're
basically 'static' factory methods.

[simonmorris, 2011-12-09 21:12:12]

http://codereview.chromium.org/8865005/diff/7001/remoting/webapp/me2mom/clien...
File remoting/webapp/me2mom/client_session.js (right):

http://codereview.chromium.org/8865005/diff/7001/remoting/webapp/me2mom/clien...
remoting/webapp/me2mom/client_session.js:440: return dict;
On 2011/12/09 19:47:56, Jamie wrote:
> Nit: Why change to this form of construction?

The other form only works with compile-time
constants.

http://codereview.chromium.org/8865005/diff/7001/remoting/webapp/me2mom/debug...
File remoting/webapp/me2mom/debug_log.js (left):

http://codereview.chromium.org/8865005/diff/7001/remoting/webapp/me2mom/debug...
remoting/webapp/me2mom/debug_log.js:655: } else if (child.nodeName == 'gr:log')
{
On 2011/12/09 19:47:56, Jamie wrote:
> Has this code really all gone, or does it still exist elsewhere?

All gone, as requested in the previous review.
There's no need for this code to handle logging stanzas
when the logging code is printing them itself.

http://codereview.chromium.org/8865005/diff/7001/remoting/webapp/me2mom/log_t...
File remoting/webapp/me2mom/log_to_server.js (right):

http://codereview.chromium.org/8865005/diff/7001/remoting/webapp/me2mom/log_t...
remoting/webapp/me2mom/log_to_server.js:91: this.statsAccumulator);
On 2011/12/09 19:47:56, Jamie wrote:
> Directly invoking a method on the prototype seems very odd to me. Is this an
> oversight, or am I missing something? I realize that it's done the same way
> above, but I think that's wrong too :)

This was just because I didn't have the convention that static methods
don't go in the prototype.

Done.

http://codereview.chromium.org/8865005/diff/7001/remoting/webapp/me2mom/log_t...
remoting/webapp/me2mom/log_to_server.js:191: };
On 2011/12/09 19:47:56, Jamie wrote:
> This function looks like it's unused.

Done.

http://codereview.chromium.org/8865005/diff/7001/remoting/webapp/me2mom/serve...
File remoting/webapp/me2mom/server_log_entry.js (right):

http://codereview.chromium.org/8865005/diff/7001/remoting/webapp/me2mom/serve...
remoting/webapp/me2mom/server_log_entry.js:110:
remoting.ServerLogEntry.prototype.KEY_ROUNDTRIP_LATENCY_ = "roundtrip-latency";
On 2011/12/09 19:47:56, Jamie wrote:
> Why not reuse the keys from ClientSession?

Because I want to be able to look down the list of keys
and verify that they match the keys that the server handles.

Also, I don't want a change in the ClientSession keys
to inadvertently change the keys sent to the server,
which would then be ignored.

http://codereview.chromium.org/8865005/diff/7001/remoting/webapp/me2mom/serve...
remoting/webapp/me2mom/server_log_entry.js:168: }
On 2011/12/09 19:47:56, Jamie wrote:
> It might be better to log these on a single line, since they're all closely
> related.

Done.

http://codereview.chromium.org/8865005/diff/7001/remoting/webapp/me2mom/serve...
remoting/webapp/me2mom/server_log_entry.js:198:
remoting.ServerLogEntry.prototype.makeStats = function(statsAccumulator) {
On 2011/12/09 19:47:56, Jamie wrote:
> I don't think this (or the previous method) belongs in the prototype; they're
> basically 'static' factory methods.

Done.

[Jamie, 2011-12-09 22:01:40]

lgtm