Fix DNS transaction error metrics

Move recording of Net.DNS.DnsTransaction.*.*.FailureError to
ResolveContext::RecordServerFailure() which is called for all server
errors and timeouts rather than just errors where a response is still
received.

Also fix a bug where ResolveContext::RecordServerFailure() was not being
called for all the errors it should be called for.

Bug: 1101073
Change-Id: I1e4032589b3ad1cab0e9cfe6f3bc7acc30c7dcca
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2276651Reviewed-by: Matt Menke <mmenke@chromium.org>
Commit-Queue: Eric Orth <ericorth@chromium.org>
Cr-Commit-Position: refs/heads/master@{#784510}

net/dns/dns_transaction.cc

@@ -1410,7 +1410,7 @@ class DnsTransactionImpl : public DnsTransaction,
           if (result.attempt) {
             resolve_context_->RecordServerFailure(
                 result.attempt->server_index(), secure_ /* is_doh_server */,
-                session_.get());
+                result.rv, session_.get());
           }
           if (MoreAttemptsAllowed()) {
             result = MakeAttempt();
@@ -1428,12 +1428,20 @@ class DnsTransactionImpl : public DnsTransaction,
         default:
           // Server failure.
           DCHECK(result.attempt);
+
+          // If attempt is not the most recent attempt, means this error is for
+          // an attempt that already timed out and was treated as complete but
+          // allowed to continue attempting in parallel with new attempts (see
+          // the ERR_DNS_TIMED_OUT case above). As the failure was already
+          // recorded at timeout time and is no longer being waited on, ignore
+          // this failure.
           if (result.attempt != attempts_.back().get()) {
-            resolve_context_->RecordServerFailure(
-                result.attempt->server_index(), secure_ /* is_doh_server */,
-                session_.get());
             return AttemptResult(ERR_IO_PENDING, nullptr);
           }
+
+          resolve_context_->RecordServerFailure(result.attempt->server_index(),
+                                                secure_ /* is_doh_server */,
+                                                result.rv, session_.get());
           if (!MoreAttemptsAllowed()) {
             return result;
           }

net/dns/dns_transaction_unittest.cc

@@ -1202,8 +1202,17 @@ TEST_F(DnsTransactionTestWithMockTime, ServerFallbackAndRotate) {
   EXPECT_TRUE(helper1.Run(transaction_factory_.get()));
 
   size_t kOrder[] = {
-      0, 1, 2, 0, 1,  // The first transaction.
-      1, 2, 0,        // The second transaction starts from the next server.
+      // The first transaction.
+      0,
+      1,
+      2,
+      0,
+      1,
+      // The second transaction starts from the next server, and 0 is skipped
+      // because it already has 2 consecutive failures.
+      1,
+      2,
+      1,
   };
   CheckServerOrder(kOrder, base::size(kOrder));
 }
@@ -3103,8 +3112,9 @@ TEST_F(DnsTransactionTestWithMockTime, RestartFinishedProbe) {
 
   // Mark server unavailabe and restart runner.
   for (int i = 0; i < ResolveContext::kAutomaticModeFailureLimit; ++i) {
-    resolve_context_->RecordServerFailure(
-        0u /* server_index */, true /* is_doh_server */, session_.get());
+    resolve_context_->RecordServerFailure(0u /* server_index */,
+                                          true /* is_doh_server */, ERR_FAILED,
+                                          session_.get());
   }
   ASSERT_FALSE(resolve_context_->GetDohServerAvailability(
       0u /* doh_server_index */, session_.get()));
@@ -3152,8 +3162,9 @@ TEST_F(DnsTransactionTestWithMockTime, FastProbeRestart) {
   // becoming unavailable and might as well replecate real behavior for the
   // test.
   for (int i = 0; i < ResolveContext::kAutomaticModeFailureLimit; ++i) {
-    resolve_context_->RecordServerFailure(
-        0u /* server_index */, true /* is_doh_server */, session_.get());
+    resolve_context_->RecordServerFailure(0u /* server_index */,
+                                          true /* is_doh_server */, ERR_FAILED,
+                                          session_.get());
   }
   ASSERT_FALSE(resolve_context_->GetDohServerAvailability(
       0u /* doh_server_index */, session_.get()));

net/dns/resolve_context.cc

@@ -7,7 +7,6 @@
 #include <algorithm>
 #include <cstdlib>
 #include <limits>
-#include <string>
 #include <utility>
 
 #include "base/check_op.h"
@@ -150,10 +149,27 @@ size_t ResolveContext::NumAvailableDohServers(const DnsSession* session) const {
 
 void ResolveContext::RecordServerFailure(size_t server_index,
                                          bool is_doh_server,
+                                         int rv,
                                          const DnsSession* session) {
+  DCHECK(rv != OK && rv != ERR_NAME_NOT_RESOLVED && rv != ERR_IO_PENDING);
+
   if (!IsCurrentSession(session))
     return;
 
+  // "FailureError" metric is only recorded for secure queries.
+  if (is_doh_server) {
+    std::string query_type =
+        GetQueryTypeForUma(server_index, true /* is_doh_server */, session);
+    DCHECK_NE(query_type, "Insecure");
+    std::string provider_id =
+        GetDohProviderIdForUma(server_index, true /* is_doh_server */, session);
+
+    base::UmaHistogramSparse(
+        base::StringPrintf("Net.DNS.DnsTransaction.%s.%s.FailureError",
+                           query_type.c_str(), provider_id.c_str()),
+        std::abs(rv));
+  }
+
   size_t num_available_doh_servers_before = NumAvailableDohServers(session);
 
   ServerStats* stats = GetServerStats(server_index, is_doh_server);
@@ -370,21 +386,11 @@ void ResolveContext::RecordRttForUma(size_t server_index,
                                      const DnsSession* session) {
   DCHECK(IsCurrentSession(session));
 
-  std::string query_type;
-  std::string provider_id;
-  if (is_doh_server) {
-    // Secure queries are validated if the DoH server state is available.
-    if (GetDohServerAvailability(server_index, session))
-      query_type = "SecureValidated";
-    else
-      query_type = "SecureNotValidated";
-    provider_id = GetDohProviderIdForHistogramFromDohConfig(
-        current_session_->config().dns_over_https_servers[server_index]);
-  } else {
-    query_type = "Insecure";
-    provider_id = GetDohProviderIdForHistogramFromNameserver(
-        current_session_->config().nameservers[server_index]);
-  }
+  std::string query_type =
+      GetQueryTypeForUma(server_index, is_doh_server, session);
+  std::string provider_id =
+      GetDohProviderIdForUma(server_index, is_doh_server, session);
+
   if (rv == OK || rv == ERR_NAME_NOT_RESOLVED) {
     base::UmaHistogramMediumTimes(
         base::StringPrintf("Net.DNS.DnsTransaction.%s.%s.SuccessTime",
@@ -395,15 +401,38 @@ void ResolveContext::RecordRttForUma(size_t server_index,
         base::StringPrintf("Net.DNS.DnsTransaction.%s.%s.FailureTime",
                            query_type.c_str(), provider_id.c_str()),
         rtt);
-    if (is_doh_server) {
-      base::UmaHistogramSparse(
-          base::StringPrintf("Net.DNS.DnsTransaction.%s.%s.FailureError",
-                             query_type.c_str(), provider_id.c_str()),
-          std::abs(rv));
-    }
   }
 }
 
+std::string ResolveContext::GetQueryTypeForUma(size_t server_index,
+                                               bool is_doh_server,
+                                               const DnsSession* session) {
+  DCHECK(IsCurrentSession(session));
+
+  if (!is_doh_server)
+    return "Insecure";
+
+  // Secure queries are validated if the DoH server state is available.
+  if (GetDohServerAvailability(server_index, session))
+    return "SecureValidated";
+
+  return "SecureNotValidated";
+}
+
+std::string ResolveContext::GetDohProviderIdForUma(size_t server_index,
+                                                   bool is_doh_server,
+                                                   const DnsSession* session) {
+  DCHECK(IsCurrentSession(session));
+
+  if (is_doh_server) {
+    return GetDohProviderIdForHistogramFromDohConfig(
+        session->config().dns_over_https_servers[server_index]);
+  }
+
+  return GetDohProviderIdForHistogramFromNameserver(
+      session->config().nameservers[server_index]);
+}
+
 void ResolveContext::NotifyDohStatusObserversOfSessionChanged() {
   for (auto& observer : doh_status_observers_)
     observer.OnSessionChanged();

net/dns/resolve_context.h

@@ -6,6 +6,7 @@
 #define NET_DNS_RESOLVE_CONTEXT_H_
 
 #include <memory>
+#include <string>
 #include <vector>
 
 #include "base/memory/weak_ptr.h"
@@ -92,9 +93,12 @@ class NET_EXPORT_PRIVATE ResolveContext : public base::CheckedObserver {
   // Record that server failed to respond (due to SRV_FAIL or timeout). If
   // |is_doh_server| and the number of failures has surpassed a threshold,
   // sets the DoH probe state to unavailable. Noop if |session| is not the
-  // current session.
+  // current session. Should only be called with with server failure |rv|s,
+  // not eg OK, ERR_NAME_NOT_RESOLVED (which at the transaction level is
+  // expected to be nxdomain), or ERR_IO_PENDING.
   void RecordServerFailure(size_t server_index,
                            bool is_doh_server,
+                           int rv,
                            const DnsSession* session);
 
   // Record that server responded successfully. Noop if |session| is not the
@@ -200,6 +204,12 @@ class NET_EXPORT_PRIVATE ResolveContext : public base::CheckedObserver {
                        base::TimeDelta rtt,
                        int rv,
                        const DnsSession* session);
+  std::string GetQueryTypeForUma(size_t server_index,
+                                 bool is_doh_server,
+                                 const DnsSession* session);
+  std::string GetDohProviderIdForUma(size_t server_index,
+                                     bool is_doh_server,
+                                     const DnsSession* session);
 
   void NotifyDohStatusObserversOfSessionChanged();
   void NotifyDohStatusObserversOfUnavailable(bool network_change);

net/dns/resolve_context_unittest.cc

@@ -197,7 +197,7 @@ TEST_F(ResolveContextTest, DohServerAvailability_DifferentSession) {
   ASSERT_TRUE(context.GetDohServerAvailability(1u, session2.get()));
   for (int i = 0; i < ResolveContext::kAutomaticModeFailureLimit; ++i) {
     context.RecordServerFailure(1u /* server_index */, true /* is_doh_server */,
-                                session1.get());
+                                ERR_FAILED, session1.get());
   }
   EXPECT_TRUE(context.GetDohServerAvailability(1u, session2.get()));
 }
@@ -300,7 +300,7 @@ TEST_F(ResolveContextTest, DohServerAvailabilityNotification) {
   for (int i = 0; i < ResolveContext::kAutomaticModeFailureLimit; ++i) {
     ASSERT_EQ(2u, context.NumAvailableDohServers(session.get()));
     context.RecordServerFailure(0u /* server_index */, true /* is_doh_server */,
-                                session.get());
+                                ERR_FAILED, session.get());
     base::RunLoop().RunUntilIdle();  // Notifications are async.
     EXPECT_EQ(1, config_observer.dns_changed_calls());
   }
@@ -313,7 +313,7 @@ TEST_F(ResolveContextTest, DohServerAvailabilityNotification) {
     EXPECT_EQ(1, config_observer.dns_changed_calls());
 
     context.RecordServerFailure(1u /* server_index */, true /* is_doh_server */,
-                                session.get());
+                                ERR_FAILED, session.get());
   }
   ASSERT_EQ(0u, context.NumAvailableDohServers(session.get()));
   base::RunLoop().RunUntilIdle();  // Notifications are async.
@@ -418,7 +418,8 @@ TEST_F(ResolveContextTest, Failures_Consecutive) {
     EXPECT_EQ(classic_itr->GetNextAttemptIndex(), 1u);
 
     context.RecordServerFailure(1u /* server_index */,
-                                false /* is_doh_server */, session.get());
+                                false /* is_doh_server */, ERR_FAILED,
+                                session.get());
   }
 
   {
@@ -464,7 +465,8 @@ TEST_F(ResolveContextTest, Failures_NonConsecutive) {
     EXPECT_EQ(classic_itr->GetNextAttemptIndex(), 1u);
 
     context.RecordServerFailure(1u /* server_index */,
-                                false /* is_doh_server */, session.get());
+                                false /* is_doh_server */, ERR_FAILED,
+                                session.get());
   }
 
   {
@@ -491,7 +493,7 @@ TEST_F(ResolveContextTest, Failures_NonConsecutive) {
 
   // Expect server stay preferred through non-consecutive failures.
   context.RecordServerFailure(1u /* server_index */, false /* is_doh_server */,
-                              session.get());
+                              ERR_FAILED, session.get());
   {
     std::unique_ptr<DnsServerIterator> classic_itr =
         context.GetClassicDnsIterator(session->config(), session.get());
@@ -518,7 +520,8 @@ TEST_F(ResolveContextTest, Failures_NoSession) {
     EXPECT_FALSE(classic_itr->AttemptAvailable());
 
     context.RecordServerFailure(1u /* server_index */,
-                                false /* is_doh_server */, session.get());
+                                false /* is_doh_server */, ERR_FAILED,
+                                session.get());
   }
   std::unique_ptr<DnsServerIterator> classic_itr =
       context.GetClassicDnsIterator(session->config(), session.get());
@@ -551,7 +554,8 @@ TEST_F(ResolveContextTest, Failures_DifferentSession) {
     EXPECT_EQ(classic_itr->GetNextAttemptIndex(), 1u);
 
     context.RecordServerFailure(1u /* server_index */,
-                                false /* is_doh_server */, session1.get());
+                                false /* is_doh_server */, ERR_FAILED,
+                                session1.get());
   }
   std::unique_ptr<DnsServerIterator> classic_itr =
       context.GetClassicDnsIterator(session2->config(), session2.get());
@@ -586,9 +590,11 @@ TEST_F(ResolveContextTest, TwoFailures) {
     EXPECT_EQ(classic_itr->GetNextAttemptIndex(), 2u);
 
     context.RecordServerFailure(0u /* server_index */,
-                                false /* is_doh_server */, session.get());
+                                false /* is_doh_server */, ERR_FAILED,
+                                session.get());
     context.RecordServerFailure(1u /* server_index */,
-                                false /* is_doh_server */, session.get());
+                                false /* is_doh_server */, ERR_FAILED,
+                                session.get());
   }
   {
     std::unique_ptr<DnsServerIterator> classic_itr =
@@ -661,7 +667,7 @@ TEST_F(ResolveContextTest, DohFailures_Consecutive) {
     EXPECT_EQ(1u, context.NumAvailableDohServers(session.get()));
     EXPECT_EQ(0, observer.server_unavailable_notifications());
     context.RecordServerFailure(1u /* server_index */, true /* is_doh_server */,
-                                session.get());
+                                ERR_FAILED, session.get());
   }
   std::unique_ptr<DnsServerIterator> doh_itr = context.GetDohIterator(
       session->config(), DnsConfig::SecureDnsMode::AUTOMATIC, session.get());
@@ -696,7 +702,7 @@ TEST_F(ResolveContextTest, DohFailures_NonConsecutive) {
     EXPECT_EQ(doh_itr->GetNextAttemptIndex(), 1u);
     EXPECT_EQ(1u, context.NumAvailableDohServers(session.get()));
     context.RecordServerFailure(1u /* server_index */, true /* is_doh_server */,
-                                session.get());
+                                ERR_FAILED, session.get());
   }
   {
     std::unique_ptr<DnsServerIterator> doh_itr = context.GetDohIterator(
@@ -721,7 +727,7 @@ TEST_F(ResolveContextTest, DohFailures_NonConsecutive) {
   // Expect a single additional failure should not make a DoH server unavailable
   // because the success resets failure tracking.
   context.RecordServerFailure(1u /* server_index */, true /* is_doh_server */,
-                              session.get());
+                              ERR_FAILED, session.get());
   {
     std::unique_ptr<DnsServerIterator> doh_itr = context.GetDohIterator(
         session->config(), DnsConfig::SecureDnsMode::AUTOMATIC, session.get());
@@ -752,7 +758,7 @@ TEST_F(ResolveContextTest, DohFailures_SuccessAfterFailures) {
 
   for (size_t i = 0; i < ResolveContext::kAutomaticModeFailureLimit; i++) {
     context.RecordServerFailure(1u /* server_index */, true /* is_doh_server */,
-                                session.get());
+                                ERR_FAILED, session.get());
   }
   ASSERT_EQ(0u, context.NumAvailableDohServers(session.get()));
   EXPECT_EQ(1, observer.server_unavailable_notifications());
@@ -787,7 +793,7 @@ TEST_F(ResolveContextTest, DohFailures_NoSession) {
   for (size_t i = 0; i < ResolveContext::kAutomaticModeFailureLimit; i++) {
     EXPECT_EQ(0u, context.NumAvailableDohServers(session.get()));
     context.RecordServerFailure(1u /* server_index */, true /* is_doh_server */,
-                                session.get());
+                                ERR_FAILED, session.get());
   }
   EXPECT_EQ(0u, context.NumAvailableDohServers(session.get()));
 }
@@ -814,7 +820,7 @@ TEST_F(ResolveContextTest, DohFailures_DifferentSession) {
   for (size_t i = 0; i < ResolveContext::kAutomaticModeFailureLimit; i++) {
     EXPECT_EQ(1u, context.NumAvailableDohServers(session2.get()));
     context.RecordServerFailure(1u /* server_index */, true /* is_doh_server */,
-                                session1.get());
+                                ERR_FAILED, session1.get());
   }
   EXPECT_EQ(1u, context.NumAvailableDohServers(session2.get()));
 }
@@ -849,9 +855,9 @@ TEST_F(ResolveContextTest, TwoDohFailures) {
     EXPECT_EQ(doh_itr->GetNextAttemptIndex(), 2u);
 
     context.RecordServerFailure(0u /* server_index */, true /* is_doh_server */,
-                                session.get());
+                                ERR_FAILED, session.get());
     context.RecordServerFailure(1u /* server_index */, true /* is_doh_server */,
-                                session.get());
+                                ERR_FAILED, session.get());
   }
 
   std::unique_ptr<DnsServerIterator> doh_itr = context.GetDohIterator(