Determine proxy connection timeout based on current network quality

Determine proxy connection timeout based on current network quality

When the session is part of NetAdaptiveProxyConnectionTimeout field
trial, the proxy connection timeout is determined based on the network
quality and the field trial parameters.

BUG=704339
Review-Url: https://codereview.chromium.org/2932993002
Cr-Commit-Position: refs/heads/master@{#479486}
Committed: https://chromium.googlesource.com/chromium/src/+/0394a689e2f1a1dad9b7512b8d87f0fdc3ea87d1

[tbansal1, 2017-06-11 07:08:42]

The CQ bit was checked by tbansal@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-11 07:08:53]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-11 08:22:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-11 08:22:41]

Dry run: This issue passed the CQ dry run.

[tbansal1, 2017-06-11 22:07:55]

The CQ bit was checked by tbansal@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-11 22:08:03]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tbansal1, 2017-06-11 22:16:20]

The CQ bit was checked by tbansal@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-11 22:16:35]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-11 23:44:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-11 23:44:27]

Dry run: This issue passed the CQ dry run.

[tbansal1, 2017-06-12 03:47:59]

Patchset #2 (id:20001) has been deleted

[tbansal1, 2017-06-12 03:48:06]

Patchset #1 (id:1) has been deleted

[tbansal1, 2017-06-12 04:16:00]

Patchset #1 (id:40001) has been deleted

[tbansal1, 2017-06-12 04:20:49]

Patchset #1 (id:60001) has been deleted

[tbansal1, 2017-06-12 04:20:59]

The CQ bit was checked by tbansal@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-12 04:21:05]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tbansal1, 2017-06-12 04:22:11]

Description was changed from

==========
PS timeout exp

BUG=
==========

to

==========
Adjust proxy connection timeout based on the field trial paramaters

BUG=704339
==========

[tbansal1, 2017-06-12 04:22:23]

Description was changed from

==========
Adjust proxy connection timeout based on the field trial paramaters

BUG=704339
==========

to

==========
Adjust proxy connection timeout based on the field trial parameters

BUG=704339
==========

[tbansal1, 2017-06-12 04:28:12]

Patchset #1 (id:80001) has been deleted

[tbansal1, 2017-06-12 04:28:34]

Patchset #1 (id:100001) has been deleted

[tbansal1, 2017-06-12 04:28:42]

The CQ bit was checked by tbansal@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-12 04:28:57]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-12 05:46:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-12 05:46:40]

Dry run: This issue passed the CQ dry run.

[tbansal1, 2017-06-12 17:04:39]

Patchset #2 (id:140001) has been deleted

[tbansal1, 2017-06-12 17:05:43]

The CQ bit was checked by tbansal@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-12 17:06:18]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tbansal1, 2017-06-12 17:08:44]

Patchset #1 (id:120001) has been deleted

[tbansal1, 2017-06-12 17:09:03]

Patchset #1 (id:160001) has been deleted

[tbansal1, 2017-06-12 17:09:23]

tbansal@chromium.org changed reviewers:
+ mmenke@chromium.org

[tbansal1, 2017-06-12 17:09:24]

mmenke: ptal. Thanks.

[tbansal1, 2017-06-12 17:10:16]

Description was changed from

==========
Adjust proxy connection timeout based on the field trial parameters

BUG=704339
==========

to

==========
Adjust proxy connection timeout based on the field trial parameters

When the session is part of NetAdaptiveProxyConnectionTimeout field
trial, the proxy connection timeout is determined based on the network
quality and the field trial parameters.

BUG=704339
==========

[tbansal1, 2017-06-12 17:11:02]

The CQ bit was checked by tbansal@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-12 17:11:29]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[mmenke, 2017-06-12 17:56:01]

One question:  Why are we only hooking up proxies?  One desktop, in particular,
they're pretty much a corner case.

I'll dig into this later today or tomorrow.

[tbansal1, 2017-06-12 18:01:11]

On 2017/06/12 17:56:01, mmenke wrote:
> One question:  Why are we only hooking up proxies?  One desktop, in
particular,
> they're pretty much a corner case.
> 
> I'll dig into this later today or tomorrow.

The (long-term) plan is to extend this to different socket pools
(tcp, quic/udp etc.). But for v0, the scope is restricted to proxies.
We will still learn a lot on Android from Flywheel users.

[commit-bot: I haz the power, 2017-06-12 20:27:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-12 20:27:51]

Dry run: This issue passed the CQ dry run.

[tbansal1, 2017-06-12 23:13:22]

Description was changed from

