Fixed current OOBE captive portal metrics.

Now metrics are recorded only when a state of a particular network is
changed. If several consecutive checks indicate the same state for a
network, only the first one is recorded. The purpose of this fix is to
remove noize from so-called "lazy detection" from metrics.

BUG=231758
TEST=unit_tests:NetworkPortalDetectorImplTest*

Review URL: https://codereview.chromium.org/99223009

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@243945 0039d316-1c4b-4281-b951-d872f2087c98

chrome/browser/chromeos/net/network_portal_detector.cc

@@ -16,6 +16,14 @@ namespace chromeos {
 
 namespace {
 
+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";
+
 NetworkPortalDetector* g_network_portal_detector = NULL;
 bool g_network_portal_detector_set_for_testing = false;
 
@@ -90,4 +98,24 @@ NetworkPortalDetector* NetworkPortalDetector::Get() {
   return g_network_portal_detector;
 }
 
+// static
+std::string NetworkPortalDetector::CaptivePortalStatusString(
+    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();
+  }
+  return kCaptivePortalStatusUnrecognized;
+}
+
 }  // namespace chromeos

chrome/browser/chromeos/net/network_portal_detector.h

@@ -116,6 +116,9 @@ class NetworkPortalDetector {
   // Gets the instance of the NetworkPortalDetector.
   static NetworkPortalDetector* Get();
 
+  // Returns non-localized string representation of |status|.
+  static std::string CaptivePortalStatusString(CaptivePortalStatus status);
+
  protected:
   NetworkPortalDetector() {}
   virtual ~NetworkPortalDetector() {}

chrome/browser/chromeos/net/network_portal_detector_impl.cc

@@ -39,31 +39,25 @@ 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(
+        NetworkPortalDetectorImpl::kShillOnlineHistogram,
+        status,
+        NetworkPortalDetector::CAPTIVE_PORTAL_STATUS_COUNT);
+  } else if (network->connection_state() == shill::kStatePortal) {
+    UMA_HISTOGRAM_ENUMERATION(
+        NetworkPortalDetectorImpl::kShillPortalHistogram,
+        status,
+        NetworkPortalDetector::CAPTIVE_PORTAL_STATUS_COUNT);
+  } else if (network->connection_state() == shill::kStateOffline) {
+    UMA_HISTOGRAM_ENUMERATION(
+        NetworkPortalDetectorImpl::kShillOfflineHistogram,
+        status,
+        NetworkPortalDetector::CAPTIVE_PORTAL_STATUS_COUNT);
   }
-  return kCaptivePortalStatusUnrecognized;
 }
 
 }  // namespace
