NQE: Record correlation metric in UMA

net/nqe/network_quality_estimator.cc

Index: net/nqe/network_quality_estimator.cc
diff --git a/net/nqe/network_quality_estimator.cc b/net/nqe/network_quality_estimator.cc
index 256722a179b707ab1a3c37adc65e94bbe6b36e32..24b934408fdbc896e3b6d3019a0e4bc9785e3d78 100644
--- a/net/nqe/network_quality_estimator.cc
+++ b/net/nqe/network_quality_estimator.cc
@@ -15,6 +15,8 @@
 #include "base/logging.h"
 #include "base/metrics/histogram.h"
 #include "base/metrics/histogram_base.h"
+#include "base/metrics/sparse_histogram.h"
+#include "base/rand_util.h"
 #include "base/single_thread_task_runner.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/threading/thread_task_runner_handle.h"
@@ -25,9 +27,11 @@
 #include "net/base/load_timing_info.h"
 #include "net/base/network_interfaces.h"
 #include "net/base/url_util.h"
+#include "net/http/http_status_code.h"
 #include "net/nqe/socket_watcher_factory.h"
 #include "net/nqe/throughput_analyzer.h"
 #include "net/url_request/url_request.h"
+#include "net/url_request/url_request_status.h"
 #include "url/gurl.h"
 
 #if defined(OS_ANDROID)
@@ -141,6 +145,22 @@ bool GetValueForVariationParam(
          base::StringToInt(it->second, variations_value);
 }
 
+// Returns the variation value for |parameter_name|. If the value is
+// unavailable, |default_value| is returned.
+double GetDoubleValueForVariationParamWithDefaultValue(

[bengr, 2016/07/08 16:49:52]

This really should live somewhere in the variations code. Is that possible?

[tbansal1, 2016/07/08 17:45:51]

I am not sure, but I am planning to add a n_q_e_config class which will take
away all this code to a different place :)

+    const std::map<std::string, std::string>& variation_params,
+    const std::string& parameter_name,
+    double default_value) {
+  const auto it = variation_params.find(parameter_name);
+  if (it == variation_params.end())
+    return default_value;
+
+  double variations_value = default_value;
+  if (!base::StringToDouble(it->second, &variations_value))
+    return default_value;
+  return variations_value;
+}
+
 // Returns the algorithm that should be used for computing effective connection
 // type based on field trial params. Returns an empty string if a valid
 // algorithm paramter is not present in the field trial params.
@@ -188,6 +208,34 @@ std::string GetHistogramSuffixObservedRTT(const base::TimeDelta& observed_rtt) {
   return kSuffixes[arraysize(kSuffixes) - 1];
 }
 
+// The least significant kTrimBits of the metric will be discarded. If trimmed

[bengr, 2016/07/08 16:49:52]

If -> If the

Also, I don't understand why you trim bits off a metric. You should explain this
somewhere.

[tbansal1, 2016/07/08 17:45:50]

Done.
I do say below in FitInKBitsPerMetricBits() function: "// Remove the last
kTrimBits. This will allow the metric to fit within
  // kBitsPerMetric while losing only the least significant bits."

