Fixed current OOBE captive portal metrics.

chrome/browser/chromeos/net/network_portal_detector_impl.cc

 // Copyright (c) 2013 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 "chrome/browser/chromeos/net/network_portal_detector_impl.h"
 
 #include "base/bind.h"
 #include "base/command_line.h"
 #include "base/logging.h"
 #include "base/message_loop/message_loop.h"
@@ skipping 21 matching lines @@
 // Minimum timeout between consecutive portal checks for the same
 // network.
 const int kMinTimeBetweenAttemptsSec = 3;
 
 // Delay before portal detection caused by changes in proxy settings.
 const int kProxyChangeDelaySec = 1;
 
 // Delay between consecutive portal checks for a network in lazy mode.
 const int kLazyCheckIntervalSec = 5;
 
-const char kCaptivePortalStatusUnknown[] = "Unknown";
-const char kCaptivePortalStatusOffline[] = "Offline";
-const char kCaptivePortalStatusOnline[]  = "Online";
-const char kCaptivePortalStatusPortal[]  = "Portal";
-const char kCaptivePortalStatusProxyAuthRequired[] =
-    "Proxy authentication required";
-const char kCaptivePortalStatusUnrecognized[] = "Unrecognized";
-
-std::string CaptivePortalStatusString(
-    NetworkPortalDetectorImpl::CaptivePortalStatus status) {
-  switch (status) {
-    case NetworkPortalDetectorImpl::CAPTIVE_PORTAL_STATUS_UNKNOWN:
-      return kCaptivePortalStatusUnknown;
-    case NetworkPortalDetectorImpl::CAPTIVE_PORTAL_STATUS_OFFLINE:
-      return kCaptivePortalStatusOffline;
-    case NetworkPortalDetectorImpl::CAPTIVE_PORTAL_STATUS_ONLINE:
-      return kCaptivePortalStatusOnline;
-    case NetworkPortalDetectorImpl::CAPTIVE_PORTAL_STATUS_PORTAL:
-      return kCaptivePortalStatusPortal;
-    case NetworkPortalDetectorImpl::CAPTIVE_PORTAL_STATUS_PROXY_AUTH_REQUIRED:
-      return kCaptivePortalStatusProxyAuthRequired;
-    case NetworkPortalDetectorImpl::CAPTIVE_PORTAL_STATUS_COUNT:
-      NOTREACHED();
+void RecordDiscrepancyWithShill(
+    const NetworkState* network,
+    const NetworkPortalDetector::CaptivePortalStatus status) {
+  if (network->connection_state() == shill::kStateOnline) {
+    UMA_HISTOGRAM_ENUMERATION(
+        "CaptivePortal.OOBE.DiscrepancyWithShill_Online",
+        status,
+        NetworkPortalDetector::CAPTIVE_PORTAL_STATUS_COUNT);
+  } else if (network->connection_state() == shill::kStatePortal) {
+    UMA_HISTOGRAM_ENUMERATION(
+        "CaptivePortal.OOBE.DiscrepancyWithShill_RestrictedPool",
+        status,
+        NetworkPortalDetector::CAPTIVE_PORTAL_STATUS_COUNT);
+  } else if (network->connection_state() == shill::kStateOffline) {
+    UMA_HISTOGRAM_ENUMERATION(
+        "CaptivePortal.OOBE.DiscrepancyWithShill_Offline",
+        status,
+        NetworkPortalDetector::CAPTIVE_PORTAL_STATUS_COUNT);
   }
-  return kCaptivePortalStatusUnrecognized;
 }
 
 }  // namespace
 
 ////////////////////////////////////////////////////////////////////////////////
 // NetworkPortalDetectorImpl, public:
 
 NetworkPortalDetectorImpl::NetworkPortalDetectorImpl(
     const scoped_refptr<net::URLRequestContextGetter>& request_context)
     : state_(STATE_IDLE),
@@ skipping 368 matching lines @@
   return state_ == STATE_PORTAL_CHECK_PENDING;
 }
 
 bool NetworkPortalDetectorImpl::IsCheckingForPortal() const {
   return state_ == STATE_CHECKING_FOR_PORTAL;
 }
 
 void NetworkPortalDetectorImpl::SetCaptivePortalState(
     const NetworkState* network,
     const CaptivePortalState& state) {
-  if (!detection_start_time_.is_null()) {
-    UMA_HISTOGRAM_TIMES("CaptivePortal.OOBE.DetectionDuration",
-                        GetCurrentTimeTicks() - detection_start_time_);
-  }
-
   if (!network) {
     NotifyPortalDetectionCompleted(network, state);
     return;
   }
 
   CaptivePortalStateMap::const_iterator it =
       portal_state_map_.find(network->path());
   if (it == portal_state_map_.end() ||
       it->second.status != state.status ||
       it->second.response_code != state.response_code) {
     VLOG(1) << "Updating Chrome Captive Portal state: "
             << "name=" << network->name() << ", "
             << "id=" << network->guid() << ", "
             << "status=" << CaptivePortalStatusString(state.status) << ", "
             << "response_code=" << state.response_code;
+
+    // Record detection duration iff detection result differs from the
+    // previous one for this network. The reason is to record only
+    // detection duration when network changes it's state, as we're
+    // interested only on distribution of detection duration, not

[Nikita (slow), 2014/01/09 13:48:56]

nit: update comment, all stats are reported only once per state changed.

[ygorshenin1, 2014/01/09 14:59:35]

Done.

+    // distribution of detection duration multiplied by number of
+    // checks.
+    RecordDetectionStats(network, state.status);
+
     portal_state_map_[network->path()] = state;
   }
   NotifyPortalDetectionCompleted(network, state);
 }
 
 void NetworkPortalDetectorImpl::NotifyPortalDetectionCompleted(
     const NetworkState* network,
     const CaptivePortalState& state) {
   FOR_EACH_OBSERVER(Observer, observers_,
                     OnPortalDetectionCompleted(network, state));
@@ skipping 13 matching lines @@
   DCHECK_LE(0, attempt_count_);
   const NetworkState* network =
       NetworkHandler::Get()->network_state_handler()->DefaultNetwork();
   if (!network)
     return kBaseRequestTimeoutSec;
   if (lazy_detection_enabled_)
     return kLazyRequestTimeoutSec;
   return attempt_count_ * kBaseRequestTimeoutSec;
 }
 
