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

net/dns/dns_config_service_win.cc

Index: net/dns/dns_config_service_win.cc
diff --git a/net/dns/dns_config_service_win.cc b/net/dns/dns_config_service_win.cc
index 0c2e2c164da4985e34b17a2048900e0d5ec81adf..977a727f2c7c8af3a8be9d6bab9c3b14c1b27b67 100644
--- a/net/dns/dns_config_service_win.cc
+++ b/net/dns/dns_config_service_win.cc
@@ -13,11 +13,13 @@
 #include "base/file_path.h"
 #include "base/logging.h"
 #include "base/memory/scoped_ptr.h"
+#include "base/metrics/histogram.h"
 #include "base/string_split.h"
 #include "base/string_util.h"
 #include "base/synchronization/lock.h"
 #include "base/threading/non_thread_safe.h"
 #include "base/threading/thread_restrictions.h"
+#include "base/time.h"
 #include "base/utf_string_conversions.h"
 #include "base/win/registry.h"
 #include "base/win/windows_version.h"
@@ -39,6 +41,41 @@ namespace {
 const wchar_t* const kPrimaryDnsSuffixPath =
     L"SOFTWARE\\Policies\\Microsoft\\System\\DNSClient";
 
+enum ConfigParseWinResult {
+  CONFIG_PARSE_WIN_OK = 0,
+  CONFIG_PARSE_WIN_READ_IPHELPER,
+  CONFIG_PARSE_WIN_READ_POLICY_SEARCHLIST,
+  CONFIG_PARSE_WIN_READ_TCPIP_SEARCHLIST,
+  CONFIG_PARSE_WIN_READ_DOMAIN,
+  CONFIG_PARSE_WIN_READ_POLICY_DEVOLUTION,
+  CONFIG_PARSE_WIN_READ_DNSCACHE_DEVOLUTION,
+  CONFIG_PARSE_WIN_READ_TCPIP_DEVOLUTION,
+  CONFIG_PARSE_WIN_READ_APPEND_MULTILABEL,
+  CONFIG_PARSE_WIN_READ_PRIMARY_SUFFIX,
+  CONFIG_PARSE_WIN_BAD_ADDRESS,
+  CONFIG_PARSE_WIN_NO_NAMESERVERS,

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

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.

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

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

+  CONFIG_PARSE_WIN_MAX  // Bounding values for enumeration.
+};
+
+static void RecordConfigParseResult(ConfigParseWinResult result) {
+  UMA_HISTOGRAM_ENUMERATION("AsyncDNS.ConfigParseWin",
+                            result, CONFIG_PARSE_WIN_MAX);
+}
+
+enum HostsParseWinResult {
+  HOSTS_PARSE_WIN_OK = 0,
+  HOSTS_PARSE_WIN_READ_HOSTS,
+  HOSTS_PARSE_WIN_READ_COMPUTER_NAME,
+  HOSTS_PARSE_WIN_READ_IPHELPER,
+  HOSTS_PARSE_WIN_BAD_ADDRESS,

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

See above comment.

+  HOSTS_PARSE_WIN_MAX  // Bounding values for enumeration.
+};
+
+static void RecordHostsParseResult(HostsParseWinResult result) {
+  UMA_HISTOGRAM_ENUMERATION("AsyncDNS.HostsParseWin",
+                            result, HOSTS_PARSE_WIN_MAX);
+}
+
 // Convenience for reading values using RegKey.
 class RegistryReader : public base::NonThreadSafe {
  public:
@@ -85,7 +122,7 @@ class RegistryReader : public base::NonThreadSafe {
   DISALLOW_COPY_AND_ASSIGN(RegistryReader);
 };
 
-// Returns NULL if failed.
+// Wrapper for GetAdaptersAddresses. Returns NULL if failed.
 scoped_ptr_malloc<IP_ADAPTER_ADDRESSES> ReadIpHelper(ULONG flags) {
   base::ThreadRestrictions::AssertIOAllowed();
 
@@ -132,6 +169,155 @@ bool ParseDomainASCII(const string16& widestr, std::string* domain) {
   return success && !domain->empty();
 }
 
+bool ReadDevolutionSetting(const RegistryReader& reader,
+                           DnsSystemSettings::DevolutionSetting* setting) {
+  return reader.ReadDword(L"UseDomainNameDevolution", &setting->enabled) &&
+         reader.ReadDword(L"DomainNameDevolutionLevel", &setting->level);
+}
+
+// Reads DnsSystemSettings from IpHelper and registry.
+bool ReadSystemSettings(DnsSystemSettings* settings) {

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

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.

+  settings->addresses = ReadIpHelper(GAA_FLAG_SKIP_ANYCAST |
+                                     GAA_FLAG_SKIP_UNICAST |
+                                     GAA_FLAG_SKIP_MULTICAST |
+                                     GAA_FLAG_SKIP_FRIENDLY_NAME);
+  if (!settings->addresses.get()) {
+    RecordConfigParseResult(CONFIG_PARSE_WIN_READ_IPHELPER);
+    return false;
+  }
+
+  RegistryReader tcpip_reader(kTcpipPath);
+  RegistryReader tcpip6_reader(kTcpip6Path);
+  RegistryReader dnscache_reader(kDnscachePath);
+  RegistryReader policy_reader(kPolicyPath);
+  RegistryReader primary_dns_suffix_reader(kPrimaryDnsSuffixPath);
+
+  if (!policy_reader.ReadString(L"SearchList",
+                                &settings->policy_search_list)) {
+    RecordConfigParseResult(CONFIG_PARSE_WIN_READ_POLICY_SEARCHLIST);
+    return false;
+  }
+
+  if (!tcpip_reader.ReadString(L"SearchList", &settings->tcpip_search_list)) {
+    RecordConfigParseResult(CONFIG_PARSE_WIN_READ_TCPIP_SEARCHLIST);
+    return false;
+  }
+
+  if (!tcpip_reader.ReadString(L"Domain", &settings->tcpip_domain)) {
+    RecordConfigParseResult(CONFIG_PARSE_WIN_READ_DOMAIN);
+    return false;
+  }
+
+  if (!ReadDevolutionSetting(policy_reader, &settings->policy_devolution)) {
+    RecordConfigParseResult(CONFIG_PARSE_WIN_READ_POLICY_DEVOLUTION);
+    return false;
+  }
+
+  if (!ReadDevolutionSetting(dnscache_reader,
+                             &settings->dnscache_devolution)) {
+    RecordConfigParseResult(CONFIG_PARSE_WIN_READ_DNSCACHE_DEVOLUTION);
+    return false;
+  }
+
+  if (!ReadDevolutionSetting(tcpip_reader, &settings->tcpip_devolution)) {
+    RecordConfigParseResult(CONFIG_PARSE_WIN_READ_TCPIP_DEVOLUTION);
+    return false;
+  }
+
+  if (!policy_reader.ReadDword(L"AppendToMultiLabelName",
+                               &settings->append_to_multi_label_name)) {
+    RecordConfigParseResult(CONFIG_PARSE_WIN_READ_APPEND_MULTILABEL);
+    return false;
+  }
+
+  if (!primary_dns_suffix_reader.ReadString(L"PrimaryDnsSuffix",
+                                            &settings->primary_dns_suffix)) {
+    RecordConfigParseResult(CONFIG_PARSE_WIN_READ_PRIMARY_SUFFIX);
+    return false;
+  }
+  return true;
+}
+
+// Default address of "localhost" and local computer name can be overridden
+// by the HOSTS file, but if it's not there, then we need to fill it in.
+bool AddLocalhostEntries(DnsHosts* phosts) {

[cbentzel, 2012/06/16 18:03:41]

Nit: We don't tend to use hungarian-style notation (phosts) - but I think this
is OK so the reference can be |hosts|.

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

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

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

Again, suggest having this return a HostsParseWinResult.

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

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]

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.

+  DnsHosts& hosts = *phosts;
+  const unsigned char kIPv4Localhost[] = { 127, 0, 0, 1 };
+  const unsigned char kIPv6Localhost[] = { 0, 0, 0, 0, 0, 0, 0, 0,
+                                           0, 0, 0, 0, 0, 0, 0, 1 };
+  IPAddressNumber loopback_ipv4(kIPv4Localhost,
+                                kIPv4Localhost + arraysize(kIPv4Localhost));
+  IPAddressNumber loopback_ipv6(kIPv6Localhost,
+                                kIPv6Localhost + arraysize(kIPv6Localhost));
+
+  // This does not override any pre-existing entries from the HOSTS file.
+  hosts.insert(std::make_pair(DnsHostsKey("localhost", ADDRESS_FAMILY_IPV4),
+                              loopback_ipv4));
+  hosts.insert(std::make_pair(DnsHostsKey("localhost", ADDRESS_FAMILY_IPV6),
+                              loopback_ipv6));
+
+  WCHAR buffer[MAX_PATH];
+  DWORD size = MAX_PATH;
+  std::string localname;
+  if (!GetComputerNameExW(ComputerNameDnsHostname, buffer, &size) ||
+      !ParseDomainASCII(buffer, &localname)) {
+    LOG(ERROR) << "Failed to read local computer name";

[cbentzel, 2012/06/16 18:03:41]

Do you need the LOG(ERROR) now that we have the histogram?

+    RecordHostsParseResult(HOSTS_PARSE_WIN_READ_COMPUTER_NAME);
+    return false;
+  }
+  StringToLowerASCII(&localname);
+
+  bool have_ipv4 =
+      hosts.count(DnsHostsKey(localname, ADDRESS_FAMILY_IPV4)) > 0;
+  bool have_ipv6 =
+      hosts.count(DnsHostsKey(localname, ADDRESS_FAMILY_IPV6)) > 0;
+
+  if (have_ipv4 && have_ipv6)
+    return true;
+
+  scoped_ptr_malloc<IP_ADAPTER_ADDRESSES> addresses =
+      ReadIpHelper(GAA_FLAG_SKIP_ANYCAST |
+                   GAA_FLAG_SKIP_DNS_SERVER |
+                   GAA_FLAG_SKIP_MULTICAST |
+                   GAA_FLAG_SKIP_FRIENDLY_NAME);
+  if (!addresses.get()) {
+    RecordHostsParseResult(HOSTS_PARSE_WIN_READ_IPHELPER);
+    return false;
+  }
+
+  // The order of adapters is the network binding order, so stick to the
+  // first good adapter for each family.
+  for (const IP_ADAPTER_ADDRESSES* adapter = addresses.get();
+       adapter != NULL && (!have_ipv4 || !have_ipv6);
+       adapter = adapter->Next) {
+    if (adapter->OperStatus != IfOperStatusUp)
+      continue;
+    if (adapter->IfType == IF_TYPE_SOFTWARE_LOOPBACK)
+      continue;
+
+    for (const IP_ADAPTER_UNICAST_ADDRESS* address =
+             adapter->FirstUnicastAddress;
+         address != NULL;
+         address = address->Next) {
+      IPEndPoint ipe;
+      if (!ipe.FromSockAddr(address->Address.lpSockaddr,
+                            address->Address.iSockaddrLength)) {
+        RecordHostsParseResult(HOSTS_PARSE_WIN_BAD_ADDRESS);
+        return false;
+      }
+      if (!have_ipv4 && (ipe.GetFamily() == AF_INET)) {
+        have_ipv4 = true;
+        hosts[DnsHostsKey(localname, ADDRESS_FAMILY_IPV4)] = ipe.address();
+      } else if (!have_ipv6 && (ipe.GetFamily() == AF_INET6)) {
+        have_ipv6 = true;
+        hosts[DnsHostsKey(localname, ADDRESS_FAMILY_IPV6)] = ipe.address();
+      }
+    }
+  }
+  RecordHostsParseResult(HOSTS_PARSE_WIN_OK);
+  return true;
+}
+
 }  // namespace
 
 FilePath GetHostsPath() {
@@ -194,6 +380,7 @@ bool ConvertSettingsToDnsConfig(const DnsSystemSettings& settings,
           ipe = IPEndPoint(ipe.address(), dns_protocol::kDefaultPort);
         config->nameservers.push_back(ipe);
       } else {
+        RecordConfigParseResult(CONFIG_PARSE_WIN_BAD_ADDRESS);
         return false;
       }
     }
@@ -208,8 +395,10 @@ bool ConvertSettingsToDnsConfig(const DnsSystemSettings& settings,
       config->search.push_back(dns_suffix);
   }
 
-  if (config->nameservers.empty())
+  if (config->nameservers.empty()) {
+    RecordConfigParseResult(CONFIG_PARSE_WIN_NO_NAMESERVERS);
     return false;  // No point continuing.
+  }
 
   // Windows always tries a multi-label name "as is" before using suffixes.
   config->ndots = 1;
@@ -306,13 +495,11 @@ bool ConvertSettingsToDnsConfig(const DnsSystemSettings& settings,
     offset = primary_suffix.find('.', offset + 1);
     config->search.push_back(primary_suffix.substr(offset + 1));
   }
-
+  RecordConfigParseResult(CONFIG_PARSE_WIN_OK);
   return true;
 }
 
