[AF Wallet] Remove metrics needed only for the launch

This CL is part of post-launch cleanup for the USS implementation of
wallet_data sync. It removes metrics that are not needed any more.

Bug: 936308
Change-Id: Ifc98ae8e5e34cb8e88774f55e3f788f6c2344f15
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1608704
Commit-Queue: Jan Krcal <jkrcal@chromium.org>
Reviewed-by: Steven Holte <holte@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#660350}

components/autofill/core/browser/webdata/autofill_wallet_sync_bridge.cc

@@ -9,7 +9,6 @@
 #include "base/base64.h"
 #include "base/feature_list.h"
 #include "base/logging.h"
-#include "base/metrics/histogram_macros.h"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "components/autofill/core/browser/autofill_profile_sync_util.h"
@@ -265,20 +264,6 @@ void AutofillWalletSyncBridge::ApplyStopSyncChanges(
       active_callback_.Run(false);
     }
 
-    // Report count of entities to delete. This use case should be pretty rare
-    // so it is okay to read it from DB again.
-    // TODO(crbug.com/853688): Remove when wallet data is launched on USS, incl.
-    // the helper function SyncWalletDataRecordClearedEntitiesCount().
-    std::vector<std::unique_ptr<AutofillProfile>> profiles;
-    std::vector<std::unique_ptr<CreditCard>> cards;
-    std::unique_ptr<PaymentsCustomerData> customer_data;
-    if (GetAutofillTable()->GetServerProfiles(&profiles) &&
-        GetAutofillTable()->GetServerCreditCards(&cards) &&
-        GetAutofillTable()->GetPaymentsCustomerData(&customer_data)) {
-      int count = profiles.size() + cards.size() + (customer_data ? 1 : 0);
-      SyncWalletDataRecordClearedEntitiesCount(count);
-    }
-
     // Do not notify the metadata bridge because we do not want to upstream the
     // deletions. The metadata bridge deletes its data independently when sync
     // gets stopped.
