NQE: Record correlation metric in UMA

net/nqe/network_quality_estimator.cc

 // Copyright 2015 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 "net/nqe/network_quality_estimator.h"
 
 #include <algorithm>
 #include <cmath>
 #include <limits>
 #include <utility>
 #include <vector>
 
 #include "base/bind_helpers.h"
 #include "base/location.h"
 #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"
 #include "base/time/default_tick_clock.h"
 #include "base/trace_event/trace_event.h"
 #include "build/build_config.h"
 #include "net/base/load_flags.h"
 #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)
 #include "net/android/network_library.h"
 #endif  // OS_ANDROID
 
 namespace {
 
 // Default value of the half life (in seconds) for computing time weighted
 // percentiles. Every half life, the weight of all observations reduces by
@@ skipping 93 matching lines @@
 
 bool GetValueForVariationParam(
     const std::map<std::string, std::string>& variation_params,
     const std::string& parameter_name,
     int32_t* variations_value) {
   const auto it = variation_params.find(parameter_name);
   return it != variation_params.end() &&
          base::StringToInt(it->second, variations_value);
 }
 
+// Returns the variation value for |parameter_name|. If the value is
+// unavailable, |default_value| is returned.
+double GetDoubleValueForVariationParamWithDefaultValue(
+    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.
 std::string GetEffectiveConnectionTypeAlgorithm(
     const std::map<std::string, std::string>& variation_params) {
   const auto it = variation_params.find("effective_connection_type_algorithm");
   if (it == variation_params.end())
     return std::string();
   return it->second;
 }
@@ skipping 27 matching lines @@
   static const char* const kSuffixes[] = {
       "0_20",     "20_60",     "60_140",    "140_300",      "300_620",
       "620_1260", "1260_2540", "2540_5100", "5100_Infinity"};
   for (size_t i = 0; i < arraysize(kSuffixes) - 1; ++i) {
     if (rtt_milliseconds <= static_cast<float>((20 * (2 << i) - 20)))
       return kSuffixes[i];
   }
   return kSuffixes[arraysize(kSuffixes) - 1];
 }
 
+// The least significant kTrimBits of the metric will be discarded. Next, it
+// will be rounded down so it fits within kBitsPerMetric bits. This implies
+// that if the metric's value is more than 2^(kTrimBits + kBitsPerMetric),
+// then it will be rounded down.

[Not at Google. Contact bengr, 2016/06/30 17:49:44]

This is confusing. Can you please rephrase it. How about: If trimmed 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.

[tbansal1, 2016/07/01 00:16:00]

Done.

+const int32_t kTrimBits = 5;
+const int32_t kBitsPerMetric = 7;

[Not at Google. Contact bengr, 2016/06/30 17:49:44]

Add:
static_assert(31 > kBitsPerMetric * 4, "4 metrics are being added to 32-bit int
with first bit reserved for sign")

[tbansal1, 2016/07/01 00:16:00]

Done.

+
+// Trims the |metric| by removing the last kTrimBits, and then rounding down
+// the |metric| such that the |metric| fits in kBitsPerMetric.
+int32_t TrimAndRoundDown(int32_t metric) {

[Not at Google. Contact bengr, 2016/06/30 17:49:44]

Rename method to FitInKBitsPerMetricBits?

[tbansal1, 2016/07/01 00:16:00]

Done.

+  // Remove the last kTrimBits. This will allow the metric to fit within
+  // kBitsPerMetric while losing only the least significant bits.
+  metric = metric >> kTrimBits;
+  if (metric >= (1 << kBitsPerMetric)) {
+    // Fit |metric| in kBitsPerMetric by rounding it down.
+    metric = (1 << kBitsPerMetric) - 1;

[Not at Google. Contact bengr, 2016/06/30 17:49:44]

Possible Refactor:
int32_t largest_value_using_kBitsPerMetric_bits =
    (1 << kBitsPerMetric) - 1"

Then use this variable in the if check above as well. 

[tbansal1, 2016/07/01 00:16:01]

Done.

+  }
+  DCHECK_EQ(0, metric >> kBitsPerMetric);
+  return metric;
+}
+
 }  // namespace
 
 namespace net {
 
 NetworkQualityEstimator::NetworkQualityEstimator(
     std::unique_ptr<ExternalEstimateProvider> external_estimates_provider,
     const std::map<std::string, std::string>& variation_params)
     : NetworkQualityEstimator(std::move(external_estimates_provider),
                               variation_params,
                               false,
@@ skipping 22 matching lines @@
       effective_connection_type_recomputation_interval_(
           base::TimeDelta::FromSeconds(15)),
       last_connection_change_(tick_clock_->NowTicks()),
       current_network_id_(
           NetworkID(NetworkChangeNotifier::ConnectionType::CONNECTION_UNKNOWN,
                     std::string())),
       downstream_throughput_kbps_observations_(weight_multiplier_per_second_),
       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");
   static_assert(kMaximumNetworkQualityCacheSize > 0,
                 "Size of the network quality cache must be > 0");
   // This limit should not be increased unless the logic for removing the
   // oldest cache entry is rewritten to use a doubly-linked-list LRU queue.
   static_assert(kMaximumNetworkQualityCacheSize <= 10,
                 "Size of the network quality cache must <= 10");
   // None of the algorithms can have an empty name.
   DCHECK(algorithm_name_to_enum_.end() ==
          algorithm_name_to_enum_.find(std::string()));
 
   DCHECK_EQ(algorithm_name_to_enum_.size(),
             static_cast<size_t>(EffectiveConnectionTypeAlgorithm::
                                     EFFECTIVE_CONNECTION_TYPE_ALGORITHM_LAST));
   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);
   NetworkChangeNotifier::AddConnectionTypeObserver(this);
   if (external_estimate_provider_) {
     RecordExternalEstimateProviderMetrics(
         EXTERNAL_ESTIMATE_PROVIDER_STATUS_AVAILABLE);
     external_estimate_provider_->SetUpdatedEstimateDelegate(this);
   } else {
     RecordExternalEstimateProviderMetrics(
@@ skipping 221 matching lines @@
 
   LoadTimingInfo load_timing_info;
   request.GetLoadTimingInfo(&load_timing_info);
 
   // If the load timing info is unavailable, it probably means that the request
   // did not go over the network.
   if (load_timing_info.send_start.is_null() ||
       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.
   base::TimeDelta observed_http_rtt =
       load_timing_info.receive_headers_end - load_timing_info.send_start;
   DCHECK_GE(observed_http_rtt, base::TimeDelta());
   if (observed_http_rtt < peak_network_quality_.http_rtt()) {
     peak_network_quality_ = nqe::internal::NetworkQuality(
         observed_http_rtt, peak_network_quality_.transport_rtt(),
         peak_network_quality_.downstream_throughput_kbps());
@@ skipping 74 matching lines @@
 void NetworkQualityEstimator::NotifyRequestCompleted(
     const URLRequest& request) {
   TRACE_EVENT0(TRACE_DISABLED_BY_DEFAULT("net"),
                "NetworkQualityEstimator::NotifyRequestCompleted");
   DCHECK(thread_checker_.CalledOnValidThread());
 
   if (!RequestSchemeIsHTTPOrHTTPS(request))
     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.
+  if (base::RandDouble() >= correlation_logging_probability_)

[Not at Google. Contact bengr, 2016/06/30 17:49:44]

Shouldn't this be base::RandDouble() <= correlation_logging_probability_?

[tbansal1, 2016/07/01 00:16:00]

No. Say correlation_logging_probability_ is 1.0. Then, we never want to return.
So, we always want the if conditional to be false. Since RandDouble returns
double between [0.0, 1.0), we want to check if RandDouble <
correlation_logging_probability_

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

Misread as condition to execute below. You are right.

+    return;
+
+  if (request.response_info().was_cached)
+    return;
+  LoadTimingInfo load_timing_info;
+  request.GetLoadTimingInfo(&load_timing_info);
+
+  // If the load timing info is unavailable, it probably means that the request
+  // did not go over the network.

[Not at Google. Contact bengr, 2016/06/30 17:49:44]

Could this be replaced with request->response_info().network_accessed?

[tbansal1, 2016/07/01 00:16:01]

Done.

+  if (load_timing_info.send_start.is_null() ||
+      load_timing_info.receive_headers_end.is_null()) {
+    return;
+  }
+
+  // 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;

[Not at Google. Contact bengr, 2016/06/30 17:49:44]

Nit: Add new line.

[tbansal1, 2016/07/01 00:16:00]

Done.

+  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))

[Not at Google. Contact bengr, 2016/06/30 17:49:44]

Why ignore older requests?

[tbansal1, 2016/07/01 00:16:00]

It is possible that network quality was recorded at t=0. Then, there is some
background traffic which keeps going on for long-long time (say after 1-2
hours). There is no point of recording correlation for that traffic because
network quality was not recently checked.

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

Acknowledged.

+    return;
+
+  DCHECK_GE(now, load_timing_info.send_start);

[Not at Google. Contact bengr, 2016/06/30 17:49:44]

Not sure this is necessary.

[tbansal1, 2016/07/01 00:16:01]

It checks that we are not adding negative values to the histogram.

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

Acknowledged.

+
+  // Histogram samples are 32-bit in size with first bit reserved for sign.
+  // Four metrics will be recorded per sample.
+  DCHECK_GE(31, kBitsPerMetric * 4);

[Not at Google. Contact bengr, 2016/06/30 17:49:44]

Prefer replacing with static assert above where kBitsPerMetric is defined.

[tbansal1, 2016/07/01 00:16:01]

Done.

+
+  const int32_t transport_rtt_milliseconds =

[Not at Google. Contact bengr, 2016/06/30 17:49:44]

Does it help to extract estimated_quality_at_last_main_frame_.transport_rtt() to
a temp variable? Say:
"transport_rtt = estimated_quality_at_last_main_frame_.transport_rtt();" ?

[tbansal1, 2016/07/01 00:16:00]

I am not sure. I wanted to declare it as a const variable. But, it may have
reduced the readability slightly.

+      estimated_quality_at_last_main_frame_.transport_rtt() !=
+              nqe::internal::InvalidRTT()
+          ? TrimAndRoundDown(
+                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()
+          ? TrimAndRoundDown(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
+          ? TrimAndRoundDown(estimated_quality_at_last_main_frame_
+                                 .downstream_throughput_kbps())
+          : 0;
+
+  const int32_t resource_load_time_milliseconds =
+      TrimAndRoundDown((now - load_timing_info.send_start).InMilliseconds());
+
+  // 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;

[Not at Google. Contact bengr, 2016/06/30 17:49:44]

Suggest adding DCHECK_EQ(0, transport_rtt_milliseconds | http_rtt_milliseconds |
.. >> kBitsPerMetric).

[tbansal1, 2016/07/01 00:16:00]

Done.

+  sample = (sample << kBitsPerMetric) + http_rtt_milliseconds;

[Not at Google. Contact bengr, 2016/06/30 17:49:44]

Minor: | seems prefereable to +.

[tbansal1, 2016/07/01 00:16:00]

Done.

+  sample = (sample << kBitsPerMetric) + downstream_throughput_kbps;
+  sample = (sample << kBitsPerMetric) + resource_load_time_milliseconds;
+
+  UMA_HISTOGRAM_SPARSE_SLOWLY("NQE.Correlation.ResourceLoadTime", sample);
 }
 
 void NetworkQualityEstimator::NotifyURLRequestDestroyed(
     const URLRequest& request) {
   DCHECK(thread_checker_.CalledOnValidThread());
 
   NotifyRequestCompleted(request);
 }
 
 void NetworkQualityEstimator::AddRTTObserver(RTTObserver* rtt_observer) {
@@ skipping 692 matching lines @@
     NotifyObserversOfEffectiveConnectionTypeChanged() {
   DCHECK(thread_checker_.CalledOnValidThread());
 
   // TODO(tbansal): Add hysteresis in the notification.
   FOR_EACH_OBSERVER(
       EffectiveConnectionTypeObserver, effective_connection_type_observer_list_,
       OnEffectiveConnectionTypeChanged(effective_connection_type_));
 }
 
 }  // namespace net