[CrOS MultiDevice] Add metric for MultiDevice.DeviceSyncService.SetSoftwareFeatureState.Success

Add success of enabling and disabling features for devices.

Bug: 870138

Change-Id: I27a96eceb4d1c9ec3fe00ad37c1606785ab4449d
Reviewed-on: https://chromium-review.googlesource.com/c/1306915
Commit-Queue: Regan Hsu <hsuregan@chromium.org>
Reviewed-by: Ryan Hansberry <hansberry@chromium.org>
Reviewed-by: Steven Holte <holte@chromium.org>
Cr-Commit-Position: refs/heads/master@{#604136}

chromeos/services/device_sync/device_sync_impl.cc

@@ -5,6 +5,7 @@
 #include "chromeos/services/device_sync/device_sync_impl.h"
 
 #include "base/memory/ptr_util.h"
+#include "base/metrics/histogram_macros.h"
 #include "base/no_destructor.h"
 #include "base/optional.h"
 #include "base/time/default_clock.h"
@@ -237,6 +238,8 @@ void DeviceSyncImpl::SetSoftwareFeatureState(
                     << "before initialization was complete. Cannot set state.";
     std::move(callback).Run(
         mojom::NetworkRequestResult::kServiceNotYetInitialized);
+
+    RecordSetSoftwareFeatureStateResult(false /* success */);
     return;
   }
 
@@ -483,6 +486,8 @@ void DeviceSyncImpl::OnSetSoftwareFeatureStateSuccess() {
                << "requesting force sync.";
   cryptauth_device_manager_->ForceSyncNow(
       cryptauth::INVOCATION_REASON_FEATURE_TOGGLED);
+
+  RecordSetSoftwareFeatureStateResult(true /* success */);
 }
 
 void DeviceSyncImpl::OnSetSoftwareFeatureStateError(
@@ -499,6 +504,8 @@ void DeviceSyncImpl::OnSetSoftwareFeatureStateError(
   it->second->InvokeCallback(
       mojo::ConvertTo<mojom::NetworkRequestResult>(error));
   id_to_pending_set_software_feature_request_map_.erase(it);
+
+  RecordSetSoftwareFeatureStateResult(false /* success */);
 }
 
 void DeviceSyncImpl::OnFindEligibleDevicesSuccess(
@@ -568,6 +575,8 @@ void DeviceSyncImpl::OnSetSoftwareFeatureTimerFired() {
     it->second->InvokeCallback(
         mojom::NetworkRequestResult::kRequestSucceededButUnexpectedResult);
     it = id_to_pending_set_software_feature_request_map_.erase(it);
+
+    RecordSetSoftwareFeatureStateResult(false /* success */);
   }
 }
 
@@ -576,6 +585,11 @@ void DeviceSyncImpl::SetPrefConnectionDelegateForTesting(
   pref_connection_delegate_ = std::move(pref_connection_delegate);
 }
 
+void DeviceSyncImpl::RecordSetSoftwareFeatureStateResult(bool success) {
+  UMA_HISTOGRAM_BOOLEAN(
+      "MultiDevice.DeviceSyncService.SetSoftwareFeatureState.Result", success);
+}
+
 }  // namespace device_sync
 
 }  // namespace chromeos

chromeos/services/device_sync/device_sync_impl.h

@@ -203,6 +203,8 @@ class DeviceSyncImpl : public DeviceSyncBase,
   void SetPrefConnectionDelegateForTesting(
       std::unique_ptr<PrefConnectionDelegate> pref_connection_delegate);
 
+  static void RecordSetSoftwareFeatureStateResult(bool success);
+
   identity::IdentityManager* identity_manager_;
   gcm::GCMDriver* gcm_driver_;
   service_manager::Connector* connector_;

chromeos/services/device_sync/device_sync_service_unittest.cc

@@ -9,6 +9,7 @@
 #include "base/optional.h"
 #include "base/run_loop.h"
 #include "base/single_thread_task_runner.h"
