M109: Fix the recording of the DegradedRecoverabilityhandler metrics

This CL fixes the recording of the following metrics to be recorded only
for TrustedVault users instead of recording them for all users.

1. Sync.TrustedVaultDegradedRecoverabilityValue
2. Sync.TrustedVaultHintDegradedRecoverabilityChangedReason

(cherry picked from commit 5ddbdda)

Fixed: 1400688
Change-Id: Ie1c07a75a8950a44b9cd911c01fc582746e2afa6
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4102120
Reviewed-by: Rushan Suleymanov <rushans@google.com>
Commit-Queue: Mahmoud Rashad <mmrashad@google.com>
Reviewed-by: Maksim Moskvitin <mmoskvitin@google.com>
Cr-Original-Commit-Position: refs/heads/main@{#1083111}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4111922
Cr-Commit-Position: refs/branch-heads/5414@{#771}
Cr-Branched-From: 4417ee5-refs/heads/main@{#1070088}

components/sync/driver/trusted_vault_histograms.cc

@@ -39,7 +39,7 @@ void RecordTrustedVaultHintDegradedRecoverabilityChangedReason(
     TrustedVaultHintDegradedRecoverabilityChangedReasonForUMA
         hint_degraded_recoverability_changed_reason) {
   base::UmaHistogramEnumeration(
-      "Sync.TrustedVaultHintDegradedRecoverabilityChangedReason",
+      "Sync.TrustedVaultHintDegradedRecoverabilityChangedReason2",
       hint_degraded_recoverability_changed_reason);
 }
 

components/sync/trusted_vault/trusted_vault_degraded_recoverability_handler.cc

@@ -61,9 +61,6 @@ TrustedVaultDegradedRecoverabilityHandler::
       account_info_(account_info) {
   degraded_recoverability_value_ =
       degraded_recoverability_state.degraded_recoverability_value();
-  base::UmaHistogramExactLinear("Sync.TrustedVaultDegradedRecoverabilityValue",
-                                degraded_recoverability_value_,
-                                sync_pb::DegradedRecoverabilityValue_ARRAYSIZE);
   if (degraded_recoverability_state
           .has_last_refresh_time_millis_since_unix_epoch()) {
     base::Time last_refresh_time =
@@ -88,15 +85,10 @@ TrustedVaultDegradedRecoverabilityHandler::
 void TrustedVaultDegradedRecoverabilityHandler::
     HintDegradedRecoverabilityChanged(
         TrustedVaultHintDegradedRecoverabilityChangedReasonForUMA reason) {
-  RecordTrustedVaultHintDegradedRecoverabilityChangedReason(reason);
-  RefreshImmediately();
-}
-
-void TrustedVaultDegradedRecoverabilityHandler::RefreshImmediately() {
-  if (!next_refresh_timer_.IsRunning()) {
-    return;
+  if (next_refresh_timer_.IsRunning()) {
+    RecordTrustedVaultHintDegradedRecoverabilityChangedReason(reason);
+    next_refresh_timer_.FireNow();
   }
-  next_refresh_timer_.FireNow();
 }
 
 void TrustedVaultDegradedRecoverabilityHandler::GetIsRecoverabilityDegraded(
@@ -122,6 +114,9 @@ void TrustedVaultDegradedRecoverabilityHandler::UpdateCurrentRefreshPeriod() {
 }
 
 void TrustedVaultDegradedRecoverabilityHandler::Start() {
+  base::UmaHistogramExactLinear("Sync.TrustedVaultDegradedRecoverabilityValue2",
+                                degraded_recoverability_value_,
+                                sync_pb::DegradedRecoverabilityValue_ARRAYSIZE);
   next_refresh_timer_.Start(
       FROM_HERE,
       ComputeTimeUntilNextRefresh(current_refresh_period_, last_refresh_time_),

components/sync/trusted_vault/trusted_vault_degraded_recoverability_handler.h

@@ -56,10 +56,6 @@ class TrustedVaultDegradedRecoverabilityHandler {
   // GetIsRecoverabilityDegraded().
   void GetIsRecoverabilityDegraded(base::OnceCallback<void(bool)> cb);
 
-  // TODO(crbug.com/1247990): Should be inlined inside
-  // HintDegradedRecoverabilityChanged().
-  void RefreshImmediately();
-
  private:
   void UpdateCurrentRefreshPeriod();
   void Start();

components/sync/trusted_vault/trusted_vault_degraded_recoverability_handler_unittest.cc

@@ -8,6 +8,7 @@
 #include <string>
 #include <utility>
 #include <vector>
+#include "base/test/metrics/histogram_tester.h"
 #include "base/test/mock_callback.h"
 #include "base/test/task_environment.h"
 #include "base/time/time.h"
@@ -102,6 +103,33 @@ class TrustedVaultDegradedRecoverabilityHandlerTest : public ::testing::Test {
       base::test::TaskEnvironment::TimeSource::MOCK_TIME};
 };
 
+TEST_F(TrustedVaultDegradedRecoverabilityHandlerTest,
+       ShouldRecordTheDegradedRecoverabilityValueOnStart) {
+  base::HistogramTester histogram_tester;
+  testing::NiceMock<MockTrustedVaultConnection> connection;
+  testing::NiceMock<MockDelegate> delegate;
+  sync_pb::LocalTrustedVaultDegradedRecoverabilityState
+      degraded_recoverability_state;
+  degraded_recoverability_state.set_degraded_recoverability_value(
+      sync_pb::DegradedRecoverabilityValue::kNotDegraded);
+
+  std::unique_ptr<TrustedVaultDegradedRecoverabilityHandler> scheduler =
+      std::make_unique<TrustedVaultDegradedRecoverabilityHandler>(
+          &connection, &delegate, MakeAccountInfoWithGaiaId("user"),
+          degraded_recoverability_state);
+  histogram_tester.ExpectUniqueSample(
+      "Sync.TrustedVaultDegradedRecoverabilityValue2",
+      /*sample=*/sync_pb::DegradedRecoverabilityValue::kNotDegraded,
+      /*expected_bucket_count=*/0);
+
+  // Start the scheduler.
+  scheduler->GetIsRecoverabilityDegraded(base::DoNothing());
+  histogram_tester.ExpectUniqueSample(
+      "Sync.TrustedVaultDegradedRecoverabilityValue2",
+      /*sample=*/sync_pb::DegradedRecoverabilityValue::kNotDegraded,
+      /*expected_bucket_count=*/1);
+}
+
 TEST_F(TrustedVaultDegradedRecoverabilityHandlerTest,
        ShouldPendTheCallbackUntilTheFirstRefreshIsCalled) {
   testing::NiceMock<MockTrustedVaultConnection> connection;
@@ -153,7 +181,8 @@ TEST_F(TrustedVaultDegradedRecoverabilityHandlerTest,
 }
 
 TEST_F(TrustedVaultDegradedRecoverabilityHandlerTest,
-       ShouldRefreshImmediately) {
+       ShouldRefreshImmediatelyAndRecordTheReason) {
+  base::HistogramTester histogram_tester;
   testing::NiceMock<MockTrustedVaultConnection> connection;
   ON_CALL(connection, DownloadIsRecoverabilityDegraded(
                           Eq(MakeAccountInfoWithGaiaId("user")), _))
@@ -180,7 +209,15 @@ TEST_F(TrustedVaultDegradedRecoverabilityHandlerTest,
   task_environment().FastForwardBy(base::Milliseconds(1));
 
   EXPECT_CALL(connection, DownloadIsRecoverabilityDegraded);
-  scheduler->RefreshImmediately();
+  scheduler->HintDegradedRecoverabilityChanged(
+      TrustedVaultHintDegradedRecoverabilityChangedReasonForUMA::
+          kPersistentAuthErrorResolved);
+  histogram_tester.ExpectUniqueSample(
+      "Sync.TrustedVaultHintDegradedRecoverabilityChangedReason2",
+      /*sample=*/
+      TrustedVaultHintDegradedRecoverabilityChangedReasonForUMA::
+          kPersistentAuthErrorResolved,
+      /*expected_bucket_count=*/1);
 }
 
 TEST_F(TrustedVaultDegradedRecoverabilityHandlerTest,
@@ -365,7 +402,8 @@ TEST_F(TrustedVaultDegradedRecoverabilityHandlerTest,
   EXPECT_CALL(delegate,
               WriteDegradedRecoverabilityState(DegradedRecoverabilityStateEq(
                   degraded_recoverability_state)));
-  scheduler->RefreshImmediately();
+  scheduler->HintDegradedRecoverabilityChanged(
+      TrustedVaultHintDegradedRecoverabilityChangedReasonForUMA());
 }
 
 TEST_F(
@@ -416,7 +454,8 @@ TEST_F(
   EXPECT_CALL(delegate,
               WriteDegradedRecoverabilityState(DegradedRecoverabilityStateEq(
                   degraded_recoverability_state)));
-  scheduler->RefreshImmediately();
+  scheduler->HintDegradedRecoverabilityChanged(
+      TrustedVaultHintDegradedRecoverabilityChangedReasonForUMA());
 }
 
 TEST_F(TrustedVaultDegradedRecoverabilityHandlerTest,

tools/metrics/histograms/metadata/sync/histograms.xml

@@ -1365,15 +1365,16 @@ chromium-metrics-reviews@google.com.
   </summary>
 </histogram>
 
-<histogram name="Sync.TrustedVaultDegradedRecoverabilityValue"
+<histogram name="Sync.TrustedVaultDegradedRecoverabilityValue2"
     enum="TrustedVaultDegradedRecoverabilityValue" expires_after="2023-04-09">
   <owner>mmrashad@google.com</owner>
   <owner>mmoskvitin@google.com</owner>
   <component>Services&gt;Sync</component>
   <summary>
-    Records the known degraded recoverability value when the primary account is
-    set. Note that the value reflects state restored from the file and recorded
-    before sending new request to the server.
+    Records the known degraded recoverability value when the degraded
+    recoverability handler is started. Note that the value reflects state
+    restored from the file and recorded before sending new request to the
+    server.
   </summary>
 </histogram>
 
@@ -1477,7 +1478,7 @@ chromium-metrics-reviews@google.com.
   </summary>
 </histogram>
 
-<histogram name="Sync.TrustedVaultHintDegradedRecoverabilityChangedReason"
+<histogram name="Sync.TrustedVaultHintDegradedRecoverabilityChangedReason2"
     enum="TrustedVaultHintDegradedRecoverabilityChangedReason"
     expires_after="2023-04-09">
   <owner>mmrashad@google.com</owner>