@@ -339,13 +324,11 @@ void AutofillWalletSyncBridge::SetSyncData(
   PopulateWalletTypesFromSyncData(entity_data, &wallet_cards, &wallet_addresses,
                                   &customer_data);
 
-  bool should_log_diff;
+  wallet_data_changed |= SetPaymentsCustomerData(std::move(customer_data));
   wallet_data_changed |=
-      SetPaymentsCustomerData(std::move(customer_data), &should_log_diff);
-  wallet_data_changed |= SetWalletCards(
-      std::move(wallet_cards), should_log_diff, notify_metadata_bridge);
-  wallet_data_changed |= SetWalletAddresses(
-      std::move(wallet_addresses), should_log_diff, notify_metadata_bridge);
+      SetWalletCards(std::move(wallet_cards), notify_metadata_bridge);
+  wallet_data_changed |=
+      SetWalletAddresses(std::move(wallet_addresses), notify_metadata_bridge);
 
   // Commit the transaction to make sure the data and the metadata with the
   // new progress marker is written down (especially on Android where we
@@ -360,7 +343,6 @@ void AutofillWalletSyncBridge::SetSyncData(
 
 bool AutofillWalletSyncBridge::SetWalletCards(
     std::vector<CreditCard> wallet_cards,
-    bool log_diff,
     bool notify_metadata_bridge) {
   // Users can set billing address of the server credit card locally, but that
   // information does not propagate to either Chrome Sync or Google Payments
@@ -378,14 +360,6 @@ bool AutofillWalletSyncBridge::SetWalletCards(
   AutofillWalletDiff<CreditCard> diff =
       ComputeAutofillWalletDiff(existing_cards, wallet_cards);
 
-  if (log_diff) {
-    UMA_HISTOGRAM_COUNTS_100("Autofill.WalletCards2.Added", diff.items_added);
-    UMA_HISTOGRAM_COUNTS_100("Autofill.WalletCards2.Removed",
-                             diff.items_removed);
-    UMA_HISTOGRAM_COUNTS_100("Autofill.WalletCards2.AddedOrRemoved",
-                             diff.items_added + diff.items_removed);
-  }
-
   if (!diff.IsEmpty()) {
     if (base::FeatureList::IsEnabled(
             ::switches::kSyncUSSAutofillWalletMetadata)) {
@@ -405,7 +379,6 @@ bool AutofillWalletSyncBridge::SetWalletCards(
 
 bool AutofillWalletSyncBridge::SetWalletAddresses(
     std::vector<AutofillProfile> wallet_addresses,
-    bool log_diff,
     bool notify_metadata_bridge) {
   // We do not have to CopyRelevantWalletMetadataFromDisk() because we will
   // never overwrite the same entity with different data (server_id is generated
@@ -425,15 +398,6 @@ bool AutofillWalletSyncBridge::SetWalletAddresses(
   AutofillWalletDiff<AutofillProfile> diff =
       ComputeAutofillWalletDiff(existing_addresses, wallet_addresses);
 
-  if (log_diff) {
-    UMA_HISTOGRAM_COUNTS_100("Autofill.WalletAddresses2.Added",
-                             diff.items_added);
-    UMA_HISTOGRAM_COUNTS_100("Autofill.WalletAddresses2.Removed",
-                             diff.items_removed);
-    UMA_HISTOGRAM_COUNTS_100("Autofill.WalletAddresses2.AddedOrRemoved",
-                             diff.items_added + diff.items_removed);
-  }
-
   if (!diff.IsEmpty()) {
     if (base::FeatureList::IsEnabled(
             ::switches::kSyncUSSAutofillWalletMetadata)) {
@@ -452,8 +416,7 @@ bool AutofillWalletSyncBridge::SetWalletAddresses(
 }
 
 bool AutofillWalletSyncBridge::SetPaymentsCustomerData(
-    std::vector<PaymentsCustomerData> customer_data,
-    bool* should_log_diff) {
+    std::vector<PaymentsCustomerData> customer_data) {
   AutofillTable* table = GetAutofillTable();
   std::unique_ptr<PaymentsCustomerData> existing_entry;
   table->GetPaymentsCustomerData(&existing_entry);
@@ -470,15 +433,6 @@ bool AutofillWalletSyncBridge::SetPaymentsCustomerData(
   }
 #endif  // DCHECK_IS_ON()
 
-  // We report the diff to metrics only if this is an incremental change where
-  // the user had sync set-up (having PaymentsCustomerData is a pre-requisite
-  // for having any other data) and continues to have sync set-up (continuing
-  // having a PaymentsCustomerData entity). As a side effect, this excludes
-  // reporting diffs for users that newly got a GPay account and sync
-  // PaymentsCustomerData for the first time but this is the best we can do to
-  // have the metrics consistent with Directory implementation.
-  *should_log_diff = existing_entry && new_entry;
-
   if (!new_entry && existing_entry) {
     // Clear the existing entry in the DB.
     GetAutofillTable()->SetPaymentsCustomerData(nullptr);

components/autofill/core/browser/webdata/autofill_wallet_sync_bridge.h

@@ -94,28 +94,21 @@ class AutofillWalletSyncBridge : public base::SupportsUserData::Data,
                    bool notify_metadata_bridge);
 
   // Sets |customer_data| to this client and returns whether any change has been
-  // applied (i.e., whether |customer_data| was different from local data) and
-  // whether we |should_log_diff|, i.e. metrics for diffs in counts of addresses
-  // and cards.
-  bool SetPaymentsCustomerData(std::vector<PaymentsCustomerData> customer_data,
-                               bool* should_log_diff);
-
-  // Sets |wallet_cards| to this client, records metrics about added/deleted
-  // data (if |log_diff| is true) and returns whether any change has been
+  // applied (i.e., whether |customer_data| was different from local data).
+  bool SetPaymentsCustomerData(std::vector<PaymentsCustomerData> customer_data);
+
+  // Sets |wallet_cards| to this client and returns whether any change has been
   // applied (i.e., whether |wallet_cards| was different from local data). If
   // |notify_metadata_bridge|, it also notifies via WebDataBackend about any
   // individual entity changes.
   bool SetWalletCards(std::vector<CreditCard> wallet_cards,
-                      bool log_diff,
                       bool notify_metadata_bridge);
 
-  // Sets |wallet_addresses| to this client, records metrics about added/deleted
-  // data (if |log_diff| is true) and returns whether any change has been
-  // applied (i.e., whether |wallet_addresses| was different from local data).
-  // If |notify_metadata_bridge|, it also notifies via WebDataBackend about any
-  // individual entity changes.
+  // Sets |wallet_addresses| to this client and returns whether any change has
+  // been applied (i.e., whether |wallet_addresses| was different from local
+  // data). If |notify_metadata_bridge|, it also notifies via WebDataBackend
+  // about any individual entity changes.
   bool SetWalletAddresses(std::vector<AutofillProfile> wallet_addresses,
-                          bool log_diff,
                           bool notify_metadata_bridge);
 
   // Computes a "diff" (items added, items removed) of two vectors of items,

components/autofill/core/browser/webdata/autofill_wallet_sync_bridge_unittest.cc

@@ -14,7 +14,6 @@
 #include "base/files/scoped_temp_dir.h"
 #include "base/run_loop.h"
 #include "base/test/bind_test_util.h"
-#include "base/test/metrics/histogram_tester.h"
 #include "base/test/mock_callback.h"
 #include "base/test/scoped_feature_list.h"
 #include "base/test/scoped_task_environment.h"
@@ -278,45 +277,6 @@ class AutofillWalletSyncBridgeTest : public UssSwitchToggler,
     real_processor_->OnUpdateReceived(state, std::move(initial_updates));
   }
 
-  void ExpectAddressesDiffInHistograms(int added, int removed) {
-    histogram_tester_.ExpectUniqueSample("Autofill.WalletAddresses2.Added",
-                                         /*bucket=*/added,
-                                         /*count=*/1);
-    histogram_tester_.ExpectUniqueSample("Autofill.WalletAddresses2.Removed",
-                                         /*bucket=*/removed,
-                                         /*count=*/1);
-    histogram_tester_.ExpectUniqueSample(
-        "Autofill.WalletAddresses2.AddedOrRemoved",
-        /*bucket=*/added + removed,
-        /*count=*/1);
-  }
-
-  void ExpectNoHistogramsForAddressesDiff() {
-    histogram_tester_.ExpectTotalCount("Autofill.WalletAddresses2.Added", 0);
-    histogram_tester_.ExpectTotalCount("Autofill.WalletAddresses2.Removed", 0);
-    histogram_tester_.ExpectTotalCount(
-        "Autofill.WalletAddresses2.AddedOrRemoved", 0);
-  }
-
-  void ExpectCardsDiffInHistograms(int added, int removed) {
-    histogram_tester_.ExpectUniqueSample("Autofill.WalletCards2.Added",
-                                         /*bucket=*/added,
-                                         /*count=*/1);
-    histogram_tester_.ExpectUniqueSample("Autofill.WalletCards2.Removed",
-                                         /*bucket=*/removed,
-                                         /*count=*/1);
-    histogram_tester_.ExpectUniqueSample("Autofill.WalletCards2.AddedOrRemoved",
-                                         /*bucket=*/added + removed,
-                                         /*count=*/1);
-  }
-
-  void ExpectNoHistogramsForCardsDiff() {
-    histogram_tester_.ExpectTotalCount("Autofill.WalletCards2.Added", 0);
-    histogram_tester_.ExpectTotalCount("Autofill.WalletCards2.Removed", 0);
-    histogram_tester_.ExpectTotalCount("Autofill.WalletCards2.AddedOrRemoved",
-                                       0);
-  }
-
   void ExpectCountsOfWalletMetadataInDB(unsigned int cards_count,
                                         unsigned int addresses_count) {
     std::map<std::string, AutofillMetadata> cards_metadata;
@@ -381,7 +341,6 @@ class AutofillWalletSyncBridgeTest : public UssSwitchToggler,
   testing::NiceMock<MockModelTypeChangeProcessor> mock_processor_;
   std::unique_ptr<syncer::ClientTagBasedModelTypeProcessor> real_processor_;
   std::unique_ptr<AutofillWalletSyncBridge> bridge_;
-  base::HistogramTester histogram_tester_;
   NiceMock<base::MockCallback<base::RepeatingCallback<void(bool)>>>
       active_callback_;
 
@@ -510,8 +469,6 @@ TEST_P(AutofillWalletSyncBridgeTest, MergeSyncData_NewWalletAddressAndCard) {
               UnorderedElementsAre(EqualsSpecifics(profile_specifics2),
                                    EqualsSpecifics(card_specifics2),
                                    EqualsSpecifics(customer_data_specifics)));
-  ExpectAddressesDiffInHistograms(/*added=*/1, /*removed=*/1);
-  ExpectCardsDiffInHistograms(/*added=*/1, /*removed=*/1);
 }
 
 // Tests that in initial sync, no metrics are recorded for new addresses and
@@ -549,8 +506,6 @@ TEST_P(AutofillWalletSyncBridgeTest,
               UnorderedElementsAre(EqualsSpecifics(profile_specifics),
                                    EqualsSpecifics(card_specifics),
                                    EqualsSpecifics(customer_data_specifics)));
-  ExpectNoHistogramsForAddressesDiff();
-  ExpectNoHistogramsForCardsDiff();
 }
 
 // Tests that when a new payments customer data is sent by the server, the
@@ -585,8 +540,6 @@ TEST_P(AutofillWalletSyncBridgeTest, MergeSyncData_NewPaymentsCustomerData) {
               UnorderedElementsAre(EqualsSpecifics(profile_specifics),
                                    EqualsSpecifics(card_specifics),
                                    EqualsSpecifics(customer_data_specifics2)));
-  ExpectAddressesDiffInHistograms(/*added=*/0, /*removed=*/0);
-  ExpectCardsDiffInHistograms(/*added=*/0, /*removed=*/0);
 }
 
 // Tests that when the server sends no cards or address, the client should
@@ -613,9 +566,6 @@ TEST_P(AutofillWalletSyncBridgeTest, MergeSyncData_NoWalletAddressOrCard) {
   }
 
   EXPECT_TRUE(GetAllLocalData().empty());
-  // No diff metrics reported when new data is empty.
-  ExpectNoHistogramsForAddressesDiff();
-  ExpectNoHistogramsForCardsDiff();
 }
 
 // Test that when the server sends the same address and card as the client has,
@@ -650,8 +600,6 @@ TEST_P(AutofillWalletSyncBridgeTest,
               UnorderedElementsAre(EqualsSpecifics(profile_specifics),
                                    EqualsSpecifics(card_specifics),
                                    EqualsSpecifics(customer_data_specifics)));
-  ExpectAddressesDiffInHistograms(/*added=*/0, /*removed=*/0);
-  ExpectCardsDiffInHistograms(/*added=*/0, /*removed=*/0);
 }
 
 // Tests that when there are multiple changes happening at the same time, the
@@ -700,8 +648,6 @@ TEST_P(AutofillWalletSyncBridgeTest,
               UnorderedElementsAre(EqualsSpecifics(profile_specifics),
                                    EqualsSpecifics(card2_specifics),
                                    EqualsSpecifics(customer_data_specifics)));
-  ExpectAddressesDiffInHistograms(/*added=*/0, /*removed=*/1);
-  ExpectCardsDiffInHistograms(/*added=*/1, /*removed=*/1);
 }
 
 // Test that all field values for a address sent form the server are copied on
@@ -841,9 +787,6 @@ TEST_P(AutofillWalletSyncBridgeTest, ApplyStopSyncChanges_ClearAllData) {
   }
 
   EXPECT_TRUE(GetAllLocalData().empty());
-  // No diff metrics reported when clearing data.
-  ExpectNoHistogramsForAddressesDiff();
-  ExpectNoHistogramsForCardsDiff();
 }
 
 TEST_P(AutofillWalletSyncBridgeTest, ApplyStopSyncChanges_KeepData) {
@@ -864,8 +807,6 @@ TEST_P(AutofillWalletSyncBridgeTest, ApplyStopSyncChanges_KeepData) {
   bridge()->ApplyStopSyncChanges(/*delete_metadata_change_list=*/nullptr);
 
   EXPECT_FALSE(GetAllLocalData().empty());
-  ExpectNoHistogramsForAddressesDiff();
-  ExpectNoHistogramsForCardsDiff();
 }
 
 TEST_P(AutofillWalletSyncBridgeTest, NotifiesWhenActivelySyncing) {

components/sync/base/data_type_histogram.cc

@@ -26,7 +26,3 @@ void SyncRecordModelTypeCountHistogram(syncer::ModelType model_type,
       kModelTypeCountHistogramPrefix + type_string;
   base::UmaHistogramCounts1M(full_histogram_name, count);
 }
-
-void SyncWalletDataRecordClearedEntitiesCount(size_t count) {
-  UMA_HISTOGRAM_COUNTS_100("Sync.Wallet.EntitiesClearedWhenDisabled", count);
-}

components/sync/base/data_type_histogram.h

@@ -21,9 +21,6 @@ void SyncRecordModelTypeMemoryHistogram(syncer::ModelType model_type,
 void SyncRecordModelTypeCountHistogram(syncer::ModelType model_type,
                                        size_t count);
 
-// Records that |count| entities have been cleared when disabling sync.
-void SyncWalletDataRecordClearedEntitiesCount(size_t count);
-
 // Helper macro for datatype specific histograms. For each datatype, invokes
 // a pre-defined PER_DATA_TYPE_MACRO(type_str), where |type_str| is the string
 // version of the datatype.

tools/metrics/histograms/histograms.xml

@@ -10157,6 +10157,9 @@ uploading your change for review.
 
 <histogram base="true" name="Autofill.WalletAddresses2" units="addresses"
     expires_after="2019-06-01">
+  <obsolete>
+    Deprecated as of 05/2019.
+  </obsolete>
 <!-- Name completed by histogram_suffixes name="AutofillWalletCardsDiff" -->
 
   <owner>sebsg@chromium.org</owner>
@@ -10242,6 +10245,9 @@ uploading your change for review.
 
 <histogram base="true" name="Autofill.WalletCards2" units="credit cards"
     expires_after="2019-06-01">
+  <obsolete>
+    Deprecated as of 05/2019.
+  </obsolete>
 <!-- Name completed by histogram_suffixes name="AutofillWalletCardsDiff" -->
 
   <owner>sebsg@chromium.org</owner>
@@ -127489,6 +127495,9 @@ should be kept until we use this API.  -->
 
 <histogram name="Sync.Wallet.EntitiesClearedWhenDisabled" units="entities"
     expires_after="M74">
+  <obsolete>
+    Deprecated as of 05/2019.
+  </obsolete>
   <owner>jkrcal@chromium.org</owner>
   <owner>feuunk@chromium.org</owner>
   <summary>