NQE: Add Transport RTT based GetECT algorithm

net/nqe/network_quality_estimator.cc

Index: net/nqe/network_quality_estimator.cc
diff --git a/net/nqe/network_quality_estimator.cc b/net/nqe/network_quality_estimator.cc
index 3756629a07e8153c0eb388221e3749b4df35d72e..aa0eed3e0df71ae155a5a4bd2bf76baa7faf5e7a 100644
--- a/net/nqe/network_quality_estimator.cc
+++ b/net/nqe/network_quality_estimator.cc
@@ -93,6 +93,13 @@ const char kDefaultKbpsObservationSuffix[] = ".DefaultMedianKbps";
 const char kThresholdURLRTTMsecSuffix[] = ".ThresholdMedianHttpRTTMsec";
 
 // Suffix of the name of the variation parameter that contains the threshold
+// transport RTTs (in milliseconds) for different effective connection types.
+// Complete name of the variation parameter would be
+// |EffectiveConnectionType|.|kThresholdTransportRTTMsecSuffix|.
+const char kThresholdTransportRTTMsecSuffix[] =
+    ".ThresholdMedianTransportRTTMsec";
+
+// Suffix of the name of the variation parameter that contains the threshold
 // downlink throughput (in kbps) for different effective connection types.
 // Complete name of the variation parameter would be
 // |EffectiveConnectionType|.|kThresholdKbpsSuffix|.
@@ -227,7 +234,10 @@ NetworkQualityEstimator::NetworkQualityEstimator(
     bool use_smaller_responses_for_tests)
     : algorithm_name_to_enum_({{"HttpRTTAndDownstreamThroughput",
                                 EffectiveConnectionTypeAlgorithm::
-                                    HTTP_RTT_AND_DOWNSTREAM_THROUGHOUT}}),
+                                    HTTP_RTT_AND_DOWNSTREAM_THROUGHOUT},
+                               {"TransportRTTOrDownstreamThroughput",

[Not at Google. Contact bengr, 2016/07/06 19:04:07]

This is brittle because it allows adding an entry to enum without updating this
hashmap. Also because copy pasting wrong values will also fail silently. Same
issue if you manually maintain a switch statement that maintains mappings. Not a
good idea to have a critical dependency that fails silently.

Instead of manually initializing string-enum pairs, suggest using macros to
autogenerate the boiler plate. Lots of examples in code base:
https://cs.chromium.org/search/?q=%5C%23define.*LIST&sq=package:chromium&type=cs

Macro doc: https://gcc.gnu.org/onlinedocs/cpp/Concatenation.html

Specifically suggest something like:

#define RTT_ALGO_LIST \
    RTT_ALGO_ENUM(HTTP_AND_DOWNSTREAM),\
    RTT_ALGO_ENUM(TRANSPORT_OR_DOWNSTREAM),\ ...

Then you can autogenerate the boilerplate by defining RTT_ALGO_ENUM.

.h file:
#define RTT_ALGO_ENUM(name) RTT_##name##_THROUGHPUT,
  typedef enum {
    CLIENT_ENUMS_LIST
  } EffectiveConnectionTypeAlgorithm;
#undef RTT_ALGO_ENUM


.cc file:
#define RTT_ALGO_ENUM(name) \
  case name: return name;

const EffectiveConnectionTypeAlgorithm GetAlgoForString(char* algo_name) {
  switch (client) {
    CLIENT_ENUMS_LIST
  }
  NOTREACHED();
  return "";
}
#undef RTT_ALGO_ENUM

[tbansal1, 2016/07/06 23:17:32]

That error would be caught by DCHECK below. See line 278 below. 
"DCHECK_EQ(algorithm_name_to_enum_.size()..."
That's the problem with using macros too...right? I am thinking of adding
functionality to give a warning (or add UMA metric) if a variation param is not
recognized, but that will take some time.

[Not at Google. Contact bengr, 2016/07/07 00:03:04]

Macro does not have this issue. You will get a compile time warning if 2 entries
are the same. Issue of mismatching string-enum will never occur since you are
only entering one; the other is auto-generated.
Is there a good way to catch the error where I accidentally add a new entry, but
forget to change the enum:
{ "SomeNewRTTAlgorithm", HTTP_RTT_AND_DOWNSTREAM_THROUGHOUT /*existing enum*/}

FWIW, I have seen this exact pattern cause a copy paste bug that reached
production.

[tbansal1, 2016/07/07 16:47:32]

Agreed. Right now this error is captured using NQE tests. Another problem with
using macro is that I need to map strings to enums (not enums to strings). Since
switch-case does not support strings, it is not clear if I can use that
framework.
I added some more test code in
NetworkQualityEstimatorTest.ObtainAlgorithmToUseFromParams
which tests this.

[Not at Google. Contact bengr, 2016/07/07 17:19:17]

It is easy to map strings to enums. You just use an if statement instead of the
case statement:
#define RTT_ALGO_ENUM(name) \
  if (name == algo_name) return RTT_##name##_THROUGHPUT;

[tbansal1, 2016/07/07 18:01:32]

"if" condtional does not catch duplicates in value (the switch does). See
example here: https://codereview.chromium.org/2129723004/

[Not at Google. Contact bengr, 2016/07/07 19:10:49]

We don't rely on the switch to catch duplicates. The enum definition will catch
duplicates. The if statement is autogenerated from the enum definition, so will
automatically not have duplicates.

[tbansal1, 2016/07/07 22:47:26]

Added TODO. For now the duplicates are catched using tests.

+                                EffectiveConnectionTypeAlgorithm::
+                                    TRANSPORT_RTT_OR_DOWNSTREAM_THROUGHOUT}}),
       use_localhost_requests_(use_local_host_requests_for_tests),
       use_small_responses_(use_smaller_responses_for_tests),
       weight_multiplier_per_second_(
@@ -386,6 +396,26 @@ void NetworkQualityEstimator::ObtainEffectiveConnectionTypeModelParams(
              rtt <= connection_thresholds_[i - 1].http_rtt());
     }
 
