[net/dns] Instrument DnsConfigService to measure performance and failures.

[net/dns] Instrument DnsConfigService to measure performance and failures.

BUG=125599
TEST=chrome://histograms

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

[szym, 2012-06-15 19:25:27]

PTAL.

Note that because those measurements are per-event, we really need a way to
asses how many users are seeing those events.

[cbentzel, 2012-06-15 20:29:36]

Some quick thoughts, will review the Win portion soon.

http://codereview.chromium.org/10543168/diff/10009/net/base/host_resolver_imp...
File net/base/host_resolver_impl.cc (right):

http://codereview.chromium.org/10543168/diff/10009/net/base/host_resolver_imp...
net/base/host_resolver_impl.cc:1358:
UMA_HISTOGRAM_BOOLEAN("AsyncDNS.HaveDnsConfig",
Does this require any changes for the dashboards? If so, would it be better to
keep it as the enumeration.

http://codereview.chromium.org/10543168/diff/16001/net/dns/dns_config_service.cc
File net/dns/dns_config_service.cc (right):

http://codereview.chromium.org/10543168/diff/16001/net/dns/dns_config_service...
net/dns/dns_config_service.cc:110:
UMA_HISTOGRAM_LONG_TIMES("AsyncDNS.ConfigNotifyPeriod",
ConfigNotifyIntervval perhaps?

http://codereview.chromium.org/10543168/diff/16001/net/dns/dns_config_service...
File net/dns/dns_config_service_posix.cc (right):

http://codereview.chromium.org/10543168/diff/16001/net/dns/dns_config_service...
net/dns/dns_config_service_posix.cc:32: #if !defined(OS_ANDROID)
Doesn't seem like the #if !defined(OS_ANDROID) is needed here, although
exercising it might be.

http://codereview.chromium.org/10543168/diff/16001/net/dns/dns_config_service...
net/dns/dns_config_service_posix.cc:33: enum ConfigParseStatus {
Normally I wouldn't like the ideas of different enums across platforms, but it
seems to make sense here.

http://codereview.chromium.org/10543168/diff/16001/net/dns/dns_config_service...
net/dns/dns_config_service_posix.cc:34: CONFIG_PARSE_OK = 0,
You should make the enum prefixed CONFIG_PARSE_POSIX, though, since
CONFIG_PARSE_BAD_ADDRESS and some others show up on both this list and Win.
Searching may be more of a pain this way.

http://codereview.chromium.org/10543168/diff/16001/net/dns/dns_config_service...
net/dns/dns_config_service_posix.cc:46: static void
ConfigParseResult(ConfigParseStatus result) {
Nit: Should start with a verb, such as RecordConfigParseResultDetails

[szym, 2012-06-15 21:22:19]

http://codereview.chromium.org/10543168/diff/10009/net/base/host_resolver_imp...
File net/base/host_resolver_impl.cc (right):

http://codereview.chromium.org/10543168/diff/10009/net/base/host_resolver_imp...
net/base/host_resolver_impl.cc:1358:
UMA_HISTOGRAM_BOOLEAN("AsyncDNS.HaveDnsConfig",
On 2012/06/15 20:29:36, cbentzel wrote:
> Does this require any changes for the dashboards? If so, would it be better to
> keep it as the enumeration.

From my understanding of histogram.{h,cc} this has _exactly_ the same behavior.
The dashboard already uses BooleanSuccess enumeration.

http://codereview.chromium.org/10543168/diff/16001/net/dns/dns_config_service.cc
File net/dns/dns_config_service.cc (right):

http://codereview.chromium.org/10543168/diff/16001/net/dns/dns_config_service...
net/dns/dns_config_service.cc:110:
UMA_HISTOGRAM_LONG_TIMES("AsyncDNS.ConfigNotifyPeriod",
On 2012/06/15 20:29:36, cbentzel wrote:
> ConfigNotifyIntervval perhaps?

Done.

http://codereview.chromium.org/10543168/diff/16001/net/dns/dns_config_service...
File net/dns/dns_config_service_posix.cc (right):

http://codereview.chromium.org/10543168/diff/16001/net/dns/dns_config_service...
net/dns/dns_config_service_posix.cc:32: #if !defined(OS_ANDROID)
On 2012/06/15 20:29:36, cbentzel wrote:
> Doesn't seem like the #if !defined(OS_ANDROID) is needed here, although
> exercising it might be.

Android compile job fails with "error: 'void
net::<unnamed>::ConfigParseResult(net::<unnamed>::ConfigParseStatus)' defined
but not used"

In Patch Set 4, I propose excluding dns_config_service_posix.* from android
altogether. The dns_config_service_posix_unittest.cc is already excluded and the
general libresolv approach will most likely not be available on Android. The
"resolv.h" header in the NDK has only an extern declaration of __res_state, and
it seems the general philosophy is that resolver should not be directly
available to user programs.

http://codereview.chromium.org/10543168/diff/16001/net/dns/dns_config_service...
net/dns/dns_config_service_posix.cc:34: CONFIG_PARSE_OK = 0,
On 2012/06/15 20:29:36, cbentzel wrote:
> You should make the enum prefixed CONFIG_PARSE_POSIX, though, since
> CONFIG_PARSE_BAD_ADDRESS and some others show up on both this list and Win.
> Searching may be more of a pain this way.

Done.

http://codereview.chromium.org/10543168/diff/16001/net/dns/dns_config_service...
net/dns/dns_config_service_posix.cc:46: static void
ConfigParseResult(ConfigParseStatus result) {
On 2012/06/15 20:29:36, cbentzel wrote:
> Nit: Should start with a verb, such as RecordConfigParseResultDetails

Done.

[szym, 2012-06-15 23:48:17]

This still needs some work. A potential problem with the measurement of
AsyncDNS.ConfigChange is that if a user disconnects from a network and
reconnects to the same one, we record this as "no-change". However, the
measurement should distinguish the cases when there is a spurious signal which
leads to unnecessary withdrawal config.

[cbentzel, 2012-06-16 18:03:40]

LGTM

http://codereview.chromium.org/10543168/diff/29003/net/dns/dns_config_service...
File net/dns/dns_config_service_win.cc (right):

http://codereview.chromium.org/10543168/diff/29003/net/dns/dns_config_service...
net/dns/dns_config_service_win.cc:243: bool AddLocalhostEntries(DnsHosts*
phosts) {
Nit: We don't tend to use hungarian-style notation (phosts) - but I think this
is OK so the reference can be |hosts|.

http://codereview.chromium.org/10543168/diff/29003/net/dns/dns_config_service...
net/dns/dns_config_service_win.cc:264: LOG(ERROR) << "Failed to read local
computer name";
Do you need the LOG(ERROR) now that we have the histogram?

[mmenke, 2012-06-18 17:19:50]

http://codereview.chromium.org/10543168/diff/29003/net/dns/dns_config_service.h
File net/dns/dns_config_service.h (right):

http://codereview.chromium.org/10543168/diff/29003/net/dns/dns_config_service...
net/dns/dns_config_service.h:142: base::TimeTicks last_invalidate_hosts_time_;
This it's worth a comment that these will be set when the config and hosts files
are first read.

http://codereview.chromium.org/10543168/diff/29003/net/dns/dns_config_service...
File net/dns/dns_config_service_posix.cc (right):

http://codereview.chromium.org/10543168/diff/29003/net/dns/dns_config_service...
net/dns/dns_config_service_posix.cc:86:
UMA_HISTOGRAM_BOOLEAN("AsyncDNS.ConfigParseResult", success_);
Do these give us anything that AsyncDNS.ConfigParsePosix does not?

http://codereview.chromium.org/10543168/diff/29003/net/dns/dns_config_service...
net/dns/dns_config_service_posix.cc:186: DnsConfig* dns_config) {
Optional:  I'd suggest making this return a ConfigParsePosixResult.  The
advantage of this is it guarantees every leaf logs a result, and it lets you do
all the histogram recording in one place.

Downside is you'd have to expose the enum in the header, or write a wrapper
function, though the former would let unit tests check the exact failure reason.

http://codereview.chromium.org/10543168/diff/29003/net/dns/dns_config_service...
File net/dns/dns_config_service_posix_unittest.cc (right):

http://codereview.chromium.org/10543168/diff/29003/net/dns/dns_config_service...
net/dns/dns_config_service_posix_unittest.cc:33: res->options = RES_INIT |
RES_DEFAULT | RES_ROTATE;
Should RES_DEFAULT be manually expanded?  I assume the purpose of RES_DEFAULT is
for people compiling themselves to modify RES_DEFAULT, and we presumably don't
want the unit tests to fail on such systems.

http://codereview.chromium.org/10543168/diff/29003/net/dns/dns_config_service...
net/dns/dns_config_service_posix_unittest.cc:127: res.options = RES_INIT |
RES_DEFAULT;
Same comment as above.

http://codereview.chromium.org/10543168/diff/29003/net/dns/dns_config_service...
File net/dns/dns_config_service_win.cc (right):

http://codereview.chromium.org/10543168/diff/29003/net/dns/dns_config_service...
net/dns/dns_config_service_win.cc:56: CONFIG_PARSE_WIN_NO_NAMESERVERS,
You should name these consistently - Currently some "Results" are what the
problem was, while others are the step that failed.  Should use the same naming
convention for them all.

http://codereview.chromium.org/10543168/diff/29003/net/dns/dns_config_service...
net/dns/dns_config_service_win.cc:70: HOSTS_PARSE_WIN_BAD_ADDRESS,
See above comment.

http://codereview.chromium.org/10543168/diff/29003/net/dns/dns_config_service...
net/dns/dns_config_service_win.cc:179: bool
ReadSystemSettings(DnsSystemSettings* settings) {
Same suggestion as for the posix version - return a ConfigParseWinResult so you
don't have to ensure something is logged just before every return statement.

http://codereview.chromium.org/10543168/diff/29003/net/dns/dns_config_service...
net/dns/dns_config_service_win.cc:243: bool AddLocalhostEntries(DnsHosts*
phosts) {
On 2012/06/16 18:03:41, cbentzel wrote:
> Nit: We don't tend to use hungarian-style notation (phosts) - but I think this
> is OK so the reference can be |hosts|.

Why not just make the function take in DnsHosts& hosts?

http://codereview.chromium.org/10543168/diff/29003/net/dns/dns_config_service...
net/dns/dns_config_service_win.cc:243: bool AddLocalhostEntries(DnsHosts*
phosts) {
Again, suggest having this return a HostsParseWinResult.

[cbentzel, 2012-06-18 18:37:17]

http://codereview.chromium.org/10543168/diff/29003/net/dns/dns_config_service...
File net/dns/dns_config_service_win.cc (right):

http://codereview.chromium.org/10543168/diff/29003/net/dns/dns_config_service...
net/dns/dns_config_service_win.cc:243: bool AddLocalhostEntries(DnsHosts*
phosts) {
On 2012/06/18 17:19:50, Matt Menke wrote:
> On 2012/06/16 18:03:41, cbentzel wrote:
> > Nit: We don't tend to use hungarian-style notation (phosts) - but I think
this
> > is OK so the reference can be |hosts|.
> 
> Why not just make the function take in DnsHosts& hosts?

Style-guide doesn't allow non-const references.

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Reference_Argu...

[mmenke, 2012-06-18 18:46:26]

http://codereview.chromium.org/10543168/diff/29003/net/dns/dns_config_service...
File net/dns/dns_config_service_win.cc (right):

http://codereview.chromium.org/10543168/diff/29003/net/dns/dns_config_service...
net/dns/dns_config_service_win.cc:243: bool AddLocalhostEntries(DnsHosts*
phosts) {
On 2012/06/18 18:37:17, cbentzel wrote:
> On 2012/06/18 17:19:50, Matt Menke wrote:
> > On 2012/06/16 18:03:41, cbentzel wrote:
> > > Nit: We don't tend to use hungarian-style notation (phosts) - but I think
> this
> > > is OK so the reference can be |hosts|.
> > 
> > Why not just make the function take in DnsHosts& hosts?
> 
> Style-guide doesn't allow non-const references.
> 
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Reference_Argu...

Ahh, right, good point.  Doing a quick search through net, I can't find this
pattern anywhere, except for a single instance in dns_hosts.h, so perhaps we
should just use it as a pointer everywhere.

[szym, 2012-06-18 20:53:08]

Thanks for the review. I applied suggested changes and added Unchanged*Interval,
which is measured from the last time config was withdrawn (time-out) to the time
a non-change update is received.

This will allow to identify non-change updates which occur long after the config
was last withdrawn. For example, a laptop which reassociates to the same WLAN as
before will not see a change when looking at |dns_config_|, but the time since
last withdrawal will be significant.

PTAL.

http://codereview.chromium.org/10543168/diff/29003/net/dns/dns_config_service...
File net/dns/dns_config_service_posix.cc (right):

http://codereview.chromium.org/10543168/diff/29003/net/dns/dns_config_service...
net/dns/dns_config_service_posix.cc:86:
UMA_HISTOGRAM_BOOLEAN("AsyncDNS.ConfigParseResult", success_);
On 2012/06/18 17:19:50, Matt Menke wrote:
> Do these give us anything that AsyncDNS.ConfigParsePosix does not?

They would share the name across platforms. Not sure if this benefit is worth an
extra histogram.

http://codereview.chromium.org/10543168/diff/29003/net/dns/dns_config_service...
File net/dns/dns_config_service_win.cc (right):

http://codereview.chromium.org/10543168/diff/29003/net/dns/dns_config_service...
net/dns/dns_config_service_win.cc:56: CONFIG_PARSE_WIN_NO_NAMESERVERS,
On 2012/06/18 17:19:50, Matt Menke wrote:
> You should name these consistently - Currently some "Results" are what the
> problem was, while others are the step that failed.  Should use the same
naming
> convention for them all.

The naming scheme is meant to describe the problem with obtaining the config.
"READ_" denotes problems occurring in ReadSystemSettings.

[mmenke, 2012-06-18 20:55:56]

I'll give it another pass tonight or tomorrow morning.

http://codereview.chromium.org/10543168/diff/29003/net/dns/dns_config_service...
File net/dns/dns_config_service_posix.cc (right):

http://codereview.chromium.org/10543168/diff/29003/net/dns/dns_config_service...
net/dns/dns_config_service_posix.cc:86:
UMA_HISTOGRAM_BOOLEAN("AsyncDNS.ConfigParseResult", success_);
On 2012/06/18 20:53:08, szym wrote:
> On 2012/06/18 17:19:50, Matt Menke wrote:
> > Do these give us anything that AsyncDNS.ConfigParsePosix does not?
> 
> They would share the name across platforms. Not sure if this benefit is worth
an
> extra histogram.

They wouldn't, though, since the windows one is called
"AsyncDNS.ConfigParseWinResult" (Notice the Win).

[szym, 2012-06-18 20:59:55]

http://codereview.chromium.org/10543168/diff/29003/net/dns/dns_config_service...
File net/dns/dns_config_service_posix.cc (right):

http://codereview.chromium.org/10543168/diff/29003/net/dns/dns_config_service...
net/dns/dns_config_service_posix.cc:86:
UMA_HISTOGRAM_BOOLEAN("AsyncDNS.ConfigParseResult", success_);
On 2012/06/18 20:55:56, Matt Menke wrote:
> On 2012/06/18 20:53:08, szym wrote:
> > On 2012/06/18 17:19:50, Matt Menke wrote:
> > > Do these give us anything that AsyncDNS.ConfigParsePosix does not?
> > 
> > They would share the name across platforms. Not sure if this benefit is
worth
> an
> > extra histogram.
> 
> They wouldn't, though, since the windows one is called
> "AsyncDNS.ConfigParseWinResult" (Notice the Win).

That was my mistake. Notice that initially both read "ConfigParseResult". I have
no strong feeling about it though, since the data can be retrieved from the
platform-specific histograms.

[mmenke, 2012-06-19 14:49:15]

LGTM

http://codereview.chromium.org/10543168/diff/32013/net/dns/dns_config_service.h
File net/dns/dns_config_service.h (right):

http://codereview.chromium.org/10543168/diff/32013/net/dns/dns_config_service...
net/dns/dns_config_service.h:106: friend class DnsHostsReader;
While you're here, might as well remove this as well.

http://codereview.chromium.org/10543168/diff/32013/net/dns/dns_config_service...
File net/dns/dns_config_service_win.cc (right):

http://codereview.chromium.org/10543168/diff/32013/net/dns/dns_config_service...
net/dns/dns_config_service_win.cc:493: // and observes changes to local IP
address.
Might as well fix this comment, too, while you're here.

http://codereview.chromium.org/10543168/diff/32013/net/dns/dns_config_service...
File net/dns/dns_config_service_win_unittest.cc (right):

http://codereview.chromium.org/10543168/diff/32013/net/dns/dns_config_service...
net/dns/dns_config_service_win_unittest.cc:193: : internal::CONFIG_PARSE_WIN_OK;
optional nit:  Might be easier to read this if you lined up the colon with the
question mark.

[commit-bot: I haz the power, 2012-06-19 18:52:26]

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

[commit-bot: I haz the power, 2012-06-19 20:48:26]

Failed to apply patch for net/dns/dns_config_service.cc:
While running patch -p1 --forward --force;
patching file net/dns/dns_config_service.cc
Hunk #1 FAILED at 5.
Hunk #2 succeeded at 152 (offset 48 lines).
Hunk #3 succeeded at 166 (offset 48 lines).
Hunk #4 succeeded at 182 (offset 48 lines).
Hunk #5 succeeded at 202 (offset 48 lines).
Hunk #6 succeeded at 252 (offset 48 lines).
1 out of 6 hunks FAILED -- saving rejects to file
net/dns/dns_config_service.cc.rej