DNS: Per-network-type and Finch-variable timeouts

DNS: Per-network-type and Finch-variable timeouts

Right now, our DNS timeouts are fixed (6 seconds between calls to thehost resolver, 1 second default retransmission timeout for the internalresolver, and 5 second maximum even with exponential backoff). Thisworks okay on some network types, but could be a lot more patient on
some types (e.g. 2G cell networks).

This CL makes these timeouts configurable via field trials, after which
I willexperiment and find optimal values for each network type, and then
bake in some per-type defaults.

Note that this modifies the behavior of the async resolver in some
cases: if the right field trial is enabled, it will reset accumulated
RTT stats whenever the network *type* changes, not just when the IP
address changes (i.e. the device switches networks entirely).

BUG=595352
Committed: https://crrev.com/cf1158bf589956c2d20180de6feed0c8334db8a0
Cr-Commit-Position: refs/heads/master@{#381991}

[Randy Smith (Not in Mondays), 2016-03-15 20:12:30]

rdsmith@chromium.org changed reviewers:
+ rdsmith@chromium.org

[Randy Smith (Not in Mondays), 2016-03-15 20:12:30]

Ok, a couple of high level questions (and nits I happened to notice when
wandering through the code to get my high level overview):
* I think it's appropriate for adding field trials to have a bug number?

* I presume that Variations aren't a good way to do this?  I know that the UMA
folks gently frown on parsing group names (only gently--I've done it--but I want
to make sure you explored the other alternatives before going this way.  Or
maybe I should just say "Make sure you get a UMA/field trial person to review
this as well".)

* What's the story with eliminating the field trial utility function for NACL? 
(This is a question based on my ignorance; when is NACL compiled, and what
libraries does it have in it?)

https://codereview.chromium.org/1778933002/diff/140001/net/dns/dns_util.cc
File net/dns/dns_util.cc (right):

https://codereview.chromium.org/1778933002/diff/140001/net/dns/dns_util.cc#ne...
net/dns/dns_util.cc:163: NetworkChangeNotifier::ConnectionType type_int,
I mildly object to the naming; other variables of this type are just named
"type", and type"int is in relation to the unusual thing done below (the size_t
version), so I'd rather see that unusual variation named something unusual.

https://codereview.chromium.org/1778933002/diff/140001/net/dns/dns_util.cc#ne...
net/dns/dns_util.cc:172: return false;
Will this catch the case when the field trial isn't set at all?  I'd expect
(haven't checked) group to be the null string in that case, and I'd expect
(haven't checked) SplitStringPiece to return a single entry, so I'd expect a
type of 0 to work when it shoudln't.

https://codereview.chromium.org/1778933002/diff/140001/net/tools/gdig/gdig.cc
File net/tools/gdig/gdig.cc (right):

https://codereview.chromium.org/1778933002/diff/140001/net/tools/gdig/gdig.cc...
net/tools/gdig/gdig.cc:80: static_cast<int>(dns_config.timeout.InSeconds()));
Why?

[Deprecated (see juliatuttle), 2016-03-16 16:45:30]

https://codereview.chromium.org/1778933002/diff/140001/net/dns/dns_util.cc
File net/dns/dns_util.cc (right):

https://codereview.chromium.org/1778933002/diff/140001/net/dns/dns_util.cc#ne...
net/dns/dns_util.cc:163: NetworkChangeNotifier::ConnectionType type_int,
On 2016/03/15 20:12:30, Randy Smith - Not in Fridays wrote:
> I mildly object to the naming; other variables of this type are just named
> "type", and type"int is in relation to the unusual thing done below (the
size_t
> version), so I'd rather see that unusual variation named something unusual.

Done.

https://codereview.chromium.org/1778933002/diff/140001/net/dns/dns_util.cc#ne...
net/dns/dns_util.cc:172: return false;
On 2016/03/15 20:12:30, Randy Smith - Not in Fridays wrote:
> Will this catch the case when the field trial isn't set at all?  I'd expect
> (haven't checked) group to be the null string in that case, and I'd expect
> (haven't checked) SplitStringPiece to return a single entry, so I'd expect a
> type of 0 to work when it shoudln't.

Technically, I think StringToInt64 would then fail, but I don't think that's a
proper way to handle it. I'll add a specific check on group at the top.

https://codereview.chromium.org/1778933002/diff/140001/net/tools/gdig/gdig.cc
File net/tools/gdig/gdig.cc (right):

https://codereview.chromium.org/1778933002/diff/140001/net/tools/gdig/gdig.cc...
net/tools/gdig/gdig.cc:80: static_cast<int>(dns_config.timeout.InSeconds()));
On 2016/03/15 20:12:30, Randy Smith - Not in Fridays wrote:
> Why?

This was wrong; see the man page for resolv.conf. timeout is in seconds, not
milliseconds.

[Deprecated (see juliatuttle), 2016-03-16 16:48:02]

PTAL, rdsmith.

On 2016/03/15 20:12:30, Randy Smith - Not in Fridays wrote:
> Ok, a couple of high level questions (and nits I happened to notice when
> wandering through the code to get my high level overview):
> * I think it's appropriate for adding field trials to have a bug number?

Added.

> * I presume that Variations aren't a good way to do this?  I know that the UMA
> folks gently frown on parsing group names (only gently--I've done it--but I
want
> to make sure you explored the other alternatives before going this way.  Or
> maybe I should just say "Make sure you get a UMA/field trial person to review
> this as well".)

Can you point me to an explanation of this? The Finch guide wasn't forthcoming.

> * What's the story with eliminating the field trial utility function for NACL?

> (This is a question based on my ignorance; when is NACL compiled, and what
> libraries does it have in it?)

NaCL uses the functions in dns_util.* for something, but not the rest of
net/dns.

[Deprecated (see juliatuttle), 2016-03-16 16:48:11]

Description was changed from

==========
DNS: Per-network-type and Finch-variable timeouts

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

to

==========
DNS: Per-network-type and Finch-variable timeouts

BUG=595352
==========

[Randy Smith (Not in Mondays), 2016-03-16 19:47:48]

On 2016/03/16 16:45:30, Julia Tuttle (deprecated) wrote:
> https://codereview.chromium.org/1778933002/diff/140001/net/dns/dns_util.cc
> File net/dns/dns_util.cc (right):
> 
>
https://codereview.chromium.org/1778933002/diff/140001/net/dns/dns_util.cc#ne...
> net/dns/dns_util.cc:163: NetworkChangeNotifier::ConnectionType type_int,
> On 2016/03/15 20:12:30, Randy Smith - Not in Fridays wrote:
> > I mildly object to the naming; other variables of this type are just named
> > "type", and type"int is in relation to the unusual thing done below (the
> size_t
> > version), so I'd rather see that unusual variation named something unusual.
> 
> Done.
> 
>
https://codereview.chromium.org/1778933002/diff/140001/net/dns/dns_util.cc#ne...
> net/dns/dns_util.cc:172: return false;
> On 2016/03/15 20:12:30, Randy Smith - Not in Fridays wrote:
> > Will this catch the case when the field trial isn't set at all?  I'd expect
> > (haven't checked) group to be the null string in that case, and I'd expect
> > (haven't checked) SplitStringPiece to return a single entry, so I'd expect a
> > type of 0 to work when it shoudln't.
> 
> Technically, I think StringToInt64 would then fail, but I don't think that's a
> proper way to handle it. I'll add a specific check on group at the top.
> 
> https://codereview.chromium.org/1778933002/diff/140001/net/tools/gdig/gdig.cc
> File net/tools/gdig/gdig.cc (right):
> 
>
https://codereview.chromium.org/1778933002/diff/140001/net/tools/gdig/gdig.cc...
> net/tools/gdig/gdig.cc:80: static_cast<int>(dns_config.timeout.InSeconds()));
> On 2016/03/15 20:12:30, Randy Smith - Not in Fridays wrote:
> > Why?
> 
> This was wrong; see the man page for resolv.conf. timeout is in seconds, not
> milliseconds.

Did you mean to upload a new PS with this message?

[Randy Smith (Not in Mondays), 2016-03-16 19:57:39]

On 2016/03/16 16:48:02, Julia Tuttle (deprecated) wrote:
> PTAL, rdsmith.
> 
> On 2016/03/15 20:12:30, Randy Smith - Not in Fridays wrote:
> > Ok, a couple of high level questions (and nits I happened to notice when
> > wandering through the code to get my high level overview):
> > * I think it's appropriate for adding field trials to have a bug number?
> 
> Added.
> 
> > * I presume that Variations aren't a good way to do this?  I know that the
UMA
> > folks gently frown on parsing group names (only gently--I've done it--but I
> want
> > to make sure you explored the other alternatives before going this way.  Or
> > maybe I should just say "Make sure you get a UMA/field trial person to
review
> > this as well".)
> 
> Can you point me to an explanation of this? The Finch guide wasn't
forthcoming.

The comment at the beginning of
https://chromium.googlesource.com/chromium/src/+/master/components/variations...
is the best I could come up with.  I think the answer is "You're in net/; you
can't use it" but you might want to get someone from finch to take a look at
what you're doing anyway.  (They might want plumbing through the embedder, but I
don't *think* they will?  But I sorta want to give them the option of weighing
in.)

> 
> > * What's the story with eliminating the field trial utility function for
NACL?
> 
> > (This is a question based on my ignorance; when is NACL compiled, and what
> > libraries does it have in it?)
> 
> NaCL uses the functions in dns_util.* for something, but not the rest of
> net/dns.

[Randy Smith (Not in Mondays), 2016-03-16 20:11:07]

This looks basically fine to me, but I'd like to have the CL description say
somewhat more.   Maybe "Allow control of DNS timeouts through finch on a
per-network type basis." as the initial sentence, and then listing the timeouts
that can be controlled in the body?

https://codereview.chromium.org/1778933002/diff/140001/net/dns/dns_session.cc
File net/dns/dns_session.cc (right):

https://codereview.chromium.org/1778933002/diff/140001/net/dns/dns_session.cc...
net/dns/dns_session.cc:45: base::TimeDelta
GetInitialTimeout(NetworkChangeNotifier::ConnectionType type,
Up to you, but from my perspective the contents of this function are simple
enough to hoist into the caller (since it's only called from one place).  Ditto
the next function.

https://codereview.chromium.org/1778933002/diff/140001/net/dns/host_resolver_...
File net/dns/host_resolver_impl.cc (right):

https://codereview.chromium.org/1778933002/diff/140001/net/dns/host_resolver_...
net/dns/host_resolver_impl.cc:494: base::TimeDelta GetUnresponsiveDelay(
Same comment as before (up to you, but simple enough function called from one
place that I think the code's more readable if it's hoisted).

https://codereview.chromium.org/1778933002/diff/140001/net/tools/gdig/gdig.cc
File net/tools/gdig/gdig.cc (right):

https://codereview.chromium.org/1778933002/diff/140001/net/tools/gdig/gdig.cc...
net/tools/gdig/gdig.cc:80: static_cast<int>(dns_config.timeout.InSeconds()));
On 2016/03/16 16:45:30, Julia Tuttle (deprecated) wrote:
> On 2016/03/15 20:12:30, Randy Smith - Not in Fridays wrote:
> > Why?
> 
> This was wrong; see the man page for resolv.conf. timeout is in seconds, not
> milliseconds.

Separate CL or mention in CL description?

[Randy Smith (Not in Mondays), 2016-03-16 20:15:03]

https://codereview.chromium.org/1778933002/diff/140001/net/dns/dns_session.cc
File net/dns/dns_session.cc (right):

https://codereview.chromium.org/1778933002/diff/140001/net/dns/dns_session.cc...
net/dns/dns_session.cc:151: InitializeServerStats();
Actually, on my third (:-}) read-through I realize that this may be a
functionality change, if we're sometimes choosing timeouts from the histogram
data.  Naively, it's a *good* change, but I'm both uncomfortable with
functionality changes that aren't called out in the CL description (in case of
non-OWNERS bisecting looking for a breakage), and I'm tempted to suggest that
they really should be in their own CL.  WDYT?

[Deprecated (see juliatuttle), 2016-03-17 16:29:46]

PTAL, rdsmith.

Would you like me to add a separate field trial for "please do/do not update
timeouts and reset stats within a DnsSession when ConnectionType changes", as we
discussed?

https://codereview.chromium.org/1778933002/diff/140001/net/dns/dns_session.cc
File net/dns/dns_session.cc (right):

https://codereview.chromium.org/1778933002/diff/140001/net/dns/dns_session.cc...
net/dns/dns_session.cc:45: base::TimeDelta
GetInitialTimeout(NetworkChangeNotifier::ConnectionType type,
On 2016/03/16 20:11:06, Randy Smith - Not in Fridays wrote:
> Up to you, but from my perspective the contents of this function are simple
> enough to hoist into the caller (since it's only called from one place). 
Ditto
> the next function.

Done.

https://codereview.chromium.org/1778933002/diff/140001/net/dns/dns_session.cc...
net/dns/dns_session.cc:151: InitializeServerStats();
On 2016/03/16 20:15:02, Randy Smith - Not in Fridays wrote:
> Actually, on my third (:-}) read-through I realize that this may be a
> functionality change, if we're sometimes choosing timeouts from the histogram
> data.  Naively, it's a *good* change, but I'm both uncomfortable with
> functionality changes that aren't called out in the CL description (in case of
> non-OWNERS bisecting looking for a breakage), and I'm tempted to suggest that
> they really should be in their own CL.  WDYT?

This isn't a functionality change unless a field trial provides differing
timeouts -- if there's no field trial, it won't do anything.

What specifically would you want in a different CL?

https://codereview.chromium.org/1778933002/diff/140001/net/dns/host_resolver_...
File net/dns/host_resolver_impl.cc (right):

https://codereview.chromium.org/1778933002/diff/140001/net/dns/host_resolver_...
net/dns/host_resolver_impl.cc:494: base::TimeDelta GetUnresponsiveDelay(
On 2016/03/16 20:11:07, Randy Smith - Not in Fridays wrote:
> Same comment as before (up to you, but simple enough function called from one
> place that I think the code's more readable if it's hoisted).  

Done.

[Deprecated (see juliatuttle), 2016-03-17 17:23:22]

Description was changed from

==========
DNS: Per-network-type and Finch-variable timeouts

BUG=595352
==========

to

==========
DNS: Per-network-type and Finch-variable timeouts

Right now, our DNS timeouts are fixed (6 seconds between calls to thehost
resolver, 1 second default retransmission timeout for the internalresolver, and
5 second maximum even with exponential backoff). Thisworks okay on some network
types, but could be a lot more patient on
some types (e.g. 2G cell networks).

This CL makes these timeouts configurable via field trials, after which 
I willexperiment and find optimal values for each network type, and then
bake in some per-type defaults.

Note that this modifies the behavior of the async resolver in some 
cases: if the right field trial is enabled, it will reset accumulated
RTT stats whenever the network *type* changes, not just when the IP
address changes (i.e. the device switches networks entirely).

BUG=595352
==========

[Randy Smith (Not in Mondays), 2016-03-17 21:52:28]

On 2016/03/16 20:11:07, Randy Smith - Not in Fridays wrote:
> This looks basically fine to me, but I'd like to have the CL description say
> somewhat more.   Maybe "Allow control of DNS timeouts through finch on a
> per-network type basis." as the initial sentence, and then listing the
timeouts
> that can be controlled in the body?
> 
> https://codereview.chromium.org/1778933002/diff/140001/net/dns/dns_session.cc
> File net/dns/dns_session.cc (right):
> 
>
https://codereview.chromium.org/1778933002/diff/140001/net/dns/dns_session.cc...
> net/dns/dns_session.cc:45: base::TimeDelta
> GetInitialTimeout(NetworkChangeNotifier::ConnectionType type,
> Up to you, but from my perspective the contents of this function are simple
> enough to hoist into the caller (since it's only called from one place). 
Ditto
> the next function.
> 
>
https://codereview.chromium.org/1778933002/diff/140001/net/dns/host_resolver_...
> File net/dns/host_resolver_impl.cc (right):
> 
>
https://codereview.chromium.org/1778933002/diff/140001/net/dns/host_resolver_...
> net/dns/host_resolver_impl.cc:494: base::TimeDelta GetUnresponsiveDelay(
> Same comment as before (up to you, but simple enough function called from one
> place that I think the code's more readable if it's hoisted).
> 
> https://codereview.chromium.org/1778933002/diff/140001/net/tools/gdig/gdig.cc
> File net/tools/gdig/gdig.cc (right):
> 
>
https://codereview.chromium.org/1778933002/diff/140001/net/tools/gdig/gdig.cc...
> net/tools/gdig/gdig.cc:80: static_cast<int>(dns_config.timeout.InSeconds()));
> On 2016/03/16 16:45:30, Julia Tuttle (deprecated) wrote:
> > On 2016/03/15 20:12:30, Randy Smith - Not in Fridays wrote:
> > > Why?
> > 
> > This was wrong; see the man page for resolv.conf. timeout is in seconds, not
> > milliseconds.
> 
> Separate CL or mention in CL description?

I don't think you ever responded to this?  At least, this change isn't mentioned
in the CL description, but it looks like it's still in the CL.

[Randy Smith (Not in Mondays), 2016-03-17 21:58:16]

LGTM conditional on mentioning the gdig.cc change in the CL description (or
putting it in another CL; it doesn't seem related to this one), and putting
spaces between the incorrect compound words in the CL description :-}.  The LE
question below is just for my curiosity.

https://codereview.chromium.org/1778933002/diff/140001/net/dns/dns_session.cc
File net/dns/dns_session.cc (right):

https://codereview.chromium.org/1778933002/diff/140001/net/dns/dns_session.cc...
net/dns/dns_session.cc:151: InitializeServerStats();
On 2016/03/17 16:29:46, Julia Tuttle (deprecated) wrote:
> On 2016/03/16 20:15:02, Randy Smith - Not in Fridays wrote:
> > Actually, on my third (:-}) read-through I realize that this may be a
> > functionality change, if we're sometimes choosing timeouts from the
histogram
> > data.  Naively, it's a *good* change, but I'm both uncomfortable with
> > functionality changes that aren't called out in the CL description (in case
of
> > non-OWNERS bisecting looking for a breakage), and I'm tempted to suggest
that
> > they really should be in their own CL.  WDYT?
> 
> This isn't a functionality change unless a field trial provides differing
> timeouts -- if there's no field trial, it won't do anything.
> 
> What specifically would you want in a different CL?

Presuming we resolved this in in-person conversation, but: I'd like to make sure
that the change to reset stats on network changed event is tested separately
from the one where you play with the timeouts, so that any changes in behavior
from the first aren't assumed to be from the second.  You seem to be on track
for doing that, so looks good.

https://codereview.chromium.org/1778933002/diff/220001/net/dns/dns_session_un...
File net/dns/dns_session_unittest.cc (right):

https://codereview.chromium.org/1778933002/diff/220001/net/dns/dns_session_un...
net/dns/dns_session_unittest.cc:226: EXPECT_LE(delta.InMilliseconds(), 10);
I have no objections, but what happened to drive changing the EXPECT to a LE? 
If this is an approximate match and we're really right on the edge, that
suggests that we'll go over at some point do to random variation, no?

[Deprecated (see juliatuttle), 2016-03-18 16:32:14]

The CQ bit was checked by ttuttle@chromium.org

[Deprecated (see juliatuttle), 2016-03-18 16:32:15]

The patchset sent to the CQ was uploaded after l-g-t-m from rdsmith@chromium.org
Link to the patchset: https://codereview.chromium.org/1778933002/#ps240001
(title: "Remove change to gdig.")

[commit-bot: I haz the power, 2016-03-18 16:32:27]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1778933002/240001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1778933002/240001

[commit-bot: I haz the power, 2016-03-18 16:37:54]

Description was changed from

==========
DNS: Per-network-type and Finch-variable timeouts

Right now, our DNS timeouts are fixed (6 seconds between calls to thehost
resolver, 1 second default retransmission timeout for the internalresolver, and
5 second maximum even with exponential backoff). Thisworks okay on some network
types, but could be a lot more patient on
some types (e.g. 2G cell networks).

This CL makes these timeouts configurable via field trials, after which 
I willexperiment and find optimal values for each network type, and then
bake in some per-type defaults.

Note that this modifies the behavior of the async resolver in some 
cases: if the right field trial is enabled, it will reset accumulated
RTT stats whenever the network *type* changes, not just when the IP
address changes (i.e. the device switches networks entirely).

BUG=595352
==========

to

==========
DNS: Per-network-type and Finch-variable timeouts

Right now, our DNS timeouts are fixed (6 seconds between calls to thehost
resolver, 1 second default retransmission timeout for the internalresolver, and
5 second maximum even with exponential backoff). Thisworks okay on some network
types, but could be a lot more patient on
some types (e.g. 2G cell networks).

This CL makes these timeouts configurable via field trials, after which 
I willexperiment and find optimal values for each network type, and then
bake in some per-type defaults.

Note that this modifies the behavior of the async resolver in some 
cases: if the right field trial is enabled, it will reset accumulated
RTT stats whenever the network *type* changes, not just when the IP
address changes (i.e. the device switches networks entirely).

BUG=595352
==========

[commit-bot: I haz the power, 2016-03-18 16:37:55]

Committed patchset #13 (id:240001)

[commit-bot: I haz the power, 2016-03-18 16:40:09]

Description was changed from

==========
DNS: Per-network-type and Finch-variable timeouts

Right now, our DNS timeouts are fixed (6 seconds between calls to thehost
resolver, 1 second default retransmission timeout for the internalresolver, and
5 second maximum even with exponential backoff). Thisworks okay on some network
types, but could be a lot more patient on
some types (e.g. 2G cell networks).

This CL makes these timeouts configurable via field trials, after which 
I willexperiment and find optimal values for each network type, and then
bake in some per-type defaults.

Note that this modifies the behavior of the async resolver in some 
cases: if the right field trial is enabled, it will reset accumulated
RTT stats whenever the network *type* changes, not just when the IP
address changes (i.e. the device switches networks entirely).

BUG=595352
==========

to

==========
DNS: Per-network-type and Finch-variable timeouts

Right now, our DNS timeouts are fixed (6 seconds between calls to thehost
resolver, 1 second default retransmission timeout for the internalresolver, and
5 second maximum even with exponential backoff). Thisworks okay on some network
types, but could be a lot more patient on
some types (e.g. 2G cell networks).

This CL makes these timeouts configurable via field trials, after which
I willexperiment and find optimal values for each network type, and then
bake in some per-type defaults.

Note that this modifies the behavior of the async resolver in some
cases: if the right field trial is enabled, it will reset accumulated
RTT stats whenever the network *type* changes, not just when the IP
address changes (i.e. the device switches networks entirely).

BUG=595352

Committed: https://crrev.com/cf1158bf589956c2d20180de6feed0c8334db8a0
Cr-Commit-Position: refs/heads/master@{#381991}
==========

[commit-bot: I haz the power, 2016-03-18 16:40:11]

Patchset 13 (id:??) landed as
https://crrev.com/cf1158bf589956c2d20180de6feed0c8334db8a0
Cr-Commit-Position: refs/heads/master@{#381991}