+    variations_value = kMinimumRTTVariationParameterMsec - 1;

[Not at Google. Contact bengr, 2016/07/07 00:03:04]

This block is extremely similar to the block above and below. I see  similar
repition in RecordAccuracyAfterMainFrame below. Any way to eliminate the
duplication?

One option I could think of is for NetworkQuality to be loosely structured (for
example HashMap with key value pairs) instead of having dedicated fields. It
will make this loop more generic instead of having a block for each field.
However, don't love this approach.

Cannot think of a better way, but thought I'd mention it.

[tbansal1, 2016/07/07 16:47:32]

I am not sure of a better way to combine them.

+    if (GetValueForVariationParam(
+            variation_params,
+            connection_type_name + kThresholdTransportRTTMsecSuffix,
+            &variations_value) &&
+        variations_value >= kMinimumRTTVariationParameterMsec) {
+      base::TimeDelta transport_rtt(
+          base::TimeDelta::FromMilliseconds(variations_value));
+      connection_thresholds_[i] = nqe::internal::NetworkQuality(

[Not at Google. Contact bengr, 2016/07/07 00:03:04]

Any reason to create new instance of NetworkQuality rather than setting field on
the existing instance?

[tbansal1, 2016/07/07 16:47:32]

Done.

+          connection_thresholds_[i].http_rtt(), transport_rtt,
+          connection_thresholds_[i].downstream_throughput_kbps());
+
+      // Verify that the transport RTT values are in decreasing order as the
+      // network quality improves.
+      DCHECK(i == 0 ||
+             connection_thresholds_[i - 1].transport_rtt() ==
+                 nqe::internal::InvalidRTT() ||
+             transport_rtt <= connection_thresholds_[i - 1].transport_rtt());
+    }
+
     variations_value = kMinimumThroughputVariationParameterKbps - 1;
     if (GetValueForVariationParam(variation_params,
                                   connection_type_name + kThresholdKbpsSuffix,
@@ -892,6 +922,12 @@ NetworkQualityEstimator::GetRecentEffectiveConnectionType(
     return GetRecentEffectiveConnectionTypeHttpRTTAndDownstreamThroughput(
         start_time);
   }
+  if (effective_connection_type_algorithm_ ==
+      EffectiveConnectionTypeAlgorithm::
+          TRANSPORT_RTT_OR_DOWNSTREAM_THROUGHOUT) {
+    return GetRecentEffectiveConnectionTypeTransportRTTOrDownstreamThroughput(
+        start_time);
+  }
   // Add additional algorithms here.
   NOTREACHED();
   return EFFECTIVE_CONNECTION_TYPE_UNKNOWN;
@@ -928,11 +964,11 @@ NetworkQualityEstimator::EffectiveConnectionType NetworkQualityEstimator::
     EffectiveConnectionType type = static_cast<EffectiveConnectionType>(i);
     if (i == EFFECTIVE_CONNECTION_TYPE_UNKNOWN)
       continue;
-    bool estimated_http_rtt_is_higher_than_threshold =
+    const bool estimated_http_rtt_is_higher_than_threshold =
         http_rtt != nqe::internal::InvalidRTT() &&
         connection_thresholds_[i].http_rtt() != nqe::internal::InvalidRTT() &&
         http_rtt >= connection_thresholds_[i].http_rtt();
-    bool estimated_throughput_is_lower_than_threshold =
+    const bool estimated_throughput_is_lower_than_threshold =
         kbps != nqe::internal::kInvalidThroughput &&
         connection_thresholds_[i].downstream_throughput_kbps() !=
             nqe::internal::kInvalidThroughput &&
@@ -951,6 +987,62 @@ NetworkQualityEstimator::EffectiveConnectionType NetworkQualityEstimator::
                                               1);
 }
 
