Job Controller: clean up metrics logging about the job racing result

net/http/http_stream_factory_impl_job_controller.cc

 // Copyright (c) 2016 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 "net/http/http_stream_factory_impl_job_controller.h"
 
 #include "base/metrics/histogram_macros.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_util.h"
 #include "base/values.h"
@@ skipping 723 matching lines @@
     // we've already got one available for a different hostname where the ip
     // address matches up?
     CancelJobs();
     return;
   }
 
   if (job->job_type() == MAIN && alternative_job_failed_)
     ReportBrokenAlternativeService();
 
   if (!bound_job_) {
-    if (main_job_ && alternative_job_) {
-      job->ReportJobSucceededForRequest();
-      MaybeRecordAlternativeProxyServerUsage(job);
-    }
+    if (main_job_ && alternative_job_)
+      ReportAlternateProtocolUsage(job);
     BindJob(job);
     return;
   }
   DCHECK(bound_job_);
 }
 
 void HttpStreamFactoryImpl::JobController::MarkRequestComplete(
     bool was_npn_negotiated,
     NextProto negotiated_protocol,
     bool using_spdy) {
@@ skipping 136 matching lines @@
   // First Alt-Svc that is not marked as broken.
   AlternativeService first_alternative_service;
 
   for (const AlternativeService& alternative_service :
        alternative_service_vector) {
     DCHECK(IsAlternateProtocolValid(alternative_service.protocol));
     if (!quic_advertised && alternative_service.protocol == QUIC)
       quic_advertised = true;
     if (http_server_properties.IsAlternativeServiceBroken(
             alternative_service)) {
-      HistogramAlternateProtocolUsage(ALTERNATE_PROTOCOL_USAGE_BROKEN);
+      HistogramAlternateProtocolUsage(ALTERNATE_PROTOCOL_USAGE_BROKEN, false);
       continue;
     }
 
 
     // Some shared unix systems may have user home directories (like
     // http://foo.com/~mike) which allow users to emit headers.  This is a bad
     // idea already, but with Alternate-Protocol, it provides the ability for a
     // single user on a multi-user system to hijack the alternate protocol.
     // These systems also enforce ports <1024 as restricted ports.  So don't
     // allow protocol upgrades to user-controllable ports.
@@ skipping 128 matching lines @@
     // Check that QUIC is enabled globally, and it is not disabled.
     if (!session_->params().enable_quic ||
         session_->quic_stream_factory()->IsQuicDisabled()) {
       return false;
     }
   }
 
   return true;
 }
 
-void HttpStreamFactoryImpl::JobController::
-    MaybeRecordAlternativeProxyServerUsage(Job* job) const {
-  if (is_preconnect_ ||
-      !alternative_job_->alternative_proxy_server().is_quic()) {
-    return;
+void HttpStreamFactoryImpl::JobController::ReportAlternateProtocolUsage(
+    Job* job) const {
+  DCHECK(main_job_ && alternative_job_);

[tbansal1, 2016/09/22 17:53:02]

Curious why use a weaker DCHECK?
Is it possible that DCHECK(main_job_.get() == job || alternative_job_.get() ==
job); is actually false?

[Zhongyi Shi, 2016/09/22 23:56:53]

The new DCHECK was to make sure both jobs are alive when we figure out
reporting. And the if and DCHECK @1066 enforces:
DCHECK(main_job_.get() == job || alternative_job_.get() ==
job).
 

[tbansal1, 2016/09/23 00:02:07]

Acknowledged.

+  bool proxy_server_used =
+      alternative_job_->alternative_proxy_server().is_quic();
+
+  if (job == alternative_job_.get()) {
+    if (job->using_existing_quic_session()) {
+      HistogramAlternateProtocolUsage(ALTERNATE_PROTOCOL_USAGE_NO_RACE,
+                                      proxy_server_used);
+    } else {
+      HistogramAlternateProtocolUsage(ALTERNATE_PROTOCOL_USAGE_WON_RACE,
+                                      proxy_server_used);
+    }
+  } else {
+    DCHECK_EQ(main_job_.get(), job);
+    HistogramAlternateProtocolUsage(ALTERNATE_PROTOCOL_USAGE_LOST_RACE,
+                                    proxy_server_used);
   }

[Ryan Hamilton, 2016/09/22 21:46:33]

nit: I think we can use some early returns:

if (job == main_job_) {
 HistogramAlternateProtocolUsage(ALTERNATE_PROTOCOL_USAGE_LOST_RACE,
                                 proxy_server_used);
  return;
}

DCHECK_EQ(alternative_job_.get(), job);
if (job->using_existing_quic_session()) {
  HistogramAlternateProtocolUsage(ALTERNATE_PROTOCOL_USAGE_NO_RACE, 
                                  proxy_server_used);
  return;
}

HistogramAlternateProtocolUsage(ALTERNATE_PROTOCOL_USAGE_WON_RACE,
                                proxy_server_used);

[Zhongyi Shi, 2016/09/22 23:56:53]

Done.

-  DCHECK(main_job_.get() == job || alternative_job_.get() == job);
-
-  enum AlternativeProxyUsage {
-    ALTERNATIVE_PROXY_USAGE_NO_RACE = 0,
-    ALTERNATIVE_PROXY_USAGE_WON_RACE,
-    ALTERNATIVE_PROXY_USAGE_LOST_RACE,
-    ALTERNATIVE_PROXY_USAGE_MAX,
-  };
-  AlternativeProxyUsage alternative_proxy_usage = ALTERNATIVE_PROXY_USAGE_MAX;
-
-  if (alternative_job_->using_existing_quic_session()) {
-    // If an existing session was used, then no TCP connection was
-    // started.
-    alternative_proxy_usage = ALTERNATIVE_PROXY_USAGE_NO_RACE;
-  } else if (job->alternative_proxy_server().is_quic()) {
-    // |job| was the alternative Job, and hence won the race.
-    alternative_proxy_usage = ALTERNATIVE_PROXY_USAGE_WON_RACE;
-  } else {
-    // |job| was the normal Job, and hence the alternative Job lost the race.
-    alternative_proxy_usage = ALTERNATIVE_PROXY_USAGE_LOST_RACE;
-  }
-  DCHECK_NE(ALTERNATIVE_PROXY_USAGE_MAX, alternative_proxy_usage);
-  UMA_HISTOGRAM_ENUMERATION("Net.QuicAlternativeProxy.Usage",
-                            alternative_proxy_usage,
-                            ALTERNATIVE_PROXY_USAGE_MAX);
 }
 
 void HttpStreamFactoryImpl::JobController::StartAlternativeProxyServerJob() {
   if (!alternative_job_ || !request_)
     return;
   DCHECK(alternative_job_->alternative_proxy_server().is_valid());
   alternative_job_->Start(request_->stream_type());
 }
 
 }  // namespace net