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..9f202159f8d9913bce72ee7c64b1c31a7ee3136d 100644
--- a/net/base/network_quality_estimator.cc
+++ b/net/base/network_quality_estimator.cc
@@ -178,7 +178,8 @@ 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()),
+      external_estimate_request_time_(base::TimeTicks()) {

[mmenke, 2015/09/11 14:26:29]

Don't need to explicitly call the default constructor for base::TimeTicks (Or
pretty much anything with a 0-argument constructor)

[tbansal1, 2015/09/11 21:24:30]

Done.

   static_assert(kMinRequestDurationMicroseconds > 0,
                 "Minimum request duration must be > 0");
   static_assert(kDefaultHalfLifeSeconds > 0,
@@ -192,8 +193,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();
 }
@@ -264,6 +272,7 @@ NetworkQualityEstimator::~NetworkQualityEstimator() {
 void NetworkQualityEstimator::NotifyHeadersReceived(const URLRequest& request) {
   DCHECK(thread_checker_.CalledOnValidThread());
 
+  MaybeQueryExternalEstimateProvider();
   if (!RequestProvidesUsefulObservations(request))
     return;
 
@@ -314,6 +323,7 @@ void NetworkQualityEstimator::NotifyRequestCompleted(
     const URLRequest& request) {
   DCHECK(thread_checker_.CalledOnValidThread());
 
+  MaybeQueryExternalEstimateProvider();
   if (!RequestProvidesUsefulObservations(request))
     return;
 
@@ -419,6 +429,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 +554,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 +563,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();

[mmenke, 2015/09/11 15:07:22]

Wait..So the frequency one calls this method can affect how many values we have
from ExternalEstimateProvider in our obvservation list?  That seems like
horrible behavior.  Or am I missing something?

[mmenke, 2015/09/11 15:17:52]

Ah, right...It's the queries vs maybe queries.

This still seems weird to me - treating the stuff from the EEP as normal
samples, and then just weighting them over time like normal samples.  I think it
would make more sense to get an estimate from observations, and an estimate from
the EEP, and combine them in some way instead.  Or alternatively, just prefer
the EEP's estimate when we have one.

[tbansal1, 2015/09/11 21:24:30]

Please see the reply below.

   if (rtt_msec_observations_.Size() == 0) {
     *rtt = InvalidRTT();
     return false;
@@ -556,10 +575,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 +589,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 +835,58 @@ bool NetworkQualityEstimator::ReadCachedNetworkQualityEstimate() {
 
 void NetworkQualityEstimator::OnUpdatedEstimateAvailable() {

[mmenke, 2015/09/11 15:07:22]

Why is this needed?  Seems like we pull it whenever we need it, anyways.

[tbansal1, 2015/09/11 21:24:31]

EEP that we are currently using on Android has async API. So, this is how it
goes with that EEP:
(1) native code calls requestUpdated() when it wants to request a fresh
estimate. This goes through JNI, calls the async GMS API, and forces it to fetch
fresh estimate.
(2) At some point in future (hopefully, within milliseconds), GMS API returns
the result.
(3) Then, the EEP Java plumbing on the Android notifies native that a new
estimate is available by calling OnUpdatedEstimateAvailable().
(4) Now, native knows that it can call the GetRTT() API, and get the fresh
result.

I can point you to the downstream CL, if needed.

I have document this life cycle partially here (Lines 870 where I call
requestUpdated()). However, a lot of this is implementation specific. So, it is
documented more properly in the downstream CL.
should care about this.

   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)) {
+    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;
+  if (!external_estimate_provider_->GetTimeSinceLastUpdate(
+          &time_since_last_update) ||
+      time_since_last_update >
+          base::TimeDelta::FromMilliseconds(
+              kExternalEstimateProviderQueryIntervalMsec)) {

[mmenke, 2015/09/11 14:26:29]

Why do both MaybeQueryExternalEstimateProvider and QueryExternalEstimateProvider
both check against kExternalEstimateProviderQueryIntervalMsec?  Yes, they use
slightly different base times, but the redundancy seems weird and unless
external_estimate_provider_->GetTimeSinceLastUpdate has a lot of overhead,
doesn't seem to get us anything.

[tbansal1, 2015/09/11 21:24:31]

One is checking when was the last time native requested EEP for updated. This
reduces the number of JNI calls to once every 
|kExternalEstimateProviderQueryIntervalMsec| milliseconds.

Second checks if the estimate provided by EEP is fresh. If not, it forces EEP to
fetch new estimate.

Now, using two different variable names to make it clearer.

+    // Request the external estimate provider for updated estimates. When the
+    // updates estimates are available, OnUpdatedEstimateAvailable() will get
+    // 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/11 14:26:29]

So we just toss in the EEP's estimates in a bucket with everything else?

[tbansal1, 2015/09/11 21:24:31]

Long-term goal is to look at other useful events: (i) Significant change in the
WiFi or cellular signal strength, (ii) Device moves from one cell tower to
another. In these cases, it is preferable to lower down the weight of Chromium
estimates by a factor, and fetch fresh EEP estimate and give it higher weight
(because EEP estimates may be better).

Combining NQE and EEP estimates in some way is also worth exploring, but I
suspect that the best way to combine would depend on network and/or user
behavior (e.g., how many pages they load). So, exploring this is low-priority.

For now, we do not have ways to detect these events. So, calling EEP on (i)
network change events (ii) Regular intervals. I do not feel strongly about (ii).
I don't have data to back it up, and can remove it for now. This would roughly
correspond to: use EEP only if NQE is unavailable. Later, when we have knowledge
of signal strength/cell tower events, then may be we can call EEP more
frequently.

+  }
+
+  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));
+  }
 }
 
 void NetworkQualityEstimator::CacheNetworkQualityEstimate() {