Clean up Network Quality Estimator params

Replace some of the if-conditionals that check finch
params by DCHECKs since those experiments have finished.
Also, clean up the tests.

fieldtrial_testing_config.json has also been updated to
only reflect the parameters that we should be
experimenting with.

Change-Id: I1c3c2669c38dc7229b9c64b930b3342fa7843ca4
Bug: 513681
Reviewed-on: https://chromium-review.googlesource.com/1239695Reviewed-by: Ryan Sturm <ryansturm@chromium.org>
Reviewed-by: Steven Holte <holte@chromium.org>
Commit-Queue: Tarun Bansal <tbansal@chromium.org>
Cr-Commit-Position: refs/heads/master@{#593698}

net/nqe/network_quality_estimator.cc

@@ -402,13 +402,15 @@ bool NetworkQualityEstimator::IsHangingRequest(
     return false;
   }
 
+  DCHECK_LT(
+      0,
+      params_->hanging_request_http_rtt_upper_bound_transport_rtt_multiplier());
+
   if (transport_rtt_observation_count_last_ect_computation_ >=
           params_->http_rtt_transport_rtt_min_count() &&
-      (params_->hanging_request_http_rtt_upper_bound_transport_rtt_multiplier() <=
-           0 ||
-       observed_http_rtt <
-           params_->hanging_request_http_rtt_upper_bound_transport_rtt_multiplier() *
-               GetTransportRTT().value_or(base::TimeDelta::FromSeconds(10)))) {
+      (observed_http_rtt <
+       params_->hanging_request_http_rtt_upper_bound_transport_rtt_multiplier() *
+           GetTransportRTT().value_or(base::TimeDelta::FromSeconds(10)))) {
     // If there are sufficient number of transport RTT samples available, use
     // the transport RTT estimate to determine if the request is hanging.
     UMA_HISTOGRAM_TIMES("NQE.RTT.NotAHangingRequest.TransportRTT",
@@ -416,11 +418,12 @@ bool NetworkQualityEstimator::IsHangingRequest(
     return false;
   }
 
-  if (params_->hanging_request_http_rtt_upper_bound_http_rtt_multiplier() <=
-          0 ||
-      observed_http_rtt <
-          params_->hanging_request_http_rtt_upper_bound_http_rtt_multiplier() *
-              GetHttpRTT().value_or(base::TimeDelta::FromSeconds(10))) {
+  DCHECK_LT(
+      0, params_->hanging_request_http_rtt_upper_bound_http_rtt_multiplier());
+
+  if (observed_http_rtt <
+      params_->hanging_request_http_rtt_upper_bound_http_rtt_multiplier() *
+          GetHttpRTT().value_or(base::TimeDelta::FromSeconds(10))) {
     // Use the HTTP RTT estimate to determine if the request is hanging.
     UMA_HISTOGRAM_TIMES("NQE.RTT.NotAHangingRequest.HttpRTT",
                         observed_http_rtt);

net/nqe/network_quality_estimator_params.cc

@@ -443,10 +443,7 @@ NetworkQualityEstimatorParams::NetworkQualityEstimatorParams(
               "hanging_request_http_rtt_upper_bound_http_rtt_multiplier",
               6)),
       hanging_request_upper_bound_min_http_rtt_(
-          base::TimeDelta::FromMilliseconds(GetValueForVariationParam(
-              params_,
-              "hanging_request_upper_bound_min_http_rtt_msec",
-              500))),
+          base::TimeDelta::FromMilliseconds(500)),
       http_rtt_transport_rtt_min_count_(
           GetValueForVariationParam(params_,
                                     "http_rtt_transport_rtt_min_count",
@@ -471,10 +468,7 @@ NetworkQualityEstimatorParams::NetworkQualityEstimatorParams(
           params_,
           "hanging_request_duration_http_rtt_multiplier",
           5)),
-      hanging_request_min_duration_(base::TimeDelta::FromMilliseconds(
-          GetValueForVariationParam(params_,
-                                    "hanging_request_min_duration_msec",
-                                    3000))),
+      hanging_request_min_duration_(base::TimeDelta::FromMilliseconds(3000)),
       add_default_platform_observations_(
           GetStringValueForVariationParamWithDefaultValue(
               params_,
@@ -502,6 +496,10 @@ NetworkQualityEstimatorParams::NetworkQualityEstimatorParams(
   DCHECK_GE(1.0, weight_multiplier_per_signal_strength_level_);
   DCHECK_LE(0.0, weight_multiplier_per_signal_strength_level_);
 
+  DCHECK_LT(0, hanging_request_duration_http_rtt_multiplier());
+  DCHECK_LT(0, hanging_request_http_rtt_upper_bound_http_rtt_multiplier());
+  DCHECK_LT(0, hanging_request_http_rtt_upper_bound_transport_rtt_multiplier());
+
   ObtainDefaultObservations(params_, default_observations_);
   ObtainTypicalNetworkQualities(params_, typical_network_quality_);
   ObtainConnectionThresholds(params_, connection_thresholds_);

net/nqe/throughput_analyzer.cc

@@ -389,10 +389,7 @@ void ThroughputAnalyzer::BoundRequestsSize() {
 void ThroughputAnalyzer::EraseHangingRequests(const URLRequest& request) {
   DCHECK(thread_checker_.CalledOnValidThread());
 
-  if (params_->hanging_request_duration_http_rtt_multiplier() <= 0) {
-    // Experiment is not enabled.
-    return;
-  }
+  DCHECK_LT(0, params_->hanging_request_duration_http_rtt_multiplier());
 
   const base::TimeTicks now = tick_clock_->NowTicks();
 

net/nqe/throughput_analyzer_unittest.cc

@@ -239,13 +239,13 @@ TEST_F(ThroughputAnalyzerTest, TestHangingRequests) {
           // |requests_hang_duration| is less than 5 times the HTTP RTT.
           // Requests should not be marked as hanging.
           5, base::TimeDelta::FromMilliseconds(1000),
-          base::TimeDelta::FromMilliseconds(2000), true,
+          base::TimeDelta::FromMilliseconds(3000), true,
       },
       {
           // |requests_hang_duration| is more than 5 times the HTTP RTT.
           // Requests should be marked as hanging.
           5, base::TimeDelta::FromMilliseconds(200),
-          base::TimeDelta::FromMilliseconds(2000), false,
+          base::TimeDelta::FromMilliseconds(3000), false,
       },
       {
           // |requests_hang_duration| is less than
@@ -259,12 +259,7 @@ TEST_F(ThroughputAnalyzerTest, TestHangingRequests) {
           // |hanging_request_min_duration_msec|. Requests should be marked as
           // hanging.
           1, base::TimeDelta::FromMilliseconds(2000),
-          base::TimeDelta::FromMilliseconds(2100), false,
-      },
-      {
-          // Experiment is disabled. Requests should be marked as hanging.
-          -1, base::TimeDelta::FromMilliseconds(100),
-          base::TimeDelta::FromMilliseconds(1000), true,
+          base::TimeDelta::FromMilliseconds(3100), false,
       },
       {
           // |requests_hang_duration| is less than 5 times the HTTP RTT.
@@ -294,7 +289,6 @@ TEST_F(ThroughputAnalyzerTest, TestHangingRequests) {
     variation_params["throughput_hanging_requests_cwnd_size_multiplier"] = "-1";
     variation_params["hanging_request_duration_http_rtt_multiplier"] =
         base::IntToString(test.hanging_request_duration_http_rtt_multiplier);
-    variation_params["hanging_request_min_duration_msec"] = "2000";
 
     NetworkQualityEstimatorParams params(variation_params);
 
@@ -521,7 +515,6 @@ TEST_F(ThroughputAnalyzerTest, TestRequestDeletedImmediately) {
   network_quality_provider.SetHttpRtt(base::TimeDelta::FromSeconds(1));
   std::map<std::string, std::string> variation_params;
   variation_params["hanging_request_duration_http_rtt_multiplier"] = "2";
-  variation_params["hanging_request_min_duration_msec"] = "2000";
   NetworkQualityEstimatorParams params(variation_params);
 
   TestThroughputAnalyzer throughput_analyzer(&network_quality_provider, &params,
@@ -543,8 +536,8 @@ TEST_F(ThroughputAnalyzerTest, TestRequestDeletedImmediately) {
   throughput_analyzer.NotifyStartTransaction(*request_not_local);
   EXPECT_EQ(1u, throughput_analyzer.CountInFlightRequests());
 
-  tick_clock.Advance(base::TimeDelta::FromMilliseconds(1900));
-  // Current time is t=1.9 seconds.
+  tick_clock.Advance(base::TimeDelta::FromMilliseconds(2900));
+  // Current time is t=2.9 seconds.
 
   throughput_analyzer.EraseHangingRequests(*request_not_local);
   EXPECT_EQ(1u, throughput_analyzer.CountInFlightRequests());

testing/variations/fieldtrial_testing_config.json

@@ -2605,13 +2605,8 @@
                 {
                     "name": "Enabled",
                     "params": {
-                        "hanging_request_http_rtt_upper_bound_http_rtt_multiplier": "6",
-                        "hanging_request_http_rtt_upper_bound_transport_rtt_multiplier": "8",
-                        "hanging_request_upper_bound_min_http_rtt_msec": "500",
-                        "lower_bound_http_rtt_transport_rtt_multiplier": "1",
                         "rssi_weight_per_signal_strength_level": "0.9",
-                        "throughput_hanging_requests_cwnd_size_multiplier": "1",
-                        "upper_bound_http_rtt_transport_rtt_multiplier": "5"
+                        "use_end_to_end_rtt": "true"
                     }
                 }
             ]