The chromoting client sends simple logging to the server.

remoting/webapp/me2mom/log_to_server.js

+// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+/**
+ * @fileoverview
+ * Module for sending log entries to the server.
+ */
+
+'use strict';
+
+/** @suppress {duplicate} */
+var remoting = remoting || {};
+
+/**
+ * @constructor
+ */
+remoting.LogToServer = function() {
+  /** @type Array.<string> */ this.pendingEntries = [];
+};
+
+remoting.LogToServer.prototype.KEY_EVENT_NAME = 'event-name';
+remoting.LogToServer.prototype.VALUE_EVENT_NAME_SESSION_STATE = 'session-state';

[Jamie, 2011/11/10 23:27:43]

Is it worth tagging these as @private (and appending underscores?)

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

Done.

+
+remoting.LogToServer.prototype.KEY_ROLE = 'role';
+remoting.LogToServer.prototype.VALUE_ROLE_CLIENT = 'client';
+
+remoting.LogToServer.prototype.KEY_SESSION_STATE = 'session-state';
+
+/**
+ * @param {remoting.ClientSession.State} state
+ * @return {string}
+ */
+remoting.LogToServer.prototype.VALUE_SESSION_STATE = function(state) {

[Jamie, 2011/11/10 23:27:43]

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.

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

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.
Done.

+  switch(state) {
+    case remoting.ClientSession.State.UNKNOWN:
+      return 'unknown';
+    case remoting.ClientSession.State.CREATED:
+      return 'created';
+    case remoting.ClientSession.State.BAD_PLUGIN_VERSION:
+      return 'bad-plugin-version';
+    case remoting.ClientSession.State.UNKNOWN_PLUGIN_ERROR:
+      return 'unknown-plugin-error';
+    case remoting.ClientSession.State.CONNECTING:
+      return 'connecting';
+    case remoting.ClientSession.State.INITIALIZING:
+      return 'initializing';
+    case remoting.ClientSession.State.CONNECTED:
+      return 'connected';
+    case remoting.ClientSession.State.CLOSED:
+      return 'closed';
+    case remoting.ClientSession.State.CONNECTION_FAILED:
+      return 'connection-failed';
+    default:
+      return 'undefined';
+  }
+};
+
+remoting.LogToServer.prototype.KEY_CONNECTION_ERROR = 'connection-error';
+
+/**
+ * @param {remoting.ClientSession.ConnectionError} connectionError
+ * @return {string}
+ */
+remoting.LogToServer.prototype.VALUE_CONNECTION_ERROR =

[Jamie, 2011/11/10 23:27:43]

Ditto for this function.

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

Done.

+    function(connectionError) {
+  switch(connectionError) {
+    case remoting.ClientSession.ConnectionError.NONE:
+      return 'none';
+    case remoting.ClientSession.ConnectionError.HOST_IS_OFFLINE:
+      return 'host-is-offline';
+    case remoting.ClientSession.ConnectionError.SESSION_REJECTED:
+      return 'session-rejected';
+    case remoting.ClientSession.ConnectionError.INCOMPATIBLE_PROTOCOL:
+      return 'incompatible-protocol';
+    case remoting.ClientSession.ConnectionError.NETWORK_FAILURE:
+      return 'network-failure';
+    case remoting.ClientSession.ConnectionError.OTHER:
+      return 'other';
+    default:
+      return 'unknown';
+  }
+}
+
+remoting.LogToServer.prototype.KEY_OS_NAME = 'os-name';
+remoting.LogToServer.prototype.VALUE_OS_NAME_WINDOWS = 'Windows';
+remoting.LogToServer.prototype.VALUE_OS_NAME_LINUX = 'Linux';
+remoting.LogToServer.prototype.VALUE_OS_NAME_MAC = 'Mac';
+remoting.LogToServer.prototype.VALUE_OS_NAME_CHROMEOS = 'ChromeOS';

[Jamie, 2011/11/10 23:27:43]

What about other OSes? Seems like we should have an 'Unknown' value here.

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

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.

