Domain Reliability: Create beacons from ConnectionAttempts.

components/domain_reliability/monitor.cc

Index: components/domain_reliability/monitor.cc
diff --git a/components/domain_reliability/monitor.cc b/components/domain_reliability/monitor.cc
index 83a6af19252425f7ee953bfe87d6b4c074500883..29f0782a33e0f4c411af4f3e5076a3e32a2acf1e 100644
--- a/components/domain_reliability/monitor.cc
+++ b/components/domain_reliability/monitor.cc
@@ -11,7 +11,10 @@
 #include "base/task_runner.h"
 #include "base/time/time.h"
 #include "components/domain_reliability/baked_in_configs.h"
+#include "net/base/ip_endpoint.h"
 #include "net/base/load_flags.h"
+#include "net/base/net_errors.h"
+#include "net/base/net_util.h"
 #include "net/http/http_response_headers.h"
 #include "net/url_request/url_request.h"
 #include "net/url_request/url_request_context.h"
@@ -19,6 +22,54 @@
 
 namespace domain_reliability {
 
+namespace {
+
+int URLRequestStatusToNetError(const net::URLRequestStatus& status) {

[davidben, 2015/05/15 18:36:50]

[Not your fault, but we really need to get rid of this nonsense sometime...]

[Deprecated (see juliatuttle), 2015/05/15 19:08:45]

Yup.

+  switch (status.status()) {
+    case net::URLRequestStatus::SUCCESS:
+      return net::OK;
+    case net::URLRequestStatus::IO_PENDING:
+      return net::ERR_IO_PENDING;
+    case net::URLRequestStatus::CANCELED:
+      return net::ERR_ABORTED;
+    case net::URLRequestStatus::FAILED:
+      return status.error();
+    default:
+      NOTREACHED();
+      return net::ERR_UNEXPECTED;
+  }
+}
+

[davidben, 2015/05/15 18:36:50]

This function is confusing since it intentionally leaves half of |beacon| as-is
while filling in other parts. This deserves documentation as to which is which.

[Deprecated (see juliatuttle), 2015/05/15 19:08:45]

Done.

+bool FillBeaconFromAttempt(
+    DomainReliabilityBeacon* beacon,
+    const net::ConnectionAttempt& attempt) {
+  if (!GetDomainReliabilityBeaconStatus(
+      attempt.result,
+      beacon->http_response_code,
+      &beacon->status)) {

[davidben, 2015/05/15 18:36:50]

Mind doing a git cl format pass? I'm not sure that's the right indentation.

[Deprecated (see juliatuttle), 2015/05/15 19:08:45]

Done.

+    return false;
+  }
+  beacon->chrome_error = attempt.result;
+  if (!attempt.endpoint.address().empty())
+    beacon->server_ip = attempt.endpoint.ToString();
+  else
+    beacon->server_ip = "";
+  return true;
+}
+
+// TODO(ttuttle): This function is absurd. See if |socket_address| in
+// HttpResponseInfo can become an IPEndPoint.
+bool ConvertHostPortPairToIPEndPoint(const net::HostPortPair& host_port_pair,
+                                     net::IPEndPoint* ip_endpoint_out) {
+  net::IPAddressNumber ip_address_number;
+  if (!net::ParseIPLiteralToNumber(host_port_pair.host(), &ip_address_number))

[davidben, 2015/05/15 18:36:50]

Are you sure this is correct? When an IPv6 address is written on its own, it's
hex and colons. When in the host of a URL, it has square brackets around it.
ParseIPLiteralToNumber seems to expect there *not* be square brackets. Which
format does net::HostPortPair supply?

https://code.google.com/p/chromium/codesearch#chromium/src/net/base/net_util....

[Deprecated (see juliatuttle), 2015/05/15 19:08:45]

Yep, it's correct. HostPortPair eventually uses url::AppendIPv6Address, which
doesn't include brackets.

[davidben, 2015/05/15 19:46:18]

Acknowledged.

+    return false;
+  *ip_endpoint_out = net::IPEndPoint(ip_address_number, host_port_pair.port());
+  return true;
+}
+
+}  // namespace
+
 DomainReliabilityMonitor::DomainReliabilityMonitor(
     const std::string& upload_reporter_string,
     const scoped_refptr<base::SingleThreadTaskRunner>& pref_thread,
@@ -150,8 +201,9 @@ void DomainReliabilityMonitor::OnCompleted(net::URLRequest* request,
   if (!started)
     return;
   RequestInfo request_info(*request);
-  if (request_info.AccessedNetwork()) {
-    OnRequestLegComplete(request_info);
+  OnRequestLegComplete(request_info);
+
+  if (request_info.response_info.network_accessed) {
     // A request was just using the network, so now is a good time to run any
     // pending and eligible uploads.
     dispatcher_.RunEligibleTasks();
@@ -221,13 +273,23 @@ DomainReliabilityMonitor::RequestInfo::RequestInfo(
       load_flags(request.load_flags()),
       is_upload(DomainReliabilityUploader::URLRequestIsUpload(request)) {
   request.GetLoadTimingInfo(&load_timing_info);
+  request.GetConnectionAttempts(&connection_attempts);
 }
 
 DomainReliabilityMonitor::RequestInfo::~RequestInfo() {}
 
-bool DomainReliabilityMonitor::RequestInfo::AccessedNetwork() const {
-  return status.status() != net::URLRequestStatus::CANCELED &&
-     response_info.network_accessed;
+// static
+bool DomainReliabilityMonitor::RequestInfo::ShouldReportRequest(
+    const DomainReliabilityMonitor::RequestInfo& request) {
+  if (request.is_upload)
+    return false;
+  if (request.load_flags & net::LOAD_DO_NOT_SEND_COOKIES)
+    return false;
+  if (request.response_info.network_accessed)
+    return true;
+  if (URLRequestStatusToNetError(request.status) != net::OK)
+    return true;

[davidben, 2015/05/15 18:36:50]

This is a change from the current behavior. Before, it looks like this was just
keying on network_accessed? Also it wouldn't record CANCELED. Why the change?
(And can that be documented in the commit message?)

Recording CANCELED seems weird. We might cancel a request internally for things.
Or maybe the user just closed a tab. That's not a network error. They just
happened to close a tab while some XHR was in progress.

[Deprecated (see juliatuttle), 2015/05/15 19:08:45]

I moved a bunch of the logic into this function -- we were checking is_upload
and LOAD_DO_NOT_SEND_COOKIES before as well.
IIRC, I talked to Matt and he thought we should be treating CANCELED the same as
ERR_ABORTED.
Currently, we're reporting it, since users often close tabs before a hanging
connection actually turns into a timeout error.

[davidben, 2015/05/15 19:46:18]

Right, and that one didn't change in behavior. They're still ANDs. Switching
from false to true means you have an OR now.
Yeah, they should be the same. But ERR_ABORTED means we cancelled the request,
not that the network failed on us.
 
(Would that not still have network_accessed true? Not sure, actually. Maybe no
since the transaction doesn't complete Start.)

I don't believe we're currently reporting it. The old code required
AccessedNetwork to be true, which doesn't allow for CANCELED.

I don't hugely mind the change in behavior, though I think it's puzzling. Just
it should be documented as a change in behavior in the commit message so it's
clear that's what you intended. (I'm not clear whether you did or no right now.)

+  return false;
 }
 
 void DomainReliabilityMonitor::OnRequestLegComplete(
@@ -236,44 +298,31 @@ void DomainReliabilityMonitor::OnRequestLegComplete(
   DCHECK(OnNetworkThread());
   DCHECK(discard_uploads_set_);
 
+  if (!RequestInfo::ShouldReportRequest(request))
+    return;
+
   int response_code;
   if (request.response_info.headers.get())
     response_code = request.response_info.headers->response_code();
   else
     response_code = -1;
-  std::string beacon_status;
-
-  int error_code = net::OK;
-  if (request.status.status() == net::URLRequestStatus::FAILED)
-    error_code = request.status.error();
-
-  // Ignore requests where:
-  // 1. The request did not access the network.
-  // 2. The request is not supposed to send cookies (to avoid associating the
-  //    request with any potentially unique data in the config).
-  // 3. The request was itself a Domain Reliability upload (to avoid loops).
-  // 4. There is no matching beacon status for the error or HTTP response code
-  //    (to avoid leaking network-local errors).
-  if (!request.AccessedNetwork() ||
-      (request.load_flags & net::LOAD_DO_NOT_SEND_COOKIES) ||
-      request.is_upload ||
-      !GetDomainReliabilityBeaconStatus(
-          error_code, response_code, &beacon_status)) {
-    return;
-  }
 
-  DomainReliabilityBeacon beacon;
-  beacon.status = beacon_status;
-  beacon.chrome_error = error_code;
-  // If the response was cached, the socket address was the address that the
-  // response was originally received from, so it shouldn't be copied into the
-  // beacon.
-  //
-  // TODO(ttuttle): Wire up a way to get the real socket address in that case.
+  net::IPEndPoint url_request_endpoint;
+  // If response was cached, socket address will be from the serialized
+  // response info in the cache, so don't report it.
+  // TODO(ttuttle): Plumb out the "current" socket address so we can always
+  // report it.
   if (!request.response_info.was_cached &&
       !request.response_info.was_fetched_via_proxy) {
-    beacon.server_ip = request.response_info.socket_address.host();
+    ConvertHostPortPairToIPEndPoint(request.response_info.socket_address,
+                                    &url_request_endpoint);
   }
+
+  net::ConnectionAttempt url_request_attempt(
+      url_request_endpoint,
+      URLRequestStatusToNetError(request.status));
+
+  DomainReliabilityBeacon beacon;
   beacon.protocol = GetDomainReliabilityProtocol(
       request.response_info.connection_info,
       request.response_info.ssl_info.is_valid());
@@ -282,6 +331,20 @@ void DomainReliabilityMonitor::OnRequestLegComplete(
   beacon.elapsed = time_->NowTicks() - beacon.start_time;
   beacon.was_proxied = request.response_info.was_fetched_via_proxy;
   beacon.domain = request.url.host();
+
+  bool url_request_attempt_is_duplicate = false;
+  for (const auto& attempt : request.connection_attempts) {
+    if (attempt.result == url_request_attempt.result)
+      url_request_attempt_is_duplicate = true;

[davidben, 2015/05/15 18:36:50]

When does this happen? This seems like the ConnectionAttempt API isn't terribly
well-defined, since this is a really loose check.

[Deprecated (see juliatuttle), 2015/05/15 19:08:45]

It is well-defined, but it doesn't cover some errors that can occur *after*
connecting, like a content length mismatch. This check makes sure that we don't
report a failed connection attempt twice, but we do report other errors that
don't count as connection attempts.

+    if (!FillBeaconFromAttempt(&beacon, attempt))
+      continue;
+    context_manager_.RouteBeacon(request.url, beacon);
+  }
+
+  if (url_request_attempt_is_duplicate)
+    return;
+  if (!FillBeaconFromAttempt(&beacon, url_request_attempt))
+    return;
   context_manager_.RouteBeacon(request.url, beacon);
 }