NQE: Adjust HTTP RTT based on RTT counts

This CL adjusts the logic in NQE to not return low HTTP RTT values
when the underlying RTT value is unreliable. This primarily happens on
Desktop non-POSIX platforms where Chrome does not have access to
RTT at TCP layer, and there are lot of hanging requests which
affect the estimation accuracy.

Change-Id: Ib31dd31bfc616289e7236be156e33491057da91a
Bug: 1101529
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2445029Reviewed-by: Ryan Sturm <ryansturm@chromium.org>
Commit-Queue: Tarun Bansal <tbansal@chromium.org>
Cr-Commit-Position: refs/heads/master@{#832705}

net/nqe/network_quality_estimator.cc

@@ -949,6 +949,59 @@ void NetworkQualityEstimator::ClampKbpsBasedOnEct() {
                    params_->upper_bound_typical_kbps_multiplier())));
 }
 
+void NetworkQualityEstimator::AdjustHttpRttBasedOnRTTCounts(
+    base::TimeDelta* http_rtt) const {
+  if (!params_->adjust_rtt_based_on_rtt_counts())
+    return;
+
+  // This is needed only when RTT from TCP sockets or
+  // QUIC/H2 connections is unavailable.
+  if (transport_rtt_observation_count_last_ect_computation_ >=
+          params_->http_rtt_transport_rtt_min_count() ||
+      end_to_end_rtt_observation_count_at_last_ect_computation_ >=
+          params_->http_rtt_transport_rtt_min_count()) {
+    UMA_HISTOGRAM_TIMES("NQE.HttpRttReduction.BasedOnRTTCounts",
+                        base::TimeDelta());
+    return;
+  }
+
+  // We prefer to use the cached value if it's available and the network change
+  // happened recently.
+  base::TimeDelta time_since_connection_change =
+      tick_clock_->NowTicks() - last_connection_change_;
+  if (cached_estimate_applied_ &&
+      time_since_connection_change <= base::TimeDelta::FromMinutes(1)) {
+    UMA_HISTOGRAM_TIMES("NQE.HttpRttReduction.BasedOnRTTCounts",
+                        base::TimeDelta());
+    return;
+  }
+
+  // If there are not enough transport RTT samples, end-to-end RTT samples and
+  // the cached estimates are unavailble/too stale, then the computed value of
+  // HTTP RTT can't be trusted due to hanging GETs. In that case, return the
+  // typical HTTP RTT for a fast connection.
+  if (current_network_id_.type == net::NetworkChangeNotifier::CONNECTION_NONE) {
+    UMA_HISTOGRAM_TIMES("NQE.HttpRttReduction.BasedOnRTTCounts",
+                        base::TimeDelta());
+    return;
+  }
+
+  base::TimeDelta upper_bound_http_rtt =
+      params_->TypicalNetworkQuality(net::EFFECTIVE_CONNECTION_TYPE_4G)
+          .http_rtt();
+  if (upper_bound_http_rtt > *http_rtt) {
+    UMA_HISTOGRAM_TIMES("NQE.HttpRttReduction.BasedOnRTTCounts",
+                        base::TimeDelta());
+    return;
+  }
+
+  DCHECK_LE(upper_bound_http_rtt, *http_rtt);
+
+  UMA_HISTOGRAM_TIMES("NQE.HttpRttReduction.BasedOnRTTCounts",
+                      *http_rtt - upper_bound_http_rtt);
+  *http_rtt = upper_bound_http_rtt;
+}
+
 EffectiveConnectionType
 NetworkQualityEstimator::GetCappedECTBasedOnSignalStrength() const {
   if (!params_->cap_ect_based_on_signal_strength())
@@ -1084,6 +1137,10 @@ void NetworkQualityEstimator::UpdateHttpRttUsingAllRttValues(
         *http_rtt, end_to_end_rtt *
                        params_->upper_bound_http_rtt_endtoend_rtt_multiplier());
   }