==========
Adjust proxy connection timeout based on the field trial parameters

When the session is part of NetAdaptiveProxyConnectionTimeout field
trial, the proxy connection timeout is determined based on the network
quality and the field trial parameters.

BUG=704339
==========

to

==========
Determine proxy connection timeout based on the field trial parameters

When the session is part of NetAdaptiveProxyConnectionTimeout field
trial, the proxy connection timeout is determined based on the network
quality and the field trial parameters.

BUG=704339
==========

[tbansal1, 2017-06-12 23:13:42]

Description was changed from

==========
Determine proxy connection timeout based on the field trial parameters

When the session is part of NetAdaptiveProxyConnectionTimeout field
trial, the proxy connection timeout is determined based on the network
quality and the field trial parameters.

BUG=704339
==========

to

==========
Determine proxy connection timeout based on current network quality

When the session is part of NetAdaptiveProxyConnectionTimeout field
trial, the proxy connection timeout is determined based on the network
quality and the field trial parameters.

BUG=704339
==========

[mmenke, 2017-06-13 19:52:01]

https://codereview.chromium.org/2932993002/diff/180001/net/http/http_proxy_cl...
File net/http/http_proxy_client_socket_pool.cc (right):

https://codereview.chromium.org/2932993002/diff/180001/net/http/http_proxy_cl...
net/http/http_proxy_client_socket_pool.cc:198:
GetInt32Param("max_proxy_connection_timeout_seconds", 20))),
This default max timeout seems way too low.  Note that DRP is unique in that
failing gives us a chance to try different IPs.  If we're ever going to be run
this on desktop, where a failed proxy connection may mean a failed page load, a
larger one seems logical, at least on desktop (Admittedly, we presumably won't
be falling back to defaults anywhere, anyways).

https://codereview.chromium.org/2932993002/diff/180001/net/http/http_proxy_cl...
File net/http/http_proxy_client_socket_pool_unittest.cc (right):

https://codereview.chromium.org/2932993002/diff/180001/net/http/http_proxy_cl...
net/http/http_proxy_client_socket_pool_unittest.cc:84:
field_trial_list_(nullptr) {
git cl format.

https://codereview.chromium.org/2932993002/diff/180001/net/http/http_proxy_cl...
net/http/http_proxy_client_socket_pool_unittest.cc:87: &ssl_socket_pool_,
&estimator_, NetLogWithSource().net_log());
Why move this?  Can still call MakeUnique in the initializer list.

https://codereview.chromium.org/2932993002/diff/180001/net/http/http_proxy_cl...
net/http/http_proxy_client_socket_pool_unittest.cc:805: }
Suggest a second set of three test with different parameters, just to be
paranoid.

[tbansal1, 2017-06-13 22:54:43]

Patchset #2 (id:200001) has been deleted

[tbansal1, 2017-06-13 23:02:30]

Patchset #2 (id:220001) has been deleted

[tbansal1, 2017-06-13 23:02:48]

mmenke: ptal. Thanks.

https://codereview.chromium.org/2932993002/diff/180001/net/http/http_proxy_cl...
File net/http/http_proxy_client_socket_pool.cc (right):

https://codereview.chromium.org/2932993002/diff/180001/net/http/http_proxy_cl...
net/http/http_proxy_client_socket_pool.cc:198:
GetInt32Param("max_proxy_connection_timeout_seconds", 20))),
On 2017/06/13 19:52:00, mmenke wrote:
> This default max timeout seems way too low.  Note that DRP is unique in that
> failing gives us a chance to try different IPs.  If we're ever going to be run
> this on desktop, where a failed proxy connection may mean a failed page load,
a
> larger one seems logical, at least on desktop (Admittedly, we presumably won't
> be falling back to defaults anywhere, anyways).

Changed it to 60 seconds. Eventually, this will be controlled by finch, so the
default value here is probably not that super-important.

https://codereview.chromium.org/2932993002/diff/180001/net/http/http_proxy_cl...
File net/http/http_proxy_client_socket_pool_unittest.cc (right):

https://codereview.chromium.org/2932993002/diff/180001/net/http/http_proxy_cl...
net/http/http_proxy_client_socket_pool_unittest.cc:84:
field_trial_list_(nullptr) {
On 2017/06/13 19:52:01, mmenke wrote:
> git cl format.

umm, this is formatted. 
|field_trial_list_| is at the same indentation level as |ssl_socket_pool_| which
looks correct to me.

https://codereview.chromium.org/2932993002/diff/180001/net/http/http_proxy_cl...
net/http/http_proxy_client_socket_pool_unittest.cc:87: &ssl_socket_pool_,
&estimator_, NetLogWithSource().net_log());
On 2017/06/13 19:52:00, mmenke wrote:
> Why move this?  Can still call MakeUnique in the initializer list.

