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

remoting/client/plugin/chromoting_instance.cc

 // Copyright (c) 2012 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.
 
 #include "remoting/client/plugin/chromoting_instance.h"
 
 #include <string>
 #include <vector>
 
 #if defined(OS_NACL)
@@ skipping 14 matching lines @@
 #include "base/threading/thread.h"
 #include "base/values.h"
 #include "crypto/random.h"
 #include "jingle/glue/thread_wrapper.h"
 #include "media/base/yuv_convert.h"
 #include "net/socket/ssl_server_socket.h"
 #include "ppapi/cpp/completion_callback.h"
 #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"
 #include "remoting/base/constants.h"
 #include "remoting/base/util.h"
 #include "remoting/client/chromoting_client.h"
 #include "remoting/client/plugin/delegating_signal_strategy.h"
 #include "remoting/client/plugin/normalizing_input_filter_cros.h"
 #include "remoting/client/plugin/normalizing_input_filter_mac.h"
 #include "remoting/client/plugin/pepper_audio_player.h"
@@ skipping 1002 matching lines @@
 void ChromotingInstance::SendPerfStats() {
   if (!video_renderer_.get()) {
     return;
   }
 
   plugin_task_runner_->PostDelayedTask(
       FROM_HERE, base::Bind(&ChromotingInstance::SendPerfStats,
                             weak_factory_.GetWeakPtr()),
       base::TimeDelta::FromMilliseconds(kPerfStatsIntervalMs));
 
   scoped_ptr<base::DictionaryValue> data(new base::DictionaryValue());

[Wez, 2015/06/29 14:56:13]

Suggest a comment here:

"Post performance stats averaged over the past few seconds, to JS, to log, and
optionally to display to the user."

or similar.

[anandc, 2015/06/30 20:17:39]

Done.

   ChromotingStats* stats = video_renderer_->GetStats();
   data->SetDouble("videoBandwidth", stats->video_bandwidth()->Rate());
   data->SetDouble("videoFrameRate", stats->video_frame_rate()->Rate());
   data->SetDouble("captureLatency", stats->video_capture_ms()->Average());
   data->SetDouble("encodeLatency", stats->video_encode_ms()->Average());
   data->SetDouble("decodeLatency", stats->video_decode_ms()->Average());
   data->SetDouble("renderLatency", stats->video_paint_ms()->Average());
   data->SetDouble("roundtripLatency", stats->round_trip_ms()->Average());
   PostLegacyJsonMessage("onPerfStats", data.Pass());
+
+  // Also update UMA histograms for video frame-rate, bandwidth and latencies.

[Wez, 2015/06/29 14:56:13]

nit: "Also" implies that there is a comment earlier on to explain what we were
previously doing.

Suggest just "Update UMA with stats averaged over the last second."

[anandc, 2015/06/30 20:17:39]

Done.

+  pp::UMAPrivate uma_interface(this);
+  // We don't expect a frame-rate greater than 40fps, so set boundary to 40.

[Wez, 2015/06/29 14:56:13]

This comment should appear next to a constant defining the value, used in the
calls below.

[anandc, 2015/06/30 20:17:39]

Thanks. Done.
Also updated to 41, based on explanation here:
https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/c/private/pp...

+  uma_interface.HistogramEnumeration(
+      "Chromoting.Video.FrameRate", stats->video_fps()->Rate(), 40);
+  uma_interface.HistogramEnumeration(
+      "Chromoting.Video.FrameRate",
+      stats->video_fps_with_empty_frames()->Rate(), 40);
+
+  // For bandwidth, set the max value to 1MB/s, spread across 1000 buckets.

[Wez, 2015/06/29 14:56:13]

Ditto.

What is the rationale for 1000 buckets?

[anandc, 2015/06/30 20:17:39]

Done.
If, after we go live with these settings, we find that the histograms we get are
not fine-grained enough, we can tweak them. 
If that does happen, we should rename the metrics, rather than change the
settings of existing metrics, because that messes up historical comparisons. 

So, while the current settings look OK to me, please let me know if you have
better recommendations for these values.

Once we have OK from our team for the current histogram values and metric names,
I plan on getting them reviewed by a UMA team member as well.

+  uma_interface.HistogramCustomCounts("Chromoting.Video.Bandwidth",
+    stats->video_bytes_per_s()->Rate(), 0, 1024000, 1000);
+
+  // For, the latency metrics use a max value of 1000ms.

[dahlke1, 2015/06/24 23:12:11]

The max latency for the past 7 days is 11,381ms. If we must have a max value, we
should give ourselves some buffer (20,000) and understand what the effect is if
we go over that max value.

[Wez, 2015/06/29 14:56:13]

Ditto re constants as above, and rationale for the 1000 buckets, and the choice
of max latency.

[anandc, 2015/06/30 20:17:39]

Done.

+  uma_interface.HistogramCustomTimes("Chromoting.Video.CaptureLatency",
+    stats->video_capture_ms()->Average(), 0, 1000, 1000);
+  uma_interface.HistogramCustomTimes("Chromoting.Video.EncodeLatency",
+    stats->video_encode_ms()->Average(), 0, 1000, 1000);
+  uma_interface.HistogramCustomTimes("Chromoting.Video.RoundTripLatency",
+    stats->round_trip_ms()->Average(), 0, 1000, 1000);
 }
 
 // static
 void ChromotingInstance::RegisterLogMessageHandler() {
   base::AutoLock lock(g_logging_lock.Get());
 
   VLOG(1) << "Registering global log handler";
 
   // Record previous handler so we can call it in a chain.
   g_logging_old_handler = logging::GetLogMessageHandler();
@@ skipping 119 matching lines @@
 
 #if !defined(OS_NACL)
   // Log messages are forwarded to the webapp only in PNaCl version of the
   // plugin, so ProcessLogToUI() needs to be called explicitly in the non-PNaCl
   // version.
   ProcessLogToUI(message);
 #endif  // !defined(OS_NACL)
 }
 
 }  // namespace remoting