Domain Reliability: Simplify configs and reports

components/domain_reliability/monitor.cc

 // Copyright 2014 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 "components/domain_reliability/monitor.h"
 
 #include "base/command_line.h"
 #include "base/logging.h"
 #include "base/profiler/scoped_tracker.h"
 #include "base/single_thread_task_runner.h"
 #include "base/task_runner.h"
 #include "components/domain_reliability/baked_in_configs.h"
+#include "components/domain_reliability/google_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"
 #include "net/url_request/url_request_context_getter.h"
 
 namespace domain_reliability {
 
@@ skipping 11 matching lines @@
       return status.error();
     default:
       NOTREACHED();
       return net::ERR_UNEXPECTED;
   }
 }
 
 // Updates the status, chrome_error, and server_ip fields of |beacon| from
 // the endpoint and result of |attempt|. If there is no matching status for
 // the result, returns false (which means the attempt should not result in a
 // beacon being reported).

[Randy Smith (Not in Mondays), 2015/10/29 22:36:11]

This comment needs to be updated?

[Deprecated (see juliatuttle), 2015/11/02 23:19:30]

Done.

-bool UpdateBeaconFromAttempt(DomainReliabilityBeacon* beacon,
-                             const net::ConnectionAttempt& attempt) {
+scoped_ptr<DomainReliabilityBeacon> CreateBeaconFromAttempt(
+    const DomainReliabilityBeacon& beacon_template,
+    const net::ConnectionAttempt& attempt) {
+  std::string status;
   if (!GetDomainReliabilityBeaconStatus(
-          attempt.result, beacon->http_response_code, &beacon->status)) {
-    return false;
+          attempt.result, beacon_template.http_response_code, &status)) {
+    return scoped_ptr<DomainReliabilityBeacon>();
   }
+
+  scoped_ptr<DomainReliabilityBeacon> beacon(
+      new DomainReliabilityBeacon(beacon_template));
+  beacon->status = status;
   beacon->chrome_error = attempt.result;
   if (!attempt.endpoint.address().empty())
     beacon->server_ip = attempt.endpoint.ToString();
   else
     beacon->server_ip = "";
-  return true;
+  return beacon.Pass();
 }
 
 }  // namespace
 
 DomainReliabilityMonitor::DomainReliabilityMonitor(
     const std::string& upload_reporter_string,
     const scoped_refptr<base::SingleThreadTaskRunner>& pref_thread,
     const scoped_refptr<base::SingleThreadTaskRunner>& network_thread)
     : time_(new ActualTime()),
       upload_reporter_string_(upload_reporter_string),
@@ skipping 78 matching lines @@
       FROM_HERE_WITH_EXPLICIT_FUNCTION(
           "436671 DomainReliabilityMonitor::AddBakedInConfigs"));
 
   DCHECK(OnNetworkThread());
   DCHECK(moved_to_network_thread_);
 
   for (size_t i = 0; kBakedInJsonConfigs[i]; ++i) {
     base::StringPiece json(kBakedInJsonConfigs[i]);
     scoped_ptr<const DomainReliabilityConfig> config =
         DomainReliabilityConfig::FromJSON(json);
-    if (!config)
+    if (!config) {
+      LOG(WARNING) << "Baked-in Domain Reliability config failed to parse: "
+                   << json;

[Randy Smith (Not in Mondays), 2015/10/29 22:36:11]

I don't think a log makes sense here--it would be spam that only you would know
to interpret.  DLOG or DVLOG seems the way to go.

[Deprecated (see juliatuttle), 2015/11/02 23:19:30]

Done.

       continue;
+    }
     context_manager_.AddContextForConfig(config.Pass());
   }
+
+  std::vector<DomainReliabilityConfig*> google_configs;
+  GetAllGoogleConfigs(&google_configs);
+  for (auto google_config : google_configs)
+    context_manager_.AddContextForConfig(make_scoped_ptr(google_config));
 }
 
 void DomainReliabilityMonitor::SetDiscardUploads(bool discard_uploads) {
   DCHECK(OnNetworkThread());
   DCHECK(moved_to_network_thread_);
   DCHECK(uploader_);
 
   uploader_->set_discard_uploads(discard_uploads);
   discard_uploads_set_ = true;
 }
