Add UMA stats for Chromoting connection details.

remoting/webapp/base/js/client_session.js

Index: remoting/webapp/base/js/client_session.js
diff --git a/remoting/webapp/base/js/client_session.js b/remoting/webapp/base/js/client_session.js
index 329decea3d212eefa74c3b3093d4bd65f85df4c4..02714474c67e19e33d27b5cb89916e0c069c5f79 100644
--- a/remoting/webapp/base/js/client_session.js
+++ b/remoting/webapp/base/js/client_session.js
@@ -139,8 +139,16 @@ remoting.ClientSession.EventHandler.prototype.onDisconnected =
 // the names given here.
 // The negative values represent state transitions that occur within the
 // web-app that have no corresponding plugin state transition.
+// NOTE: The enums here correspond to the Chromoting.Connections enumerated
+// histogram defined in src/tools/metrics/histograms/histograms.xml. UMA
+// histograms don't work well with negative values, so only non-negative values
+// have been used for Chromoting.Connections.
+// The maximum values for the UMA enumerated histogram is included here for use
+// when uploading values to UMA.
+// The 2 lists should be kept in sync, and any new enums should be append-only.
 /** @enum {number} */
 remoting.ClientSession.State = {
+  MIN_STATE_ENUM: -3,
   CONNECTION_CANCELED: -3,  // Connection closed (gracefully) before connecting.
   CONNECTION_DROPPED: -2,  // Succeeded, but subsequently closed with an error.
   CREATED: -1,
@@ -150,7 +158,10 @@ remoting.ClientSession.State = {
   AUTHENTICATED: 3,
   CONNECTED: 4,
   CLOSED: 5,
-  FAILED: 6
+  FAILED: 6,
+  HISTOGRAM_MAX: 10, // According to src/base/metrics/histogram.h, for a UMA

[Ilya Sherman, 2015/08/21 21:39:48]

Optional nit: I think it might be clearer to define a constant that's equal to 6
or 7 here (i.e. is obviously related to "FAILED"), and then perform math to
compute the maximum for the histogram below.

[anandc, 2015/08/21 21:56:10]

Done.

+                     // enumerated histogram, the upper limit should be 1 above
+                     // the max-enum.
 };
 
 /**
@@ -566,11 +577,31 @@ remoting.ClientSession.prototype.setState_ = function(newState) {
   }
 
   this.notifyStateChanges_(oldState, this.state_);
+  // Record state count in an UMA enumerated histogram.
+  recordState(this.state_);
   this.logger_.logClientSessionStateChange(
       this.state_, this.error_, this.xmppErrorCache_.getFirstError());
 };
 
 /**
+ * Records a Chromoting Connection State, stored in an UMA enumerated histogram.
+ * @param {remoting.ClientSession.State} state State identifier.
+ */
+function recordState(state) {
+  var metricDescription = {
+    metricName: 'Chromoting.Connections',
+    type: 'histogram-linear',
+    min: 1, // According to histogram.h, min should be 1.
+    max: remoting.ClientSession.State.HISTOGRAM_MAX,
+    // The # of buckets should include 1 for overflow.

[Ilya Sherman, 2015/08/21 21:39:48]

This comment is misleading -- the +1 below is actually for the underflow bucket,
which is the bucket with index 0.  The overflow bucket is accounted for by
setting max to be 1 more than the maximum value you ever plan to record.  (The
reason that the underflow bucket is used freely, but the overflow bucket is not,
is that this allows the enumeration to be extended in subsequent versions of
Chrome.)

[anandc, 2015/08/21 21:56:10]

Done.

+    buckets: remoting.ClientSession.State.HISTOGRAM_MAX + 1
+  };
+
+  chrome.metricsPrivate.recordValue(metricDescription, state -

[kelvinp, 2015/08/21 21:32:09]

Consider state == MIN_STATE_ENUM, the difference would be 0, which is smaller
than the min of 1 defined above. 

[Ilya Sherman, 2015/08/21 21:39:48]

That is actually ok -- enumerated histograms typically do use the 0-indexed
bucket, which is normally the underflow bucket, because enumerated histograms
don't have any underflow.  This is a weird quirk of the histograms API, and
would be much easier to reason about if the metricsPrivate API just provided a
method for recording enumerated histograms.

[anandc, 2015/08/21 21:56:10]

Acknowledged.

[anandc, 2015/08/21 21:56:10]

Added comment explaining how this works. Tested by seeing that count for the
Canceled action (and, luckily, not Dropped, which would be harder to test) which
maps to MIN_STATE_ENUM gets updated as expected.

+      remoting.ClientSession.State.MIN_STATE_ENUM);
+}
+
+/**
  * @param {remoting.ClientSession.State} oldState The new state for the session.
  * @param {remoting.ClientSession.State} newState The new state for the session.
  * @private