Done.

https://codereview.chromium.org/2932993002/diff/180001/net/http/http_proxy_cl...
net/http/http_proxy_client_socket_pool_unittest.cc:805: }
On 2017/06/13 19:52:01, mmenke wrote:
> Suggest a second set of three test with different parameters, just to be
> paranoid.

Done.

[mmenke, 2017-06-14 18:10:01]

One other question, and I'm happy - what metrics are you going to look at?  Do
we have any HTTP-Proxy-specific metrics?  Should we add some?  Just thinking
time-to-connect and error rate.

https://codereview.chromium.org/2932993002/diff/180001/net/http/http_proxy_cl...
File net/http/http_proxy_client_socket_pool_unittest.cc (right):

https://codereview.chromium.org/2932993002/diff/180001/net/http/http_proxy_cl...
net/http/http_proxy_client_socket_pool_unittest.cc:84:
field_trial_list_(nullptr) {
On 2017/06/13 23:02:48, tbansal1 wrote:
> On 2017/06/13 19:52:01, mmenke wrote:
> > git cl format.
> 
> umm, this is formatted. 
> |field_trial_list_| is at the same indentation level as |ssl_socket_pool_|
which
> looks correct to me.

Sorry, was confused by the ssl_socket_pool_ arguments.

[tbansal1, 2017-06-14 18:43:07]

On 2017/06/14 18:10:01, mmenke wrote:
> One other question, and I'm happy - what metrics are you going to look at?  Do
> we have any HTTP-Proxy-specific metrics?  Should we add some?  Just thinking
> time-to-connect and error rate.
I added the metrics earlier:
https://codereview.chromium.org/2768173002/
Those metrics cover both time-to-connect and error rates.
> 
>
https://codereview.chromium.org/2932993002/diff/180001/net/http/http_proxy_cl...
> File net/http/http_proxy_client_socket_pool_unittest.cc (right):
> 
>
https://codereview.chromium.org/2932993002/diff/180001/net/http/http_proxy_cl...
> net/http/http_proxy_client_socket_pool_unittest.cc:84:
> field_trial_list_(nullptr) {
> On 2017/06/13 23:02:48, tbansal1 wrote:
> > On 2017/06/13 19:52:01, mmenke wrote:
> > > git cl format.
> > 
> > umm, this is formatted. 
> > |field_trial_list_| is at the same indentation level as |ssl_socket_pool_|
> which
> > looks correct to me.
> 
> Sorry, was confused by the ssl_socket_pool_ arguments.

[mmenke, 2017-06-14 18:45:38]

Don't even remember reviewing that CL.

LGTM!

[tbansal1, 2017-06-14 18:46:53]

The CQ bit was checked by tbansal@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-14 18:47:33]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-14 20:13:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-14 20:13:19]

Dry run: This issue passed the CQ dry run.

[tbansal1, 2017-06-14 20:15:02]

The CQ bit was checked by tbansal@chromium.org

[commit-bot: I haz the power, 2017-06-14 20:15:23]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-14 20:19:13]

CQ is committing da patch.

Bot data: {"patchset_id": 240001, "attempt_start_ts": 1497471302189740,
"parent_rev": "a67fad5ca2650a022181a7d06f502a94aa6f09f1", "commit_rev":
"0394a689e2f1a1dad9b7512b8d87f0fdc3ea87d1"}

[commit-bot: I haz the power, 2017-06-14 20:19:27]

Description was changed from

==========
Determine proxy connection timeout based on current network quality

When the session is part of NetAdaptiveProxyConnectionTimeout field
trial, the proxy connection timeout is determined based on the network
quality and the field trial parameters.

BUG=704339
==========

to

==========
Determine proxy connection timeout based on current network quality

When the session is part of NetAdaptiveProxyConnectionTimeout field
trial, the proxy connection timeout is determined based on the network
quality and the field trial parameters.

BUG=704339

Review-Url: https://codereview.chromium.org/2932993002
Cr-Commit-Position: refs/heads/master@{#479486}
Committed:
https://chromium.googlesource.com/chromium/src/+/0394a689e2f1a1dad9b7512b8d87...
==========

[commit-bot: I haz the power, 2017-06-14 20:19:29]

Committed patchset #2 (id:240001) as
https://chromium.googlesource.com/chromium/src/+/0394a689e2f1a1dad9b7512b8d87...