Record background advertisement metric analogs for host connection results.

Bug: 819279
Change-Id: Iae36309deae890bfa84ac251eb6bfb4dca94d8f9
Reviewed-on: https://chromium-review.googlesource.com/1009066
Commit-Queue: Ryan Hansberry <hansberry@chromium.org>
Reviewed-by: Steven Holte <holte@chromium.org>
Reviewed-by: Kyle Horimoto <khorimoto@chromium.org>
Cr-Commit-Position: refs/heads/master@{#550437}

chromeos/components/tether/ble_connection_manager.cc

@@ -378,8 +378,7 @@ void BleConnectionManager::OnReceivedAdvertisementFromDevice(
 
   device_id_to_is_background_advertisement_map_[device_id] =
       is_background_advertisement;
-  for (auto& observer : metrics_observer_list_)
-    observer.OnAdvertisementReceived(device_id, is_background_advertisement);
+  NotifyAdvertisementReceived(device_id, is_background_advertisement);
 
   // Stop trying to connect to that device, since it has been found.
   StopConnectionAttemptAndMoveToEndOfQueue(device_id);
@@ -584,6 +583,13 @@ void BleConnectionManager::OnGattCharacteristicsNotAvailable(
   ad_hoc_ble_advertisement_->RequestGattServicesForDevice(device_id);
 }
 
+void BleConnectionManager::NotifyAdvertisementReceived(
+    const std::string& device_id,
+    bool is_background_advertisement) {
+  for (auto& observer : metrics_observer_list_)
+    observer.OnAdvertisementReceived(device_id, is_background_advertisement);
+}
+
 void BleConnectionManager::NotifyMessageReceived(std::string device_id,
                                                  std::string payload) {
   PA_LOG(INFO) << "Message received - Device ID: \""

chromeos/components/tether/ble_connection_manager.h

@@ -154,6 +154,8 @@ class BleConnectionManager : public BleScanner::Observer {
       bool is_background_advertisement) override;
 
  protected:
+  void NotifyAdvertisementReceived(const std::string& device_id,
+                                   bool is_background_advertisement);
   void NotifyMessageReceived(std::string device_id, std::string payload);
   void NotifySecureChannelStatusChanged(
       std::string device_id,

chromeos/components/tether/fake_ble_connection_manager.h

@@ -65,6 +65,8 @@ class FakeBleConnectionManager : public BleConnectionManager {
       const std::string& device_id,
       cryptauth::SecureChannel::Status* status) const override;
 
+  using BleConnectionManager::NotifyAdvertisementReceived;
+
  private:
   struct StatusAndRegisteredConnectionReasons {
     StatusAndRegisteredConnectionReasons();

chromeos/components/tether/host_connection_metrics_logger.cc

@@ -5,17 +5,34 @@
 #include "chromeos/components/tether/host_connection_metrics_logger.h"
 
 #include "base/metrics/histogram_macros.h"
+#include "base/time/default_clock.h"
 
 namespace chromeos {
 
 namespace tether {
 
-HostConnectionMetricsLogger::HostConnectionMetricsLogger() = default;
+HostConnectionMetricsLogger::HostConnectionMetricsLogger(
+    BleConnectionManager* connection_manager,
+    ActiveHost* active_host)
+    : connection_manager_(connection_manager),
+      active_host_(active_host),
+      clock_(base::DefaultClock::GetInstance()) {
+  connection_manager_->AddMetricsObserver(this);
+  active_host_->AddObserver(this);
+}
 
-HostConnectionMetricsLogger::~HostConnectionMetricsLogger() = default;
+HostConnectionMetricsLogger::~HostConnectionMetricsLogger() {
+  connection_manager_->RemoveMetricsObserver(this);
+  active_host_->RemoveObserver(this);
+}
 
 void HostConnectionMetricsLogger::RecordConnectionToHostResult(
-    ConnectionToHostResult result) {
+    ConnectionToHostResult result,
+    const std::string& device_id) {
+  // Persist this value for later use in RecordConnectionResultSuccess(). It
+  // will be cleared once used.
+  active_host_device_id_ = device_id;
+
   switch (result) {
     case ConnectionToHostResult::CONNECTION_RESULT_PROVISIONING_FAILED:
       RecordConnectionResultProvisioningFailure(
@@ -83,6 +100,23 @@ void HostConnectionMetricsLogger::RecordConnectionToHostResult(
   };
 }
 
+void HostConnectionMetricsLogger::OnAdvertisementReceived(
+    const std::string& device_id,
+    bool is_background_advertisement) {
+  device_id_to_received_background_advertisement_[device_id] =
+      is_background_advertisement;
+}
+
+void HostConnectionMetricsLogger::OnActiveHostChanged(
+    const ActiveHost::ActiveHostChangeInfo& change_info) {
+  if (change_info.new_status == ActiveHost::ActiveHostStatus::CONNECTING) {
+    connect_to_host_start_time_ = clock_->Now();
+  } else if (change_info.new_status ==
+             ActiveHost::ActiveHostStatus::CONNECTED) {
+    RecordConnectToHostDuration(change_info.new_active_host->GetDeviceId());
+  }
+}
+
 void HostConnectionMetricsLogger::RecordConnectionResultProvisioningFailure(
     ConnectionToHostResult_ProvisioningFailureEventType event_type) {
   UMA_HISTOGRAM_ENUMERATION(
@@ -94,9 +128,21 @@ void HostConnectionMetricsLogger::RecordConnectionResultProvisioningFailure(
 
 void HostConnectionMetricsLogger::RecordConnectionResultSuccess(
     ConnectionToHostResult_SuccessEventType event_type) {
-  UMA_HISTOGRAM_ENUMERATION(
-      "InstantTethering.ConnectionToHostResult.SuccessRate", event_type,
-      ConnectionToHostResult_SuccessEventType::SUCCESS_MAX);
+  DCHECK(!active_host_device_id_.empty());
+
+  bool is_background_advertisement =
+      device_id_to_received_background_advertisement_[active_host_device_id_];
+  active_host_device_id_.clear();
+
+  if (is_background_advertisement) {
+    UMA_HISTOGRAM_ENUMERATION(
+        "InstantTethering.ConnectionToHostResult.SuccessRate.Background",
+        event_type, ConnectionToHostResult_SuccessEventType::SUCCESS_MAX);
+  } else {
+    UMA_HISTOGRAM_ENUMERATION(
+        "InstantTethering.ConnectionToHostResult.SuccessRate", event_type,
+        ConnectionToHostResult_SuccessEventType::SUCCESS_MAX);
+  }
 
   RecordConnectionResultProvisioningFailure(
       ConnectionToHostResult_ProvisioningFailureEventType::OTHER);
@@ -136,6 +182,32 @@ void HostConnectionMetricsLogger::RecordConnectionResultFailureTetheringTimeout(
       ConnectionToHostResult_FailureEventType::TETHERING_TIMED_OUT);
 }
 
+void HostConnectionMetricsLogger::RecordConnectToHostDuration(
+    const std::string device_id) {
+  DCHECK(!connect_to_host_start_time_.is_null());
+
+  bool is_background_advertisement =
+      device_id_to_received_background_advertisement_[device_id];
+
+  base::TimeDelta connect_to_host_duration =
+      clock_->Now() - connect_to_host_start_time_;
+  connect_to_host_start_time_ = base::Time();
+
+  if (is_background_advertisement) {
+    UMA_HISTOGRAM_MEDIUM_TIMES(
+        "InstantTethering.Performance.ConnectToHostDuration.Background",
+        connect_to_host_duration);
+  } else {
+    UMA_HISTOGRAM_MEDIUM_TIMES(
+        "InstantTethering.Performance.ConnectToHostDuration",
+        connect_to_host_duration);
+  }
+}
+
+void HostConnectionMetricsLogger::SetClockForTesting(base::Clock* test_clock) {
+  clock_ = test_clock;
+}
+
 }  // namespace tether
 
 }  // namespace chromeos

chromeos/components/tether/host_connection_metrics_logger.h

@@ -5,8 +5,17 @@
 #ifndef CHROMEOS_COMPONENTS_TETHER_HOST_CONNECTION_METRICS_LOGGER_H_
 #define CHROMEOS_COMPONENTS_TETHER_HOST_CONNECTION_METRICS_LOGGER_H_
 
+#include <map>
+#include <string>
+
 #include "base/gtest_prod_util.h"
 #include "base/macros.h"
+#include "chromeos/components/tether/active_host.h"
+#include "chromeos/components/tether/ble_connection_manager.h"
+
+namespace base {
+class Clock;
+}
 
 namespace chromeos {
 
@@ -15,7 +24,9 @@ namespace tether {
 // Wrapper around metrics reporting for host connection results. Clients are
 // expected to report the result of a host connection attempt once it has
 // concluded.
-class HostConnectionMetricsLogger {
+class HostConnectionMetricsLogger
+    : public BleConnectionManager::MetricsObserver,
+      public ActiveHost::Observer {
  public:
   enum ConnectionToHostResult {
     CONNECTION_RESULT_PROVISIONING_FAILED,
@@ -33,19 +44,40 @@ class HostConnectionMetricsLogger {
   };
 
   // Record the result of an attempted host connection.
-  virtual void RecordConnectionToHostResult(ConnectionToHostResult result);
+  virtual void RecordConnectionToHostResult(ConnectionToHostResult result,
+                                            const std::string& device_id);
 
-  HostConnectionMetricsLogger();
+  HostConnectionMetricsLogger(BleConnectionManager* connection_manager,
+                              ActiveHost* active_host);
   virtual ~HostConnectionMetricsLogger();
 
+ protected:
+  // BleConnectionManager::MetricsObserver:
+  void OnAdvertisementReceived(const std::string& device_id,
+                               bool is_background_advertisement) override;
+
+  // ActiveHost::Observer:
+  void OnActiveHostChanged(
+      const ActiveHost::ActiveHostChangeInfo& change_info) override;
+
  private:
   friend class HostConnectionMetricsLoggerTest;
   FRIEND_TEST_ALL_PREFIXES(HostConnectionMetricsLoggerTest,
                            RecordConnectionResultProvisioningFailure);
   FRIEND_TEST_ALL_PREFIXES(HostConnectionMetricsLoggerTest,
                            RecordConnectionResultSuccess);
+  FRIEND_TEST_ALL_PREFIXES(HostConnectionMetricsLoggerTest,
+                           RecordConnectionResultSuccess_Background);
+  FRIEND_TEST_ALL_PREFIXES(
+      HostConnectionMetricsLoggerTest,
+      RecordConnectionResultSuccess_Background_DifferentDevice);
   FRIEND_TEST_ALL_PREFIXES(HostConnectionMetricsLoggerTest,
                            RecordConnectionResultFailure);
+  FRIEND_TEST_ALL_PREFIXES(HostConnectionMetricsLoggerTest,
+                           RecordConnectionResultFailure_Background);
+  FRIEND_TEST_ALL_PREFIXES(
+      HostConnectionMetricsLoggerTest,
+      RecordConnectionResultFailure_Background_DifferentDevice);
   FRIEND_TEST_ALL_PREFIXES(
       HostConnectionMetricsLoggerTest,
       RecordConnectionResultFailureClientConnection_Timeout);
@@ -69,6 +101,10 @@ class HostConnectionMetricsLogger {
                            RecordConnectionResultFailureEnablingHotspotFailed);
   FRIEND_TEST_ALL_PREFIXES(HostConnectionMetricsLoggerTest,
                            RecordConnectionResultFailureEnablingHotspotTimeout);
+  FRIEND_TEST_ALL_PREFIXES(HostConnectionMetricsLoggerTest,
+                           RecordConnectToHostDuration);
+  FRIEND_TEST_ALL_PREFIXES(HostConnectionMetricsLoggerTest,
+                           RecordConnectToHostDuration_Background);
 
   // An Instant Tethering connection can fail for several different reasons.
   // Though traditionally success and each failure case would be logged to a
@@ -146,6 +182,18 @@ class HostConnectionMetricsLogger {
   void RecordConnectionResultFailureTetheringTimeout(
       ConnectionToHostResult_FailureTetheringTimeoutEventType event_type);
 
+  void RecordConnectToHostDuration(const std::string device_id);
+
+  void SetClockForTesting(base::Clock* test_clock);
+
+  BleConnectionManager* connection_manager_;
+  ActiveHost* active_host_;
+  base::Clock* clock_;
+
+  std::map<std::string, bool> device_id_to_received_background_advertisement_;
+  base::Time connect_to_host_start_time_;
+  std::string active_host_device_id_;
+
   DISALLOW_COPY_AND_ASSIGN(HostConnectionMetricsLogger);
 };
 

chromeos/components/tether/mock_host_connection_metrics_logger.cc

@@ -4,11 +4,17 @@
 
 #include "chromeos/components/tether/mock_host_connection_metrics_logger.h"
 
+#include "chromeos/components/tether/active_host.h"
+#include "chromeos/components/tether/ble_connection_manager.h"
+
 namespace chromeos {
 
 namespace tether {
 
-MockHostConnectionMetricsLogger::MockHostConnectionMetricsLogger() = default;
+MockHostConnectionMetricsLogger::MockHostConnectionMetricsLogger(
+    BleConnectionManager* connection_manager,
+    ActiveHost* active_host)
+    : HostConnectionMetricsLogger(connection_manager, active_host) {}
 
 MockHostConnectionMetricsLogger::~MockHostConnectionMetricsLogger() = default;
 

chromeos/components/tether/mock_host_connection_metrics_logger.h

@@ -5,6 +5,8 @@
 #ifndef CHROMEOS_COMPONENTS_TETHER_MOCK_HOST_CONNECTION_METRICS_LOGGER_H_
 #define CHROMEOS_COMPONENTS_TETHER_MOCK_HOST_CONNECTION_METRICS_LOGGER_H_
 
+#include <string>
+
 #include "chromeos/components/tether/host_connection_metrics_logger.h"
 #include "testing/gmock/include/gmock/gmock.h"
 
@@ -12,13 +14,18 @@ namespace chromeos {
 
 namespace tether {
 
+class ActiveHost;
+class BleConnectionManager;
+
 class MockHostConnectionMetricsLogger : public HostConnectionMetricsLogger {
  public:
-  MockHostConnectionMetricsLogger();
+  MockHostConnectionMetricsLogger(BleConnectionManager* connection_manager,
+                                  ActiveHost* active_host);
   ~MockHostConnectionMetricsLogger() override;
 
-  MOCK_METHOD1(RecordConnectionToHostResult,
-               void(HostConnectionMetricsLogger::ConnectionToHostResult));
+  MOCK_METHOD2(RecordConnectionToHostResult,
+               void(HostConnectionMetricsLogger::ConnectionToHostResult,
+                    const std::string&));
 
  private:
   DISALLOW_COPY_AND_ASSIGN(MockHostConnectionMetricsLogger);

chromeos/components/tether/synchronous_shutdown_object_container_impl.cc

@@ -164,7 +164,9 @@ SynchronousShutdownObjectContainerImpl::SynchronousShutdownObjectContainerImpl(
                                                   host_scanner_.get(),
                                                   session_manager)),
       host_connection_metrics_logger_(
-          std::make_unique<HostConnectionMetricsLogger>()),
+          std::make_unique<HostConnectionMetricsLogger>(
+              asychronous_container->ble_connection_manager(),
+              active_host_.get())),
       tether_connector_(std::make_unique<TetherConnectorImpl>(
           network_state_handler_,
           wifi_hotspot_connector_.get(),
@@ -177,8 +179,7 @@ SynchronousShutdownObjectContainerImpl::SynchronousShutdownObjectContainerImpl(
           notification_presenter,
           host_connection_metrics_logger_.get(),
           asychronous_container->disconnect_tethering_request_sender(),
-          asychronous_container->wifi_hotspot_disconnector(),
-          base::DefaultClock::GetInstance())),
+          asychronous_container->wifi_hotspot_disconnector())),
       tether_disconnector_(std::make_unique<TetherDisconnectorImpl>(
           active_host_.get(),
           asychronous_container->wifi_hotspot_disconnector(),

chromeos/components/tether/tether_connector_impl.cc

@@ -6,9 +6,9 @@
 
 #include "base/bind.h"
 #include "base/metrics/histogram_macros.h"
-#include "base/time/clock.h"
 #include "chromeos/components/proximity_auth/logging/logging.h"
 #include "chromeos/components/tether/active_host.h"
+#include "chromeos/components/tether/ble_connection_manager.h"
 #include "chromeos/components/tether/device_id_tether_network_guid_map.h"
 #include "chromeos/components/tether/disconnect_tethering_request_sender.h"
 #include "chromeos/components/tether/host_connection_metrics_logger.h"
@@ -48,8 +48,7 @@ TetherConnectorImpl::TetherConnectorImpl(
     NotificationPresenter* notification_presenter,
     HostConnectionMetricsLogger* host_connection_metrics_logger,
     DisconnectTetheringRequestSender* disconnect_tethering_request_sender,
-    WifiHotspotDisconnector* wifi_hotspot_disconnector,
-    base::Clock* clock)
+    WifiHotspotDisconnector* wifi_hotspot_disconnector)
     : network_state_handler_(network_state_handler),
       wifi_hotspot_connector_(wifi_hotspot_connector),
       active_host_(active_host),
@@ -62,7 +61,6 @@ TetherConnectorImpl::TetherConnectorImpl(
       host_connection_metrics_logger_(host_connection_metrics_logger),
       disconnect_tethering_request_sender_(disconnect_tethering_request_sender),
       wifi_hotspot_disconnector_(wifi_hotspot_disconnector),
-      clock_(clock),
       weak_ptr_factory_(this) {}
 
 TetherConnectorImpl::~TetherConnectorImpl() {
@@ -105,7 +103,6 @@ void TetherConnectorImpl::ConnectToNetwork(
   device_id_pending_connection_ = device_id;
   success_callback_ = success_callback;
   error_callback_ = error_callback;
-  connect_to_host_start_time_ = clock_->Now();
   active_host_->SetActiveHostConnecting(device_id, tether_network_guid);
 
   tether_host_fetcher_->FetchTetherHost(
@@ -274,7 +271,9 @@ void TetherConnectorImpl::SetConnectionFailed(
   // Save a copy of the callback before resetting it below.
   network_handler::StringResultCallback error_callback = error_callback_;
 
+  std::string failed_connection_device_id = device_id_pending_connection_;
   device_id_pending_connection_.clear();
+
   success_callback_.Reset();
   error_callback_.Reset();
 
@@ -282,7 +281,7 @@ void TetherConnectorImpl::SetConnectionFailed(
   active_host_->SetActiveHostDisconnected();
 
   host_connection_metrics_logger_->RecordConnectionToHostResult(
-      connection_to_host_result);
+      connection_to_host_result, failed_connection_device_id);
 
   if (error_name == NetworkConnectionHandler::kErrorConnectFailed) {
     // Only show notification if the error is kErrorConnectFailed. Other error
@@ -301,10 +300,8 @@ void TetherConnectorImpl::SetConnectionSucceeded(
 
   host_connection_metrics_logger_->RecordConnectionToHostResult(
       HostConnectionMetricsLogger::ConnectionToHostResult::
-          CONNECTION_RESULT_SUCCESS);
-  UMA_HISTOGRAM_MEDIUM_TIMES(
-      "InstantTethering.Performance.ConnectToHostDuration",
-      clock_->Now() - connect_to_host_start_time_);
+          CONNECTION_RESULT_SUCCESS,
+      device_id);
 
   notification_presenter_->RemoveSetupRequiredNotification();
 

chromeos/components/tether/tether_connector_impl.h

@@ -12,10 +12,6 @@
 #include "chromeos/components/tether/tether_connector.h"
 #include "chromeos/network/network_connection_handler.h"
 
-namespace base {
-class Clock;
-}  // namespace base
-
 namespace chromeos {
 
 class NetworkStateHandler;
@@ -53,8 +49,7 @@ class TetherConnectorImpl : public TetherConnector,
       NotificationPresenter* notification_presenter,
       HostConnectionMetricsLogger* host_connection_metrics_logger,
       DisconnectTetheringRequestSender* disconnect_tethering_request_sender,
-      WifiHotspotDisconnector* wifi_hotspot_disconnector,
-      base::Clock* clock_);
+      WifiHotspotDisconnector* wifi_hotspot_disconnector);
   ~TetherConnectorImpl() override;
 
   void ConnectToNetwork(
@@ -108,7 +103,6 @@ class TetherConnectorImpl : public TetherConnector,
   HostConnectionMetricsLogger* host_connection_metrics_logger_;
   DisconnectTetheringRequestSender* disconnect_tethering_request_sender_;
   WifiHotspotDisconnector* wifi_hotspot_disconnector_;
-  base::Clock* clock_;
 
   std::string device_id_pending_connection_;
   base::Closure success_callback_;

tools/metrics/histograms/histograms.xml

@@ -32515,6 +32515,22 @@ http://cs/file:chrome/histograms.xml - but prefer this file for new entries.
   </summary>
 </histogram>
 
+<histogram
+    name="InstantTethering.ConnectionToHostResult.SuccessRate.Background"
+    enum="InstantTethering_ConnectionToHostResult_SuccessRate">
+  <owner>hansberry@chromium.org</owner>
+  <summary>
+    Captures the count of successful and failed connection attempts.
+
+    This metric counts the top-level user action from beginning to connect, all
+    the way through success or failure of the connection (excluding any
+    programmatic retries within the connection attempt).
+
+    This histogram is the background advertisement analog of
+    InstantTethering.Performance.ConnectionToHostResult.SuccessRate.
+  </summary>
+</histogram>
+
 <histogram name="InstantTethering.FeatureState"
     enum="InstantTethering_FeatureState">
   <owner>hansberry@chromium.org</owner>
@@ -32703,6 +32719,19 @@ http://cs/file:chrome/histograms.xml - but prefer this file for new entries.
   </summary>
 </histogram>
 
+<histogram name="InstantTethering.Performance.ConnectToHostDuration.Background"
+    units="ms">
+  <owner>hansberry@chromium.org</owner>
+  <summary>
+    The duration of time it takes for the client to connect to the host, from
+    the moment the user taps 'Connect', until the client connects to the host's
+    hotspot. This does not include timeouts.
+
+    This histogram is the background advertisement analog of
+    InstantTethering.Performance.ConnectToHostDuration.Background.
+  </summary>
+</histogram>
+
 <histogram name="InstantTethering.Performance.ConnectToHotspotDuration"
     units="ms">
   <owner>hansberry@chromium.org</owner>