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

remoting/client/plugin/chromoting_instance.cc

Index: remoting/client/plugin/chromoting_instance.cc
diff --git a/remoting/client/plugin/chromoting_instance.cc b/remoting/client/plugin/chromoting_instance.cc
index c32d31ded27bcd4d333f047d9af82c1536d5442a..ac787960e439fddbe24d1ec63bec3b3a415a6021 100644
--- a/remoting/client/plugin/chromoting_instance.cc
+++ b/remoting/client/plugin/chromoting_instance.cc
@@ -32,6 +32,7 @@
 #include "ppapi/cpp/dev/url_util_dev.h"
 #include "ppapi/cpp/image_data.h"
 #include "ppapi/cpp/input_event.h"
+#include "ppapi/cpp/private/uma_private.h"
 #include "ppapi/cpp/rect.h"
 #include "ppapi/cpp/var_array_buffer.h"
 #include "ppapi/cpp/var_dictionary.h"
@@ -67,12 +68,25 @@ namespace {
 // Default DPI to assume for old clients that use notifyClientResolution.
 const int kDefaultDPI = 96;
 
-// Interval at which to sample performance statistics.
-const int kPerfStatsIntervalMs = 1000;
-
 // URL scheme used by Chrome apps and extensions.
 const char kChromeExtensionUrlScheme[] = "chrome-extension";
 
+// The boundary value for the FPS histogram: we don't expect video frame-rate to
+// be greater than 40fps. Leaving some room for future improvements, we'll set
+// the max frame rate to 60fps.
+// Histograms expect samples to be less than the boundary value, so set to 61.
+const int kMaxFrameRate = 61;

[Wez, 2015/07/14 20:24:17]

nit: Be explicit as to units, e.g. kMaxFramesPerSec?

[anandc, 2015/07/15 03:59:54]

Done.

+
+// For bandwidth, based on expected real-world numbers, we'll use a histogram
+// ranging from 0 to 10MB/s, spread across 100 buckets.
+// Histograms are log-scaled by default. This results in fine-grained buckets at
+// lower values and wider-ranged buckets closer to the maximum.
+// Values above the maximum defined here end up in the right-most bucket.

[Wez, 2015/07/14 20:24:16]

What does it mean for a bucket to be "right-most"? There doesn't seem to be any
other mention of right or left in these comments, nor code?

[anandc, 2015/07/15 03:59:55]

Yes, that is unclear. The bucket with max values could be right-most or
bottom-most depending on how it is graphed.
Corrected to say max-bucket.

[Wez, 2015/07/16 19:03:36]

Acknowledged.

+// See $/src/base/metrics/histogram.h for more details.

[Wez, 2015/07/14 20:24:16]

Include the units that these constants are in as a suffix on the name, e.g.
kBandwidthHistogramMinBps

[anandc, 2015/07/15 03:59:54]

Done.

+const int kBandwidthHistogramMin = 1;

[Wez, 2015/07/14 20:24:17]

In the comment you state that the minimum is zero, not 1?

[anandc, 2015/07/15 03:59:54]

UMA normalizes a histogram's minimum to 1 if it is < 1. Added a clarification.

+const int kBandwidthHistogramMax = 10000000;

[Wez, 2015/07/14 20:24:17]

Express this as 10 * 1000 * 1000 for clarity.

Do you mean to use MegaByte (MB), versus MebiByte (MiB) here?

[anandc, 2015/07/15 03:59:55]

MegaByte.

+const int kBandwidthHistogramNumBuckets = 100;

[Wez, 2015/07/14 20:24:16]

nit: "Num" in the name adds nothing - we know it's a number, 'cos it's an int.
:)

[anandc, 2015/07/15 03:59:54]

Done.

+
 #if defined(USE_OPENSSL)
 // Size of the random seed blob used to initialize RNG in libjingle. Libjingle
 // uses the seed only for OpenSSL builds. OpenSSL needs at least 32 bytes of
