Record Lo-Fi NQE prediction accuracy at different intervals

components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc

Index: components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc
diff --git a/components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc b/components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc
index ee6589a20b0f0da63f9664ba6e3401f3ad0c299a..97deaa919f9671ac3c6d3a31322614c12e157ba3 100644
--- a/components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc
+++ b/components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc
@@ -6,15 +6,17 @@
 
 #include <stddef.h>
 #include <utility>
-#include <vector>
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/macros.h"
 #include "base/metrics/field_trial.h"
+#include "base/metrics/histogram_base.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/metrics/sparse_histogram.h"
+#include "base/single_thread_task_runner.h"
 #include "base/strings/string_number_conversions.h"
+#include "base/time/default_tick_clock.h"
 #include "components/data_reduction_proxy/core/browser/data_reduction_proxy_configurator.h"
 #include "components/data_reduction_proxy/core/common/data_reduction_proxy_config_values.h"
 #include "components/data_reduction_proxy/core/common/data_reduction_proxy_event_creator.h"
@@ -78,6 +80,48 @@ bool FindProxyInList(const std::vector<net::ProxyServer>& proxy_list,
   return false;
 }
 
+// Returns a descriptive name corresponding to |connection_type|.
+const char* GetNameForConnectionType(
+    net::NetworkChangeNotifier::ConnectionType connection_type) {
+  switch (connection_type) {
+    case net::NetworkChangeNotifier::CONNECTION_UNKNOWN:
+      return "Unknown";
+    case net::NetworkChangeNotifier::CONNECTION_ETHERNET:
+      return "Ethernet";
+    case net::NetworkChangeNotifier::CONNECTION_WIFI:
+      return "WiFi";
+    case net::NetworkChangeNotifier::CONNECTION_2G:
+      return "2G";
+    case net::NetworkChangeNotifier::CONNECTION_3G:
+      return "3G";
+    case net::NetworkChangeNotifier::CONNECTION_4G:
+      return "4G";
+    case net::NetworkChangeNotifier::CONNECTION_NONE:
+      return "None";
+    case net::NetworkChangeNotifier::CONNECTION_BLUETOOTH:
+      return "Bluetooth";
+    default:

[Alexei Svitkine (slow), 2016/04/18 20:35:09]

Nit: Omit the default cases and handle all the values in the enum. This way, if
someone adds a new value they'll get a warning and will have to update the code.

(In practice, change the default: to "case CONNECTION_LAST:" to handle all the
cases.)

Move the NOTREACHED() to the end of the function below the return "";

[tbansal1, 2016/04/18 22:37:35]

Done.

+      NOTREACHED();
+      break;
+  }
+  return "";
+}
+
+// Returns an enumerated histogram that should be used to record the given
+// statistic. |max_limit| is the maximum value that can be stored in the
+// histogram. Number of buckets in the enumerated histogram are one more than
+// |max_limit|.
+base::HistogramBase* GetEnumeratedHistogram(
+    const std::string& prefix,
+    net::NetworkChangeNotifier::ConnectionType type,
+    int32_t max_limit) {
+  DCHECK_GT(max_limit, 0);
+
+  return base::Histogram::FactoryGet(

[megjablon, 2016/04/18 18:58:49]

#include "base/metrics/histogram.h"

[tbansal1, 2016/04/18 22:37:35]

Done.

+      prefix + GetNameForConnectionType(type), 0, max_limit, max_limit + 1,
+      base::HistogramBase::kUmaTargetedHistogramFlag);
+}
+
 // Following UMA is plotted to measure how frequently Lo-Fi state changes.
 // Too frequent changes are undesirable.
 void RecordAutoLoFiRequestHeaderStateChange(bool previous_header_low,
@@ -227,6 +271,7 @@ class SecureProxyChecker : public net::URLFetcherDelegate {
 };
 
 DataReductionProxyConfig::DataReductionProxyConfig(
+    scoped_refptr<base::SingleThreadTaskRunner> io_task_runner,
     net::NetLog* net_log,
     std::unique_ptr<DataReductionProxyConfigValues> config_values,
     DataReductionProxyConfigurator* configurator,
@@ -235,21 +280,25 @@ DataReductionProxyConfig::DataReductionProxyConfig(
       unreachable_(false),
       enabled_by_user_(false),
       config_values_(std::move(config_values)),
+      io_task_runner_(io_task_runner),
       net_log_(net_log),
       configurator_(configurator),
       event_creator_(event_creator),
       auto_lofi_minimum_rtt_(base::TimeDelta::Max()),
       auto_lofi_maximum_kbps_(0),
       auto_lofi_hysteresis_(base::TimeDelta::Max()),
-      network_quality_last_checked_(base::TimeTicks()),
       network_prohibitively_slow_(false),
       connection_type_(net::NetworkChangeNotifier::GetConnectionType()),
       lofi_off_(false),
-      last_query_(base::TimeTicks::Now()),
       network_quality_at_last_query_(NETWORK_QUALITY_AT_LAST_QUERY_UNKNOWN),
-      previous_state_lofi_on_(false) {
+      previous_state_lofi_on_(false),
+      tick_clock_(new base::DefaultTickClock()),
+      weak_factory_(this) {
+  DCHECK(io_task_runner_);
   DCHECK(configurator);
   DCHECK(event_creator);
+  DCHECK(GetTickClock());
+
   if (params::IsLoFiDisabledViaFlags())
     SetLoFiModeOff();
   // Constructed on the UI thread, but should be checked on the IO thread.
@@ -271,6 +320,16 @@ void DataReductionProxyConfig::InitializeOnIOThread(const scoped_refptr<
   PopulateAutoLoFiParams();
   AddDefaultProxyBypassRules();
   net::NetworkChangeNotifier::AddIPAddressObserver(this);
+
+  // Record accuracy at 3 different intervals. The values used here must remain
+  // in sync with the suffixes specified in
+  // tools/metrics/histograms/histograms.xml.
+  lofi_accuracy_recording_intervals_.push_back(
+      base::TimeDelta::FromSeconds(15));
+  lofi_accuracy_recording_intervals_.push_back(
+      base::TimeDelta::FromSeconds(30));
+  lofi_accuracy_recording_intervals_.push_back(
+      base::TimeDelta::FromSeconds(60));
 }
 
 void DataReductionProxyConfig::ReloadConfig() {
@@ -419,17 +478,33 @@ bool DataReductionProxyConfig::IsNetworkQualityProhibitivelySlow(
     network_quality_at_last_query_ =
         is_network_currently_slow ? NETWORK_QUALITY_AT_LAST_QUERY_SLOW
                                   : NETWORK_QUALITY_AT_LAST_QUERY_NOT_SLOW;
+
+    if ((params::IsIncludedInLoFiEnabledFieldTrial() ||
+         params::IsIncludedInLoFiControlFieldTrial()) &&
+        !params::IsLoFiSlowConnectionsOnlyViaFlags()) {
+      // Post tasks to record accuracy of network quality prediction at
+      // different intervals.
+      for (const base::TimeDelta& measuring_delay :
+           lofi_accuracy_recording_intervals()) {
+        io_task_runner_->PostDelayedTask(
+            FROM_HERE,
+            base::Bind(&DataReductionProxyConfig::RecordAutoLoFiAccuracyRate,
+                       weak_factory_.GetWeakPtr(), network_quality_estimator,
+                       measuring_delay),
+            measuring_delay);
+      }
+    }
   }
 
   // Return the cached entry if the last update was within the hysteresis
   // duration and if the connection type has not changed.
   if (!network_type_changed && !network_quality_last_checked_.is_null() &&
-      base::TimeTicks::Now() - network_quality_last_checked_ <=
+      GetTickClock()->NowTicks() - network_quality_last_checked_ <=
           auto_lofi_hysteresis_) {
     return network_prohibitively_slow_;
   }
 
-  network_quality_last_checked_ = base::TimeTicks::Now();
+  network_quality_last_checked_ = GetTickClock()->NowTicks();
 
   if (!is_network_quality_available)
     return false;
@@ -494,7 +569,7 @@ bool DataReductionProxyConfig::IsProxyBypassed(
       retry_map.find(proxy_server.ToURI());
 
   if (found == retry_map.end() ||
-      found->second.bad_until < base::TimeTicks::Now()) {
+      found->second.bad_until < GetTickClock()->NowTicks()) {
     return false;
   }
 
@@ -622,6 +697,10 @@ void DataReductionProxyConfig::OnIPAddressChanged() {
     DCHECK(config_values_->allowed());
     RecordNetworkChangeEvent(IP_CHANGED);
 
+    // Reset |network_quality_at_last_query_| to prevent recording of network
+    // quality prediction accuracy if there was a change in the IP address.
+    network_quality_at_last_query_ = NETWORK_QUALITY_AT_LAST_QUERY_UNKNOWN;
+
     bool should_use_secure_proxy = params::ShouldUseSecureProxyByDefault();
     if (!should_use_secure_proxy && secure_proxy_allowed_) {
       secure_proxy_allowed_ = false;
@@ -691,12 +770,28 @@ void DataReductionProxyConfig::SetLoFiModeOff() {
 }
 
 void DataReductionProxyConfig::RecordAutoLoFiAccuracyRate(
-    const net::NetworkQualityEstimator* network_quality_estimator) const {
+    const net::NetworkQualityEstimator* network_quality_estimator,
+    const base::TimeDelta& measuring_duration) const {
   DCHECK(thread_checker_.CalledOnValidThread());
   DCHECK(network_quality_estimator);
-  DCHECK(params::IsIncludedInLoFiEnabledFieldTrial());
-  DCHECK_NE(network_quality_at_last_query_,
-            NETWORK_QUALITY_AT_LAST_QUERY_UNKNOWN);
+  DCHECK((params::IsIncludedInLoFiEnabledFieldTrial() ||
+          params::IsIncludedInLoFiControlFieldTrial()) &&
+         !params::IsLoFiSlowConnectionsOnlyViaFlags());
+
+  if (network_quality_at_last_query_ == NETWORK_QUALITY_AT_LAST_QUERY_UNKNOWN)
+    return;
+
+  const base::TimeTicks now = GetTickClock()->NowTicks();
+
+  // Return if the time since |last_query_| is less than |measuring_duration|.
+  // This may happen if another main frame request started during last
+  // |measuring_duration|.
+  if (now - last_query_ < measuring_duration)
+    return;
+
+  // Return if the time since |last_query_| is off by a factor of 2.
+  if (now - last_query_ > 2 * measuring_duration)
+    return;
 
   base::TimeDelta rtt_since_last_page_load;
   if (!network_quality_estimator->GetRecentURLRequestRTTMedian(
@@ -746,48 +841,20 @@ void DataReductionProxyConfig::RecordAutoLoFiAccuracyRate(
     }
   }
 
-  switch (connection_type_) {
-    case net::NetworkChangeNotifier::CONNECTION_UNKNOWN:
-      UMA_HISTOGRAM_ENUMERATION("DataReductionProxy.AutoLoFiAccuracy.Unknown",
-                                accuracy, AUTO_LOFI_ACCURACY_INDEX_BOUNDARY);
-      break;
-    case net::NetworkChangeNotifier::CONNECTION_ETHERNET:
-      UMA_HISTOGRAM_ENUMERATION("DataReductionProxy.AutoLoFiAccuracy.Ethernet",
-                                accuracy, AUTO_LOFI_ACCURACY_INDEX_BOUNDARY);
-      break;
-    case net::NetworkChangeNotifier::CONNECTION_WIFI:
-      UMA_HISTOGRAM_ENUMERATION("DataReductionProxy.AutoLoFiAccuracy.WiFi",
-                                accuracy, AUTO_LOFI_ACCURACY_INDEX_BOUNDARY);
-      break;
-    case net::NetworkChangeNotifier::CONNECTION_2G:
-      UMA_HISTOGRAM_ENUMERATION("DataReductionProxy.AutoLoFiAccuracy.2G",
-                                accuracy, AUTO_LOFI_ACCURACY_INDEX_BOUNDARY);
-      break;
-    case net::NetworkChangeNotifier::CONNECTION_3G:
-      UMA_HISTOGRAM_ENUMERATION("DataReductionProxy.AutoLoFiAccuracy.3G",
-                                accuracy, AUTO_LOFI_ACCURACY_INDEX_BOUNDARY);
-      break;
-    case net::NetworkChangeNotifier::CONNECTION_4G:
-      UMA_HISTOGRAM_ENUMERATION("DataReductionProxy.AutoLoFiAccuracy.4G",
-                                accuracy, AUTO_LOFI_ACCURACY_INDEX_BOUNDARY);
-      break;
-    case net::NetworkChangeNotifier::CONNECTION_NONE:
-      UMA_HISTOGRAM_ENUMERATION("DataReductionProxy.AutoLoFiAccuracy.None",
-                                accuracy, AUTO_LOFI_ACCURACY_INDEX_BOUNDARY);
-      break;
-    case net::NetworkChangeNotifier::CONNECTION_BLUETOOTH:
-      UMA_HISTOGRAM_ENUMERATION("DataReductionProxy.AutoLoFiAccuracy.Bluetooth",
-                                accuracy, AUTO_LOFI_ACCURACY_INDEX_BOUNDARY);
-      break;
-    default:
-      NOTREACHED();
-      break;
-  }
+  const char prefix[] = "DataReductionProxy.LoFi.Accuracy.";

[Alexei Svitkine (slow), 2016/04/18 20:35:09]

Nit: For cont strings defined inside function, add "static". This avoids a bunch
of extra generate binary code by the compiler (there was a thread about this on
Chromium dev a couple years back).

[tbansal1, 2016/04/18 22:37:35]

Done.

+  base::HistogramBase* accuracy_histogram = GetEnumeratedHistogram(
+      prefix + base::IntToString(measuring_duration.InSeconds()) + ".",

[Alexei Svitkine (slow), 2016/04/18 20:35:09]

Can you add a DCHECK somewhere that measuring_duration.InSeconds() is one of the
three supported values? (15/30/60)

[tbansal1, 2016/04/18 22:37:35]

Done.

+      connection_type_, AUTO_LOFI_ACCURACY_INDEX_BOUNDARY - 1);
+
+  accuracy_histogram->Add(accuracy);
 }
 
 bool DataReductionProxyConfig::ShouldEnableLoFiMode(
     const net::URLRequest& request) {
   DCHECK(thread_checker_.CalledOnValidThread());
+  DCHECK((request.load_flags() & net::LOAD_MAIN_FRAME) != 0);
+  DCHECK(!request.url().SchemeIsCryptographic());
+
   net::NetworkQualityEstimator* network_quality_estimator;
   network_quality_estimator =
       request.context() ? request.context()->network_quality_estimator()
@@ -814,16 +881,7 @@ bool DataReductionProxyConfig::ShouldEnableLoFiModeInternal(
     const net::NetworkQualityEstimator* network_quality_estimator) {
   DCHECK(thread_checker_.CalledOnValidThread());
 
-  // Record Lo-Fi accuracy rate only if the session is Lo-Fi enabled
-  // field trial, and the user has not enabled Lo-Fi on slow connections
-  // via flags.
-  if (network_quality_estimator &&
-      network_quality_at_last_query_ != NETWORK_QUALITY_AT_LAST_QUERY_UNKNOWN &&
-      params::IsIncludedInLoFiEnabledFieldTrial() &&
-      !params::IsLoFiSlowConnectionsOnlyViaFlags()) {
-    RecordAutoLoFiAccuracyRate(network_quality_estimator);
-  }
-  last_query_ = base::TimeTicks::Now();
+  last_query_ = GetTickClock()->NowTicks();
   network_quality_at_last_query_ = NETWORK_QUALITY_AT_LAST_QUERY_UNKNOWN;
 
   // If Lo-Fi has been turned off, its status can't change.
@@ -856,4 +914,13 @@ void DataReductionProxyConfig::GetNetworkList(
   net::GetNetworkList(interfaces, policy);
 }
 
+const std::vector<base::TimeDelta>&
+DataReductionProxyConfig::lofi_accuracy_recording_intervals() const {
+  return lofi_accuracy_recording_intervals_;
+}
+
+base::TickClock* DataReductionProxyConfig::GetTickClock() const {
+  return tick_clock_.get();
+}
+
 }  // namespace data_reduction_proxy