Tether: record each type of host connection result.

Tether: record each type of host connection result.

BUG=672263
Review-Url: https://codereview.chromium.org/2949343002
Cr-Commit-Position: refs/heads/master@{#486209}
Committed: https://chromium.googlesource.com/chromium/src/+/82f5bf70aee06e7a832af55527b4543818e2e0fc

[Ryan Hansberry, 2017-06-22 20:33:19]

Description was changed from

==========
Tether: record basic usage metrics.

BUG=
==========

to

==========
Tether: record basic usage metrics.

BUG=672263
==========

[Ryan Hansberry, 2017-06-22 20:33:20]

hansberry@chromium.org changed reviewers:
+ khorimoto@chromium.org

[Ryan Hansberry, 2017-07-05 21:22:45]

Description was changed from

==========
Tether: record basic usage metrics.

BUG=672263
==========

to

==========
Tether: record each type of host connection result.

BUG=672263
==========

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

https://codereview.chromium.org/2949343002/diff/80001/chromeos/components/tet...
File chromeos/components/tether/tether_connector.cc (right):

https://codereview.chromium.org/2949343002/diff/80001/chromeos/components/tet...
chromeos/components/tether/tether_connector.cc:180: if (error_code ==
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.

https://codereview.chromium.org/2949343002/diff/80001/chromeos/components/tet...
chromeos/components/tether/tether_connector.cc:189: const std::string&
tether_network_guid =
GetTetherNetworkGuidForDeviceId() doesn't return a reference - please remove the
&.

https://codereview.chromium.org/2949343002/diff/80001/chromeos/components/tet...
chromeos/components/tether/tether_connector.cc:311:
host_connection_metrics_logger_->RecordConnectionToHostResult(
Likewise, this belongs in SetConnectionSucceeded().

https://codereview.chromium.org/2949343002/diff/80001/chromeos/components/tet...
File chromeos/components/tether/tether_connector_unittest.cc (right):

https://codereview.chromium.org/2949343002/diff/80001/chromeos/components/tet...
chromeos/components/tether/tether_connector_unittest.cc:253:
std::unique_ptr<MockHostConnectionMetricsLogger>
Please make this a std::unique_ptr<StrictMock<MockHostConnectionMetricsLogger>>
so that we can ensure the correct mock functions are called in all cases.

You should add a "using testing::StrictMock;" at the top of the test file.

https://codereview.chromium.org/2949343002/diff/80001/chromeos/components/tet...
chromeos/components/tether/tether_connector_unittest.cc:374:
TEST_F(TetherConnectorTest, TestProvisioningFailureOnHost) {
This seems like TestConnectTetheringOperationFails() and
TestTetheringTimeoutOnHost() except with a different error value. Can you make a
helper function and call it from these tests?

https://codereview.chromium.org/2949343002/diff/80001/chromeos/components/tet...
chromeos/components/tether/tether_connector_unittest.cc:414:
TEST_F(TetherConnectorTest, TestTetheringTimeoutOnHost_SetupRequired) {
Likewise, this seems like the same thing as
TestConnectTetheringOperationFails_SetupRequired() except with a different error
value. Can you share code?

https://codereview.chromium.org/2949343002/diff/80001/chromeos/components/tet...
File chromeos/components/tether/wifi_hotspot_connector.h (right):

https://codereview.chromium.org/2949343002/diff/80001/chromeos/components/tet...
chromeos/components/tether/wifi_hotspot_connector.h:1: // Copyright 2016 The
Chromium Authors. All rights reserved.
Please revert changes to WifiHotspotConnector. You can cover this case in
TetherConnector. Look at ConnectToNetwork() for the logic of what happens when
an in-progress connection is interrupted.

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

https://codereview.chromium.org/2949343002/diff/80001/chromeos/components/tet...
File chromeos/components/tether/tether_connector.cc (right):

https://codereview.chromium.org/2949343002/diff/80001/chromeos/components/tet...
chromeos/components/tether/tether_connector.cc:180: if (error_code ==
On 2017/07/05 21:49:13, Kyle Horimoto wrote:
> 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.

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.

https://codereview.chromium.org/2949343002/diff/80001/chromeos/components/tet...
chromeos/components/tether/tether_connector.cc:189: const std::string&
tether_network_guid =
On 2017/07/05 21:49:13, Kyle Horimoto wrote:
> GetTetherNetworkGuidForDeviceId() doesn't return a reference - please remove
the
> &.

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

https://codereview.chromium.org/2949343002/diff/80001/chromeos/components/tet...
chromeos/components/tether/tether_connector.cc:311:
host_connection_metrics_logger_->RecordConnectionToHostResult(
On 2017/07/05 21:49:13, Kyle Horimoto wrote:
> Likewise, this belongs in SetConnectionSucceeded().

Done.

https://codereview.chromium.org/2949343002/diff/80001/chromeos/components/tet...
File chromeos/components/tether/tether_connector_unittest.cc (right):

https://codereview.chromium.org/2949343002/diff/80001/chromeos/components/tet...
chromeos/components/tether/tether_connector_unittest.cc:253:
std::unique_ptr<MockHostConnectionMetricsLogger>
On 2017/07/05 21:49:13, Kyle Horimoto wrote:
> Please make this a
std::unique_ptr<StrictMock<MockHostConnectionMetricsLogger>>
> so that we can ensure the correct mock functions are called in all cases.
> 
> You should add a "using testing::StrictMock;" at the top of the test file.

Done.

https://codereview.chromium.org/2949343002/diff/80001/chromeos/components/tet...
chromeos/components/tether/tether_connector_unittest.cc:374:
TEST_F(TetherConnectorTest, TestProvisioningFailureOnHost) {
On 2017/07/05 21:49:13, Kyle Horimoto wrote:
> This seems like TestConnectTetheringOperationFails() and
> TestTetheringTimeoutOnHost() except with a different error value. Can you make
a
> helper function and call it from these tests?

Good suggestion. Done.

https://codereview.chromium.org/2949343002/diff/80001/chromeos/components/tet...
chromeos/components/tether/tether_connector_unittest.cc:414:
TEST_F(TetherConnectorTest, TestTetheringTimeoutOnHost_SetupRequired) {
On 2017/07/05 21:49:13, Kyle Horimoto wrote:
> Likewise, this seems like the same thing as
> TestConnectTetheringOperationFails_SetupRequired() except with a different
error
> value. Can you share code?

Done.

https://codereview.chromium.org/2949343002/diff/80001/chromeos/components/tet...
File chromeos/components/tether/wifi_hotspot_connector.h (right):

https://codereview.chromium.org/2949343002/diff/80001/chromeos/components/tet...
chromeos/components/tether/wifi_hotspot_connector.h:1: // Copyright 2016 The
Chromium Authors. All rights reserved.
On 2017/07/05 21:49:13, Kyle Horimoto wrote:
> Please revert changes to WifiHotspotConnector. You can cover this case in
> TetherConnector. Look at ConnectToNetwork() for the logic of what happens when
> an in-progress connection is interrupted.

Done.

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

Will revisit tests once I get more clarity on the rest of your CL.

https://codereview.chromium.org/2949343002/diff/80001/chromeos/components/tet...
File chromeos/components/tether/tether_connector.cc (right):

https://codereview.chromium.org/2949343002/diff/80001/chromeos/components/tet...
chromeos/components/tether/tether_connector.cc:180: if (error_code ==
On 2017/07/07 02:33:16, Ryan Hansberry wrote:
> On 2017/07/05 21:49:13, Kyle Horimoto wrote:
> > 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.
> 
> 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.

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.

https://codereview.chromium.org/2949343002/diff/100001/chromeos/components/te...
File chromeos/components/tether/tether_connector.cc (right):

https://codereview.chromium.org/2949343002/diff/100001/chromeos/components/te...
chromeos/components/tether/tether_connector.cc:105:
host_connection_metrics_logger_->RecordConnectionToHostResult(
Move this down to after the device ID check. This shouldn't be logged if it
doesn't actually correspond to an active attempt.

https://codereview.chromium.org/2949343002/diff/100001/chromeos/components/te...
chromeos/components/tether/tether_connector.cc:230:
host_connection_metrics_logger_->RecordConnectionToHostResult(
See comment on other CL - I think this should just be an internal error. Also,
should be moved to SetConnectionFailed().

https://codereview.chromium.org/2949343002/diff/100001/chromeos/components/te...
chromeos/components/tether/tether_connector.cc:316:
host_connection_metrics_logger_->RecordConnectionToHostResult(
Same thing - think you should move to SetConnectionFailed().

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

https://codereview.chromium.org/2949343002/diff/80001/chromeos/components/tet...
File chromeos/components/tether/tether_connector.cc (right):

https://codereview.chromium.org/2949343002/diff/80001/chromeos/components/tet...
chromeos/components/tether/tether_connector.cc:180: if (error_code ==
On 2017/07/07 17:06:33, Kyle Horimoto wrote:
> On 2017/07/07 02:33:16, Ryan Hansberry wrote:
> > On 2017/07/05 21:49:13, Kyle Horimoto wrote:
> > > 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.
> > 
> > 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.
> 
> 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.

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

https://codereview.chromium.org/2949343002/diff/100001/chromeos/components/te...
File chromeos/components/tether/tether_connector.cc (right):

https://codereview.chromium.org/2949343002/diff/100001/chromeos/components/te...
chromeos/components/tether/tether_connector.cc:105:
host_connection_metrics_logger_->RecordConnectionToHostResult(
On 2017/07/07 17:06:33, Kyle Horimoto wrote:
> Move this down to after the device ID check. This shouldn't be logged if it
> doesn't actually correspond to an active attempt.

Done.

https://codereview.chromium.org/2949343002/diff/100001/chromeos/components/te...
chromeos/components/tether/tether_connector.cc:230:
host_connection_metrics_logger_->RecordConnectionToHostResult(
On 2017/07/07 17:06:33, Kyle Horimoto wrote:
> See comment on other CL - I think this should just be an internal error. Also,
> should be moved to SetConnectionFailed().

Done.

https://codereview.chromium.org/2949343002/diff/100001/chromeos/components/te...
chromeos/components/tether/tether_connector.cc:316:
host_connection_metrics_logger_->RecordConnectionToHostResult(
On 2017/07/07 17:06:33, Kyle Horimoto wrote:
> Same thing - think you should move to SetConnectionFailed().

Done.

[Kyle Horimoto, 2017-07-12 17:23:21]

https://codereview.chromium.org/2949343002/diff/140001/chromeos/components/te...
File chromeos/components/tether/tether_connector.cc (right):

https://codereview.chromium.org/2949343002/diff/140001/chromeos/components/te...
chromeos/components/tether/tether_connector.cc:183:
HostConnectionMetricsLogger::ConnectionToHostResult connection_to_host_result;
Move this to a helper function, and remove the commented-out code.

https://codereview.chromium.org/2949343002/diff/140001/chromeos/components/te...
chromeos/components/tether/tether_connector.cc:245: //
host_connection_metrics_logger_->RecordConnectionToHostResult(
Remove.

https://codereview.chromium.org/2949343002/diff/140001/chromeos/components/te...
chromeos/components/tether/tether_connector.cc:337: //
host_connection_metrics_logger_->RecordConnectionToHostResult(
Remove.

https://codereview.chromium.org/2949343002/diff/140001/chromeos/components/te...
File chromeos/components/tether/tether_connector_unittest.cc (right):

https://codereview.chromium.org/2949343002/diff/140001/chromeos/components/te...
chromeos/components/tether/tether_connector_unittest.cc:234: void
TestConnectTetheringOperationFails(
VerifyConnectTetheringOperationFails

https://codereview.chromium.org/2949343002/diff/140001/chromeos/components/te...
chromeos/components/tether/tether_connector_unittest.cc:244:
cryptauth::RemoteDevice test_device = test_devices_[setup_required ? 1 : 0];
It's hard to tell why setup_required determines the index used without reading
through the whole test. Add a comment explaining that this corresponds to the
AddTetherNetwork() call in SetUpTetherNetworks() above.

[Ryan Hansberry, 2017-07-12 17:51:18]

https://codereview.chromium.org/2949343002/diff/140001/chromeos/components/te...
File chromeos/components/tether/tether_connector.cc (right):

https://codereview.chromium.org/2949343002/diff/140001/chromeos/components/te...
chromeos/components/tether/tether_connector.cc:183:
HostConnectionMetricsLogger::ConnectionToHostResult connection_to_host_result;
On 2017/07/12 17:23:21, Kyle Horimoto wrote:
> Move this to a helper function, and remove the commented-out code.

Oops :) Done.

https://codereview.chromium.org/2949343002/diff/140001/chromeos/components/te...
chromeos/components/tether/tether_connector.cc:245: //
host_connection_metrics_logger_->RecordConnectionToHostResult(
On 2017/07/12 17:23:21, Kyle Horimoto wrote:
> Remove.

Done.

https://codereview.chromium.org/2949343002/diff/140001/chromeos/components/te...
chromeos/components/tether/tether_connector.cc:337: //
host_connection_metrics_logger_->RecordConnectionToHostResult(
On 2017/07/12 17:23:21, Kyle Horimoto wrote:
> Remove.

Done.

https://codereview.chromium.org/2949343002/diff/140001/chromeos/components/te...
File chromeos/components/tether/tether_connector_unittest.cc (right):

https://codereview.chromium.org/2949343002/diff/140001/chromeos/components/te...
chromeos/components/tether/tether_connector_unittest.cc:234: void
TestConnectTetheringOperationFails(
On 2017/07/12 17:23:21, Kyle Horimoto wrote:
> VerifyConnectTetheringOperationFails

Done.

https://codereview.chromium.org/2949343002/diff/140001/chromeos/components/te...
chromeos/components/tether/tether_connector_unittest.cc:244:
cryptauth::RemoteDevice test_device = test_devices_[setup_required ? 1 : 0];
On 2017/07/12 17:23:21, Kyle Horimoto wrote:
> It's hard to tell why setup_required determines the index used without reading
> through the whole test. Add a comment explaining that this corresponds to the
> AddTetherNetwork() call in SetUpTetherNetworks() above.

Done.

[Kyle Horimoto, 2017-07-12 17:55:32]

https://codereview.chromium.org/2949343002/diff/160001/chromeos/components/te...
File chromeos/components/tether/tether_connector.cc (right):

https://codereview.chromium.org/2949343002/diff/160001/chromeos/components/te...
chromeos/components/tether/tether_connector.cc:316: } else if (error_code ==
You return in the above if(), so don't do an else. It indents a lot more and
makes your code harder to read.

https://codereview.chromium.org/2949343002/diff/160001/chromeos/components/te...
chromeos/components/tether/tether_connector.cc:322: bool setup_was_required =
Just use:

if (cache->RequiresSetup(guid))
  return TIMEOUT_SETUP_REQUIRED;

return TIMEOUT_SETUP_NOT_REQUIRED;

[Ryan Hansberry, 2017-07-12 18:06:25]

https://codereview.chromium.org/2949343002/diff/160001/chromeos/components/te...
File chromeos/components/tether/tether_connector.cc (right):

https://codereview.chromium.org/2949343002/diff/160001/chromeos/components/te...
chromeos/components/tether/tether_connector.cc:316: } else if (error_code ==
On 2017/07/12 17:55:32, Kyle Horimoto wrote:
> You return in the above if(), so don't do an else. It indents a lot more and
> makes your code harder to read.

Done.

https://codereview.chromium.org/2949343002/diff/160001/chromeos/components/te...
chromeos/components/tether/tether_connector.cc:322: bool setup_was_required =
On 2017/07/12 17:55:32, Kyle Horimoto wrote:
> Just use:
> 
> if (cache->RequiresSetup(guid))
>   return TIMEOUT_SETUP_REQUIRED;
> 
> return TIMEOUT_SETUP_NOT_REQUIRED;

Done.

[Kyle Horimoto, 2017-07-12 18:35:01]

lgtm

[Ryan Hansberry, 2017-07-12 18:35:49]

The CQ bit was checked by hansberry@chromium.org

[commit-bot: I haz the power, 2017-07-12 18:36:08]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-07-12 21:50:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-07-12 21:50:52]

Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[Ryan Hansberry, 2017-07-12 21:55:41]

The CQ bit was checked by hansberry@chromium.org

[commit-bot: I haz the power, 2017-07-12 21:55:59]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-07-12 22:05:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-07-12 22:05:14]

Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Ryan Hansberry, 2017-07-12 22:52:56]

The CQ bit was checked by hansberry@chromium.org

[Ryan Hansberry, 2017-07-12 22:52:57]

The patchset sent to the CQ was uploaded after l-g-t-m from
khorimoto@chromium.org
Link to the patchset: https://codereview.chromium.org/2949343002/#ps200001
(title: "Fix crash.")

[commit-bot: I haz the power, 2017-07-12 22:53:15]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-07-13 01:44:40]

CQ is committing da patch.

Bot data: {"patchset_id": 200001, "attempt_start_ts": 1499899976941330,
"parent_rev": "5ec33f235d0efe6500423bf0882a5001b2974914", "commit_rev":
"82f5bf70aee06e7a832af55527b4543818e2e0fc"}

[commit-bot: I haz the power, 2017-07-13 01:44:52]

Description was changed from

==========
Tether: record each type of host connection result.

BUG=672263
==========

to

==========
Tether: record each type of host connection result.

BUG=672263

Review-Url: https://codereview.chromium.org/2949343002
Cr-Commit-Position: refs/heads/master@{#486209}
Committed:
https://chromium.googlesource.com/chromium/src/+/82f5bf70aee06e7a832af55527b4...
==========

[commit-bot: I haz the power, 2017-07-13 01:44:53]

Committed patchset #11 (id:200001) as
https://chromium.googlesource.com/chromium/src/+/82f5bf70aee06e7a832af55527b4...