[AF Profile] Add metrics for debugging difference between USS/Directory

This CL adds metrics that break up origin for each locally committed change by autofill_profile sync. This is needed for debugging the USS launch. The new code will be removed again after the investigation is over. Bug: 904390 Change-Id: I87175131fe2a4d298bf1e76e0fa1840b18dbe0f8 Reviewed-on: https://chromium-review.googlesource.com/c/1412477 Reviewed-by: Marc Treib <treib@chromium.org> Reviewed-by: Sebastien Seguin-Gagnon <sebsg@chromium.org> Reviewed-by: Jesse Doherty <jwd@chromium.org> Commit-Queue: Jan Krcal <jkrcal@chromium.org> Cr-Commit-Position: refs/heads/master@{#631629}
Denger-Network · Feb 13, 2019 · 5d1e827 · 5d1e827
1 parent 3df82ef
commit 5d1e827
Show file tree

Hide file tree

Showing 8 changed files with 194 additions and 5 deletions.
diff --git a/components/autofill/core/browser/autofill_profile_sync_util.cc b/components/autofill/core/browser/autofill_profile_sync_util.cc
@@ -5,10 +5,14 @@
 #include "components/autofill/core/browser/autofill_profile_sync_util.h"
 
 #include "base/guid.h"
+// TODO(crbug.com/904390): Remove when the investigation is over.
+#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_data_util.h"
 #include "components/autofill/core/browser/autofill_profile.h"
+// TODO(crbug.com/904390): Remove when the investigation is over.
+#include "components/autofill/core/browser/autofill_profile_comparator.h"
 #include "components/autofill/core/browser/country_names.h"
 #include "components/autofill/core/browser/field_types.h"
 #include "components/autofill/core/browser/proto/autofill_sync.pb.h"
@@ -235,4 +239,28 @@ std::string GetStorageKeyFromAutofillProfileSpecifics(
   return specifics.guid();
 }
 
+bool IsLocalProfileEqualToServerProfile(
+    const std::vector<std::unique_ptr<AutofillProfile>>& server_profiles,
+    const AutofillProfile& local_profile,
+    const std::string& app_locale) {
+  AutofillProfileComparator comparator(app_locale);
+  for (const auto& server_profile : server_profiles) {
+    // The same logic as when deciding whether to convert into a new profile in
+    // PersonalDataManager::MergeServerAddressesIntoProfiles.
+    if (comparator.AreMergeable(*server_profile, local_profile) &&
+        (!local_profile.IsVerified() || !server_profile->IsVerified())) {
+      return true;
+    }
+  }
+  return false;
+}
+
+void ReportAutofillProfileAddOrUpdateOrigin(
+    AutofillProfileSyncChangeOrigin origin) {
+  UMA_HISTOGRAM_ENUMERATION("Sync.AutofillProfile.AddOrUpdateOrigin", origin);
+}
+void ReportAutofillProfileDeleteOrigin(AutofillProfileSyncChangeOrigin origin) {
+  UMA_HISTOGRAM_ENUMERATION("Sync.AutofillProfile.DeleteOrigin", origin);
+}
+
 }  // namespace autofill
diff --git a/components/autofill/core/browser/autofill_profile_sync_util.h b/components/autofill/core/browser/autofill_profile_sync_util.h
@@ -7,6 +7,8 @@
 
 #include <memory>
 #include <string>
+// TODO(crbug.com/904390): Remove when the investigation is over.
+#include <vector>
 
 namespace syncer {
 struct EntityData;
@@ -40,6 +42,24 @@ std::string GetStorageKeyFromAutofillProfile(const AutofillProfile& entry);
 std::string GetStorageKeyFromAutofillProfileSpecifics(
     const sync_pb::AutofillProfileSpecifics& specifics);
 
+// TODO(crbug.com/904390): Remove when the investigation is over.
+bool IsLocalProfileEqualToServerProfile(
+    const std::vector<std::unique_ptr<AutofillProfile>>& server_profiles,
+    const AutofillProfile& local_profile,
+    const std::string& app_locale);
+
+// TODO(crbug.com/904390): Remove when the investigation is over.
+enum class AutofillProfileSyncChangeOrigin {
+  kTrulyLocal = 0,
+  kConvertedLocal = 1,
+  kIncrementalRemote = 2,
+  kInitial = 3,
+  kMaxValue = kInitial,
+};
+void ReportAutofillProfileAddOrUpdateOrigin(
+    AutofillProfileSyncChangeOrigin origin);
+void ReportAutofillProfileDeleteOrigin(AutofillProfileSyncChangeOrigin origin);
+
 }  // namespace autofill
 
 #endif  // COMPONENTS_AUTOFILL_CORE_BROWSER_AUTOFILL_PROFILE_SYNC_UTIL_H_