+
+remoting.LogToServer.prototype.KEY_OS_VERSION = 'os-version';
+
+remoting.LogToServer.prototype.KEY_CPU = 'cpu';
+
+remoting.LogToServer.prototype.KEY_BROWSER_VERSION = 'browser-version';
+
+remoting.LogToServer.prototype.KEY_WEBAPP_VERSION = 'webapp-version';
+
+/**
+ * Logs a client session state change.
+ *
+ * @param {remoting.ClientSession.State} state
+ * @param {remoting.ClientSession.ConnectionError} connectionError
+ */
+remoting.LogToServer.prototype.logClientSessionStateChange =
+    function(state, connectionError) {
+  var entry = this.makeClientSessionStateChange(state, connectionError);
+  this.addHostFields(entry);

[Jamie, 2011/11/10 23:27:43]

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?

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

Done.

+  this.addChromeVersionField(entry);
+  this.addWebappVersionField(entry);
+  this.log(entry);
+};
+
+/**
+ * Sends a log entry to the server.
+ *
+ * @private
+ * @param {Object.<string, string>} entry
+ */
+remoting.LogToServer.prototype.log = function(entry) {
+  // Convert the entry to a string.
+  var entryStr = '<gr:entry ';
+  for (var key in entry) {
+    entryStr += key + '="' + entry[key] + '" ';

[Jamie, 2011/11/10 23:27:43]

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.

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

Done.

+  }
+  entryStr += '/>';
+  // Store that string.
+  this.pendingEntries.push(entryStr);
+  // Stop if there's no connection to the server.
+  if (!remoting.wcs) {
+    return;
+  }
+  // Send all pending entries to the server.
+  var stanza = '<cli:iq to="remoting@bot.talk.google.com" type="set" ' +
+      'xmlns:cli="jabber:client"><gr:log xmlns:gr="google:remoting">';
+  while (this.pendingEntries.length > 0) {
+    stanza += /** @type string */ this.pendingEntries.shift();
+  }
+  stanza += '</gr:log></cli:iq>';
+  remoting.debug.log('Sending log iq: ' + stanza);

[Jamie, 2011/11/10 23:27:43]

Is it worth using Gary's pretty-printing code to log this?

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

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

+  remoting.wcs.sendIq(stanza);

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

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]

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.

