Populate EEP estimate in NQE

net/base/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/base/network_quality_estimator.h"
 
 #include <float.h>
 #include <algorithm>
 #include <cmath>
 #include <limits>
@@ skipping 160 matching lines @@
     bool allow_smaller_responses_for_tests)
     : allow_localhost_requests_(allow_local_host_requests_for_tests),
       allow_small_responses_(allow_smaller_responses_for_tests),
       last_connection_change_(base::TimeTicks::Now()),
       current_network_id_(
           NetworkID(NetworkChangeNotifier::ConnectionType::CONNECTION_UNKNOWN,
                     std::string())),
       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,
                 "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");
 
   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();
 }
 
 // static
 const base::TimeDelta NetworkQualityEstimator::InvalidRTT() {
   return base::TimeDelta::Max();
 }
 
 void NetworkQualityEstimator::ObtainOperatingParams(
@@ skipping 50 matching lines @@
 }
 
 NetworkQualityEstimator::~NetworkQualityEstimator() {
   DCHECK(thread_checker_.CalledOnValidThread());
   NetworkChangeNotifier::RemoveConnectionTypeObserver(this);
 }
 
 void NetworkQualityEstimator::NotifyHeadersReceived(const URLRequest& request) {
   DCHECK(thread_checker_.CalledOnValidThread());
 
+  MaybeQueryExternalEstimateProvider();
   if (!RequestProvidesUsefulObservations(request))
     return;
 
   // Update |estimated_median_network_quality_| if this is a main frame request.
   if (request.load_flags() & LOAD_MAIN_FRAME) {
     estimated_median_network_quality_ = NetworkQuality(
         GetRTTEstimateInternal(base::TimeTicks(), 50),
         GetDownlinkThroughputKbpsEstimateInternal(base::TimeTicks(), 50));
   }
 
@@ skipping 30 matching lines @@
     if (estimated_median_network_quality_.rtt() != InvalidRTT()) {
       RecordRTTUMA(estimated_median_network_quality_.rtt().InMilliseconds(),
                    observed_rtt.InMilliseconds());
     }
 }
 
 void NetworkQualityEstimator::NotifyRequestCompleted(
     const URLRequest& request) {
   DCHECK(thread_checker_.CalledOnValidThread());
 
+  MaybeQueryExternalEstimateProvider();
   if (!RequestProvidesUsefulObservations(request))
     return;
 
   base::TimeTicks now = base::TimeTicks::Now();
   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() ||
@@ skipping 85 matching lines @@
   return request.url().is_valid() &&
          (allow_localhost_requests_ || !IsLocalhost(request.url().host())) &&
          request.url().SchemeIsHTTPOrHTTPS() &&
          // Verify that response headers are received, so it can be ensured that
          // response is not cached.
          !request.response_info().response_time.is_null() &&
          !request.was_cached() &&
          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());
   if (peak_network_quality_.rtt() != InvalidRTT()) {
     switch (current_network_id_.type) {
       case NetworkChangeNotifier::CONNECTION_UNKNOWN:
         UMA_HISTOGRAM_TIMES("NQE.FastestRTT.Unknown",
                             peak_network_quality_.rtt());
         break;
       case NetworkChangeNotifier::CONNECTION_ETHERNET:
@@ skipping 99 matching lines @@
   // Write the estimates of the previous network to the cache.
   CacheNetworkQualityEstimate();
 
   // Clear the local state.
   last_connection_change_ = base::TimeTicks::Now();
   peak_network_quality_ = NetworkQuality();
   downstream_throughput_kbps_observations_.Clear();
   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())
     AddDefaultEstimates();
   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;
   }
   *rtt = GetRTTEstimateInternal(base::TimeTicks(), 50);
   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;
   }
   *kbps = GetDownlinkThroughputKbpsEstimateInternal(base::TimeTicks(), 50);
   return (*kbps != kInvalidThroughput);
 }
 
 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);
 }
 
 NetworkQualityEstimator::Observation::Observation(int32_t value,
                                                   base::TimeTicks timestamp)
     : value(value), timestamp(timestamp) {
   DCHECK_GE(value, 0);
   DCHECK(!timestamp.is_null());
 }
@@ skipping 208 matching lines @@
   DCHECK_NE(InvalidRTT(), network_quality.rtt());
   DCHECK_NE(kInvalidThroughput, network_quality.downstream_throughput_kbps());
 
   downstream_throughput_kbps_observations_.AddObservation(Observation(
       network_quality.downstream_throughput_kbps(), base::TimeTicks::Now()));
   rtt_msec_observations_.AddObservation(Observation(
       network_quality.rtt().InMilliseconds(), base::TimeTicks::Now()));
   return true;
 }
 
 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() {
   DCHECK(thread_checker_.CalledOnValidThread());
   DCHECK_LE(cached_network_qualities_.size(),
             static_cast<size_t>(kMaximumNetworkQualityCacheSize));
 
   // If the network name is unavailable, caching should not be performed.
   if (current_network_id_.id.empty())
     return;
@@ skipping 82 matching lines @@
 
 NetworkQualityEstimator::NetworkQuality&
     NetworkQualityEstimator::NetworkQuality::
     operator=(const NetworkQuality& other) {
   rtt_ = other.rtt_;
   downstream_throughput_kbps_ = other.downstream_throughput_kbps_;
   return *this;
 }
 
 }  // namespace net