Introduce PasswordManager.AccountStoreVsProfileStore2.* histograms

The existing versions of these histograms (without "2") are recorded for
all users, whether they're opted in to the account-scoped store or not,
so they contain a lot of useless entries.
The new version "2" histograms are only recorded for opted-in users.

The old histograms are *not* deprecated yet, in case we want to compare
some numbers from M87 (which only has the old versions) with numbers
from later Chrome versions.

Bug: 1145971
Change-Id: I633769d4b77fbefef17014d613b83a45c3580b3b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2526064
Commit-Queue: Marc Treib <treib@chromium.org>
Reviewed-by: Robert Kaplow <rkaplow@chromium.org>
Reviewed-by: Mohamed Amir Yosef <mamir@chromium.org>
Cr-Commit-Position: refs/heads/master@{#825808}

components/password_manager/core/browser/store_metrics_reporter.cc

@@ -7,6 +7,7 @@
 #include "base/metrics/histogram_functions.h"
 #include "base/sequenced_task_runner.h"
 #include "base/threading/sequenced_task_runner_handle.h"
+#include "components/password_manager/core/browser/password_feature_manager.h"
 #include "components/password_manager/core/browser/password_manager_client.h"
 #include "components/password_manager/core/browser/password_manager_metrics_util.h"
 #include "components/password_manager/core/browser/password_store.h"
@@ -45,8 +46,9 @@ class StoreMetricsReporter::MultiStoreMetricsReporter {
  public:
   MultiStoreMetricsReporter(PasswordStore* profile_store,
                             PasswordStore* account_store,
+                            bool is_opted_in,
                             base::OnceClosure done_callback)
-      : done_callback_(std::move(done_callback)) {
+      : is_opted_in_(is_opted_in), done_callback_(std::move(done_callback)) {
     DCHECK(profile_store);
     DCHECK(account_store);
     profile_store_consumer_ = std::make_unique<Consumer>(
@@ -144,10 +146,24 @@ class StoreMetricsReporter::MultiStoreMetricsReporter {
     base::UmaHistogramCounts100(
         "PasswordManager.AccountStoreVsProfileStore.Conflicting", conflicting);
 
+    if (is_opted_in_) {
+      base::UmaHistogramCounts100(
+          "PasswordManager.AccountStoreVsProfileStore2.Additional", additional);
+      base::UmaHistogramCounts100(
+          "PasswordManager.AccountStoreVsProfileStore2.Missing", missing);
+      base::UmaHistogramCounts100(
+          "PasswordManager.AccountStoreVsProfileStore2.Identical", identical);
+      base::UmaHistogramCounts100(
+          "PasswordManager.AccountStoreVsProfileStore2.Conflicting",
+          conflicting);
+    }
+
     std::move(done_callback_).Run();
     // Note: |this| might be destroyed now.
   }
 
+  const bool is_opted_in_;
+
   base::OnceClosure done_callback_;
 
   std::unique_ptr<Consumer> profile_store_consumer_;
@@ -197,20 +213,23 @@ StoreMetricsReporter::StoreMetricsReporter(
     // delayed task runs.
     scoped_refptr<PasswordStore> retained_profile_store = profile_store;
     scoped_refptr<PasswordStore> retained_account_store = account_store;
+    bool is_opted_in =
+        client->GetPasswordFeatureManager()->IsOptedInForAccountStorage();
     base::SequencedTaskRunnerHandle::Get()->PostDelayedTask(
         FROM_HERE,
         base::BindOnce(&StoreMetricsReporter::ReportMultiStoreMetrics,
                        weak_ptr_factory_.GetWeakPtr(), retained_profile_store,
-                       retained_account_store),
+                       retained_account_store, is_opted_in),
         base::TimeDelta::FromSeconds(30));
   }
 }
 
 void StoreMetricsReporter::ReportMultiStoreMetrics(
     scoped_refptr<PasswordStore> profile_store,
-    scoped_refptr<PasswordStore> account_store) {
+    scoped_refptr<PasswordStore> account_store,
+    bool is_opted_in) {
   multi_store_reporter_ = std::make_unique<MultiStoreMetricsReporter>(
-      profile_store.get(), account_store.get(),
+      profile_store.get(), account_store.get(), is_opted_in,
       base::BindOnce(&StoreMetricsReporter::MultiStoreMetricsDone,
                      weak_ptr_factory_.GetWeakPtr()));
 }

components/password_manager/core/browser/store_metrics_reporter.h

@@ -51,7 +51,8 @@ class StoreMetricsReporter {
   class MultiStoreMetricsReporter;
 
   void ReportMultiStoreMetrics(scoped_refptr<PasswordStore> profile_store,
-                               scoped_refptr<PasswordStore> account_store);
+                               scoped_refptr<PasswordStore> account_store,
+                               bool is_opted_in);
   void MultiStoreMetricsDone();
 
   std::unique_ptr<MultiStoreMetricsReporter> multi_store_reporter_;

components/password_manager/core/browser/store_metrics_reporter_unittest.cc

@@ -183,23 +183,52 @@ TEST_F(StoreMetricsReporterTest, MultiStoreMetrics) {
   profile_store->AddLogin(
       CreateForm(kRealm2, "identicaluser1", "identicalpass1"));
 
-  base::HistogramTester histogram_tester;
-
-  StoreMetricsReporter reporter(&client_, sync_service(), identity_manager(),
-                                &prefs_);
-  // Wait for the metrics to get reported. This is delayed by 30 seconds, and
-  // then involves queries to the stores, i.e. to background task runners.
-  FastForwardBy(base::TimeDelta::FromSeconds(30));
-  RunUntilIdle();
-
-  histogram_tester.ExpectUniqueSample(
-      "PasswordManager.AccountStoreVsProfileStore.Additional", 2, 1);
-  histogram_tester.ExpectUniqueSample(
-      "PasswordManager.AccountStoreVsProfileStore.Missing", 4, 1);
-  histogram_tester.ExpectUniqueSample(
-      "PasswordManager.AccountStoreVsProfileStore.Identical", 2, 1);
-  histogram_tester.ExpectUniqueSample(
-      "PasswordManager.AccountStoreVsProfileStore.Conflicting", 1, 1);
+  for (bool opted_in : {false, true}) {
+    EXPECT_CALL(*client_.GetPasswordFeatureManager(),
+                IsOptedInForAccountStorage())
+        .WillRepeatedly(Return(opted_in));
+
+    base::HistogramTester histogram_tester;
+
+    StoreMetricsReporter reporter(&client_, sync_service(), identity_manager(),
+                                  &prefs_);
+    // Wait for the metrics to get reported. This is delayed by 30 seconds, and
+    // then involves queries to the stores, i.e. to background task runners.
+    FastForwardBy(base::TimeDelta::FromSeconds(30));
+    RunUntilIdle();
+
+    // The original version of the metrics (without "2") is still recorded, even
+    // if the user isn't opted in to the account storage.
+    histogram_tester.ExpectUniqueSample(
+        "PasswordManager.AccountStoreVsProfileStore.Additional", 2, 1);
+    histogram_tester.ExpectUniqueSample(
+        "PasswordManager.AccountStoreVsProfileStore.Missing", 4, 1);
+    histogram_tester.ExpectUniqueSample(
+        "PasswordManager.AccountStoreVsProfileStore.Identical", 2, 1);
+    histogram_tester.ExpectUniqueSample(
+        "PasswordManager.AccountStoreVsProfileStore.Conflicting", 1, 1);
+
+    // Version "2" of the metrics is only recorded if the user is opted in.
+    if (opted_in) {
+      histogram_tester.ExpectUniqueSample(
+          "PasswordManager.AccountStoreVsProfileStore2.Additional", 2, 1);
+      histogram_tester.ExpectUniqueSample(
+          "PasswordManager.AccountStoreVsProfileStore2.Missing", 4, 1);
+      histogram_tester.ExpectUniqueSample(
+          "PasswordManager.AccountStoreVsProfileStore2.Identical", 2, 1);
+      histogram_tester.ExpectUniqueSample(
+          "PasswordManager.AccountStoreVsProfileStore2.Conflicting", 1, 1);
+    } else {
+      histogram_tester.ExpectTotalCount(
+          "PasswordManager.AccountStoreVsProfileStore2.Additional", 0);
+      histogram_tester.ExpectTotalCount(
+          "PasswordManager.AccountStoreVsProfileStore2.Missing", 0);
+      histogram_tester.ExpectTotalCount(
+          "PasswordManager.AccountStoreVsProfileStore2.Identical", 0);
+      histogram_tester.ExpectTotalCount(
+          "PasswordManager.AccountStoreVsProfileStore2.Conflicting", 0);
+    }
+  }
 
   account_store->ShutdownOnUIThread();
   profile_store->ShutdownOnUIThread();

tools/metrics/histograms/histograms_xml/password/histograms.xml

@@ -447,49 +447,59 @@ reviews. Googlers can read more about this at go/gwsq-gerrit.
   </summary>
 </histogram>
 
-<histogram name="PasswordManager.AccountStoreVsProfileStore.Additional"
+<histogram name="PasswordManager.AccountStoreVsProfileStore.{DifferenceType}"
     units="accounts" expires_after="M90">
   <owner>mamir@chromium.org</owner>
   <owner>treib@chromium.org</owner>
   <summary>
-    The number of accounts stored in the password manager's account-scoped store
-    that don't exist in the profile-scoped store. Recorded once per run of
-    Chrome, soon after startup.
-  </summary>
-</histogram>
-
-<histogram name="PasswordManager.AccountStoreVsProfileStore.Conflicting"
-    units="accounts" expires_after="M90">
-  <owner>mamir@chromium.org</owner>
-  <owner>treib@chromium.org</owner>
-  <summary>
-    The number of accounts stored in the password manager with a conflicting
-    password between the account-scoped store and profile-scoped store (i.e. the
-    signon realm and username match, but the password does not). Recorded once
-    per run of Chrome, soon after startup.
-  </summary>
-</histogram>
-
-<histogram name="PasswordManager.AccountStoreVsProfileStore.Identical"
+    The number of accounts {DifferenceType}. Recorded once per run of Chrome,
+    soon after startup. Recorded independent of whether the user is opted in to
+    the account-scoped storage or not, i.e. will contain a lot of useless
+    entries - you might want to look at AccountStoreVsProfileStore2 instead.
+  </summary>
+  <token key="DifferenceType">
+    <variant name="Additional"
+        summary="stored in the password manager's account-scoped store that
+                 don't exist in the profile-scoped store"/>
+    <variant name="Conflicting"
+        summary="stored in the password manager with a conflicting password
+                 between the account-scoped store and profile-scoped store
+                 (i.e. the signon realm and username match, but the password
+                 does not)"/>
+    <variant name="Identical"
+        summary="stored in both the password manager's account-scoped store
+                 and profile-scoped store"/>
+    <variant name="Missing"
+        summary="stored in the password manager's profile-scoped store that
+                 don't exist in the account-scoped store"/>
+  </token>
+</histogram>
+
+<histogram name="PasswordManager.AccountStoreVsProfileStore2.{DifferenceType}"
     units="accounts" expires_after="M90">
   <owner>mamir@chromium.org</owner>
   <owner>treib@chromium.org</owner>
   <summary>
-    The number of accounts stored in both the password manager's account-scoped
-    store and profile-scoped store. Recorded once per run of Chrome, soon after
-    startup.
-  </summary>
-</histogram>
-
-<histogram name="PasswordManager.AccountStoreVsProfileStore.Missing"
-    units="accounts" expires_after="M90">
-  <owner>mamir@chromium.org</owner>
-  <owner>treib@chromium.org</owner>
-  <summary>
-    The number of accounts stored in the password manager's profile-scoped store
-    that don't exist in the account-scoped store. Recorded once per run of
-    Chrome, soon after startup.
-  </summary>
+    The number of accounts {DifferenceType}. Recorded once per run of Chrome,
+    soon after startup, only for users who are opted in to the account-scoped
+    storage.
+  </summary>
+  <token key="DifferenceType">
+    <variant name="Additional"
+        summary="stored in the password manager's account-scoped store that
+                 don't exist in the profile-scoped store"/>
+    <variant name="Conflicting"
+        summary="stored in the password manager with a conflicting password
+                 between the account-scoped store and profile-scoped store
+                 (i.e. the signon realm and username match, but the password
+                 does not)"/>
+    <variant name="Identical"
+        summary="stored in both the password manager's account-scoped store
+                 and profile-scoped store"/>
+    <variant name="Missing"
+        summary="stored in the password manager's profile-scoped store that
+                 don't exist in the account-scoped store"/>
+  </token>
 </histogram>
 
 <histogram name="PasswordManager.AffiliationBackend.FetchSize" units="facets"