[Sync::USS] Adding UMA mertics to track bookmark positioning schemes

Bug: 516866
Change-Id: Ib0b5ea0ad7e4ff08c9d0ae6b65cfd91d9a468a75
Reviewed-on: https://chromium-review.googlesource.com/1236254
Commit-Queue: Mohamed Amir Yosef <mamir@chromium.org>
Reviewed-by: Robert Kaplow <rkaplow@chromium.org>
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#592878}

components/sync/engine_impl/model_type_worker.cc

@@ -16,6 +16,7 @@
 #include "base/guid.h"
 #include "base/logging.h"
 #include "base/metrics/histogram_functions.h"
+#include "base/metrics/histogram_macros.h"
 #include "base/strings/stringprintf.h"
 #include "base/trace_event/memory_usage_estimator.h"
 #include "components/sync/base/cancelation_signal.h"
@@ -29,6 +30,8 @@
 #include "components/sync/engine_impl/syncer_proto_util.h"
 #include "components/sync/protocol/proto_memory_estimations.h"
 
+namespace syncer {
+
 namespace {
 
 bool ContainsDuplicate(std::vector<std::string> values) {
@@ -36,9 +39,19 @@ bool ContainsDuplicate(std::vector<std::string> values) {
   return std::adjacent_find(values.begin(), values.end()) != values.end();
 }
 
-}  // namespace
+// Enumeration of possible values for the positioning schemes used in Sync
+// entities. Used in UMA metrics. Do not re-order or delete these entries; they
+// are used in a UMA histogram. Please edit SyncPositioningScheme in enums.xml
+// if a value is added.
+enum class SyncPositioningScheme {
+  UNIQUE_POSITION = 0,
+  POSITION_IN_PARENT = 1,
+  INSERT_AFTER_ITEM_ID = 2,
+  MISSING = 3,
+  kMaxValue = MISSING
+};
 
-namespace syncer {
+}  // namespace
 
 ModelTypeWorker::ModelTypeWorker(
     ModelType type,
@@ -190,9 +203,12 @@ ModelTypeWorker::DecryptionStatus ModelTypeWorker::PopulateUpdateResponseData(
   data.parent_id = update_entity.parent_id_string();
 
   // Handle deprecated positioning fields. Relevant only for bookmarks.
-  // TODO(crbug.com/516866): Add coressponding UMA metrics for each case below.
+  bool has_position_scheme = false;
+  SyncPositioningScheme syncPositioningScheme;
   if (update_entity.has_unique_position()) {
     data.unique_position = update_entity.unique_position();
+    has_position_scheme = true;
+    syncPositioningScheme = SyncPositioningScheme::UNIQUE_POSITION;
   } else if (update_entity.has_position_in_parent() ||
              update_entity.has_insert_after_item_id()) {
     bool missing_originator_fields = false;
@@ -210,18 +226,28 @@ ModelTypeWorker::DecryptionStatus ModelTypeWorker::PopulateUpdateResponseData(
                   /*client_tag=*/update_entity.originator_cache_guid() +
                       update_entity.originator_client_item_id());
 
-    if (update_entity.has_originator_cache_guid()) {
+    if (update_entity.has_position_in_parent()) {
       data.unique_position =
           UniquePosition::FromInt64(update_entity.position_in_parent(), suffix)
               .ToProto();
-
+      has_position_scheme = true;
+      syncPositioningScheme = SyncPositioningScheme::POSITION_IN_PARENT;
     } else {
       // If update_entity has insert_after_item_id, use 0 index.
+      DCHECK(update_entity.has_insert_after_item_id());
       data.unique_position = UniquePosition::FromInt64(0, suffix).ToProto();
+      has_position_scheme = true;
+      syncPositioningScheme = SyncPositioningScheme::INSERT_AFTER_ITEM_ID;
     }
   } else if (SyncerProtoUtil::ShouldMaintainPosition(update_entity) &&
              !update_entity.deleted()) {
     DLOG(ERROR) << "Missing required position information in update.";
+    has_position_scheme = true;
+    syncPositioningScheme = SyncPositioningScheme::MISSING;
+  }
+  if (has_position_scheme) {
+    UMA_HISTOGRAM_ENUMERATION("Sync.Entities.PositioningScheme",
+                              syncPositioningScheme);
   }
 
   data.server_defined_unique_tag = update_entity.server_defined_unique_tag();

components/sync/engine_impl/model_type_worker_unittest.cc

@@ -57,6 +57,14 @@ const std::string kHash1(GenerateTagHash(kTag1));
 const std::string kHash2(GenerateTagHash(kTag2));
 const std::string kHash3(GenerateTagHash(kTag3));
 
+enum class ExpectedSyncPositioningScheme {
+  UNIQUE_POSITION = 0,
+  POSITION_IN_PARENT = 1,
+  INSERT_AFTER_ITEM_ID = 2,
+  MISSING = 3,
+  kMaxValue = MISSING
+};
+
 EntitySpecifics GenerateSpecifics(const std::string& tag,
                                   const std::string& value) {
   EntitySpecifics specifics;
@@ -1355,6 +1363,7 @@ TEST_F(ModelTypeWorkerTest, PopulateUpdateResponseData) {
 
   FakeEncryptor encryptor;
   Cryptographer cryptographer(&encryptor);
+  base::HistogramTester histogram_tester;
 
   EXPECT_EQ(ModelTypeWorker::SUCCESS,
             ModelTypeWorker::PopulateUpdateResponseData(&cryptographer, entity,
@@ -1370,6 +1379,12 @@ TEST_F(ModelTypeWorkerTest, PopulateUpdateResponseData) {
   EXPECT_FALSE(data.is_deleted());
   EXPECT_EQ(kTag1, data.specifics.preference().name());
   EXPECT_EQ(kValue1, data.specifics.preference().value());
+
+  histogram_tester.ExpectUniqueSample(
+      "Sync.Entities.PositioningScheme",
+      /*sample=*/
+      ExpectedSyncPositioningScheme::UNIQUE_POSITION,
+      /*count=*/1);
 }
 
 TEST_F(ModelTypeWorkerTest, PopulateUpdateResponseDataWithPositionInParent) {
@@ -1384,6 +1399,7 @@ TEST_F(ModelTypeWorkerTest, PopulateUpdateResponseDataWithPositionInParent) {
   UpdateResponseData response_data;
   FakeEncryptor encryptor;
   Cryptographer cryptographer(&encryptor);
+  base::HistogramTester histogram_tester;
 
   EXPECT_EQ(ModelTypeWorker::SUCCESS,
             ModelTypeWorker::PopulateUpdateResponseData(&cryptographer, entity,
@@ -1391,6 +1407,12 @@ TEST_F(ModelTypeWorkerTest, PopulateUpdateResponseDataWithPositionInParent) {
   const EntityData& data = response_data.entity.value();
   EXPECT_TRUE(
       syncer::UniquePosition::FromProto(data.unique_position).IsValid());
+
+  histogram_tester.ExpectUniqueSample(
+      "Sync.Entities.PositioningScheme",
+      /*sample=*/
+      ExpectedSyncPositioningScheme::POSITION_IN_PARENT,
+      /*count=*/1);
 }
 
 TEST_F(ModelTypeWorkerTest, PopulateUpdateResponseDataWithInsertAfterItemId) {
@@ -1405,6 +1427,7 @@ TEST_F(ModelTypeWorkerTest, PopulateUpdateResponseDataWithInsertAfterItemId) {
   UpdateResponseData response_data;
   FakeEncryptor encryptor;
   Cryptographer cryptographer(&encryptor);
+  base::HistogramTester histogram_tester;
 
   EXPECT_EQ(ModelTypeWorker::SUCCESS,
             ModelTypeWorker::PopulateUpdateResponseData(&cryptographer, entity,
@@ -1412,6 +1435,63 @@ TEST_F(ModelTypeWorkerTest, PopulateUpdateResponseDataWithInsertAfterItemId) {
   const EntityData& data = response_data.entity.value();
   EXPECT_TRUE(
       syncer::UniquePosition::FromProto(data.unique_position).IsValid());
+  histogram_tester.ExpectUniqueSample(
+      "Sync.Entities.PositioningScheme",
+      /*sample=*/
+      ExpectedSyncPositioningScheme::INSERT_AFTER_ITEM_ID,
+      /*count=*/1);
+}
+
+TEST_F(ModelTypeWorkerTest,
+       PopulateUpdateResponseDataWithBookmarkMissingPosition) {
+  InitializeCommitOnly();
+  sync_pb::SyncEntity entity;
+
+  entity.set_client_defined_unique_tag("CLIENT_TAG");
+  entity.set_server_defined_unique_tag("SERVER_TAG");
+  EntitySpecifics specifics;
+  specifics.mutable_bookmark()->set_url("http://www.url.com");
+
+  entity.mutable_specifics()->CopyFrom(specifics);
+
+  UpdateResponseData response_data;
+  FakeEncryptor encryptor;
+  Cryptographer cryptographer(&encryptor);
+  base::HistogramTester histogram_tester;
+
+  EXPECT_EQ(ModelTypeWorker::SUCCESS,
+            ModelTypeWorker::PopulateUpdateResponseData(&cryptographer, entity,
+                                                        &response_data));
+  const EntityData& data = response_data.entity.value();
+  EXPECT_FALSE(
+      syncer::UniquePosition::FromProto(data.unique_position).IsValid());
+  histogram_tester.ExpectUniqueSample("Sync.Entities.PositioningScheme",
+                                      /*sample=*/
+                                      ExpectedSyncPositioningScheme::MISSING,
+                                      /*count=*/1);
+}
+
+TEST_F(ModelTypeWorkerTest,
+       PopulateUpdateResponseDataWithNonBookmarkHasNoPosition) {
+  InitializeCommitOnly();
+  sync_pb::SyncEntity entity;
+
+  EntitySpecifics specifics;
+  entity.mutable_specifics()->CopyFrom(GenerateSpecifics(kTag1, kValue1));
+
+  UpdateResponseData response_data;
+  FakeEncryptor encryptor;
+  Cryptographer cryptographer(&encryptor);
+  base::HistogramTester histogram_tester;
+
+  EXPECT_EQ(ModelTypeWorker::SUCCESS,
+            ModelTypeWorker::PopulateUpdateResponseData(&cryptographer, entity,
+                                                        &response_data));
+  const EntityData& data = response_data.entity.value();
+  EXPECT_FALSE(
+      syncer::UniquePosition::FromProto(data.unique_position).IsValid());
+  histogram_tester.ExpectTotalCount("Sync.Entities.PositioningScheme",
+                                    /*count=*/0);
 }
 
 class GetLocalChangesRequestTest : public testing::Test {

tools/metrics/histograms/enums.xml

@@ -47637,6 +47637,13 @@ would be helpful to identify which type is being sent.
   <int value="4" label="Not sync password hash change event"/>
 </enum>
 
+<enum name="SyncPositioningScheme">
+  <int value="0" label="UNIQUE_POSITION"/>
+  <int value="1" label="POSITION_IN_PARENT"/>
+  <int value="2" label="INSERT_AFTER_ITEM_ID"/>
+  <int value="3" label="MISSING"/>
+</enum>
+
 <enum name="SyncPromoAction">
   <int value="0" label="The promo was shown to the user"/>
   <int value="1" label="The user clicked the promo to sign in"/>

tools/metrics/histograms/histograms.xml

@@ -103673,6 +103673,17 @@ uploading your change for review.
   </summary>
 </histogram>
 
+<histogram name="Sync.Entities.PositioningScheme" enum="SyncPositioningScheme"
+    expires_after="M75">
+  <owner>mamir@chromium.org</owner>
+  <summary>
+    The positioning scheme used within sync entities. It is reported for data
+    types migrated to USS only. While it is reported for all data types, the
+    positioning information are expected to be set for bookmarks only.
+    &quot;MISSING&quot; is reported only for non deleted bookmarks.
+  </summary>
+</histogram>
+
 <histogram name="Sync.EventCodes" enum="SyncEventCode">
   <owner>zea@chromium.org</owner>
   <summary>A UI event occured.</summary>