Domain Reliability: Simplify configs and reports

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 2db9882d1e0b4345f37215bea1cb288ffd106446..23658807f2e86a26155ce0072bc1e5c9ebba7580 100644
--- a/components/domain_reliability/monitor.cc
+++ b/components/domain_reliability/monitor.cc
@@ -10,6 +10,8 @@
 #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"
@@ -42,18 +44,24 @@ int URLRequestStatusToNetError(const net::URLRequestStatus& status) {
 // 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
@@ -152,10 +160,18 @@ void DomainReliabilityMonitor::AddBakedInConfigs() {
     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) {
@@ -298,15 +314,15 @@ void DomainReliabilityMonitor::OnRequestLegComplete(
   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
@@ -317,16 +333,20 @@ void DomainReliabilityMonitor::OnRequestLegComplete(
   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>