NQE: Exclude network observations from private networks

net/nqe/network_quality_estimator_util.cc

Index: net/nqe/network_quality_estimator_util.cc
diff --git a/net/nqe/network_quality_estimator_util.cc b/net/nqe/network_quality_estimator_util.cc
new file mode 100644
index 0000000000000000000000000000000000000000..cdc64858ba241d2bbd8b99858d9a7f2f0e8ea556
--- /dev/null
+++ b/net/nqe/network_quality_estimator_util.cc
@@ -0,0 +1,59 @@
+// Copyright 2017 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/nqe/network_quality_estimator_util.h"
+
+#include "net/base/address_list.h"
+#include "net/base/host_port_pair.h"
+#include "net/base/ip_address.h"
+#include "net/base/ip_endpoint.h"
+#include "net/base/net_errors.h"
+#include "net/base/url_util.h"
+#include "net/dns/host_resolver.h"
+
+namespace net {
+
+namespace nqe {
+
+namespace internal {
+
+bool IsPrivateHost(HostResolver* host_resolver,

[bengr, 2017/06/13 20:35:36]

Maybe this should be in url_util?

[tbansal1, 2017/06/13 22:36:09]

+mmenke: Do you think it will be useful to include this in a more common place
such as url_util?

[tbansal1, 2017/06/14 17:27:04]

I am in favor of keeping it in //net/nqe, and later if some other consumer comes
up (e.g., data saver proxy),
we should move it to a common place.

[tbansal1, 2017/06/16 00:57:42]

Acknowledged.

+                   const HostPortPair& host_port_pair,
+                   const NetLogWithSource& net_log) {
+  if (IsLocalhost(host_port_pair.host()))
+    return true;
+
+  // Check if |host_port_pair.host()| is a reserved IP address.
+  net::IPAddress ip_address;
+  if (ip_address.AssignFromIPLiteral(host_port_pair.host()) &&
+      ip_address.IsReserved()) {

[mmenke, 2017/06/14 18:23:31]

Is this right?  RFC 1918 predates IPv6 reserved addresses, and per earlier
comment, more ranges are reserved than that RFC calls private addresses.

Also, that RFC bizarrely doesn't seem to include 127.*...What are we trying to
do here?

[tbansal1, 2017/06/16 00:57:42]

I added some more comments in the network_quality_estimator_util.h file. Let me
know id it is still not clear.

+    return true;
+  }
+
+  if (host_resolver) {
+    // Try resolving |host_port_pair.host()| synchronously.
+    HostResolver::RequestInfo resolve_info(host_port_pair);
+    resolve_info.set_allow_cached_response(true);
+    AddressList addresses;
+    // Resolve synchronously using the resolver's cache.
+    int rv = host_resolver->ResolveFromCache(resolve_info, &addresses, net_log);

[bengr, 2017/06/13 20:35:36]

What happens if it isn't cached? How often does that happen? What if someone
else calls your method before the host is resolved?

[tbansal1, 2017/06/16 00:57:42]

Depends on when it is queried. Currently, for RTT, this is queried after HTTP
headers
are received. By that time the result should be there in the cache (if DNS
caching was enabled). 

For throughput, it is queried right after StartTransaction() which happens
before DNS resolution. So, it would result in a cache miss for the first time.
Subsequent requests should get a cache hit.

+
+    DCHECK_NE(rv, ERR_IO_PENDING);
+    if (rv == OK && !addresses.empty()) {
+      // Checking only the first address should be sufficient.
+      IPEndPoint ip_end_point = addresses.front();
+      ip_address = ip_end_point.address();
+      if (ip_address.IsReserved()) {
+        return true;
+      }
+    }
+  }
+  return false;
+}
+
+}  // namespace internal
+
+}  // namespace nqe
+
+}  // namespace net