Populate EEP estimate in NQE

net/base/network_quality_estimator.cc

Index: net/base/network_quality_estimator.cc
diff --git a/net/base/network_quality_estimator.cc b/net/base/network_quality_estimator.cc
index 708a13c2bbf5cec591f882e6adf4d49ee9045d56..765ceb73ee3352553f0bdb0d82db49785cd5892b 100644
--- a/net/base/network_quality_estimator.cc
+++ b/net/base/network_quality_estimator.cc
@@ -178,7 +178,7 @@ NetworkQualityEstimator::NetworkQualityEstimator(
       downstream_throughput_kbps_observations_(
           GetWeightMultiplierPerSecond(variation_params)),
       rtt_msec_observations_(GetWeightMultiplierPerSecond(variation_params)),
-      external_estimates_provider_(external_estimates_provider.Pass()) {
+      external_estimate_provider_(external_estimates_provider.Pass()) {
   static_assert(kMinRequestDurationMicroseconds > 0,
                 "Minimum request duration must be > 0");
   static_assert(kDefaultHalfLifeSeconds > 0,
@@ -192,8 +192,15 @@ NetworkQualityEstimator::NetworkQualityEstimator(
 
   ObtainOperatingParams(variation_params);
   NetworkChangeNotifier::AddConnectionTypeObserver(this);
-  if (external_estimates_provider_)
-    external_estimates_provider_->SetUpdatedEstimateDelegate(this);
+  if (external_estimate_provider_) {
+    RecordExternalEstimateProviderMetrics(
+        EXTERNAL_ESTIMATE_PROVIDER_STATUS_AVAILABLE);
+    external_estimate_provider_->SetUpdatedEstimateDelegate(this);
+    QueryExternalEstimateProvider();
+  } else {
+    RecordExternalEstimateProviderMetrics(
+        EXTERNAL_ESTIMATE_PROVIDER_STATUS_NOT_AVAILABLE);
+  }
   current_network_id_ = GetCurrentNetworkID();
   AddDefaultEstimates();
 }
@@ -210,6 +217,9 @@ void NetworkQualityEstimator::ObtainOperatingParams(
   for (size_t i = 0; i <= NetworkChangeNotifier::CONNECTION_LAST; ++i) {
     NetworkChangeNotifier::ConnectionType type =
         static_cast<NetworkChangeNotifier::ConnectionType>(i);
+    DCHECK_EQ(InvalidRTT(), default_observations_[i].rtt());
+    DCHECK_EQ(kInvalidThroughput,
+              default_observations_[i].downstream_throughput_kbps());
     int32_t variations_value = kMinimumRTTVariationParameterMsec - 1;
     // Name of the parameter that holds the RTT value for this connection type.
     std::string rtt_parameter_name =
@@ -264,6 +274,7 @@ NetworkQualityEstimator::~NetworkQualityEstimator() {
 void NetworkQualityEstimator::NotifyHeadersReceived(const URLRequest& request) {
   DCHECK(thread_checker_.CalledOnValidThread());
 
+  MaybeQueryExternalEstimateProvider();
   if (!RequestProvidesUsefulObservations(request))
     return;
 
@@ -314,6 +325,7 @@ void NetworkQualityEstimator::NotifyRequestCompleted(
     const URLRequest& request) {
   DCHECK(thread_checker_.CalledOnValidThread());
 
+  MaybeQueryExternalEstimateProvider();
   if (!RequestProvidesUsefulObservations(request))
     return;
 
@@ -419,6 +431,12 @@ bool NetworkQualityEstimator::RequestProvidesUsefulObservations(
          request.creation_time() >= last_connection_change_;
 }
 
+void NetworkQualityEstimator::RecordExternalEstimateProviderMetrics(
+    NQEExternalEstimateProviderStatus status) const {
+  UMA_HISTOGRAM_ENUMERATION("NQE.ExternalEstimateProviderStatus", status,
+                            EXTERNAL_ESTIMATE_PROVIDER_STATUS_BOUNDARY);
+}
+
 void NetworkQualityEstimator::OnConnectionTypeChanged(
     NetworkChangeNotifier::ConnectionType type) {
   DCHECK(thread_checker_.CalledOnValidThread());
@@ -538,6 +556,8 @@ void NetworkQualityEstimator::OnConnectionTypeChanged(
   rtt_msec_observations_.Clear();
   current_network_id_ = GetCurrentNetworkID();
 
+  QueryExternalEstimateProvider();
+
   // Read any cached estimates for the new network. If cached estimates are
   // unavailable, add the default estimates.
   if (!ReadCachedNetworkQualityEstimate())
@@ -545,9 +565,10 @@ void NetworkQualityEstimator::OnConnectionTypeChanged(
   estimated_median_network_quality_ = NetworkQuality();
 }
 
-bool NetworkQualityEstimator::GetRTTEstimate(base::TimeDelta* rtt) const {
+bool NetworkQualityEstimator::GetRTTEstimate(base::TimeDelta* rtt) {
   DCHECK(thread_checker_.CalledOnValidThread());
   DCHECK(rtt);
+  MaybeQueryExternalEstimateProvider();
   if (rtt_msec_observations_.Size() == 0) {
     *rtt = InvalidRTT();
     return false;
@@ -556,10 +577,10 @@ bool NetworkQualityEstimator::GetRTTEstimate(base::TimeDelta* rtt) const {
   return (*rtt != InvalidRTT());
 }
 
-bool NetworkQualityEstimator::GetDownlinkThroughputKbpsEstimate(
-    int32_t* kbps) const {
+bool NetworkQualityEstimator::GetDownlinkThroughputKbpsEstimate(int32_t* kbps) {
   DCHECK(thread_checker_.CalledOnValidThread());
   DCHECK(kbps);
+  MaybeQueryExternalEstimateProvider();
   if (downstream_throughput_kbps_observations_.Size() == 0) {
     *kbps = kInvalidThroughput;
     return false;
@@ -570,18 +591,22 @@ bool NetworkQualityEstimator::GetDownlinkThroughputKbpsEstimate(
 
 bool NetworkQualityEstimator::GetRecentMedianRTT(
     const base::TimeTicks& begin_timestamp,
-    base::TimeDelta* rtt) const {
+    base::TimeDelta* rtt) {
   DCHECK(thread_checker_.CalledOnValidThread());
   DCHECK(rtt);
+
+  MaybeQueryExternalEstimateProvider();
   *rtt = GetRTTEstimateInternal(begin_timestamp, 50);
   return (*rtt != InvalidRTT());
 }
 
 bool NetworkQualityEstimator::GetRecentMedianDownlinkThroughputKbps(
     const base::TimeTicks& begin_timestamp,
-    int32_t* kbps) const {
+    int32_t* kbps) {
   DCHECK(thread_checker_.CalledOnValidThread());
   DCHECK(kbps);
+
+  MaybeQueryExternalEstimateProvider();
   *kbps = GetDownlinkThroughputKbpsEstimateInternal(begin_timestamp, 50);
   return (*kbps != kInvalidThroughput);
 }
@@ -812,8 +837,61 @@ bool NetworkQualityEstimator::ReadCachedNetworkQualityEstimate() {
 
 void NetworkQualityEstimator::OnUpdatedEstimateAvailable() {
   DCHECK(thread_checker_.CalledOnValidThread());
-  DCHECK(external_estimates_provider_);
-  // TODO(tbansal): Query provider for the recent value.
+  DCHECK(external_estimate_provider_);
+
+  RecordExternalEstimateProviderMetrics(
+      EXTERNAL_ESTIMATE_PROVIDER_STATUS_CALLBACK);
+  QueryExternalEstimateProvider();
+}
+
+void NetworkQualityEstimator::MaybeQueryExternalEstimateProvider() {
+  if (base::TimeTicks::Now() - external_estimate_request_time_ >=
+      base::TimeDelta::FromMilliseconds(
+          kExternalEstimateProviderQueryIntervalMsec)) {

[mmenke, 2015/09/14 20:07:06]

None of the tests check this logic.

[tbansal1, 2015/09/14 23:46:56]

Obsolete.

+    QueryExternalEstimateProvider();
+  }
+}
+
+void NetworkQualityEstimator::QueryExternalEstimateProvider() {
+  DCHECK(thread_checker_.CalledOnValidThread());
+
+  if (!external_estimate_provider_)
+    return;
+  RecordExternalEstimateProviderMetrics(
+      EXTERNAL_ESTIMATE_PROVIDER_STATUS_QUERIED);
+  external_estimate_request_time_ = base::TimeTicks::Now();
+
+  base::TimeDelta time_since_last_update;
+
+  // Request a new estimate if estimate is not available, or if the available
+  // estimate is not fresh.
+  if (!external_estimate_provider_->GetTimeSinceLastUpdate(
+          &time_since_last_update) ||
+      time_since_last_update >
+          base::TimeDelta::FromMilliseconds(
+              kExternalEstimateProviderFreshnessDurationMsec)) {
+    // Request the external estimate provider for updated estimates. When the
+    // updates estimates are available, OnUpdatedEstimateAvailable() will be
+    // called.
+    external_estimate_provider_->RequestUpdate();
+    return;
+  }
+
+  RecordExternalEstimateProviderMetrics(
+      EXTERNAL_ESTIMATE_PROVIDER_STATUS_QUERY_SUCCESSFUL);
+  base::TimeDelta rtt;
+  if (external_estimate_provider_->GetRTT(&rtt)) {
+    rtt_msec_observations_.AddObservation(Observation(
+        rtt.InMilliseconds(), base::TimeTicks::Now() - time_since_last_update));

[mmenke, 2015/09/14 20:07:06]

I still think just randomly adding these sources together like this is a
mistake.

Suppose Android's system estimate does the same thing you're doing...Suddenly
you're weighting previous RTT measurements significantly more (They affect the
current estimate, and the previous system estimate).

It also has weird effects in that the more consumers you have sampling the data,
the greater weight system measurements have, which seems a bit weird.

[tbansal1, 2015/09/14 23:46:56]

Modified the CL to add estimate from external provider only on network changes.
However, that estimate is still mixed with the other observations. So, there is
no periodic sampling of EEP anymore.

There is another CL that tags the Observation with the source from where the
observation came
(https://codereview.chromium.org/1273173002/diff/300001/net/base/network_quali...).
With that, it would be possible to distinguish the observations, if there is a
need to do that.

+  }
+
+  int32_t downstream_throughput_kbps;
+  if (external_estimate_provider_->GetDownstreamThroughputKbps(
+          &downstream_throughput_kbps)) {
+    downstream_throughput_kbps_observations_.AddObservation(
+        Observation(downstream_throughput_kbps,
+                    base::TimeTicks::Now() - time_since_last_update));

[mmenke, 2015/09/14 20:07:06]

I don't think any of the tests check that these observations are actually added,
or are combined correctly with estimates from other sources.

[tbansal1, 2015/09/14 23:46:56]

Added test to make sure that the observation size increases.

+  }
 }
 
 void NetworkQualityEstimator::CacheNetworkQualityEstimate() {