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

net/dns/dns_config_service_posix.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "net/dns/dns_config_service_posix.h"
 
 #include <string>
 
 #include "base/basictypes.h"
 #include "base/bind.h"
 #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"
 #include "net/dns/dns_protocol.h"
 #include "net/dns/serial_worker.h"
 
 namespace net {
 
+#if !defined(OS_ANDROID)
 namespace {
 
 #ifndef _PATH_RESCONF  // Normally defined in <resolv.h>
 #define _PATH_RESCONF "/etc/resolv.conf"
 #endif
 
 const FilePath::CharType* kFilePathHosts = FILE_PATH_LITERAL("/etc/hosts");
 
+enum ConfigParsePosixResult {
+  CONFIG_PARSE_POSIX_OK = 0,
+  CONFIG_PARSE_POSIX_RES_INIT_FAILED,
+  CONFIG_PARSE_POSIX_RES_INIT_UNSET,
+  CONFIG_PARSE_POSIX_BAD_ADDRESS,
+  CONFIG_PARSE_POSIX_BAD_EXT_STRUCT,
+  CONFIG_PARSE_POSIX_NULL_ADDRESS,
+  CONFIG_PARSE_POSIX_NO_NAMESERVERS,
+  CONFIG_PARSE_POSIX_MISSING_OPTIONS,
+  CONFIG_PARSE_POSIX_UNHANDLED_OPTIONS,
+  CONFIG_PARSE_POSIX_MAX  // Bounding values for enumeration.
+};
+
+static void RecordConfigParseResult(ConfigParsePosixResult result) {
+  UMA_HISTOGRAM_ENUMERATION("AsyncDNS.ConfigParsePosix",
+                            result, CONFIG_PARSE_POSIX_MAX);
+}
+
 // A SerialWorker that uses libresolv to initialize res_state and converts
 // it to DnsConfig.
 class ConfigReader : public SerialWorker {
  public:
   typedef base::Callback<void(const DnsConfig& config)> CallbackType;
   explicit ConfigReader(const CallbackType& callback)
       : callback_(callback), success_(false) {}
 
   void DoWork() OVERRIDE {
+    base::TimeTicks start_time = base::TimeTicks::Now();
     success_ = false;
-#if defined(OS_ANDROID)
-    NOTIMPLEMENTED();
-#elif defined(OS_OPENBSD)
+#if defined(OS_OPENBSD)
     // 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 {
+      RecordConfigParseResult(CONFIG_PARSE_POSIX_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 {
+      RecordConfigParseResult(CONFIG_PARSE_POSIX_RES_INIT_FAILED);
+    }
     // Prefer res_ndestroy where available.
 #if defined(OS_MACOSX) || defined(OS_FREEBSD)
     res_ndestroy(&res);
 #else
     res_nclose(&res);
 #endif
 #endif
+    UMA_HISTOGRAM_BOOLEAN("AsyncDNS.ConfigParseResult", success_);

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

Do these give us anything that AsyncDNS.ConfigParsePosix does not?

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

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

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

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

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

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.

+    UMA_HISTOGRAM_TIMES("AsyncDNS.ConfigParseDuration",
+                        base::TimeTicks::Now() - start_time);
   }
 
   void OnWorkFinished() OVERRIDE {
     DCHECK(!IsCancelled());
     if (success_) {
       callback_.Run(dns_config_);
     } else {
       LOG(WARNING) << "Failed to read DnsConfig.";
     }
   }
@@ skipping 13 matching lines @@
 class HostsReader : public SerialWorker {
  public:
   typedef base::Callback<void(const DnsHosts& hosts)> CallbackType;
   explicit HostsReader(const CallbackType& callback)
       : path_(kFilePathHosts), callback_(callback), success_(false) {}
 
  private:
   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 {
     if (success_) {
       callback_.Run(hosts_);
     } else {
       LOG(WARNING) << "Failed to read DnsHosts.";
     }
   }
 
@@ skipping 38 matching lines @@
   if (detail & NetworkChangeNotifier::CHANGE_DNS_SETTINGS) {
     InvalidateConfig();
     config_reader_->WorkNow();
   }
   if (detail & NetworkChangeNotifier::CHANGE_DNS_HOSTS) {
     InvalidateHosts();
     hosts_reader_->WorkNow();
   }
 }
 
-#if !defined(OS_ANDROID)
 bool ConvertResStateToDnsConfig(const struct __res_state& res,
                                 DnsConfig* dns_config) {

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

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.

   CHECK(dns_config != NULL);
-  DCHECK(res.options & RES_INIT);
+  if (!(res.options & RES_INIT)) {
+    RecordConfigParseResult(CONFIG_PARSE_POSIX_RES_INIT_UNSET);
+    return false;
+  }
 
   dns_config->nameservers.clear();
 
 #if defined(OS_MACOSX) || defined(OS_FREEBSD)
   union res_sockaddr_union addresses[MAXNS];
   int nscount = res_getservers(const_cast<res_state>(&res), addresses, MAXNS);
   DCHECK_GE(nscount, 0);
   DCHECK_LE(nscount, MAXNS);
   for (int i = 0; i < nscount; ++i) {
     IPEndPoint ipe;
     if (!ipe.FromSockAddr(
             reinterpret_cast<const struct sockaddr*>(&addresses[i]),
             sizeof addresses[i])) {
+      RecordConfigParseResult(CONFIG_PARSE_POSIX_BAD_ADDRESS);
       return false;
     }
     dns_config->nameservers.push_back(ipe);
   }
 #elif defined(OS_LINUX)
   COMPILE_ASSERT(arraysize(res.nsaddr_list) >= MAXNS &&
                  arraysize(res._u._ext.nsaddrs) >= MAXNS,
                  incompatible_libresolv_res_state);
   DCHECK_LE(res.nscount, MAXNS);
   // Initially, glibc stores IPv6 in |_ext.nsaddrs| and IPv4 in |nsaddr_list|.
   // In res_send.c:res_nsend, it merges |nsaddr_list| into |nsaddrs|,
   // but we have to combine the two arrays ourselves.
   for (int i = 0; i < res.nscount; ++i) {
     IPEndPoint ipe;
     const struct sockaddr* addr = NULL;
     size_t addr_len = 0;
     if (res.nsaddr_list[i].sin_family) {  // The indicator used by res_nsend.
       addr = reinterpret_cast<const struct sockaddr*>(&res.nsaddr_list[i]);
       addr_len = sizeof res.nsaddr_list[i];
     } else if (res._u._ext.nsaddrs[i] != NULL) {
       addr = reinterpret_cast<const struct sockaddr*>(res._u._ext.nsaddrs[i]);
       addr_len = sizeof *res._u._ext.nsaddrs[i];
     } else {
+      RecordConfigParseResult(CONFIG_PARSE_POSIX_BAD_EXT_STRUCT);
       return false;
     }
-    if (!ipe.FromSockAddr(addr, addr_len))
+    if (!ipe.FromSockAddr(addr, addr_len)) {
+      RecordConfigParseResult(CONFIG_PARSE_POSIX_BAD_ADDRESS);
       return false;
+    }
     dns_config->nameservers.push_back(ipe);
   }
 #else  // !(defined(OS_LINUX) || defined(OS_MACOSX) || defined(OS_FREEBSD))
   DCHECK_LE(res.nscount, MAXNS);
   for (int i = 0; i < res.nscount; ++i) {
     IPEndPoint ipe;
     if (!ipe.FromSockAddr(
             reinterpret_cast<const struct sockaddr*>(&res.nsaddr_list[i]),
             sizeof res.nsaddr_list[i])) {
+      RecordConfigParseResult(CONFIG_PARSE_POSIX_BAD_ADDRESS);
       return false;
     }
     dns_config->nameservers.push_back(ipe);
   }
 #endif
 
   dns_config->search.clear();
   for (int i = 0; (i < MAXDNSRCH) && res.dnsrch[i]; ++i) {
     dns_config->search.push_back(std::string(res.dnsrch[i]));
   }
 
   dns_config->ndots = res.ndots;
   dns_config->timeout = base::TimeDelta::FromSeconds(res.retrans);
   dns_config->attempts = res.retry;
 #if defined(RES_ROTATE)
   dns_config->rotate = res.options & RES_ROTATE;
 #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) {
+    RecordConfigParseResult(CONFIG_PARSE_POSIX_MISSING_OPTIONS);
+    return false;
+  }
+
+  unsigned kUnhandledOptions = RES_USEVC | RES_IGNTC | RES_USE_DNSSEC;
+  if (res.options & kUnhandledOptions) {
+    RecordConfigParseResult(CONFIG_PARSE_POSIX_UNHANDLED_OPTIONS);
+    return false;
+  }
+
+  if (dns_config->nameservers.empty()) {
+    RecordConfigParseResult(CONFIG_PARSE_POSIX_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) {
+      RecordConfigParseResult(CONFIG_PARSE_POSIX_NULL_ADDRESS);
       return false;
+    }
+  }
+  RecordConfigParseResult(CONFIG_PARSE_POSIX_OK);
   return true;
 }
-#endif  // !defined(OS_ANDROID)
 
 }  // namespace internal
 
 // static
 scoped_ptr<DnsConfigService> DnsConfigService::CreateSystemService() {
   return scoped_ptr<DnsConfigService>(new internal::DnsConfigServicePosix());
 }
 
+#else  // defined(OS_ANDROID)
+// Android NDK provides only a stub <resolv.h> header.
+class StubDnsConfigService : public DnsConfigService {
+ public:
+  StubDnsConfigService() {}
+  virtual ~StubDnsConfigService() {}
+  virtual void OnDNSChanged(unsigned detail) OVERRIDE {}
+};
+// static
+scoped_ptr<DnsConfigService> DnsConfigService::CreateSystemService() {
+  return scoped_ptr<DnsConfigService>(new StubDnsConfigService());
+}
+#endif
+
 }  // namespace net