Tether: record each type of host connection result.

chromeos/components/tether/tether_connector.cc

Index: chromeos/components/tether/tether_connector.cc
diff --git a/chromeos/components/tether/tether_connector.cc b/chromeos/components/tether/tether_connector.cc
index ee34c45041305f63f17e5e79e9226f1312daf6b7..da35093d4aae9cff6579847108081fa433d3cd76 100644
--- a/chromeos/components/tether/tether_connector.cc
+++ b/chromeos/components/tether/tether_connector.cc
@@ -7,6 +7,7 @@
 #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"
@@ -29,7 +30,8 @@ TetherConnector::TetherConnector(
     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),
@@ -39,6 +41,7 @@ TetherConnector::TetherConnector(
       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() {
@@ -174,6 +177,32 @@ void TetherConnector::OnConnectTetheringFailure(
                   << 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);
@@ -278,6 +307,10 @@ void TetherConnector::OnWifiConnection(const std::string& device_id,
   }
 
   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