diff --git a/components/autofill/core/browser/webdata/autofill_profile_sync_bridge.cc b/components/autofill/core/browser/webdata/autofill_profile_sync_bridge.cc
@@ -49,6 +49,20 @@ namespace {
 // Address to this variable used as the user data key.
 static int kAutofillProfileSyncBridgeUserDataKey = 0;
 
+// TODO(crbug.com/904390): Remove when the investigation is over.
+base::Optional<AutofillProfile> FindLocalProfileByStorageKey(
+    const std::string& storage_key,
+    AutofillTable* table) {
+  std::vector<std::unique_ptr<AutofillProfile>> local_profiles;
+  table->GetAutofillProfiles(&local_profiles);
+  for (const auto& local_profile : local_profiles) {
+    if (storage_key == GetStorageKeyFromAutofillProfile(*local_profile)) {
+      return *local_profile;
+    }
+  }
+  return base::nullopt;
+}
+
 }  // namespace
 
 // static
@@ -124,8 +138,9 @@ Optional<syncer::ModelError> AutofillProfileSyncBridge::MergeSyncData(
 
   RETURN_IF_ERROR(
       initial_sync_tracker.MergeSimilarEntriesForInitialSync(app_locale_));
-  RETURN_IF_ERROR(
-      FlushSyncTracker(std::move(metadata_change_list), &initial_sync_tracker));
+  RETURN_IF_ERROR(FlushSyncTracker(std::move(metadata_change_list),
+                                   &initial_sync_tracker,
+                                   AutofillProfileSyncChangeOrigin::kInitial));
 
   web_data_backend_->NotifyThatSyncHasStarted(syncer::AUTOFILL_PROFILE);
   return base::nullopt;
@@ -156,7 +171,8 @@ Optional<ModelError> AutofillProfileSyncBridge::ApplySyncChanges(
     }
   }
 