+};
+
+/**
+ * Makes a log entry with basic fields.
+ *
+ * @private
+ * @return {Object.<string, string>}
+ */
+remoting.LogToServer.prototype.makeBasicEntry = function() {

[Jamie, 2011/11/10 23:27:43]

This could be a ctor for a LogEntry class.

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

Done.

+  /** @type Object.<string, string> */ var entry = {};
+  entry[this.KEY_ROLE] = this.VALUE_ROLE_CLIENT;
+  return entry;
+};
+
+/**
+ * Makes a log entry for a change of client session state.
+ *
+ * @private
+ * @param {remoting.ClientSession.State} state
+ * @param {remoting.ClientSession.ConnectionError} connectionError
+ * @return {Object.<string, string>}
+ */
+remoting.LogToServer.prototype.makeClientSessionStateChange =
+    function(state, connectionError) {
+  var entry = this.makeBasicEntry();
+  entry[this.KEY_EVENT_NAME] = this.VALUE_EVENT_NAME_SESSION_STATE;
+  entry[this.KEY_SESSION_STATE] = this.VALUE_SESSION_STATE(state);
+  if (connectionError != remoting.ClientSession.ConnectionError.NONE) {
+    entry[this.KEY_CONNECTION_ERROR] =
+        this.VALUE_CONNECTION_ERROR(connectionError);
+  }
+  return entry;
+};
+
+/**
+ * Adds fields describing the host to a given log entry.
+ *
+ * @private
+ * @param {Object.<string, string>} entry
+ */
+remoting.LogToServer.prototype.addHostFields = function(entry) {
+  var host = this.getHostData();
+  if (host) {
+    if (host.os_name.length > 0) {
+      entry[this.KEY_OS_NAME] = host.os_name;
+    }
+    if (host.os_version.length > 0) {
+      entry[this.KEY_OS_VERSION] = host.os_version;
+    }
+    if (host.cpu.length > 0) {
+      entry[this.KEY_CPU] = host.cpu;
+    }
+  }
+};
+
+/**
+ * Extracts host data from the userAgent string.
+ *
+ * @private
+ * @return {{os_name:string, os_version:string, cpu:string} | null}
+ */
+remoting.LogToServer.prototype.getHostData = function() {

[Jamie, 2011/11/10 23:27:43]

This doesn't need to be a member function. Better to make it a simple function
inside an enclosing anonymising function.

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

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

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

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.

+  return this.extractHostDataFrom(navigator.userAgent);
+};
+
+/**
+ * Extracts host data from the given userAgent string.
+ *
+ * @private
+ * @param {string} s
+ * @return {{os_name:string, os_version:string, cpu:string} | null}
+ */
+remoting.LogToServer.prototype.extractHostDataFrom = function(s) {

[Jamie, 2011/11/10 23:27:43]

Ditto. In fact, unless it's called with anything other than navigator.userAgent,
it can probably just be merged into getHostData.

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

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.

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

Fair point, thanks for clarifying.

+  // Sample userAgent strings:
+  // 'Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/535.2 ' +
+  //   '(KHTML, like Gecko) Chrome/15.0.874.106 Safari/535.2'
+  // 'Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/535.8 ' +
+  //   '(KHTML, like Gecko) Chrome/17.0.933.0 Safari/535.8'
+  // 'Mozilla/5.0 (Macintosh; Intel Mac OS X 10_6_8) AppleWebKit/535.1 ' +
+  //   '(KHTML, like Gecko) Chrome/14.0.835.202 Safari/535.1'
+  // 'Mozilla/5.0 (X11; CrOS i686 14.811.154) AppleWebKit/535.1 ' +
+  //   '(KHTML, like Gecko) Chrome/14.0.835.204 Safari/535.1'
+  var match = new RegExp('Windows NT ([0-9\\.]*)').exec(s);
+  if (match && (match.length >= 2)) {
+    return {
+        'os_name': this.VALUE_OS_NAME_WINDOWS,
+        'os_version': match[1],
+        'cpu': ''
+    };
+  }
+  match = new RegExp('Linux ([a-zA-Z0-9_]*)').exec(s);
+  if (match && (match.length >= 2)) {
+    return {
+        'os_name': this.VALUE_OS_NAME_LINUX,
+        'os_version' : '',
+        'cpu': match[1]
+    };
+  }
+  match = new RegExp('([a-zA-Z]*) Mac OS X ([0-9_]*)').exec(s);
+  if (match && (match.length >= 3)) {
+    return {
+        'os_name': this.VALUE_OS_NAME_MAC,
+        'os_version': match[2].replace(/_/g, '.'),
+        'cpu': match[1]
+    };
+  }
+  match = new RegExp('CrOS ([a-zA-Z0-9]*) ([0-9.]*)').exec(s);
+  if (match && (match.length >= 3)) {
+    return {
+        'os_name': this.VALUE_OS_NAME_CHROMEOS,
+        'os_version': match[2],
+        'cpu': match[1]
+    };
+  }
+  return null;
+};
+
+/**
+ * Adds a field specifying the browser version to a given log entry.
+ *
+ * @private
+ * @param {Object.<string, string>} entry
+ */
+remoting.LogToServer.prototype.addChromeVersionField = function(entry) {
+  var version = this.getChromeVersion();
+  if (version != null) {
+    entry[this.KEY_BROWSER_VERSION] = version;
+  }
+};
+
+/**
+ * Extracts the Chrome version from the userAgent string.
+ *
+ * @private
+ * @return {string | null}
+ */
+remoting.LogToServer.prototype.getChromeVersion = function() {

[Jamie, 2011/11/10 23:27:43]

My comments for getHostData also apply to these functions.

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

Mine too.

+  return this.extractChromeVersionFrom(navigator.userAgent);
+};
+
+/**
+ * Extracts the Chrome version from the given userAgent string.
+ *
+ * @private
+ * @param {string} s
+ * @return {string | null}
+ */
+remoting.LogToServer.prototype.extractChromeVersionFrom = function(s) {
+  var match = new RegExp('Chrome/([0-9.]*)').exec(s);
+  if (match && (match.length >= 2)) {
+    return match[1];
+  }
+  return null;
+};
+
+/**
+ * Adds a field specifying the webapp version to a given log entry.
+ *
+ * @private
+ * @param {Object.<string, string>} entry
+ */
+remoting.LogToServer.prototype.addWebappVersionField = function(entry) {
+  entry[this.KEY_WEBAPP_VERSION] = chrome.app.getDetails().version;
+};