+#include "base/test/metrics/histogram_tester.h"
 #include "base/test/null_task_runner.h"
 #include "base/test/scoped_task_environment.h"
 #include "base/test/simple_test_clock.h"
@@ -845,6 +846,10 @@ class DeviceSyncServiceTest : public testing::Test {
     return last_debug_info_result_;
   }
 
+  const base::HistogramTester& histogram_tester() const {
+    return histogram_tester_;
+  }
+
  private:
   void OnAddObserverCompleted(base::OnceClosure quit_closure) {
     std::move(quit_closure).Run();
@@ -961,6 +966,8 @@ class DeviceSyncServiceTest : public testing::Test {
   std::unique_ptr<FakeDeviceSyncObserver> fake_device_sync_observer_;
   mojom::DeviceSyncPtr device_sync_;
 
+  base::HistogramTester histogram_tester_;
+
   DISALLOW_COPY_AND_ASSIGN(DeviceSyncServiceTest);
 };
 
@@ -1159,6 +1166,11 @@ TEST_F(DeviceSyncServiceTest, SetSoftwareFeatureState_Success) {
   auto last_response = GetLastSetSoftwareFeatureStateResponseAndReset();
   EXPECT_TRUE(last_response);
   EXPECT_EQ(device_sync::mojom::NetworkRequestResult::kSuccess, *last_response);
+
+  histogram_tester().ExpectBucketCount<bool>(
+      "MultiDevice.DeviceSyncService.SetSoftwareFeatureState.Result", false, 0);
+  histogram_tester().ExpectBucketCount<bool>(
+      "MultiDevice.DeviceSyncService.SetSoftwareFeatureState.Result", true, 1);
 }
 
 TEST_F(DeviceSyncServiceTest,
@@ -1200,6 +1212,11 @@ TEST_F(DeviceSyncServiceTest,
   EXPECT_EQ(device_sync::mojom::NetworkRequestResult::
                 kRequestSucceededButUnexpectedResult,
             *last_response);
+
+  histogram_tester().ExpectBucketCount<bool>(
+      "MultiDevice.DeviceSyncService.SetSoftwareFeatureState.Result", false, 1);
+  histogram_tester().ExpectBucketCount<bool>(
+      "MultiDevice.DeviceSyncService.SetSoftwareFeatureState.Result", true, 0);
 }
 
 TEST_F(DeviceSyncServiceTest, SetSoftwareFeatureState_Error) {
@@ -1237,6 +1254,11 @@ TEST_F(DeviceSyncServiceTest, SetSoftwareFeatureState_Error) {
   auto last_response = GetLastSetSoftwareFeatureStateResponseAndReset();
   EXPECT_TRUE(last_response);
   EXPECT_EQ(mojom::NetworkRequestResult::kOffline, *last_response);
+
+  histogram_tester().ExpectBucketCount<bool>(
+      "MultiDevice.DeviceSyncService.SetSoftwareFeatureState.Result", false, 1);
+  histogram_tester().ExpectBucketCount<bool>(
+      "MultiDevice.DeviceSyncService.SetSoftwareFeatureState.Result", true, 0);
 }
 
 TEST_F(DeviceSyncServiceTest, FindEligibleDevices) {

tools/metrics/histograms/histograms.xml

@@ -50779,6 +50779,12 @@ uploading your change for review.
   </summary>
 </histogram>
 
+<histogram name="MultiDevice.DeviceSyncService.SetSoftwareFeatureState.Result"
+    enum="BooleanSuccess">
+  <owner>hansberry@chromium.org</owner>
+  <summary>Result of enabling and disabling features for devices.</summary>
+</histogram>
+
 <histogram name="MultiDevice.PostOOBESetupFlow.PageShown"
     enum="MultiDevice_PostOOBESetupFlow_Page">
   <owner>hansberry@chromium.org</owner>