-  return FlushSyncTracker(std::move(metadata_change_list), &tracker);
+  return FlushSyncTracker(std::move(metadata_change_list), &tracker,
+                          AutofillProfileSyncChangeOrigin::kIncrementalRemote);
 }
 
 void AutofillProfileSyncBridge::GetData(StorageKeyList storage_keys,
@@ -215,13 +231,35 @@ void AutofillProfileSyncBridge::ActOnLocalChange(
       std::make_unique<syncer::SyncMetadataStoreChangeList>(
           GetAutofillTable(), syncer::AUTOFILL_PROFILE);
 
+  // TODO(crbug.com/904390): Remove when the investigation is over.
+  base::Optional<AutofillProfile> local_profile;
+  if (change.type() == AutofillProfileChange::REMOVE) {
+    local_profile =
+        FindLocalProfileByStorageKey(change.key(), GetAutofillTable());
+  } else {
+    local_profile = *change.data_model();
+  }
+  std::vector<std::unique_ptr<AutofillProfile>> server_profiles;
+  GetAutofillTable()->GetServerProfiles(&server_profiles);
+  bool is_converted_from_server = false;
+  if (local_profile != base::nullopt) {
+    is_converted_from_server = IsLocalProfileEqualToServerProfile(
+        server_profiles, *local_profile, app_locale_);
+  }
+
   switch (change.type()) {
     case AutofillProfileChange::ADD:
     case AutofillProfileChange::UPDATE:
       change_processor()->Put(
           change.key(),
           CreateEntityDataFromAutofillProfile(*change.data_model()),
           metadata_change_list.get());
+
+      // TODO(crbug.com/904390): Remove when the investigation is over.
+      ReportAutofillProfileAddOrUpdateOrigin(
+          is_converted_from_server
+              ? AutofillProfileSyncChangeOrigin::kConvertedLocal
+              : AutofillProfileSyncChangeOrigin::kTrulyLocal);
       break;
     case AutofillProfileChange::REMOVE:
       // Removals have no data_model() so this change can still be for a
@@ -231,6 +269,15 @@ void AutofillProfileSyncBridge::ActOnLocalChange(
       // TODO(jkrcal): implement a hash map of known storage_keys and use it
       // here.
       change_processor()->Delete(change.key(), metadata_change_list.get());
+
+      // TODO(crbug.com/904390): Remove when the investigation is over.
+      if (local_profile != base::nullopt) {
+        // Report only if we delete an existing entity.
+        ReportAutofillProfileDeleteOrigin(
+            is_converted_from_server
+                ? AutofillProfileSyncChangeOrigin::kConvertedLocal
+                : AutofillProfileSyncChangeOrigin::kTrulyLocal);
+      }
       break;
     case AutofillProfileChange::EXPIRE:
       // EXPIRE changes are not being issued for profiles.
@@ -245,7 +292,8 @@ void AutofillProfileSyncBridge::ActOnLocalChange(
 
 base::Optional<syncer::ModelError> AutofillProfileSyncBridge::FlushSyncTracker(
     std::unique_ptr<MetadataChangeList> metadata_change_list,
-    AutofillProfileSyncDifferenceTracker* tracker) {
+    AutofillProfileSyncDifferenceTracker* tracker,
+    AutofillProfileSyncChangeOrigin origin) {
   DCHECK(tracker);
 
   RETURN_IF_ERROR(tracker->FlushToLocal(
@@ -259,6 +307,9 @@ base::Optional<syncer::ModelError> AutofillProfileSyncBridge::FlushSyncTracker(
     change_processor()->Put(GetStorageKeyFromAutofillProfile(*entry),
                             CreateEntityDataFromAutofillProfile(*entry),
                             metadata_change_list.get());
+
+    // TODO(crbug.com/904390): Remove when the investigation is over.
+    ReportAutofillProfileAddOrUpdateOrigin(origin);
   }
 
   return static_cast<syncer::SyncMetadataStoreChangeList*>(

diff --git a/components/autofill/core/browser/webdata/autofill_profile_sync_bridge.h b/components/autofill/core/browser/webdata/autofill_profile_sync_bridge.h
@@ -29,6 +29,7 @@ class AutofillProfileSyncDifferenceTracker;
 class AutofillTable;
 class AutofillWebDataBackend;
 class AutofillWebDataService;
+enum class AutofillProfileSyncChangeOrigin;
 
 // Sync bridge implementation for AUTOFILL_PROFILE model type. Takes care of
 // propagating local autofill profiles to other clients as well as incorporating
@@ -90,7 +91,9 @@ class AutofillProfileSyncBridge
   // Flushes changes accumulated within |tracker| both to local and to sync.
   base::Optional<syncer::ModelError> FlushSyncTracker(
       std::unique_ptr<syncer::MetadataChangeList> metadata_change_list,
-      AutofillProfileSyncDifferenceTracker* tracker);
+      AutofillProfileSyncDifferenceTracker* tracker,
+      // TODO(crbug.com/904390): Remove |origin| when the investigation is over.
+      AutofillProfileSyncChangeOrigin origin);
 
   // Synchronously load sync metadata from the autofill table and pass it to the
   // processor so that it can start tracking changes.

diff --git a/components/autofill/core/browser/webdata/autofill_profile_syncable_service.cc b/components/autofill/core/browser/webdata/autofill_profile_syncable_service.cc
@@ -17,6 +17,8 @@
 #include "components/autofill/core/browser/autofill_country.h"
 #include "components/autofill/core/browser/autofill_profile.h"
 #include "components/autofill/core/browser/autofill_profile_comparator.h"
+// TODO(crbug.com/904390): Remove when the investigation is over.
+#include "components/autofill/core/browser/autofill_profile_sync_util.h"
 #include "components/autofill/core/browser/country_names.h"
 #include "components/autofill/core/browser/form_group.h"
 #include "components/autofill/core/browser/webdata/autofill_table.h"
@@ -184,6 +186,10 @@ AutofillProfileSyncableService::MergeDataAndStartSyncing(
         syncer::SyncChange(FROM_HERE,
                            syncer::SyncChange::ACTION_ADD,
                            CreateData(*(it.second))));
+
+    // TODO(crbug.com/904390): Remove when the investigation is over.
+    ReportAutofillProfileAddOrUpdateOrigin(
+        AutofillProfileSyncChangeOrigin::kInitial);
     profiles_map_[it.first] = it.second;
   }
 
@@ -192,6 +198,10 @@ AutofillProfileSyncableService::MergeDataAndStartSyncing(
         syncer::SyncChange(FROM_HERE,
                            syncer::SyncChange::ACTION_UPDATE,
                            CreateData(*(bundle.profiles_to_sync_back[i]))));
+
+    // TODO(crbug.com/904390): Remove when the investigation is over.
+    ReportAutofillProfileAddOrUpdateOrigin(
+        AutofillProfileSyncChangeOrigin::kInitial);
   }
 
   if (!new_changes.empty()) {
@@ -603,6 +613,24 @@ void AutofillProfileSyncableService::ActOnChange(
     return;
   }
 
+  // TODO(crbug.com/904390): Remove when the investigation is over.
+  const AutofillProfile* local_profile = nullptr;
+  if (change.type() == AutofillProfileChange::REMOVE) {
+    if (profiles_map_.find(change.key()) != profiles_map_.end()) {
+      local_profile = profiles_map_[change.key()];
+    }
+  } else {
+    local_profile = change.data_model();
+  }
+  bool is_converted_from_server = false;
+  // |webdata_backend_| may be null in unit-tests.
+  if (local_profile != nullptr && webdata_backend_ != nullptr) {
+    std::vector<std::unique_ptr<AutofillProfile>> server_profiles;
+    GetAutofillTable()->GetServerProfiles(&server_profiles);
+    is_converted_from_server = IsLocalProfileEqualToServerProfile(
+        server_profiles, *local_profile, app_locale_);
+  }
+
   syncer::SyncChangeList new_changes;
   DataBundle bundle;
   switch (change.type()) {
@@ -616,6 +644,12 @@ void AutofillProfileSyncableService::ActOnChange(
       profiles_.push_back(
           std::make_unique<AutofillProfile>(*(change.data_model())));
       profiles_map_[change.data_model()->guid()] = profiles_.back().get();
+
+      // TODO(crbug.com/904390): Remove when the investigation is over.
+      ReportAutofillProfileAddOrUpdateOrigin(
+          is_converted_from_server
+              ? AutofillProfileSyncChangeOrigin::kConvertedLocal
+              : AutofillProfileSyncChangeOrigin::kTrulyLocal);
       break;
     case AutofillProfileChange::UPDATE: {
       auto it = profiles_map_.find(change.data_model()->guid());
@@ -625,6 +659,12 @@ void AutofillProfileSyncableService::ActOnChange(
           syncer::SyncChange(FROM_HERE,
                              syncer::SyncChange::ACTION_UPDATE,
                              CreateData(*(change.data_model()))));
+
+      // TODO(crbug.com/904390): Remove when the investigation is over.
+      ReportAutofillProfileAddOrUpdateOrigin(
+          is_converted_from_server
+              ? AutofillProfileSyncChangeOrigin::kConvertedLocal
+              : AutofillProfileSyncChangeOrigin::kTrulyLocal);
       break;
     }
     case AutofillProfileChange::REMOVE: {
@@ -636,6 +676,11 @@ void AutofillProfileSyncableService::ActOnChange(
             syncer::SyncChange(FROM_HERE, syncer::SyncChange::ACTION_DELETE,
                                CreateData(empty_profile)));
         profiles_map_.erase(change.key());
+        // TODO(crbug.com/904390): Remove when the investigation is over.
+        ReportAutofillProfileDeleteOrigin(
+            is_converted_from_server
+                ? AutofillProfileSyncChangeOrigin::kConvertedLocal
+                : AutofillProfileSyncChangeOrigin::kTrulyLocal);
       }
       break;
     }

diff --git a/components/browser_sync/profile_sync_service_autofill_unittest.cc b/components/browser_sync/profile_sync_service_autofill_unittest.cc
@@ -144,6 +144,11 @@ class AutofillTableMock : public AutofillTable {
   MOCK_METHOD1(UpdateAutofillProfile, bool(const AutofillProfile&));   // NOLINT
   MOCK_METHOD1(AddAutofillProfile, bool(const AutofillProfile&));      // NOLINT
   MOCK_METHOD1(RemoveAutofillProfile, bool(const std::string&));       // NOLINT
+
+  // TODO(crbug.com/904390): Remove when the investigation is over.
+  MOCK_CONST_METHOD1(
+      GetServerProfiles,
+      bool(std::vector<std::unique_ptr<AutofillProfile>>*));  // NOLINT
 };
 
 MATCHER_P(MatchProfiles, profile, "") {
@@ -950,6 +955,10 @@ TEST_F(ProfileSyncServiceAutofillTest, ProcessUserChangeAddProfile) {
       "Alicia", "Saenz", "joewayne@me.xyz", "Fox", "1212 Center.", "Bld. 5",
       "Orlando", "FL", "32801", "US", "19482937549");
 
+  // TODO(crbug.com/904390): Remove when the investigation is over. This call is
+  // needed in the AutofillProfileChanged() callback.
+  EXPECT_CALL(autofill_table(), GetServerProfiles(_)).WillOnce(Return(true));
+
   AutofillProfileChange change(AutofillProfileChange::ADD, added_profile.guid(),
                                &added_profile);
   web_data_service()->OnAutofillProfileChanged(change);
@@ -989,6 +998,10 @@ TEST_F(ProfileSyncServiceAutofillTest, ProcessUserChangeRemoveProfile) {
   StartAutofillProfileSyncService(add_autofill.callback());
   ASSERT_TRUE(add_autofill.success());
 
+  // TODO(crbug.com/904390): Remove when the investigation is over. This call is
+  // needed in the AutofillProfileChanged() callback.
+  EXPECT_CALL(autofill_table(), GetServerProfiles(_)).WillOnce(Return(true));
+
   AutofillProfileChange change(AutofillProfileChange::REMOVE,
                                sync_profile.guid(), nullptr);
   web_data_service()->OnAutofillProfileChanged(change);

diff --git a/tools/metrics/histograms/enums.xml b/tools/metrics/histograms/enums.xml
@@ -3204,6 +3204,15 @@ uploading your change for review.  These are checked by presubmit scripts.
   <int value="2" label="New profile created"/>
 </enum>
 
+<enum name="AutofillProfileSyncChangeOrigin">
+  <int value="0" label="Change in a local profile"/>
+  <int value="1"
+      label="Change in a converted local profile (that still equals to a
+             server profile)"/>
+  <int value="2" label="Resolving conflict in incremental remote update"/>
+  <int value="3" label="Upload / conflict resolution in initial sync"/>
+</enum>
+
 <enum name="AutofillQuality">
   <int value="0" label="Submitted"/>
   <int value="1" label="Autofilled"/>

diff --git a/tools/metrics/histograms/histograms.xml b/tools/metrics/histograms/histograms.xml
@@ -114124,6 +114124,26 @@ uploading your change for review.
   </summary>
 </histogram>
 
+<histogram name="Sync.AutofillProfile.AddOrUpdateOrigin"
+    enum="AutofillProfileSyncChangeOrigin" expires_after="M75">
+  <owner>jkrcal@chromium.org</owner>
+  <owner>treib@chromium.org</owner>
+  <summary>
+    Reported for autofill profile sync once per every locally committed entity
+    creation/update. It breaks down the origin of such a local change.
+  </summary>
+</histogram>
+
+<histogram name="Sync.AutofillProfile.DeleteOrigin"
+    enum="AutofillProfileSyncChangeOrigin" expires_after="M73">
+  <owner>jkrcal@chromium.org</owner>
+  <owner>treib@chromium.org</owner>
+  <summary>
+    Reported for autofill profile sync once per every locally committed entity
+    deletion. It breaks down the origin of such a local deletion.
+  </summary>
+</histogram>
+
 <histogram name="Sync.AutofillProfileAssociationTime" units="ms">
   <obsolete>
     Replaced by Sync.AutofillProfilesAssociationTime.