@@ skipping 122 matching lines @@
 
   int response_code;
   if (request.response_info.headers.get())
     response_code = request.response_info.headers->response_code();
   else
     response_code = -1;
 
   net::ConnectionAttempt url_request_attempt(
       request.remote_endpoint, URLRequestStatusToNetError(request.status));
 
-  DomainReliabilityBeacon beacon;
-  beacon.protocol = GetDomainReliabilityProtocol(
-      request.response_info.connection_info,
-      request.response_info.ssl_info.is_valid());
-  beacon.http_response_code = response_code;
-  beacon.start_time = request.load_timing_info.request_start;
-  beacon.elapsed = time_->NowTicks() - beacon.start_time;
-  beacon.was_proxied = request.response_info.was_fetched_via_proxy;
-  beacon.domain = request.url.host();
+  DomainReliabilityBeacon beacon_template;
+  beacon_template.protocol =
+      GetDomainReliabilityProtocol(request.response_info.connection_info,
+                                   request.response_info.ssl_info.is_valid());
+  beacon_template.http_response_code = response_code;
+  beacon_template.start_time = request.load_timing_info.request_start;
+  beacon_template.elapsed = time_->NowTicks() - beacon_template.start_time;
+  beacon_template.was_proxied = request.response_info.was_fetched_via_proxy;
+  beacon_template.url = request.url;
 
   // This is not foolproof -- it's possible that we'll see the same error twice
   // (e.g. an SSL error during connection on one attempt, and then an error
   // that maps to the same code during a read).
   // TODO(ttuttle): Find a way for this code to reliably tell whether we
   // eventually established a connection or not.
   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;
-    if (!UpdateBeaconFromAttempt(&beacon, attempt))
-      continue;
-    context_manager_.RouteBeacon(request.url, beacon);
+
+    scoped_ptr<DomainReliabilityBeacon> beacon =
+        CreateBeaconFromAttempt(beacon_template, attempt);

[Randy Smith (Not in Mondays), 2015/10/29 22:36:11]

Why are we creating a new beacon rather than updating the one being passed in? 
(I.e. allocating it in this routine.)  The reason I'd rather do the allocation
here is that the name "beacon_template" feels to me like something abstract,
without any information in it relevant to the current request, whereas what's
actually going on is that we're setting some request-relevant information into
the template, then copying that into another beacon, where we set more request
relevant information.  I'd think that would be clearer if we just used one
object.

[Deprecated (see juliatuttle), 2015/11/02 23:19:30]

I think we're doing the opposite -- creating a new beacon based on the template,
then filling in the fields in the new beacon.

[Randy Smith (Not in Mondays), 2015/11/03 21:48:12]

That's how I'd describe what CreateBeaconFromAttempt() is doing.  But this
routine (OnRequestLegComplete) has a beacon local variable (beacon_template)
which is being partially filled in, then passed into CreateBeaconFromAttempt,
which takes that passed in beacon, copies it, and fills stuff in in the new
beacon).  

Of course, on this pass I see why you're doing it (Look!  A for loop!  :-} :-J.)
 I think I'd still prefer the alternative of creating the beacon and partially
filling it in here, then asking CreateBeaconFromAttempt to fill in the rest. 
The create partially filled template which the subroutine copies to create a new
beacon doesn't feel good to me.  There would be no more lines of code, and you'd
never use a template class.  You'd be repeatedly executing the same lines for
what's currently in the template, but that's equivalent performance wise to the
cost of the copy constructor.  

Still, I think this is a matter of style and preference, so I'm good either way.

[Randy Smith (Not in Mondays), 2015/11/09 21:23:24]

Just wanted to confirm that you read this and made a choice to keep the current
form, rather than skipped over it.

+    if (beacon)
+      context_manager_.RouteBeacon(beacon.Pass());
   }
 
   if (url_request_attempt_is_duplicate)
     return;
-  if (!UpdateBeaconFromAttempt(&beacon, url_request_attempt))
-    return;
-  context_manager_.RouteBeacon(request.url, beacon);
+
+  scoped_ptr<DomainReliabilityBeacon> beacon =
+      CreateBeaconFromAttempt(beacon_template, url_request_attempt);
+  if (beacon)
+    context_manager_.RouteBeacon(beacon.Pass());
 }
 
 base::WeakPtr<DomainReliabilityMonitor>
 DomainReliabilityMonitor::MakeWeakPtr() {
   return weak_factory_.GetWeakPtr();
 }
 
 }  // namespace domain_reliability