Report video and network stats averaged over 1s, and create corresponding UMA metrics.

remoting/client/chromoting_stats.h

Index: remoting/client/chromoting_stats.h
diff --git a/remoting/client/chromoting_stats.h b/remoting/client/chromoting_stats.h
index da7601b7f9222d3026fe768713db067a8a4c0b78..2a58c0959c47a678792bcea22b99c353f63201ee 100644
--- a/remoting/client/chromoting_stats.h
+++ b/remoting/client/chromoting_stats.h
@@ -18,17 +18,49 @@ class ChromotingStats {
   ChromotingStats();
   virtual ~ChromotingStats();
 
-  RateCounter* video_bandwidth() { return &video_bandwidth_; }
-  RateCounter* video_frame_rate() { return &video_frame_rate_; }
+  // Window for UMA stats updates and rate-counters.
+  static const int kUMAStatsTimeWindowInSeconds = 1;

[Wez, 2015/07/08 23:27:54]

Why is this "TimeWindow" and the one below "RateWindow" and the final one
"UpdateWindow"?

The combination of the names + comments doesn't give me any real insight into
how these are used - why the UMA stats are "time" while the displays stats are
"rates", and what it is that we update every 60s - the latter is the existing
(non-UMA) stats logging, isn't it?

Why not name these to match the _display vs _uma naming, below?

Also, why are these statics on this class, rather than constants within the
implementation?

[Wez, 2015/07/08 23:42:29]

nit: kUmaStats...

[anandc, 2015/07/09 19:17:09]

Acknowledged.

[anandc, 2015/07/09 19:17:09]

Cleaned these up.
Also added comment for why the static is declared here.

+  // The time-window for display-stats rate-counters.
+  static const int kDisplayStatsRateWindowInSeconds = 3;
+  // The time interval for updating display stats.
+  static const int kDisplayStatsUpdateWindowInSeconds = 60;
+
+  // The following 2 metrics are used in the stats captured every 60s for
+  // display to users.

[Wez, 2015/07/08 23:27:54]

Display stats are captured every second for display to users, not every 60s. Do
you mean the stats upload?

[anandc, 2015/07/09 19:17:09]

Sorry, these were quite confusing. PTAL at latest comments.

+  // Bytes/sec for non-empty packets, measured over a 1s time-window.
+  RateCounter* video_bandwidth_display() { return &video_bandwidth_display_; }
+  // frames/sec for non-empty video-packets, measured over a 1s time-window.
+  RateCounter* video_frame_rate_display() { return &video_frame_rate_display_; }
+
+  // The next 3 metrics are used to record statistics for upload to UMA.
+  // Bytes/sec for non-empty video-packets, measured over a 1s time-window.

[Wez, 2015/07/08 23:27:54]

You're calling this video_bandwidth_uma but then specifying in the comment the
size of the window - either call this video_bandwidth_1s so it's clear it's the
video-bandwidth over the past second, or remove the window aspect from the
comment, since it is documented via the constants above.

[anandc, 2015/07/09 19:17:09]

Done.

+  RateCounter* video_bandwidth_uma() { return &video_bandwidth_uma_; }
+  // frames/sec for non-empty video-packets, measured over a 1s time-window.
+  RateCounter* video_frame_rate_uma() { return &video_frame_rate_uma_; }
+  // packets/sec, including empty video-packets, measured over a 1s time-window.
+
+  RateCounter* video_packet_rate_uma() { return &video_packet_rate_uma_; }
+
+  // The following metrics report averages calculated when updating stats.
+  // For UMA, the updates occur every second. For stats shown to users, they are
+  // updated every 60s.

[Wez, 2015/07/08 23:27:55]

This isn't true, AFAIK; these stats are updated after every frame, and can be
sampled at any time - UMA & display will presumably sample every 1s and non-UMA
stats upload every 60s.

[Wez, 2015/07/08 23:42:29]

It's also misleading since these stats are averaged over the past 10s, in
contrast to the display values (averaged over 3s) and the UMA values (averaged
over 1s).

[anandc, 2015/07/09 19:17:09]

Acknowledged.

[anandc, 2015/07/09 19:17:09]

Acknowledged.

+  // Time to capture video.
   RunningAverage* video_capture_ms() { return &video_capture_ms_; }
+  // Time to encode a captured video-packet.
   RunningAverage* video_encode_ms() { return &video_encode_ms_; }
+  // Time to decode a video-packet.
   RunningAverage* video_decode_ms() { return &video_decode_ms_; }
+  // Time to paint, if there is a picture to paint or on force-repaint.
   RunningAverage* video_paint_ms() { return &video_paint_ms_; }
+  // Round-trip latency.
   RunningAverage* round_trip_ms() { return &round_trip_ms_; }
 
  private:
-  RateCounter video_bandwidth_;
-  RateCounter video_frame_rate_;
+  RateCounter video_bandwidth_display_;
+  RateCounter video_frame_rate_display_;
+  RateCounter video_bandwidth_uma_;
+  RateCounter video_frame_rate_uma_;
+  RateCounter video_packet_rate_uma_;
   RunningAverage video_capture_ms_;
   RunningAverage video_encode_ms_;
   RunningAverage video_decode_ms_;