+
+  // Put upper bound on |http_rtt| if there is not enough HTTP RTT samples
+  // available.
+  AdjustHttpRttBasedOnRTTCounts(http_rtt);
 }
 
 EffectiveConnectionType

net/nqe/network_quality_estimator.h

@@ -553,6 +553,10 @@ class NET_EXPORT_PRIVATE NetworkQualityEstimator
   // value lower than |effective_connection_type_| may be returned.
   EffectiveConnectionType GetCappedECTBasedOnSignalStrength() const;
 
+  // When RTT counts are low, it may be impossible to predict accurate ECT. In
+  // that case, we just give the highest value.
+  void AdjustHttpRttBasedOnRTTCounts(base::TimeDelta* http_rtt) const;
+
   // Clamps the throughput estimate based on the current effective connection
   // type.
   void ClampKbpsBasedOnEct();

net/nqe/network_quality_estimator_params.cc

@@ -522,6 +522,11 @@ NetworkQualityEstimatorParams::NetworkQualityEstimatorParams(
               params_,
               "wifi_signal_strength_query_interval_seconds",
               30 * 60))),
+      adjust_rtt_based_on_rtt_counts_(
+          GetStringValueForVariationParamWithDefaultValue(
+              params_,
+              "adjust_rtt_based_on_rtt_counts",
+              "false") == "true"),
       use_small_responses_(false) {
   DCHECK(hanging_request_http_rtt_upper_bound_transport_rtt_multiplier_ == -1 ||
          hanging_request_http_rtt_upper_bound_transport_rtt_multiplier_ > 0);

net/nqe/network_quality_estimator_params.h

@@ -267,6 +267,16 @@ class NET_EXPORT NetworkQualityEstimatorParams {
     return wifi_signal_strength_query_interval_;
   }
 
+  // Returns true if RTTs should be adjusted based on RTT counts.
+  // If there are not enough transport RTT samples, end-to-end RTT samples and
+  // the cached estimates are unavailble/too stale, then the computed value of
+  // HTTP RTT can't be trusted due to hanging GETs. In that case, NQE returns
+  // the typical HTTP RTT for a fast connection if
+  // adjust_rtt_based_on_rtt_counts() returns true.
+  bool adjust_rtt_based_on_rtt_counts() const {
+    return adjust_rtt_based_on_rtt_counts_;
+  }
+
   // Sets the forced effective connection type as |type|.
   void SetForcedEffectiveConnectionTypeForTesting(EffectiveConnectionType type);
 
@@ -302,6 +312,7 @@ class NET_EXPORT NetworkQualityEstimatorParams {
   const double upper_bound_typical_kbps_multiplier_;
   const bool get_signal_strength_and_detailed_network_id_;
   const base::TimeDelta wifi_signal_strength_query_interval_;
+  const bool adjust_rtt_based_on_rtt_counts_;
 
   bool use_small_responses_;
 

net/nqe/network_quality_estimator_test_util.h

@@ -218,6 +218,9 @@ class TestNetworkQualityEstimator : public NetworkQualityEstimator {
 
   const NetworkQualityEstimatorParams* params() const;
 
+  void RecordSpdyPingLatency(const HostPortPair& host_port_pair,
+                             base::TimeDelta rtt) override;
+
   using NetworkQualityEstimator::SetTickClockForTesting;
   using NetworkQualityEstimator::OnConnectionTypeChanged;
   using NetworkQualityEstimator::OnUpdatedTransportRTTAvailable;
@@ -239,9 +242,6 @@ class TestNetworkQualityEstimator : public NetworkQualityEstimator {
       std::unique_ptr<NetworkQualityEstimatorParams> params,
       std::unique_ptr<RecordingBoundTestNetLog> net_log);
 
-  void RecordSpdyPingLatency(const HostPortPair& host_port_pair,
-                             base::TimeDelta rtt) override;
-
   // NetworkQualityEstimator implementation that returns the overridden
   // network id and signal strength (instead of invoking platform APIs).
   nqe::internal::NetworkID GetCurrentNetworkID() const override;

net/nqe/network_quality_estimator_unittest.cc

@@ -656,6 +656,52 @@ TEST_F(NetworkQualityEstimatorTest, QuicObservations) {
       "NQE.RTT.ObservationSource", NETWORK_QUALITY_OBSERVATION_SOURCE_QUIC, 1);
   histogram_tester.ExpectTotalCount("NQE.EndToEndRTT.OnECTComputation", 1);
   histogram_tester.ExpectTotalCount("NQE.RTT.ObservationSource", 2);
+
+  // Verify that the QUIC RTT samples are used when computing transport RTT
+  // estimate.
+  EXPECT_EQ(base::TimeDelta::FromMilliseconds(10), estimator.GetTransportRTT());
+  EXPECT_FALSE(estimator.GetHttpRTT().has_value());
+}
+
+// Verifies that the QUIC RTT samples are used when computing transport RTT
+// estimate.
+TEST_F(NetworkQualityEstimatorTest,
+       QuicObservationsUsedForTransportRTTComputation) {
+  base::HistogramTester histogram_tester;
+  std::map<std::string, std::string> variation_params;
+  variation_params["add_default_platform_observations"] = "false";
+  TestNetworkQualityEstimator estimator(variation_params);
+  estimator.OnUpdatedTransportRTTAvailable(
+      SocketPerformanceWatcherFactory::PROTOCOL_QUIC,
+      base::TimeDelta::FromMilliseconds(10), base::nullopt);
+  histogram_tester.ExpectBucketCount(
+      "NQE.RTT.ObservationSource", NETWORK_QUALITY_OBSERVATION_SOURCE_QUIC, 1);
+  histogram_tester.ExpectTotalCount("NQE.EndToEndRTT.OnECTComputation", 1);
+  histogram_tester.ExpectTotalCount("NQE.RTT.ObservationSource", 1);
+
+  EXPECT_EQ(base::TimeDelta::FromMilliseconds(10), estimator.GetTransportRTT());
+  EXPECT_FALSE(estimator.GetHttpRTT().has_value());
+}
+
+// Verifies that the H2 RTT samples are used when computing transport RTT
+// estimate.
+TEST_F(NetworkQualityEstimatorTest,
+       H2ObservationsUsedForTransportRTTComputation) {
+  base::HistogramTester histogram_tester;
+  std::map<std::string, std::string> variation_params;
+  variation_params["add_default_platform_observations"] = "false";
+  TestNetworkQualityEstimator estimator(variation_params);
+  estimator.RecordSpdyPingLatency(
+      net::HostPortPair::FromString("www.test.com:443"),
+      base::TimeDelta::FromMilliseconds(10));
+  histogram_tester.ExpectBucketCount(
+      "NQE.RTT.ObservationSource", NETWORK_QUALITY_OBSERVATION_SOURCE_H2_PINGS,
+      1);
+  histogram_tester.ExpectTotalCount("NQE.EndToEndRTT.OnECTComputation", 1);
+  histogram_tester.ExpectTotalCount("NQE.RTT.ObservationSource", 1);
+
+  EXPECT_EQ(base::TimeDelta::FromMilliseconds(10), estimator.GetTransportRTT());
+  EXPECT_FALSE(estimator.GetHttpRTT().has_value());
 }
 
 TEST_F(NetworkQualityEstimatorTest, StoreObservations) {
@@ -3197,4 +3243,65 @@ TEST_F(NetworkQualityEstimatorTest, CheckSignalStrengthDisabledByDefault) {
   EXPECT_FALSE(signal_strength);
 }
 
+// Tests that the HTTP RTT and ECT are adjusted when the count of transport RTTs
+// is low. The test adds only HTTP RTT observations and does not add any
+// transport RTT observations. Absence of transport RTT observations should
+// trigger adjusting of HTTP RTT if param |add_default_platform_observations| is
+// set to true.
+TEST_F(NetworkQualityEstimatorTest, AdjustHttpRttBasedOnRttCounts) {
+  for (const bool adjust_rtt_based_on_rtt_counts : {false, true}) {
+    base::SimpleTestTickClock tick_clock;
+    tick_clock.Advance(base::TimeDelta::FromMinutes(1));
+
+    std::map<std::string, std::string> variation_params;
+    variation_params["add_default_platform_observations"] = "false";
+
+    if (adjust_rtt_based_on_rtt_counts) {
+      variation_params["adjust_rtt_based_on_rtt_counts"] = "true";
+    }
+
+    TestNetworkQualityEstimator estimator(variation_params);
+    estimator.DisableOfflineCheckForTesting(true);
+    base::RunLoop().RunUntilIdle();
+
+    base::TimeDelta typical_http_rtt_4g =
+        estimator.params()
+            ->TypicalNetworkQuality(EFFECTIVE_CONNECTION_TYPE_4G)
+            .http_rtt();
+
+    estimator.SetTickClockForTesting(&tick_clock);
+    estimator.SimulateNetworkChange(
+        NetworkChangeNotifier::ConnectionType::CONNECTION_UNKNOWN, "test");
+    tick_clock.Advance(base::TimeDelta::FromMinutes(1));
+
+    const base::TimeDelta rtt = base::TimeDelta::FromSeconds(1);
+    uint64_t host = 1u;
+
+    // Fill the observation buffer so that ECT computations are not triggered
+    // due to observation buffer's size increasing to 1.5x.
+    for (size_t i = 0; i < estimator.params()->observation_buffer_size(); ++i) {
+      estimator.AddAndNotifyObserversOfRTT(NetworkQualityEstimator::Observation(
+          rtt.InMilliseconds(), tick_clock.NowTicks(), INT32_MIN,
+          NETWORK_QUALITY_OBSERVATION_SOURCE_HTTP, host));
+    }
+    // If |adjust_rtt_based_on_rtt_counts| is set, then the HTTP RTT should be
+    // that of a typical 4G connection. Otherwise, the RTT estimate should be
+    // based only on the RTT of the observations added to the buffer.
+    EXPECT_EQ(adjust_rtt_based_on_rtt_counts ? typical_http_rtt_4g : rtt,
+              estimator.GetHttpRTT().value());
+    tick_clock.Advance(base::TimeDelta::FromMinutes(60));
+
+    const base::TimeDelta rtt_new = base::TimeDelta::FromSeconds(3);
+    for (size_t i = 0;
+         i < estimator.params()->count_new_observations_received_compute_ect();
+         ++i) {
+      estimator.AddAndNotifyObserversOfRTT(NetworkQualityEstimator::Observation(
+          rtt_new.InMilliseconds(), tick_clock.NowTicks(), INT32_MIN,
+          NETWORK_QUALITY_OBSERVATION_SOURCE_HTTP, host));
+    }
+    EXPECT_EQ(adjust_rtt_based_on_rtt_counts ? typical_http_rtt_4g : rtt_new,
+              estimator.GetHttpRTT().value());
+  }
+}
+
 }  // namespace net

tools/metrics/histograms/histograms_xml/others/histograms.xml

@@ -10003,6 +10003,15 @@ reviews. Googlers can read more about this at go/gwsq-gerrit.
   </summary>
 </histogram>
 
+<histogram name="NQE.HttpRttReduction.BasedOnRTTCounts" units="ms"
+    expires_after="2021-10-01">
+  <owner>tbansal@chromium.org</owner>
+  <owner>bengr@chromium.org</owner>
+  <summary>
+    Duration by which HTTP RTT was reduced because of low confidence.
+  </summary>
+</histogram>
+
 <histogram name="NQE.Kbps.OnECTComputation" units="Kbps"
     expires_after="2021-05-16">
   <owner>bengr@chromium.org</owner>