Tether: record each type of host connection result.

chromeos/components/tether/tether_connector.cc

 // Copyright 2017 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 "chromeos/components/tether/tether_connector.h"
 
 #include "base/bind.h"
 #include "chromeos/components/tether/active_host.h"
 #include "chromeos/components/tether/device_id_tether_network_guid_map.h"
+#include "chromeos/components/tether/host_connection_metrics_logger.h"
 #include "chromeos/components/tether/host_scan_cache.h"
 #include "chromeos/components/tether/notification_presenter.h"
 #include "chromeos/components/tether/tether_host_fetcher.h"
 #include "chromeos/components/tether/wifi_hotspot_connector.h"
 #include "chromeos/network/network_handler.h"
 #include "chromeos/network/network_state.h"
 #include "chromeos/network/network_state_handler.h"
 #include "components/proximity_auth/logging/logging.h"
 
 namespace chromeos {
 
 namespace tether {
 
 TetherConnector::TetherConnector(
     NetworkStateHandler* network_state_handler,
     WifiHotspotConnector* wifi_hotspot_connector,
     ActiveHost* active_host,
     TetherHostFetcher* tether_host_fetcher,
     BleConnectionManager* connection_manager,
     TetherHostResponseRecorder* tether_host_response_recorder,
     DeviceIdTetherNetworkGuidMap* device_id_tether_network_guid_map,
     HostScanCache* host_scan_cache,
-    NotificationPresenter* notification_presenter)
+    NotificationPresenter* notification_presenter,
+    HostConnectionMetricsLogger* host_connection_metrics_logger)
     : network_state_handler_(network_state_handler),
       wifi_hotspot_connector_(wifi_hotspot_connector),
       active_host_(active_host),
       tether_host_fetcher_(tether_host_fetcher),
       connection_manager_(connection_manager),
       tether_host_response_recorder_(tether_host_response_recorder),
       device_id_tether_network_guid_map_(device_id_tether_network_guid_map),
       host_scan_cache_(host_scan_cache),
       notification_presenter_(notification_presenter),
+      host_connection_metrics_logger_(host_connection_metrics_logger),
       weak_ptr_factory_(this) {}
 
 TetherConnector::~TetherConnector() {
   if (connect_tethering_operation_) {
     connect_tethering_operation_->RemoveObserver(this);
   }
 }
 
 void TetherConnector::ConnectToNetwork(
     const std::string& tether_network_guid,
@@ skipping 115 matching lines @@
     PA_LOG(INFO) << "Received failed ConnectTetheringResponse from device with "
                  << "ID " << remote_device.GetTruncatedDeviceIdForLogs()
                  << ", but a connection to another device has already started.";
     return;
   }
 
   PA_LOG(WARNING) << "Connection to device with ID "
                   << remote_device.GetTruncatedDeviceIdForLogs()
                   << " could not complete. Error code: " << error_code;
 
+  if (error_code ==

[Kyle Horimoto, 2017/07/05 21:49:13]

You should move this to SetConnectionFailed() instead. There are other ways that
the connection can fail other than just an error with the operation.

You will need to add an extra parameter to the function.

[Ryan Hansberry, 2017/07/07 02:33:16]

You're right that it doesn't just fail with the operation failing, but what
parameter were you thinking of passing that would capture operation failures,
the connection being canceled, tether host fetch failure, and wifi connection
timeout? I suppose that you could pass a
HostConnectionMetricsLogger::ConnectionToHostResult enum value, but that means
this logic needs to remain here.

As a sidenote, I realized that tether host fetch failure and cancellation via
the Disconnect button are also client connection errors. I've created  a CL that
modifies that metric appropriately here:
https://chromium-review.googlesource.com/c/562573/ and rebased this CL on top of
it.

[Kyle Horimoto, 2017/07/07 17:06:33]

Yep, I was thinking you could pass a ConnectionToHostResult enum value to that
function. Can you clarify what you mean by "this logic needs to remain here"?
Not sure what you meant there.

I've commented on that CL, and I'm not sure I agree with your new metric. You
can read my more detailed response there, but I essentially think that this
error should be categorized in an "internal error" bucket.

In any case, I still think you should move this to SetConnectionFailed(). Let me
know if you have another idea.

[Ryan Hansberry, 2017/07/12 16:37:06]

Think I've done what you were expecting -- PTAL.

+      ConnectTetheringResponse_ResponseCode::
+          ConnectTetheringResponse_ResponseCode_PROVISIONING_FAILED) {
+    host_connection_metrics_logger_->RecordConnectionToHostResult(
+        HostConnectionMetricsLogger::ConnectionToHostResult::
+            CONNECTION_RESULT_PROVISIONING_FAILED);
+  } else if (error_code ==
+             ConnectTetheringResponse_ResponseCode::
+                 ConnectTetheringResponse_ResponseCode_TETHERING_TIMEOUT) {
+    const std::string& tether_network_guid =

[Kyle Horimoto, 2017/07/05 21:49:13]

GetTetherNetworkGuidForDeviceId() doesn't return a reference - please remove the
&.

[Ryan Hansberry, 2017/07/07 02:33:16]

Good catch -- fixed it at another place in this file that made the same mistake.

+        device_id_tether_network_guid_map_->GetTetherNetworkGuidForDeviceId(
+            remote_device.GetDeviceId());
+    bool setup_was_required =
+        host_scan_cache_->DoesHostRequireSetup(tether_network_guid);
+    host_connection_metrics_logger_->RecordConnectionToHostResult(
+        setup_was_required
+            ? HostConnectionMetricsLogger::ConnectionToHostResult::
+                  CONNECTION_RESULT_FAILURE_TETHERING_TIMED_OUT_FIRST_TIME_SETUP_WAS_REQUIRED
+            : HostConnectionMetricsLogger::ConnectionToHostResult::
+                  CONNECTION_RESULT_FAILURE_TETHERING_TIMED_OUT_FIRST_TIME_SETUP_WAS_NOT_REQUIRED);
+  } else {
+    host_connection_metrics_logger_->RecordConnectionToHostResult(
+        HostConnectionMetricsLogger::ConnectionToHostResult::
+            CONNECTION_RESULT_FAILURE_UNKNOWN_ERROR);
+  }
+
   connect_tethering_operation_->RemoveObserver(this);
   connect_tethering_operation_.reset();
   SetConnectionFailed(NetworkConnectionHandler::kErrorConnectFailed);
 }
 
 void TetherConnector::OnTetherHostToConnectFetched(
     const std::string& device_id,
     std::unique_ptr<cryptauth::RemoteDevice> tether_host_to_connect) {
   if (device_id_pending_connection_ != device_id) {
     PA_LOG(INFO) << "Device to connect to has changed while device with ID "
@@ skipping 84 matching lines @@
     PA_LOG(ERROR) << "Failed to connect to the hotspot belonging to the device "
                   << "with ID "
                   << cryptauth::RemoteDevice::TruncateDeviceIdForLogs(device_id)
                   << ".";
 
     SetConnectionFailed(NetworkConnectionHandler::kErrorConnectFailed);
     return;
   }
 
   SetConnectionSucceeded(device_id, wifi_network_guid);
+
+  host_connection_metrics_logger_->RecordConnectionToHostResult(

[Kyle Horimoto, 2017/07/05 21:49:13]

Likewise, this belongs in SetConnectionSucceeded().

[Ryan Hansberry, 2017/07/07 02:33:16]

Done.

+      HostConnectionMetricsLogger::ConnectionToHostResult::
+          CONNECTION_RESULT_SUCCESS);
 }
 
 }  // namespace tether
 
 }  // namespace chromeos