@@ -71,6 +65,17 @@ std::string CaptivePortalStatusString(
 ////////////////////////////////////////////////////////////////////////////////
 // NetworkPortalDetectorImpl, public:
 
+const char NetworkPortalDetectorImpl::kDetectionResultHistogram[] =
+  "CaptivePortal.OOBE.DetectionResult";
+const char NetworkPortalDetectorImpl::kDetectionDurationHistogram[] =
+  "CaptivePortal.OOBE.DetectionDuration";
+const char NetworkPortalDetectorImpl::kShillOnlineHistogram[] =
+  "CaptivePortal.OOBE.DiscrepancyWithShill_Online";
+const char NetworkPortalDetectorImpl::kShillPortalHistogram[] =
+  "CaptivePortal.OOBE.DiscrepancyWithShill_RestrictedPool";
+const char NetworkPortalDetectorImpl::kShillOfflineHistogram[] =
+  "CaptivePortal.OOBE.DiscrepancyWithShill_Offline";
+
 NetworkPortalDetectorImpl::NetworkPortalDetectorImpl(
     const scoped_refptr<net::URLRequestContextGetter>& request_context)
     : state_(STATE_IDLE),
@@ -452,11 +457,6 @@ bool NetworkPortalDetectorImpl::IsCheckingForPortal() const {
 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;
@@ -472,6 +472,12 @@ void NetworkPortalDetectorImpl::SetCaptivePortalState(
             << "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 all stats
+    // only when network changes it's state.
+    RecordDetectionStats(network, state.status);
+
     portal_state_map_[network->path()] = state;
   }
   NotifyPortalDetectionCompleted(network, state);
@@ -505,4 +511,46 @@ int NetworkPortalDetectorImpl::GetRequestTimeoutSec() const {
   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(kDetectionDurationHistogram,
+                               GetCurrentTimeTicks() - detection_start_time_);
+  }
+  UMA_HISTOGRAM_ENUMERATION(kDetectionResultHistogram,
+                            status,
+                            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);
+      }
+      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

chrome/browser/chromeos/net/network_portal_detector_impl.h

@@ -42,6 +42,12 @@ class NetworkPortalDetectorImpl
       public chromeos::NetworkStateHandlerObserver,
       public content::NotificationObserver {
  public:
+  static const char kDetectionResultHistogram[];
+  static const char kDetectionDurationHistogram[];
+  static const char kShillOnlineHistogram[];
+  static const char kShillPortalHistogram[];
+  static const char kShillOfflineHistogram[];
+
   explicit NetworkPortalDetectorImpl(
       const scoped_refptr<net::URLRequestContextGetter>& request_context);
   virtual ~NetworkPortalDetectorImpl();
@@ -187,6 +193,11 @@ class NetworkPortalDetectorImpl
   // * otherwise, timeout equals to |attempt_count_| * kBaseRequestTimeoutSec
   int GetRequestTimeoutSec() const;
 
+  // Record detection stats such as detection duration and detection
+  // result in UMA.
+  void RecordDetectionStats(const NetworkState* network,
+                            CaptivePortalStatus status);
+
   // Name of the default network.
   std::string default_network_name_;
 

chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc

@@ -124,8 +124,8 @@ bool IsOnline(NetworkStateInformer::State state,
       reason != ErrorScreenActor::ERROR_REASON_LOADING_TIMEOUT;
 }
 
-bool IsUnderCaptivePortal(NetworkStateInformer::State state,
-                          ErrorScreenActor::ErrorReason reason) {
+bool IsBehindCaptivePortal(NetworkStateInformer::State state,
+                           ErrorScreenActor::ErrorReason reason) {
   return state == NetworkStateInformer::CAPTIVE_PORTAL ||
       reason == ErrorScreenActor::ERROR_REASON_PORTAL_DETECTED;
 }
@@ -162,82 +162,6 @@ std::string GetNetworkName(const std::string& service_path) {
   return network->name();
 }
 
-// Returns captive portal state for a network by its service path.
-NetworkPortalDetector::CaptivePortalState GetCaptivePortalState(
-    const std::string& service_path) {
-  NetworkPortalDetector* detector = NetworkPortalDetector::Get();
-  const NetworkState* network = NetworkHandler::Get()->network_state_handler()->
-      GetNetworkState(service_path);
-  if (!detector || !network)
-    return NetworkPortalDetector::CaptivePortalState();
-  return detector->GetCaptivePortalState(network);
-}
-
-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 {
-    UMA_HISTOGRAM_ENUMERATION(
-        "CaptivePortal.OOBE.DiscrepancyWithShill_Offline",
-        status,
-        NetworkPortalDetector::CAPTIVE_PORTAL_STATUS_COUNT);
-  }
-}
-
-// Record state and descripancies with shill (e.g. shill thinks that
-// network is online but NetworkPortalDetector claims that it's behind
-// portal) for the network identified by |service_path|.
-void RecordNetworkPortalDetectorStats(const std::string& service_path) {
-  const NetworkState* network = NetworkHandler::Get()->network_state_handler()->
-      GetNetworkState(service_path);
-  if (!network)
-    return;
-  NetworkPortalDetector::CaptivePortalState state =
-      GetCaptivePortalState(service_path);
-  if (state.status == NetworkPortalDetector::CAPTIVE_PORTAL_STATUS_UNKNOWN)
-    return;
-
-  UMA_HISTOGRAM_ENUMERATION("CaptivePortal.OOBE.DetectionResult",
-                            state.status,
-                            NetworkPortalDetector::CAPTIVE_PORTAL_STATUS_COUNT);
-
-  switch (state.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, state.status);
-      break;
-    case NetworkPortalDetector::CAPTIVE_PORTAL_STATUS_ONLINE:
-      if (network->connection_state() != shill::kStateOnline)
-        RecordDiscrepancyWithShill(network, state.status);
-      break;
-    case NetworkPortalDetector::CAPTIVE_PORTAL_STATUS_PORTAL:
-      if (network->connection_state() != shill::kStatePortal)
-        RecordDiscrepancyWithShill(network, state.status);
-      break;
-    case NetworkPortalDetector::CAPTIVE_PORTAL_STATUS_PROXY_AUTH_REQUIRED:
-      if (network->connection_state() != shill::kStateOnline)
-        RecordDiscrepancyWithShill(network, state.status);
-      break;
-    case NetworkPortalDetector::CAPTIVE_PORTAL_STATUS_COUNT:
-      NOTREACHED();
-      break;
-  }
-}
-
 static bool SetUserInputMethodImpl(
     const std::string& username,
     chromeos::input_method::InputMethodManager* manager) {
@@ -606,7 +530,7 @@ void SigninScreenHandler::UpdateStateInternal(
   connecting_closure_.Cancel();
 
   const bool is_online = IsOnline(state, reason);
-  const bool is_under_captive_portal = IsUnderCaptivePortal(state, reason);
+  const bool is_behind_captive_portal = IsBehindCaptivePortal(state, reason);
   const bool is_gaia_loading_timeout =
       (reason == ErrorScreenActor::ERROR_REASON_LOADING_TIMEOUT);
   const bool is_gaia_error =
@@ -618,7 +542,7 @@ void SigninScreenHandler::UpdateStateInternal(
       is_online && last_network_state_ != NetworkStateInformer::ONLINE;
   last_network_state_ = state;
 
-  if (is_online || !is_under_captive_portal)
+  if (is_online || !is_behind_captive_portal)
     error_screen_actor_->HideCaptivePortal();
 
   // Hide offline message (if needed) and return if current screen is
@@ -663,19 +587,15 @@ void SigninScreenHandler::SetupAndShowOfflineMessage(
     NetworkStateInformer:: State state,
     ErrorScreenActor::ErrorReason reason) {
   const std::string network_path = network_state_informer_->network_path();
-  const bool is_under_captive_portal = IsUnderCaptivePortal(state, reason);
+  const bool is_behind_captive_portal = IsBehindCaptivePortal(state, reason);
   const bool is_proxy_error = IsProxyError(state, reason, FrameError());
   const bool is_gaia_loading_timeout =
       (reason == ErrorScreenActor::ERROR_REASON_LOADING_TIMEOUT);
 
-  // Record portal detection stats only if we're going to show or
-  // change state of the error screen.
-  RecordNetworkPortalDetectorStats(network_path);
-
   if (is_proxy_error) {
     error_screen_actor_->SetErrorState(ErrorScreen::ERROR_STATE_PROXY,
                                        std::string());
-  } else if (is_under_captive_portal) {
+  } else if (is_behind_captive_portal) {
     // Do not bother a user with obsessive captive portal showing. This
     // check makes captive portal being shown only once: either when error
     // screen is shown for the first time or when switching from another