[net] Add Net.UnspecResolvedIPv6 to measure if getaddrinfo resolves IPv6 addresses.

[net] Add Net.UnspecResolvedIPv6 to measure if getaddrinfo resolves IPv6 addresses.

Assuming that "www.google.com" has IPv6 addresses, count the "network sessions"
(in between IP address changes) during which we asked getaddrinfo for AF_UNSPEC family
but have not received any IPv6 addresses. This will tells us how many of the times
Net.IPv6Status = IPV6_GLOBAL_ADDRESS_PRESENT is wrong.

BUG=171549
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=179374

[szym, 2013-01-24 01:29:24]

Hard-coding a domain name is normally not allowed, but I ask for exception for
this measurement.

[szym, 2013-01-28 18:21:20]

ping

[szym, 2013-01-28 21:08:17]

Adding Matt for backup review, in case Chris becomes unavailable.

[mmenke, 2013-01-28 21:09:35]

On 2013/01/28 21:08:17, szym wrote:
> Adding Matt for backup review, in case Chris becomes unavailable.

I'll try and get to it tomorrow.

[mmenke, 2013-01-29 16:10:27]

https://codereview.chromium.org/12051052/diff/1/net/base/host_resolver_impl.cc
File net/base/host_resolver_impl.cc (right):

https://codereview.chromium.org/12051052/diff/1/net/base/host_resolver_impl.c...
net/base/host_resolver_impl.cc:1411: resolver_->resolved_known_ipv6_hostname_ =
true;
This seems like the wrong way to track this.  I think we only want this to run
once per DNS config (Or maybe even only once per HostResolver).

With this code, if we think a client can look up IPv6 addresses and it can't, we
log a histogram entry every time we look up www.google.com.  On the other hand,
if a client can look up IPv6 addresses, then we only log one entry until the
config changes.

https://codereview.chromium.org/12051052/diff/1/net/base/host_resolver_impl.c...
net/base/host_resolver_impl.cc:1411: resolver_->resolved_known_ipv6_hostname_ =
true;
If there's an error, should we even be logging a histogram?

[szym, 2013-01-29 16:55:12]

https://codereview.chromium.org/12051052/diff/1/net/base/host_resolver_impl.cc
File net/base/host_resolver_impl.cc (right):

https://codereview.chromium.org/12051052/diff/1/net/base/host_resolver_impl.c...
net/base/host_resolver_impl.cc:1411: resolver_->resolved_known_ipv6_hostname_ =
true;
On 2013/01/29 16:10:27, Matt Menke wrote:
> If there's an error, should we even be logging a histogram?

Good catch. I previously had this code in HostResolverImpl::CacheResult (no
errors), but forgot about the errors when I moved it here.

Done.

https://codereview.chromium.org/12051052/diff/1/net/base/host_resolver_impl.c...
net/base/host_resolver_impl.cc:1411: resolver_->resolved_known_ipv6_hostname_ =
true;
On 2013/01/29 16:10:27, Matt Menke wrote:
> This seems like the wrong way to track this.  I think we only want this to run
> once per DNS config (Or maybe even only once per HostResolver).

We want it once per "network configuration". If we did it once per HostResolver
we would miss all the users who were disconnected when Chrome started.

> With this code, if we think a client can look up IPv6 addresses and it can't,
we
> log a histogram entry every time we look up http://www.google.com.  On the
other hand,
> if a client can look up IPv6 addresses, then we only log one entry until the
> config changes.

Once we look up www.google.com, we set |resolved_known_ipv6_hostname_ = true|
and therefore we won't log again. That is until the next OnIPAddressChanged when
we reset it to false.

The reason why I do this in OnIPAddressChanged rather than OnDNSChanged is that
we really want to test what getaddrinfo does when IPv6ProbeJob thinks we have
IPv6. This should be independent of the DNS configuration.

[mmenke, 2013-01-29 17:00:46]

LGTM

https://codereview.chromium.org/12051052/diff/1/net/base/host_resolver_impl.cc
File net/base/host_resolver_impl.cc (right):

https://codereview.chromium.org/12051052/diff/1/net/base/host_resolver_impl.c...
net/base/host_resolver_impl.cc:1411: resolver_->resolved_known_ipv6_hostname_ =
true;
On 2013/01/29 16:55:12, szym wrote:
> On 2013/01/29 16:10:27, Matt Menke wrote:
> > This seems like the wrong way to track this.  I think we only want this to
run
> > once per DNS config (Or maybe even only once per HostResolver).
> 
> We want it once per "network configuration". If we did it once per
HostResolver
> we would miss all the users who were disconnected when Chrome started.

Good point, though that's at the cost of overrepresenting users who change
networks a lot (Which may be a good thing, actually), or who have false IP
address changed events trigger every second (hopefully rare).

> > With this code, if we think a client can look up IPv6 addresses and it
can't,
> we
> > log a histogram entry every time we look up http://www.google.com.  On the
> other hand,
> > if a client can look up IPv6 addresses, then we only log one entry until the
> > config changes.
> 
> Once we look up http://www.google.com, we set |resolved_known_ipv6_hostname_ =
true|
> and therefore we won't log again. That is until the next OnIPAddressChanged
when
> we reset it to false.
> 
> The reason why I do this in OnIPAddressChanged rather than OnDNSChanged is
that
> we really want to test what getaddrinfo does when IPv6ProbeJob thinks we have
> IPv6. This should be independent of the DNS configuration.

Sorry, was misreading the code.  For some reason, was conflating setting
got_ipv6_address to true with setting resolved_known_ipv6_hostname_ to true.

[commit-bot: I haz the power, 2013-01-29 17:10:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/szym@chromium.org/12051052/10001

[szym, 2013-01-29 17:10:25]

Thanks for review.

[commit-bot: I haz the power, 2013-01-29 17:50:20]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-01-29 18:04:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/szym@chromium.org/12051052/10001

[commit-bot: I haz the power, 2013-01-29 18:45:02]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.