-// Watches registry for changes and reads config from registry and IP helper.
-// Reading and opening of reg keys is always performed on WorkerPool. Setting
-// up watches requires IO loop.
+// Reads config from registry and IP helper. All work performed on WorkerPool.
 class DnsConfigServiceWin::ConfigReader : public SerialWorker {
  public:
   explicit ConfigReader(DnsConfigServiceWin* service)
@@ -320,59 +507,18 @@ class DnsConfigServiceWin::ConfigReader : public SerialWorker {
         success_(false) {}
 
  private:
-  bool ReadDevolutionSetting(const RegistryReader& reader,
-                             DnsSystemSettings::DevolutionSetting& setting) {
-    return reader.ReadDword(L"UseDomainNameDevolution", &setting.enabled) &&
-           reader.ReadDword(L"DomainNameDevolutionLevel", &setting.level);
-  }
+  virtual ~ConfigReader() {}
 
   virtual void DoWork() OVERRIDE {
     // Should be called on WorkerPool.
-    success_ = false;
-
+    base::TimeTicks start_time = base::TimeTicks::Now();
     DnsSystemSettings settings = {};
-    settings.addresses = ReadIpHelper(GAA_FLAG_SKIP_ANYCAST |
-                                      GAA_FLAG_SKIP_UNICAST |
-                                      GAA_FLAG_SKIP_MULTICAST |
-                                      GAA_FLAG_SKIP_FRIENDLY_NAME);
-    if (!settings.addresses.get())
-      return;  // no point reading the rest
-
-    RegistryReader tcpip_reader(kTcpipPath);
-    RegistryReader tcpip6_reader(kTcpip6Path);
-    RegistryReader dnscache_reader(kDnscachePath);
-    RegistryReader policy_reader(kPolicyPath);
-    RegistryReader primary_dns_suffix_reader(kPrimaryDnsSuffixPath);
-
-    if (!policy_reader.ReadString(L"SearchList",
-                                  &settings.policy_search_list))
-      return;
-
-    if (!tcpip_reader.ReadString(L"SearchList", &settings.tcpip_search_list))
-      return;
-
-    if (!tcpip_reader.ReadString(L"Domain", &settings.tcpip_domain))
-      return;
-
-    if (!ReadDevolutionSetting(policy_reader, settings.policy_devolution))
-      return;
-
-    if (!ReadDevolutionSetting(dnscache_reader,
-                               settings.dnscache_devolution))
-      return;
-
-    if (!ReadDevolutionSetting(tcpip_reader, settings.tcpip_devolution))
-      return;
-
-    if (!policy_reader.ReadDword(L"AppendToMultiLabelName",
-                                 &settings.append_to_multi_label_name))
-      return;
-
-    if (!primary_dns_suffix_reader.ReadString(L"PrimaryDnsSuffix",
-                                              &settings.primary_dns_suffix))
-      return;
-
-    success_ = ConvertSettingsToDnsConfig(settings, &dns_config_);
+    success_ = ReadSystemSettings(&settings);
+    if (success_)
+      success_ = ConvertSettingsToDnsConfig(settings, &dns_config_);
+    UMA_HISTOGRAM_BOOLEAN("AsyncDNS.ConfigParseWinResult", success_);
+    UMA_HISTOGRAM_TIMES("AsyncDNS.ConfigParseDuration",
+                        base::TimeTicks::Now() - start_time);
   }
 
   virtual void OnWorkFinished() OVERRIDE {
@@ -401,88 +547,19 @@ class DnsConfigServiceWin::HostsReader : public SerialWorker {
   }
 
  private:
+  virtual ~HostsReader() {}
+
   virtual void DoWork() OVERRIDE {
+    base::TimeTicks start_time = base::TimeTicks::Now();
     success_ = ParseHostsFile(path_, &hosts_);
-
-    if (!success_)
-      return;
-
-    success_ = false;
-
-    // Default address of "localhost" and local computer name can be overridden
-    // by the HOSTS file, but if it's not there, then we need to fill it in.
-
-    const unsigned char kIPv4Localhost[] = { 127, 0, 0, 1 };
-    const unsigned char kIPv6Localhost[] = { 0, 0, 0, 0, 0, 0, 0, 0,
-                                             0, 0, 0, 0, 0, 0, 0, 1 };
-    IPAddressNumber loopback_ipv4(kIPv4Localhost,
-                                  kIPv4Localhost + arraysize(kIPv4Localhost));
-    IPAddressNumber loopback_ipv6(kIPv6Localhost,
-                                  kIPv6Localhost + arraysize(kIPv6Localhost));
-
-    // This does not override any pre-existing entries from the HOSTS file.
-    hosts_.insert(std::make_pair(DnsHostsKey("localhost", ADDRESS_FAMILY_IPV4),
-                                 loopback_ipv4));
-    hosts_.insert(std::make_pair(DnsHostsKey("localhost", ADDRESS_FAMILY_IPV6),
-                                 loopback_ipv6));
-
-    WCHAR buffer[MAX_PATH];
-    DWORD size = MAX_PATH;
-    std::string localname;
-    if (!GetComputerNameExW(ComputerNameDnsHostname, buffer, &size) ||
-        !ParseDomainASCII(buffer, &localname)) {
-      LOG(ERROR) << "Failed to read local computer name";
-      return;
-    }
-    StringToLowerASCII(&localname);
-
-    bool have_ipv4 =
-        hosts_.count(DnsHostsKey(localname, ADDRESS_FAMILY_IPV4)) > 0;
-    bool have_ipv6 =
-        hosts_.count(DnsHostsKey(localname, ADDRESS_FAMILY_IPV6)) > 0;
-
-    if (have_ipv4 && have_ipv6) {
-      success_ = true;
-      return;
-    }
-
-    scoped_ptr_malloc<IP_ADAPTER_ADDRESSES> addresses =
-        ReadIpHelper(GAA_FLAG_SKIP_ANYCAST |
-                     GAA_FLAG_SKIP_DNS_SERVER |
-                     GAA_FLAG_SKIP_MULTICAST |
-                     GAA_FLAG_SKIP_FRIENDLY_NAME);
-    if (!addresses.get())
-      return;
-
-    // The order of adapters is the network binding order, so stick to the
-    // first good adapter for each family.
-    for (const IP_ADAPTER_ADDRESSES* adapter = addresses.get();
-         adapter != NULL && (!have_ipv4 || !have_ipv6);
-         adapter = adapter->Next) {
-      if (adapter->OperStatus != IfOperStatusUp)
-        continue;
-      if (adapter->IfType == IF_TYPE_SOFTWARE_LOOPBACK)
-        continue;
-
-      for (const IP_ADAPTER_UNICAST_ADDRESS* address =
-               adapter->FirstUnicastAddress;
-           address != NULL;
-           address = address->Next) {
-        IPEndPoint ipe;
-        if (!ipe.FromSockAddr(address->Address.lpSockaddr,
-                              address->Address.iSockaddrLength)) {
-          return;
-        }
-        if (!have_ipv4 && (ipe.GetFamily() == AF_INET)) {
-          have_ipv4 = true;
-          hosts_[DnsHostsKey(localname, ADDRESS_FAMILY_IPV4)] = ipe.address();
-        } else if (!have_ipv6 && (ipe.GetFamily() == AF_INET6)) {
-          have_ipv6 = true;
-          hosts_[DnsHostsKey(localname, ADDRESS_FAMILY_IPV6)] = ipe.address();
-        }
-      }
+    if (success_) {
+      success_ = AddLocalhostEntries(&hosts_);
+    } else {
+      RecordHostsParseResult(HOSTS_PARSE_WIN_READ_HOSTS);
     }
-    success_ = true;
+    UMA_HISTOGRAM_BOOLEAN("AsyncDNS.HostParseResult", success_);
+    UMA_HISTOGRAM_TIMES("AsyncDNS.HostsParseDuration",
+                        base::TimeTicks::Now() - start_time);
   }
 
   virtual void OnWorkFinished() OVERRIDE {
@@ -508,7 +585,6 @@ DnsConfigServiceWin::DnsConfigServiceWin()
       hosts_reader_(new HostsReader(this)) {}
 
 DnsConfigServiceWin::~DnsConfigServiceWin() {
-  DCHECK(CalledOnValidThread());
   config_reader_->Cancel();
   hosts_reader_->Cancel();
   NetworkChangeNotifier::RemoveIPAddressObserver(this);