Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2936823002:
NQE: Exclude network observations from private networks (Closed)
DescriptionNQE: Exclude network observations from private networks
Skip RTTs from private networks when computing the network quality in
Network Quality Estimator (NQE). Similarly, if a request to a private
network is in-flight, throughput observations are not taken.
BUG=731797
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
Review-Url: https://codereview.chromium.org/2936823002
Cr-Commit-Position: refs/heads/master@{#480357}
Committed: https://chromium.googlesource.com/chromium/src/+/82edab49485f9be425fb78f3ec14115358ffaf52
Patch Set 1 : ps #
Total comments: 13
Patch Set 2 : bengr, mmenke comments #
Total comments: 14
Patch Set 3 : rebased, mmenke comments #
Total comments: 2
Patch Set 4 : bengr comments, rebased #
Messages
Total messages: 69 (55 generated)
Description was changed from ========== nqe private net requests BUG= ========== to ========== nqe private net requests BUG=731797 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester ==========
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #1 (id:1) has been deleted
Patchset #2 (id:40001) has been deleted
Patchset #1 (id:20001) has been deleted
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== nqe private net requests BUG=731797 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester ========== to ========== nqe private net requests Skip RTTs from private networks. Also, if a request to private subnet is in-flight, throughput observations are not taken. BUG=731797 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== nqe private net requests Skip RTTs from private networks. Also, if a request to private subnet is in-flight, throughput observations are not taken. BUG=731797 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester ========== to ========== NQE: Exclude network observations from private networks Skip RTTs from private networks when computing the network quality in Network Quality Estimator (NQE). Similarly, if a request to private network is in-flight, throughput observations are not taken. BUG=731797 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester ==========
Description was changed from ========== NQE: Exclude network observations from private networks Skip RTTs from private networks when computing the network quality in Network Quality Estimator (NQE). Similarly, if a request to private network is in-flight, throughput observations are not taken. BUG=731797 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester ========== to ========== NQE: Exclude network observations from private networks Skip RTTs from private networks when computing the network quality in Network Quality Estimator (NQE). Similarly, if a request to a private network is in-flight, throughput observations are not taken. BUG=731797 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester ==========
tbansal@chromium.org changed reviewers: + bengr@chromium.org
bengr: ptal. Thanks.
https://codereview.chromium.org/2936823002/diff/60001/net/nqe/network_quality... File net/nqe/network_quality_estimator_util.cc (right): https://codereview.chromium.org/2936823002/diff/60001/net/nqe/network_quality... net/nqe/network_quality_estimator_util.cc:21: bool IsPrivateHost(HostResolver* host_resolver, Maybe this should be in url_util? https://codereview.chromium.org/2936823002/diff/60001/net/nqe/network_quality... net/nqe/network_quality_estimator_util.cc:40: int rv = host_resolver->ResolveFromCache(resolve_info, &addresses, net_log); What happens if it isn't cached? How often does that happen? What if someone else calls your method before the host is resolved? https://codereview.chromium.org/2936823002/diff/60001/net/nqe/network_quality... File net/nqe/network_quality_estimator_util.h (right): https://codereview.chromium.org/2936823002/diff/60001/net/nqe/network_quality... net/nqe/network_quality_estimator_util.h:21: // private Internet as defined by RFC 1918. IsPrivateHost() makes the best Add to the comment that these are hosts with IP addresses that match: 10.0.0.0 - 10.255.255.255 (10/8 prefix) 172.16.0.0 - 172.31.255.255 (172.16/12 prefix) 192.168.0.0 - 192.168.255.255 (192.168/16 prefix) https://codereview.chromium.org/2936823002/diff/60001/net/nqe/network_quality... net/nqe/network_quality_estimator_util.h:24: NET_EXPORT_PRIVATE bool IsPrivateHost(HostResolver* host_resolver, This seems like something more generic than nqe. Perhaps put it in some more general purpose net util file?
tbansal@chromium.org changed reviewers: + mmenke@chromium.org
mmenke: ptal at the comment in net/nqe/network_quality_estimator_util.cc. I am interested in knowing if we are better off in moving the function from net/nqe/network_quality_estimator_util.cc to a more common place in //net? https://codereview.chromium.org/2936823002/diff/60001/net/nqe/network_quality... File net/nqe/network_quality_estimator_util.cc (right): https://codereview.chromium.org/2936823002/diff/60001/net/nqe/network_quality... net/nqe/network_quality_estimator_util.cc:21: bool IsPrivateHost(HostResolver* host_resolver, On 2017/06/13 20:35:36, bengr wrote: > Maybe this should be in url_util? +mmenke: Do you think it will be useful to include this in a more common place such as url_util?
https://codereview.chromium.org/2936823002/diff/60001/net/nqe/network_quality... File net/nqe/network_quality_estimator_util.cc (right): https://codereview.chromium.org/2936823002/diff/60001/net/nqe/network_quality... net/nqe/network_quality_estimator_util.cc:21: bool IsPrivateHost(HostResolver* host_resolver, On 2017/06/13 22:36:09, tbansal1 wrote: > On 2017/06/13 20:35:36, bengr wrote: > > Maybe this should be in url_util? > > +mmenke: Do you think it will be useful to include this in a more common place > such as url_util? 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.
tbansal@chromium.org changed reviewers: - mmenke@chromium.org
mmenke@chromium.org changed reviewers: + mmenke@chromium.org
I don't think we want this as a standard method - both the cache behavior and the address ranges it checks are a bit funky. https://codereview.chromium.org/2936823002/diff/60001/net/nqe/network_quality... File net/nqe/network_quality_estimator_util.cc (right): https://codereview.chromium.org/2936823002/diff/60001/net/nqe/network_quality... net/nqe/network_quality_estimator_util.cc:30: ip_address.IsReserved()) { 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? https://codereview.chromium.org/2936823002/diff/60001/net/nqe/network_quality... File net/nqe/network_quality_estimator_util.h (right): https://codereview.chromium.org/2936823002/diff/60001/net/nqe/network_quality... net/nqe/network_quality_estimator_util.h:21: // private Internet as defined by RFC 1918. IsPrivateHost() makes the best On 2017/06/13 20:35:36, bengr wrote: > Add to the comment that these are hosts with IP addresses that match: > > 10.0.0.0 - 10.255.255.255 (10/8 prefix) > 172.16.0.0 - 172.31.255.255 (172.16/12 prefix) > 192.168.0.0 - 192.168.255.255 (192.168/16 prefix) This is actually not what the method is doing. IPAddress::IsReserved checks a lot more reserved ranges than those reserved for local networks. I don't think we have a check for just these 3 prefixes in net/, though dns/address_sorter_posix.cc may have similar logic somewhere.
Tarun, let me know when you'd like me to take another look.
Patchset #2 (id:80001) has been deleted
bengr/mmenke ptal. Thanks. https://codereview.chromium.org/2936823002/diff/60001/net/nqe/network_quality... File net/nqe/network_quality_estimator_util.cc (right): https://codereview.chromium.org/2936823002/diff/60001/net/nqe/network_quality... net/nqe/network_quality_estimator_util.cc:21: bool IsPrivateHost(HostResolver* host_resolver, On 2017/06/14 17:27:04, tbansal1 wrote: > On 2017/06/13 22:36:09, tbansal1 wrote: > > On 2017/06/13 20:35:36, bengr wrote: > > > Maybe this should be in url_util? > > > > +mmenke: Do you think it will be useful to include this in a more common place > > such as url_util? > > 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. Acknowledged. https://codereview.chromium.org/2936823002/diff/60001/net/nqe/network_quality... net/nqe/network_quality_estimator_util.cc:30: ip_address.IsReserved()) { On 2017/06/14 18:23:31, mmenke wrote: > 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? I added some more comments in the network_quality_estimator_util.h file. Let me know id it is still not clear. https://codereview.chromium.org/2936823002/diff/60001/net/nqe/network_quality... net/nqe/network_quality_estimator_util.cc:40: int rv = host_resolver->ResolveFromCache(resolve_info, &addresses, net_log); On 2017/06/13 20:35:36, bengr wrote: > What happens if it isn't cached? How often does that happen? What if someone > else calls your method before the host is resolved? 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. https://codereview.chromium.org/2936823002/diff/60001/net/nqe/network_quality... File net/nqe/network_quality_estimator_util.h (right): https://codereview.chromium.org/2936823002/diff/60001/net/nqe/network_quality... net/nqe/network_quality_estimator_util.h:21: // private Internet as defined by RFC 1918. IsPrivateHost() makes the best On 2017/06/14 18:23:31, mmenke wrote: > On 2017/06/13 20:35:36, bengr wrote: > > Add to the comment that these are hosts with IP addresses that match: > > > > 10.0.0.0 - 10.255.255.255 (10/8 prefix) > > 172.16.0.0 - 172.31.255.255 (172.16/12 prefix) > > 192.168.0.0 - 192.168.255.255 (192.168/16 prefix) > > This is actually not what the method is doing. IPAddress::IsReserved checks a > lot more reserved ranges than those reserved for local networks. I don't think > we have a check for just these 3 prefixes in net/, though > dns/address_sorter_posix.cc may have similar logic somewhere. Right. I have added few more comments to the method. https://codereview.chromium.org/2936823002/diff/60001/net/nqe/network_quality... net/nqe/network_quality_estimator_util.h:24: NET_EXPORT_PRIVATE bool IsPrivateHost(HostResolver* host_resolver, On 2017/06/13 20:35:36, bengr wrote: > This seems like something more generic than nqe. Perhaps put it in some more > general purpose net util file? We can think about moving it to a more common place if there is some other consumer. Right now, it looks very NQE specific. It also might have few false positives (IPs that are not private may be marked as such).
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2936823002/diff/100001/net/nqe/network_qualit... File net/nqe/network_quality_estimator_util.cc (right): https://codereview.chromium.org/2936823002/diff/100001/net/nqe/network_qualit... net/nqe/network_quality_estimator_util.cc:24: if (IsLocalhost(host_port_pair.host())) Again, not needed if host_resolver is non-null, per below comment. https://codereview.chromium.org/2936823002/diff/100001/net/nqe/network_qualit... net/nqe/network_quality_estimator_util.cc:30: ip_address.IsReserved()) { If host_resolver is guaranteed to be non-NULL, this check isn't needed, since host_resolver will handle IP literals. Otherwise, if AssignFromIPLiteral is true, and IsReserved is false, could optionally just return false here. Shouldn't really matter much. https://codereview.chromium.org/2936823002/diff/100001/net/nqe/network_qualit... net/nqe/network_quality_estimator_util.cc:34: if (host_resolver) { If you decide to keep this optional (I recommend against that, unless it's really needed), should have a test with an empty host_resolver that reaches the final "return false;" https://codereview.chromium.org/2936823002/diff/100001/net/nqe/network_qualit... File net/nqe/network_quality_estimator_util_unittest.cc (right): https://codereview.chromium.org/2936823002/diff/100001/net/nqe/network_qualit... net/nqe/network_quality_estimator_util_unittest.cc:74: net_log_ptr->bound())); Also check localhost6? https://codereview.chromium.org/2936823002/diff/100001/net/nqe/network_qualit... net/nqe/network_quality_estimator_util_unittest.cc:76: net_log_ptr->bound())); Also suggest 0.0.0.0 (It's used as an alias for 127.0.0.1 on some systems) https://codereview.chromium.org/2936823002/diff/100001/net/nqe/network_qualit... net/nqe/network_quality_estimator_util_unittest.cc:86: EXPECT_EQ(1u, mock_host_resolver.num_resolve_from_cache()); Suggest checking some IPv6 hosts (::1, sure there are others), and an IPv6 IP this is not reserved as well. 2607:f8b0:4006:819::200e is a public one that's not reserved (It's what I got from a DNS lookup of ipv6.google.com). https://codereview.chromium.org/2936823002/diff/100001/net/nqe/network_qualit... net/nqe/network_quality_estimator_util_unittest.cc:95: net_log_ptr->bound())); Test with a hostname that's not cached, so ResolveFromCache fails?
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Patchset #3 (id:120001) has been deleted
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Patchset #3 (id:140001) has been deleted
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #3 (id:160001) has been deleted
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
mmenke: ptal. Thanks. https://codereview.chromium.org/2936823002/diff/100001/net/nqe/network_qualit... File net/nqe/network_quality_estimator_util.cc (right): https://codereview.chromium.org/2936823002/diff/100001/net/nqe/network_qualit... net/nqe/network_quality_estimator_util.cc:24: if (IsLocalhost(host_port_pair.host())) On 2017/06/16 15:03:58, mmenke wrote: > Again, not needed if host_resolver is non-null, per below comment. Done. https://codereview.chromium.org/2936823002/diff/100001/net/nqe/network_qualit... net/nqe/network_quality_estimator_util.cc:30: ip_address.IsReserved()) { On 2017/06/16 15:03:58, mmenke wrote: > If host_resolver is guaranteed to be non-NULL, this check isn't needed, since > host_resolver will handle IP literals. > > Otherwise, if AssignFromIPLiteral is true, and IsReserved is false, could > optionally just return false here. Shouldn't really matter much. Done. https://codereview.chromium.org/2936823002/diff/100001/net/nqe/network_qualit... net/nqe/network_quality_estimator_util.cc:34: if (host_resolver) { On 2017/06/16 15:03:58, mmenke wrote: > If you decide to keep this optional (I recommend against that, unless it's > really needed), should have a test with an empty host_resolver that reaches the > final "return false;" Done. https://codereview.chromium.org/2936823002/diff/100001/net/nqe/network_qualit... File net/nqe/network_quality_estimator_util_unittest.cc (right): https://codereview.chromium.org/2936823002/diff/100001/net/nqe/network_qualit... net/nqe/network_quality_estimator_util_unittest.cc:74: net_log_ptr->bound())); On 2017/06/16 15:03:58, mmenke wrote: > Also check localhost6? Done. https://codereview.chromium.org/2936823002/diff/100001/net/nqe/network_qualit... net/nqe/network_quality_estimator_util_unittest.cc:76: net_log_ptr->bound())); On 2017/06/16 15:03:58, mmenke wrote: > Also suggest 0.0.0.0 (It's used as an alias for 127.0.0.1 on some systems) Done. https://codereview.chromium.org/2936823002/diff/100001/net/nqe/network_qualit... net/nqe/network_quality_estimator_util_unittest.cc:86: EXPECT_EQ(1u, mock_host_resolver.num_resolve_from_cache()); On 2017/06/16 15:03:58, mmenke wrote: > Suggest checking some IPv6 hosts (::1, sure there are others), and an IPv6 IP > this is not reserved as well. > > 2607:f8b0:4006:819::200e is a public one that's not reserved (It's what I got > from a DNS lookup of http://ipv6.google.com). Done. https://codereview.chromium.org/2936823002/diff/100001/net/nqe/network_qualit... net/nqe/network_quality_estimator_util_unittest.cc:95: net_log_ptr->bound())); On 2017/06/16 15:03:58, mmenke wrote: > Test with a hostname that's not cached, so ResolveFromCache fails? Done.
LGTM (Just looked at the util file)
lgtm with nit https://codereview.chromium.org/2936823002/diff/180001/net/nqe/throughput_ana... File net/nqe/throughput_analyzer_unittest.cc (right): https://codereview.chromium.org/2936823002/diff/180001/net/nqe/throughput_ana... net/nqe/throughput_analyzer_unittest.cc:71: // Use a mock resolver to force example.com to resolve to a public IP address. nit: Use -> Uses
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2936823002/diff/180001/net/nqe/throughput_ana... File net/nqe/throughput_analyzer_unittest.cc (right): https://codereview.chromium.org/2936823002/diff/180001/net/nqe/throughput_ana... net/nqe/throughput_analyzer_unittest.cc:71: // Use a mock resolver to force example.com to resolve to a public IP address. On 2017/06/18 20:42:59, bengr wrote: > nit: Use -> Uses Done.
The CQ bit was checked by tbansal@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bengr@chromium.org, mmenke@chromium.org Link to the patchset: https://codereview.chromium.org/2936823002/#ps200001 (title: "bengr comments, rebased")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 200001, "attempt_start_ts": 1497851449588010,
"parent_rev": "61854ff1c3e6140382cb48447d9da1e1fb51a444", "commit_rev":
"82edab49485f9be425fb78f3ec14115358ffaf52"}
Message was sent while issue was closed.
Description was changed from ========== NQE: Exclude network observations from private networks Skip RTTs from private networks when computing the network quality in Network Quality Estimator (NQE). Similarly, if a request to a private network is in-flight, throughput observations are not taken. BUG=731797 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester ========== to ========== NQE: Exclude network observations from private networks Skip RTTs from private networks when computing the network quality in Network Quality Estimator (NQE). Similarly, if a request to a private network is in-flight, throughput observations are not taken. BUG=731797 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester Review-Url: https://codereview.chromium.org/2936823002 Cr-Commit-Position: refs/heads/master@{#480357} Committed: https://chromium.googlesource.com/chromium/src/+/82edab49485f9be425fb78f3ec14... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:200001) as https://chromium.googlesource.com/chromium/src/+/82edab49485f9be425fb78f3ec14... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