+void NetworkPortalDetectorImpl::RecordDetectionStats(
+    const NetworkState* network,
+    CaptivePortalStatus status) {
+  // Don't record stats for offline state.
+  if (!network)
+    return;
+
+  if (!detection_start_time_.is_null()) {
+    UMA_HISTOGRAM_MEDIUM_TIMES("CaptivePortal.OOBE.DetectionDuration",
+                               GetCurrentTimeTicks() - detection_start_time_);
+  }
+  UMA_HISTOGRAM_ENUMERATION("CaptivePortal.OOBE.DetectionResult", status,

[Nikita (slow), 2014/01/09 13:48:56]

nit: status on a separate line.

[ygorshenin1, 2014/01/09 14:59:35]

Done.

+                            NetworkPortalDetector::CAPTIVE_PORTAL_STATUS_COUNT);
+  switch (status) {
+    case NetworkPortalDetector::CAPTIVE_PORTAL_STATUS_UNKNOWN:
+      NOTREACHED();
+      break;
+    case NetworkPortalDetector::CAPTIVE_PORTAL_STATUS_OFFLINE:
+      if (network->connection_state() == shill::kStateOnline ||
+          network->connection_state() == shill::kStatePortal)
+        RecordDiscrepancyWithShill(network, status);

[Nikita (slow), 2014/01/09 13:48:56]

nit: {}

[ygorshenin1, 2014/01/09 14:59:35]

Done.

+      break;
+    case NetworkPortalDetector::CAPTIVE_PORTAL_STATUS_ONLINE:
+      if (network->connection_state() != shill::kStateOnline)
+        RecordDiscrepancyWithShill(network, status);
+      break;
+    case NetworkPortalDetector::CAPTIVE_PORTAL_STATUS_PORTAL:
+      if (network->connection_state() != shill::kStatePortal)
+        RecordDiscrepancyWithShill(network, status);
+      break;
+    case NetworkPortalDetector::CAPTIVE_PORTAL_STATUS_PROXY_AUTH_REQUIRED:
+      if (network->connection_state() != shill::kStateOnline)
+        RecordDiscrepancyWithShill(network, status);
+      break;
+    case NetworkPortalDetector::CAPTIVE_PORTAL_STATUS_COUNT:
+      NOTREACHED();
+      break;
+  }
+}
+
 }  // namespace chromeos