@@ -728,11 +742,12 @@ void ChromotingInstance::HandleConnect(const base::DictionaryValue& data) {
   client_->Start(signal_strategy_.get(), authenticator.Pass(),
                  transport_factory.Pass(), host_jid, capabilities);
 
-  // Start timer that periodically sends perf stats.
+  // Start timers that periodically send perf stats.

[Wez, 2015/07/14 20:24:17]

timers -> timer

[anandc, 2015/07/15 03:59:55]

Done.

   plugin_task_runner_->PostDelayedTask(
       FROM_HERE, base::Bind(&ChromotingInstance::SendPerfStats,
                             weak_factory_.GetWeakPtr()),
-      base::TimeDelta::FromMilliseconds(kPerfStatsIntervalMs));
+      base::TimeDelta::FromSeconds(
+        ChromotingStats::kStatsUpdateFrequencyInSeconds));

[Alexei Svitkine (slow), 2015/07/14 20:16:33]

Nit: Indent 2 more.

[Wez, 2015/07/14 20:24:17]

nit: Indentation - have you run git cl format on this CL?

[anandc, 2015/07/15 03:59:54]

Sorry, I was using clang-format and it was raising a lot more style-changes than
just git cl format.
Ran git cl format now, and fixed. Thanks.

[anandc, 2015/07/15 03:59:55]

Acknowledged.

[Wez, 2015/07/16 19:03:36]

Hmmm, if clang-format has more complaints then can you email me a sample of what
it's upset about - perhaps we can clean that up as a follow-up.

[anandc, 2015/07/16 20:18:47]

Yes, I'm planning to do that in a separate CL (already spoke to sergeyu@ about
it).

 }
 
 void ChromotingInstance::HandleDisconnect(const base::DictionaryValue& data) {
@@ -1052,8 +1067,12 @@ void ChromotingInstance::SendPerfStats() {
   plugin_task_runner_->PostDelayedTask(
       FROM_HERE, base::Bind(&ChromotingInstance::SendPerfStats,
                             weak_factory_.GetWeakPtr()),
-      base::TimeDelta::FromMilliseconds(kPerfStatsIntervalMs));
+      base::TimeDelta::FromSeconds(
+        ChromotingStats::kStatsUpdateFrequencyInSeconds));

[Alexei Svitkine (slow), 2015/07/14 20:16:33]

Ditto.

[Wez, 2015/07/14 20:24:17]

nit: Indentation.

[anandc, 2015/07/15 03:59:54]

Done.

[anandc, 2015/07/15 03:59:55]

Acknowledged.

 
+  // Update performance stats and send them to the client for display to users.

[Wez, 2015/07/14 20:24:17]

nit: This doesn't update the stats, it just fetches them from the VideoRenderer
and marshals them to post to the app.

[anandc, 2015/07/15 03:59:55]

Done.

+  // The rate metrics are averaged over 1s, and the latency metrics are averaged
+  // over the 10 most recent samples.

[Wez, 2015/07/14 20:24:17]

This is already documented in ChromotingStats - don't duplicate it here.

[anandc, 2015/07/15 03:59:54]

Done.

   scoped_ptr<base::DictionaryValue> data(new base::DictionaryValue());
   ChromotingStats* stats = video_renderer_->GetStats();
   data->SetDouble("videoBandwidth", stats->video_bandwidth()->Rate());
@@ -1064,6 +1083,17 @@ void ChromotingInstance::SendPerfStats() {
   data->SetDouble("renderLatency", stats->video_paint_ms()->Average());
   data->SetDouble("roundtripLatency", stats->round_trip_ms()->Average());
   PostLegacyJsonMessage("onPerfStats", data.Pass());
+
+  // Upload to UMA the video frame-rate, packet-rate and bandwidth stats,
+  // averaged over 1s.

[Wez, 2015/07/14 20:24:16]

nit: Record the video frame-rate, ... to UMA.

Again, no need to describe the averaging here, since that's described in
ChromotingStats.

[anandc, 2015/07/15 03:59:55]

Done.

+  pp::UMAPrivate uma(this);
+  uma.HistogramEnumeration("Chromoting.Video.FrameRate",
+      stats->video_frame_rate()->Rate(), kMaxFrameRate);
+  uma.HistogramEnumeration("Chromoting.Video.PacketRate",
+      stats->video_packet_rate()->Rate(), kMaxFrameRate);
+  uma.HistogramCustomCounts("Chromoting.Video.Bandwidth",
+      stats->video_bandwidth()->Rate(), kBandwidthHistogramMin,
+      kBandwidthHistogramMax, kBandwidthHistogramNumBuckets);
 }
 
 // static