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

net/dns/dns_config_service_posix.cc

Index: net/dns/dns_config_service_posix.cc
diff --git a/net/dns/dns_config_service_posix.cc b/net/dns/dns_config_service_posix.cc
index 5daaf69aac7b1c1357d08ce7b099a83eac75c56f..4cf5bf9a250d03231930c078050f41d63d2153f6 100644
--- a/net/dns/dns_config_service_posix.cc
+++ b/net/dns/dns_config_service_posix.cc
@@ -11,6 +11,8 @@
 #include "base/file_path.h"
 #include "base/file_util.h"
 #include "base/memory/scoped_ptr.h"
+#include "base/metrics/histogram.h"
+#include "base/time.h"
 #include "net/base/ip_endpoint.h"
 #include "net/base/net_util.h"
 #include "net/dns/dns_hosts.h"
@@ -27,6 +29,26 @@ namespace {
 
 const FilePath::CharType* kFilePathHosts = FILE_PATH_LITERAL("/etc/hosts");
 
+#if !defined(OS_ANDROID)

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

Doesn't seem like the #if !defined(OS_ANDROID) is needed here, although
exercising it might be.

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

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.

+enum ConfigParseStatus {

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

Normally I wouldn't like the ideas of different enums across platforms, but it
seems to make sense here.

+  CONFIG_PARSE_OK = 0,

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

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.

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

Done.

+  CONFIG_PARSE_RES_INIT_FAILED,
+  CONFIG_PARSE_RES_INIT_UNSET,
+  CONFIG_PARSE_BAD_ADDRESS,
+  CONFIG_PARSE_BAD_EXT_STRUCT,
+  CONFIG_PARSE_NULL_ADDRESS,
+  CONFIG_PARSE_NO_NAMESERVERS,
+  CONFIG_PARSE_MISSING_OPTIONS,
+  CONFIG_PARSE_UNHANDLED_OPTIONS,
+  CONFIG_PARSE_MAX  // Bounding values for enumeration.
+};
+
+static void ConfigParseResult(ConfigParseStatus result) {

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

Nit: Should start with a verb, such as RecordConfigParseResultDetails

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

Done.

+  UMA_HISTOGRAM_ENUMERATION("AsyncDNS.ConfigParsePosix",
+                            result, CONFIG_PARSE_MAX);
+}
+#endif  // !defined(OS_ANDROID)
+
 // A SerialWorker that uses libresolv to initialize res_state and converts
 // it to DnsConfig.
 class ConfigReader : public SerialWorker {
@@ -36,6 +58,7 @@ class ConfigReader : public SerialWorker {
       : callback_(callback), success_(false) {}
 
   void DoWork() OVERRIDE {
+    base::TimeTicks start_time = base::TimeTicks::Now();
     success_ = false;
 #if defined(OS_ANDROID)
     NOTIMPLEMENTED();
@@ -43,14 +66,19 @@ class ConfigReader : public SerialWorker {
     // Note: res_ninit in glibc always returns 0 and sets RES_INIT.
     // res_init behaves the same way.
     memset(&_res, 0, sizeof(_res));
-    if ((res_init() == 0) && (_res.options & RES_INIT))
+    if (res_init() == 0) {
       success_ = internal::ConvertResStateToDnsConfig(_res, &dns_config_);
+    } else {
+      ConfigParseResult(CONFIG_PARSE_RES_INIT_FAILED);
+    }
 #else  // all other OS_POSIX
     struct __res_state res;
     memset(&res, 0, sizeof(res));
-    if ((res_ninit(&res) == 0) && (res.options & RES_INIT))
+    if (res_ninit(&res) == 0) {
       success_ = internal::ConvertResStateToDnsConfig(res, &dns_config_);
-
+    } else {
+      ConfigParseResult(CONFIG_PARSE_RES_INIT_FAILED);
+    }
     // Prefer res_ndestroy where available.
 #if defined(OS_MACOSX) || defined(OS_FREEBSD)
     res_ndestroy(&res);
@@ -58,6 +86,9 @@ class ConfigReader : public SerialWorker {
     res_nclose(&res);
 #endif
 #endif
+    UMA_HISTOGRAM_BOOLEAN("AsyncDNS.ConfigParseResult", success_);
+    UMA_HISTOGRAM_TIMES("AsyncDNS.ConfigParseDuration",
+                        base::TimeTicks::Now() - start_time);
   }
 
   void OnWorkFinished() OVERRIDE {
@@ -91,7 +122,11 @@ class HostsReader : public SerialWorker {
   virtual ~HostsReader() {}
 
   virtual void DoWork() OVERRIDE {
+    base::TimeTicks start_time = base::TimeTicks::Now();
     success_ = ParseHostsFile(path_, &hosts_);
+    UMA_HISTOGRAM_BOOLEAN("AsyncDNS.HostParseResult", success_);
+    UMA_HISTOGRAM_TIMES("AsyncDNS.HostsParseDuration",
+                        base::TimeTicks::Now() - start_time);
   }
 
   virtual void OnWorkFinished() OVERRIDE {
@@ -154,7 +189,10 @@ void DnsConfigServicePosix::OnDNSChanged(unsigned detail) {
 bool ConvertResStateToDnsConfig(const struct __res_state& res,
                                 DnsConfig* dns_config) {
   CHECK(dns_config != NULL);
-  DCHECK(res.options & RES_INIT);
+  if (!(res.options & RES_INIT)) {
+    ConfigParseResult(CONFIG_PARSE_RES_INIT_UNSET);
+    return false;
+  }
 
   dns_config->nameservers.clear();
 
@@ -168,6 +206,7 @@ bool ConvertResStateToDnsConfig(const struct __res_state& res,
     if (!ipe.FromSockAddr(
             reinterpret_cast<const struct sockaddr*>(&addresses[i]),
             sizeof addresses[i])) {
+      ConfigParseResult(CONFIG_PARSE_BAD_ADDRESS);
       return false;
     }
     dns_config->nameservers.push_back(ipe);
@@ -191,10 +230,13 @@ bool ConvertResStateToDnsConfig(const struct __res_state& res,
       addr = reinterpret_cast<const struct sockaddr*>(res._u._ext.nsaddrs[i]);
       addr_len = sizeof *res._u._ext.nsaddrs[i];
     } else {
+      ConfigParseResult(CONFIG_PARSE_BAD_EXT_STRUCT);
       return false;
     }
-    if (!ipe.FromSockAddr(addr, addr_len))
+    if (!ipe.FromSockAddr(addr, addr_len)) {
+      ConfigParseResult(CONFIG_PARSE_BAD_ADDRESS);
       return false;
+    }
     dns_config->nameservers.push_back(ipe);
   }
 #else  // !(defined(OS_LINUX) || defined(OS_MACOSX) || defined(OS_FREEBSD))
@@ -204,6 +246,7 @@ bool ConvertResStateToDnsConfig(const struct __res_state& res,
     if (!ipe.FromSockAddr(
             reinterpret_cast<const struct sockaddr*>(&res.nsaddr_list[i]),
             sizeof res.nsaddr_list[i])) {
+      ConfigParseResult(CONFIG_PARSE_BAD_ADDRESS);
       return false;
     }
     dns_config->nameservers.push_back(ipe);
@@ -223,12 +266,35 @@ bool ConvertResStateToDnsConfig(const struct __res_state& res,
 #endif
   dns_config->edns0 = res.options & RES_USE_EDNS0;
 
+  // The current implementation assumes these options are set. They normally
+  // cannot be overwritten by /etc/resolv.conf
+  unsigned kRequiredOptions = RES_RECURSE | RES_DEFNAMES | RES_DNSRCH;
+  if ((res.options & kRequiredOptions) != kRequiredOptions) {
+    ConfigParseResult(CONFIG_PARSE_MISSING_OPTIONS);
+    return false;
+  }
+
+  unsigned kUnhandledOptions = RES_USEVC | RES_IGNTC | RES_USE_DNSSEC;
+  if (res.options & kUnhandledOptions) {
+    ConfigParseResult(CONFIG_PARSE_UNHANDLED_OPTIONS);
+    return false;
+  }
+
+  if (dns_config->nameservers.empty()) {
+    ConfigParseResult(CONFIG_PARSE_NO_NAMESERVERS);
+    return false;
+  }
+
   // If any name server is 0.0.0.0, assume the configuration is invalid.
   // TODO(szym): Measure how often this happens. http://crbug.com/125599
   const IPAddressNumber kEmptyAddress(kIPv4AddressSize);
-  for (unsigned i = 0; i < dns_config->nameservers.size(); ++i)
-    if (dns_config->nameservers[i].address() == kEmptyAddress)
+  for (unsigned i = 0; i < dns_config->nameservers.size(); ++i) {
+    if (dns_config->nameservers[i].address() == kEmptyAddress) {
+      ConfigParseResult(CONFIG_PARSE_NULL_ADDRESS);
       return false;
+    }
+  }
+  ConfigParseResult(CONFIG_PARSE_OK);
   return true;
 }
 #endif  // !defined(OS_ANDROID)