The chromoting client sends simple logging to the server.

The chromoting client sends simple logging to the server.

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

[simonmorris, 2011-11-10 22:44:02]

PTAL

[Jamie, 2011-11-10 23:27:42]

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

http://codereview.chromium.org/8527011/diff/1/remoting/webapp/me2mom/log_to_s...
remoting/webapp/me2mom/log_to_server.js:23:
remoting.LogToServer.prototype.VALUE_EVENT_NAME_SESSION_STATE = 'session-state';
Is it worth tagging these as @private (and appending underscores?)

http://codereview.chromium.org/8527011/diff/1/remoting/webapp/me2mom/log_to_s...
remoting/webapp/me2mom/log_to_server.js:34:
remoting.LogToServer.prototype.VALUE_SESSION_STATE = function(state) {
I worry about having to remember to keep this translation (and the next one) in
sync if we add error types. Can we include the number in the undefined case? Is
it even worth doing this translation, rather than just logging the numbers?

I'm also not wild about the function name, though I can see you're going for
consistency with the other VALUE_ constants. I'd suggest stringifyClientState or
something similar, but it's not something I feel strongly about.

http://codereview.chromium.org/8527011/diff/1/remoting/webapp/me2mom/log_to_s...
remoting/webapp/me2mom/log_to_server.js:65:
remoting.LogToServer.prototype.VALUE_CONNECTION_ERROR =
Ditto for this function.

http://codereview.chromium.org/8527011/diff/1/remoting/webapp/me2mom/log_to_s...
remoting/webapp/me2mom/log_to_server.js:89:
remoting.LogToServer.prototype.VALUE_OS_NAME_CHROMEOS = 'ChromeOS';
What about other OSes? Seems like we should have an 'Unknown' value here.

http://codereview.chromium.org/8527011/diff/1/remoting/webapp/me2mom/log_to_s...
remoting/webapp/me2mom/log_to_server.js:108: this.addHostFields(entry);
This feels a little awkward. Would it be much more effort to move these add*
methods onto a separate Entry class? Failing that, could they be global
functions, hidden inside a scoping function like we've used elsewhere?

http://codereview.chromium.org/8527011/diff/1/remoting/webapp/me2mom/log_to_s...
remoting/webapp/me2mom/log_to_server.js:124: entryStr += key + '="' + entry[key]
+ '" ';
I feel we ought to be XML-escaping these. I don't think it's a problem now
because the set of strings we log is constrained, but as it grows it will be one
less thing to worry about.

http://codereview.chromium.org/8527011/diff/1/remoting/webapp/me2mom/log_to_s...
remoting/webapp/me2mom/log_to_server.js:140: remoting.debug.log('Sending log iq:
' + stanza);
Is it worth using Gary's pretty-printing code to log this?

http://codereview.chromium.org/8527011/diff/1/remoting/webapp/me2mom/log_to_s...
remoting/webapp/me2mom/log_to_server.js:150:
remoting.LogToServer.prototype.makeBasicEntry = function() {
This could be a ctor for a LogEntry class.

http://codereview.chromium.org/8527011/diff/1/remoting/webapp/me2mom/log_to_s...
remoting/webapp/me2mom/log_to_server.js:203:
remoting.LogToServer.prototype.getHostData = function() {
This doesn't need to be a member function. Better to make it a simple function
inside an enclosing anonymising function.

http://codereview.chromium.org/8527011/diff/1/remoting/webapp/me2mom/log_to_s...
remoting/webapp/me2mom/log_to_server.js:214:
remoting.LogToServer.prototype.extractHostDataFrom = function(s) {
Ditto. In fact, unless it's called with anything other than navigator.userAgent,
it can probably just be merged into getHostData.

http://codereview.chromium.org/8527011/diff/1/remoting/webapp/me2mom/log_to_s...
remoting/webapp/me2mom/log_to_server.js:278:
remoting.LogToServer.prototype.getChromeVersion = function() {
My comments for getHostData also apply to these functions.

[Wez, 2011-11-11 01:41:04]

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

http://codereview.chromium.org/8527011/diff/1/remoting/webapp/me2mom/log_to_s...
remoting/webapp/me2mom/log_to_server.js:141: remoting.wcs.sendIq(stanza);
If we're unable to send the stanza then we'll lose log lines, right?

Should we log them to local storage so that we can re-attempt logging them
later, to avoid stats being skewed by network outages?

[simonmorris, 2011-11-11 17:11:23]

PTAL

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

http://codereview.chromium.org/8527011/diff/1/remoting/webapp/me2mom/log_to_s...
remoting/webapp/me2mom/log_to_server.js:23:
remoting.LogToServer.prototype.VALUE_EVENT_NAME_SESSION_STATE = 'session-state';
On 2011/11/10 23:27:43, Jamie wrote:
> Is it worth tagging these as @private (and appending underscores?)

Done.

http://codereview.chromium.org/8527011/diff/1/remoting/webapp/me2mom/log_to_s...
remoting/webapp/me2mom/log_to_server.js:34:
remoting.LogToServer.prototype.VALUE_SESSION_STATE = function(state) {
On 2011/11/10 23:27:43, Jamie wrote:
> I worry about having to remember to keep this translation (and the next one)
in
> sync if we add error types. Can we include the number in the undefined case?
Is
> it even worth doing this translation, rather than just logging the numbers?
>

I've added a number in the undefined case. But if we only logged numbers, then
whoever read the logs would have to refer back to client_session.js to interpret
them. A better fix might be to have the log key names attached to the State
constants as properties. That pushes the log value definitions away from this
file, but it lets type-checking make sure that the right strings are defined.

> I'm also not wild about the function name, though I can see you're going for
> consistency with the other VALUE_ constants. I'd suggest stringifyClientState
or
> something similar, but it's not something I feel strongly about.

Done.

http://codereview.chromium.org/8527011/diff/1/remoting/webapp/me2mom/log_to_s...
remoting/webapp/me2mom/log_to_server.js:65:
remoting.LogToServer.prototype.VALUE_CONNECTION_ERROR =
On 2011/11/10 23:27:43, Jamie wrote:
> Ditto for this function.

Done.

http://codereview.chromium.org/8527011/diff/1/remoting/webapp/me2mom/log_to_s...
remoting/webapp/me2mom/log_to_server.js:89:
remoting.LogToServer.prototype.VALUE_OS_NAME_CHROMEOS = 'ChromeOS';
On 2011/11/10 23:27:43, Jamie wrote:
> What about other OSes? Seems like we should have an 'Unknown' value here.

The logging stanza, and the logging backend, are happy with
omitted values. It's simpler not to send a value if we don't
have one to send.

http://codereview.chromium.org/8527011/diff/1/remoting/webapp/me2mom/log_to_s...
remoting/webapp/me2mom/log_to_server.js:108: this.addHostFields(entry);
On 2011/11/10 23:27:43, Jamie wrote:
> This feels a little awkward. Would it be much more effort to move these add*
> methods onto a separate Entry class? Failing that, could they be global
> functions, hidden inside a scoping function like we've used elsewhere?

Done.

http://codereview.chromium.org/8527011/diff/1/remoting/webapp/me2mom/log_to_s...
remoting/webapp/me2mom/log_to_server.js:124: entryStr += key + '="' + entry[key]
+ '" ';
On 2011/11/10 23:27:43, Jamie wrote:
> I feel we ought to be XML-escaping these. I don't think it's a problem now
> because the set of strings we log is constrained, but as it grows it will be
one
> less thing to worry about.

Done.

http://codereview.chromium.org/8527011/diff/1/remoting/webapp/me2mom/log_to_s...
remoting/webapp/me2mom/log_to_server.js:140: remoting.debug.log('Sending log iq:
' + stanza);
On 2011/11/10 23:27:43, Jamie wrote:
> Is it worth using Gary's pretty-printing code to log this?

Done, though the prett-printing code doesn't do anything
with them. I'll send a separate CL to Gary.

http://codereview.chromium.org/8527011/diff/1/remoting/webapp/me2mom/log_to_s...
remoting/webapp/me2mom/log_to_server.js:141: remoting.wcs.sendIq(stanza);
On 2011/11/11 01:41:04, Wez wrote:
> If we're unable to send the stanza then we'll lose log lines, right?
> 
> Should we log them to local storage so that we can re-attempt logging them
> later, to avoid stats being skewed by network outages?

WCS sends stanzas asynchronously, so we don't know at this
stage whether the stanza was sent successfully. WCS also
copes with temporary network outages - I think it enqueues
stanzas and leaves them in the queue if the network is out.

We could listen for a response to the log stanza, and retry
if we don't get one. But that would introduce failure modes
of its own, has a fairly high cost/benefit ratio, and isn't
needed in this CL.

http://codereview.chromium.org/8527011/diff/1/remoting/webapp/me2mom/log_to_s...
remoting/webapp/me2mom/log_to_server.js:150:
remoting.LogToServer.prototype.makeBasicEntry = function() {
On 2011/11/10 23:27:43, Jamie wrote:
> This could be a ctor for a LogEntry class.

Done.

http://codereview.chromium.org/8527011/diff/1/remoting/webapp/me2mom/log_to_s...
remoting/webapp/me2mom/log_to_server.js:203:
remoting.LogToServer.prototype.getHostData = function() {
On 2011/11/10 23:27:43, Jamie wrote:
> This doesn't need to be a member function. Better to make it a simple function
> inside an enclosing anonymising function.

I don't understand why that would be better. Maybe this
comment has been addressed by having a separate
ServerLogEntry class?

http://codereview.chromium.org/8527011/diff/1/remoting/webapp/me2mom/log_to_s...
remoting/webapp/me2mom/log_to_server.js:214:
remoting.LogToServer.prototype.extractHostDataFrom = function(s) {
On 2011/11/10 23:27:43, Jamie wrote:
> Ditto. In fact, unless it's called with anything other than
navigator.userAgent,
> it can probably just be merged into getHostData.

When I tested the code, it was called with the strings
listed in the next comment. When we get unit tests,
they will call this function.

http://codereview.chromium.org/8527011/diff/1/remoting/webapp/me2mom/log_to_s...
remoting/webapp/me2mom/log_to_server.js:278:
remoting.LogToServer.prototype.getChromeVersion = function() {
On 2011/11/10 23:27:43, Jamie wrote:
> My comments for getHostData also apply to these functions.

Mine too.

[Jamie, 2011-11-11 19:08:40]

Regarding the "private method" vs. "scoped global function" question, I think
it's largely philosophical so if you disagree, I'm not going to force the issue.
However, FWIW, I try to think how I'd do the same thing in C++. I always like to
keep my .h files small, so any function that doesn't act on an object's internal
state tends to get demoted to a static function in the corresponding .cxx file.
In Javascript we don't quite have the same separation, so it's less valuable,
but that's my reasoning.

Apart from that, LGTM.

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

http://codereview.chromium.org/8527011/diff/1/remoting/webapp/me2mom/log_to_s...
remoting/webapp/me2mom/log_to_server.js:203:
remoting.LogToServer.prototype.getHostData = function() {
On 2011/11/11 17:11:23, simonmorris wrote:
> On 2011/11/10 23:27:43, Jamie wrote:
> > This doesn't need to be a member function. Better to make it a simple
function
> > inside an enclosing anonymising function.
> 
> I don't understand why that would be better. Maybe this
> comment has been addressed by having a separate
> ServerLogEntry class?

I just meant that you don't need a LogToServer object in order to find out what
the local navigator info is. If it's something specific to this class, then it
belongs in this file, but making it a global function, scoped by an anonymous
function keeps it private.

The new implementation inside ServerLogEntry doesn't really address the problem,
although since that's a smaller class it bothers me less.

http://codereview.chromium.org/8527011/diff/1/remoting/webapp/me2mom/log_to_s...
remoting/webapp/me2mom/log_to_server.js:214:
remoting.LogToServer.prototype.extractHostDataFrom = function(s) {
On 2011/11/11 17:11:23, simonmorris wrote:
> On 2011/11/10 23:27:43, Jamie wrote:
> > Ditto. In fact, unless it's called with anything other than
> navigator.userAgent,
> > it can probably just be merged into getHostData.
> 
> When I tested the code, it was called with the strings
> listed in the next comment. When we get unit tests,
> they will call this function.
> 

Fair point, thanks for clarifying.

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

http://codereview.chromium.org/8527011/diff/4001/remoting/webapp/me2mom/log_t...
remoting/webapp/me2mom/log_to_server.js:30: var entry =
remoting.ServerLogEntry.prototype.makeClientSessionStateChange(
If this is effectively a static function, there's no need to add it to the
prototype. I was actually thinking along the lines of a constructor function,
but since we'd want to overload it, a static factory method is probably cleaner.

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

http://codereview.chromium.org/8527011/diff/4001/remoting/webapp/me2mom/serve...
remoting/webapp/me2mom/server_log_entry.js:42:
remoting.ServerLogEntry.prototype.getValueForSessionState = function(state) {
This also doesn't need to be a member function.