+// metric value is greater than what can be fit in kBitsPerMetric bits, then the
+// largest value that can be represented in kBitsPerMetric bits is returned.
+const int32_t kTrimBits = 5;
+const int32_t kBitsPerMetric = 7;
+
+static_assert(31 >= kBitsPerMetric * 4,
+              "Four metrics would not fit in a 32-bit int with first bit "
+              "reserved for sign");
+
+// Trims the |metric| by removing the last kTrimBits, and then rounding down
+// the |metric| such that the |metric| fits in kBitsPerMetric.
+int32_t FitInKBitsPerMetricBits(int32_t metric) {

[bengr, 2016/07/08 16:49:52]

I don't understand the point of this method. Is this some sort of optimization
or some workaround due to some limitation of the UMA API?

[tbansal1, 2016/07/08 17:45:51]

Yes, the goal is to record 4 32-bit samples in a single UMA sample (which is
itself 31 bit). So, somehow we need to trim down the 4 metrics from 32 bits to 7
bits. I did check with the UMA team before doing this.

[bengr, 2016/07/19 00:32:31]

Why do you have to store 4 samples in one UMA sample?

[tbansal1, 2016/07/20 00:28:02]

Added comments.

+  // Remove the last kTrimBits. This will allow the metric to fit within
+  // kBitsPerMetric while losing only the least significant bits.
+  metric = metric >> kTrimBits;
+
+  // kLargestValuePossible is the largest value that can be recorded using
+  // kBitsPerMetric.
+  static const int32_t kLargestValuePossible = (1 << kBitsPerMetric) - 1;

[Not at Google. Contact bengr, 2016/07/01 18:26:26]

Suggest moving up with the other constants, instead of redefining in every call.

[tbansal1, 2016/07/01 20:11:26]

As discussed offline, I would prefer to keep the scope restricted to function.
Also, there are other examples of similar code:
https://cs.chromium.org/search/?q=%5C+%5C+static%5C+const%5C+int%5C+k%5Ba-zA-...

+  if (metric > kLargestValuePossible) {
+    // Fit |metric| in kBitsPerMetric by clamping it down.
+    metric = kLargestValuePossible;
+  }
+  DCHECK_EQ(0, metric >> kBitsPerMetric);
+  return metric;
+}
+
 }  // namespace
 
 namespace net {
@@ -230,6 +278,11 @@ NetworkQualityEstimator::NetworkQualityEstimator(
       rtt_observations_(weight_multiplier_per_second_),
       external_estimate_provider_(std::move(external_estimates_provider)),
       effective_connection_type_(EFFECTIVE_CONNECTION_TYPE_UNKNOWN),
+      correlation_logging_probability_(
+          GetDoubleValueForVariationParamWithDefaultValue(
+              variation_params,
+              "correlation_logging_probability",
+              0.0)),
       weak_ptr_factory_(this) {
   static_assert(kDefaultHalfLifeSeconds > 0,
                 "Default half life duration must be > 0");
@@ -249,6 +302,8 @@ NetworkQualityEstimator::NetworkQualityEstimator(
   DCHECK_NE(EffectiveConnectionTypeAlgorithm::
                 EFFECTIVE_CONNECTION_TYPE_ALGORITHM_LAST,
             effective_connection_type_algorithm_);
+  DCHECK_LE(0.0, correlation_logging_probability_);
+  DCHECK_GE(1.0, correlation_logging_probability_);
 
   ObtainOperatingParams(variation_params);
   ObtainEffectiveConnectionTypeModelParams(variation_params);
@@ -490,6 +545,7 @@ void NetworkQualityEstimator::NotifyHeadersReceived(const URLRequest& request) {
       load_timing_info.receive_headers_end.is_null()) {
     return;
   }
+  DCHECK(!request.response_info().was_cached);
 
   // Duration between when the resource was requested and when the response
   // headers were received.
@@ -584,6 +640,84 @@ void NetworkQualityEstimator::NotifyRequestCompleted(
     return;
 
   throughput_analyzer_->NotifyRequestCompleted(request);
+  RecordCorrelationMetric(request);
+}
+
+void NetworkQualityEstimator::RecordCorrelationMetric(
+    const URLRequest& request) const {
+  DCHECK(thread_checker_.CalledOnValidThread());
+
+  // The histogram is recorded with probability
+  // |correlation_logging_probability_| to reduce overhead.

[bengr, 2016/07/08 16:49:52]

What is the overhead?

[tbansal1, 2016/07/08 17:45:50]

Done. Added more comments.

+  if (base::RandDouble() >= correlation_logging_probability_)
+    return;
+
+  if (request.response_info().was_cached ||
+      !request.response_info().network_accessed) {
+    return;
+  }
+
+  LoadTimingInfo load_timing_info;
+  request.GetLoadTimingInfo(&load_timing_info);
+  DCHECK(!load_timing_info.send_start.is_null() &&
+         !load_timing_info.receive_headers_end.is_null());
+
+  // Record UMA only for successful requests that have completed.
+  if (!request.status().is_success() || request.status().is_io_pending())
+    return;
+  if (request.GetResponseCode() != HTTP_OK)
+    return;
+  if (load_timing_info.receive_headers_end < last_main_frame_request_)
+    return;
+
+  const base::TimeTicks now = tick_clock_->NowTicks();
+  // Record UMA only for requests that started recently.
+  if (now - last_main_frame_request_ > base::TimeDelta::FromSeconds(15))
+    return;
+
+  DCHECK_GE(now, load_timing_info.send_start);
+
+  const int32_t transport_rtt_milliseconds =
+      estimated_quality_at_last_main_frame_.transport_rtt() !=
+              nqe::internal::InvalidRTT()
+          ? FitInKBitsPerMetricBits(
+                estimated_quality_at_last_main_frame_.transport_rtt()
+                    .InMilliseconds())
+          : 0;
+
+  const int32_t http_rtt_milliseconds =
+      estimated_quality_at_last_main_frame_.http_rtt() !=
+              nqe::internal::InvalidRTT()
+          ? FitInKBitsPerMetricBits(
+                estimated_quality_at_last_main_frame_.http_rtt()
+                    .InMilliseconds())
+          : 0;
+
+  const int32_t downstream_throughput_kbps =
+      estimated_quality_at_last_main_frame_.downstream_throughput_kbps() !=
+              nqe::internal::kInvalidThroughput
+          ? FitInKBitsPerMetricBits(estimated_quality_at_last_main_frame_
+                                        .downstream_throughput_kbps())
+          : 0;
+
+  const int32_t resource_load_time_milliseconds = FitInKBitsPerMetricBits(
+      (now - load_timing_info.send_start).InMilliseconds());
+
+  DCHECK_EQ(0, (transport_rtt_milliseconds | http_rtt_milliseconds |
+                downstream_throughput_kbps | resource_load_time_milliseconds) >>
+                   kBitsPerMetric);
+
+  // First 32 - (4* kBitsPerMetric) of the sample are unset. Next
+  // kBitsPerMetric of the sample contain |transport_rtt_milliseconds|.
+  // Next kBitsPerMetric contain |http_rtt_milliseconds|. Next kBitsPerMetric
+  // contain |downstream_throughput_kbps|. And, the last kBitsPerMetric
+  // contain |resource_load_time_milliseconds|.
+  int32_t sample = transport_rtt_milliseconds;
+  sample = (sample << kBitsPerMetric) | http_rtt_milliseconds;
+  sample = (sample << kBitsPerMetric) | downstream_throughput_kbps;
+  sample = (sample << kBitsPerMetric) | resource_load_time_milliseconds;
+
+  UMA_HISTOGRAM_SPARSE_SLOWLY("NQE.Correlation.ResourceLoadTime", sample);
 }
 
 void NetworkQualityEstimator::NotifyURLRequestDestroyed(