+NetworkQualityEstimator::EffectiveConnectionType NetworkQualityEstimator::
+    GetRecentEffectiveConnectionTypeTransportRTTOrDownstreamThroughput(
+        const base::TimeTicks& start_time) const {
+  DCHECK(thread_checker_.CalledOnValidThread());
+
+  // If the device is currently offline, then return

[Not at Google. Contact bengr, 2016/07/06 19:04:07]

FWIW code is very readable, so this comment seems unnecessary to me.

[tbansal1, 2016/07/06 23:17:32]

Done.

+  // EFFECTIVE_CONNECTION_TYPE_OFFLINE.
+  if (GetCurrentNetworkID().type == NetworkChangeNotifier::CONNECTION_NONE)
+    return EFFECTIVE_CONNECTION_TYPE_OFFLINE;
+
+  base::TimeDelta transport_rtt = nqe::internal::InvalidRTT();
+  if (!GetRecentTransportRTTMedian(start_time, &transport_rtt))
+    transport_rtt = nqe::internal::InvalidRTT();

[Not at Google. Contact bengr, 2016/07/06 19:04:07]

Can we have GetRecentTransportRTTMedian(..) set transport_rtt to InvalidRTT on
failure so that we can avoid this set, test, and reset?

Actually, couldn't it simply return TimeDelta instead of a bool?

[tbansal1, 2016/07/06 23:17:32]

Yeah, I need to fix that. It is easier now since GetRecentTransportRTTMedian is
a private API. I would prefer to do that in a separate CL.

[Not at Google. Contact bengr, 2016/07/07 00:03:04]

Add TODO?

[tbansal1, 2016/07/07 16:47:32]

Done in .h file.

+
+  int32_t kbps = nqe::internal::kInvalidThroughput;
+  if (!GetRecentMedianDownlinkThroughputKbps(start_time, &kbps))
+    kbps = nqe::internal::kInvalidThroughput;

[Not at Google. Contact bengr, 2016/07/06 19:04:07]

Similar to above comment: Can GetRecentMedianDownlinkThroughputKbps(..) just
return the kbps?

[tbansal1, 2016/07/06 23:17:32]

same as above.

[Not at Google. Contact bengr, 2016/07/07 00:03:04]

Acknowledged.

+
+  if (transport_rtt == nqe::internal::InvalidRTT() &&
+      kbps == nqe::internal::kInvalidThroughput) {
+    // Quality of the current network is unknown.
+    return EFFECTIVE_CONNECTION_TYPE_UNKNOWN;
+  }
+
+  // Search from the slowest connection type to the fastest to find the
+  // EffectiveConnectionType that best matches the current connection's
+  // performance. The match is done by comparing RTT and throughput.
+  for (size_t i = 0; i < EFFECTIVE_CONNECTION_TYPE_LAST; ++i) {
+    EffectiveConnectionType type = static_cast<EffectiveConnectionType>(i);
+    if (i == EFFECTIVE_CONNECTION_TYPE_UNKNOWN)
+      continue;
+    const bool estimated_transport_rtt_is_higher_than_threshold =
+        transport_rtt != nqe::internal::InvalidRTT() &&
+        connection_thresholds_[i].transport_rtt() !=
+            nqe::internal::InvalidRTT() &&
+        transport_rtt >= connection_thresholds_[i].transport_rtt();
+    const bool estimated_throughput_is_lower_than_threshold =
+        kbps != nqe::internal::kInvalidThroughput &&
+        connection_thresholds_[i].downstream_throughput_kbps() !=
+            nqe::internal::kInvalidThroughput &&
+        kbps <= connection_thresholds_[i].downstream_throughput_kbps();
+
+    // Return |type| as the effective connection type if the current network's
+    // transport RTT is worse than the threshold RTT for |type|, or if the
+    // current network's throughput is lower than the threshold throughput for
+    // |type|.
+    if (estimated_transport_rtt_is_higher_than_threshold ||
+        estimated_throughput_is_lower_than_threshold) {
+      return type;
+    }
+  }
+  // Return the fastest connection type.
+  return static_cast<EffectiveConnectionType>(EFFECTIVE_CONNECTION_TYPE_LAST -
+                                              1);
+}
+
 void NetworkQualityEstimator::AddEffectiveConnectionTypeObserver(
     EffectiveConnectionTypeObserver* observer) {
   DCHECK(thread_checker_.CalledOnValidThread());