Measuring data use of different ModelTypes in Sync Service.

sync/engine/syncer_unittest.cc

Index: sync/engine/syncer_unittest.cc
diff --git a/sync/engine/syncer_unittest.cc b/sync/engine/syncer_unittest.cc
index dcd326e8c887888ddab0eb867fafb20be8a07bcb..50f4485c5235048e9782c770b2c8e4d1af89e6a3 100644
--- a/sync/engine/syncer_unittest.cc
+++ b/sync/engine/syncer_unittest.cc
@@ -20,6 +20,7 @@
 #include "base/memory/scoped_ptr.h"
 #include "base/message_loop/message_loop.h"
 #include "base/strings/string_number_conversions.h"
+#include "base/test/histogram_tester.h"
 #include "base/time/time.h"
 #include "build/build_config.h"
 #include "sync/engine/backoff_delay_provider.h"
@@ -666,6 +667,118 @@ TEST_F(SyncerTest, GetCommitIdsFiltersThrottledEntries) {
   }
 }
 
+// This test has three steps. In the first step, a BOOKMARK update is received.
+// In the next step, syncing BOOKMARKS is disabled, so no BOOKMARK is sent or
+// received. In the last step, a BOOKMARK update is committed.
+TEST_F(SyncerTest, DataUseHistogramsTest) {
+  base::HistogramTester histogram_tester;
+  sync_pb::EntitySpecifics bookmark_data;
+  AddDefaultFieldValue(BOOKMARKS, &bookmark_data);
+
+  mock_server_->AddUpdateDirectory(1, 0, "A", 10, 10,
+                                   foreign_cache_guid(), "-1");
+  int download_bytes_bookmark = 0;
+  vector<int> progress_bookmark = {0, 0, 0};

[Nicolas Zea, 2015/08/19 22:01:51]

nit: std:: here and below

[amohammadkhan, 2015/08/20 00:20:13]

Done.

+  vector<int> progress_all = {0, 0, 0};

[sclittle, 2015/08/19 22:41:35]

Instead of using these progress vectors and comparing them, could you just use a
different histogram_tester for each of the blocks below? i.e. maybe scope the
histogram_tester within those blocks. You might be able to cut out the for loops
in each of the blocks if there would only be 1 sample then.

[amohammadkhan, 2015/08/20 00:20:13]

By comparing these values I am showing the progress and we will need the for
loops anyways. (Because of the progress makers of other types) If you see any
other drawbacks besides the loops please let me know.

+  std::vector<base::Bucket> samples;
+  EXPECT_TRUE(SyncShareNudge());
+  {
+    histogram_tester.ExpectTotalCount("DataUse.Sync.Upload.Count", 0);
+    histogram_tester.ExpectTotalCount("DataUse.Sync.Upload.Bytes", 0);
+    histogram_tester.ExpectTotalCount("DataUse.Sync.Download.Count", 1);
+    histogram_tester.ExpectUniqueSample("DataUse.Sync.Download.Count",
+                                        BOOKMARKS, 1);
+    samples = histogram_tester.GetAllSamples("DataUse.Sync.Download.Bytes");
+    EXPECT_TRUE(samples.size() == 1);

[Nicolas Zea, 2015/08/19 22:01:51]

nit: use EXPECT_EQ, EXPECT_GE, EXPECT_GT where possible here and below

[amohammadkhan, 2015/08/20 00:20:13]

Done.

+    EXPECT_TRUE(samples.at(0).min == BOOKMARKS);
+    EXPECT_TRUE(samples.at(0).count >= 0);
+    download_bytes_bookmark = samples.at(0).count;
+
+    samples = histogram_tester.GetAllSamples(
+        "DataUse.Sync.ProgressMarker.Bytes");
+
+    for(std::vector<base::Bucket>::iterator it = samples.begin();

[sclittle, 2015/08/19 22:41:35]

nit: use range-based for, e.g. for (const base::Bucket& bucket : samples)

[amohammadkhan, 2015/08/20 00:20:13]

Done.

+        it != samples.end(); ++it) {
+      if (it->min == BOOKMARKS)
+        progress_bookmark.at(0) += it->count;

[Nicolas Zea, 2015/08/19 22:01:51]

there should only be one bucket with value BOOKMARKS right? I think it's cleaner
to do an assignment then, rather than += (here and below)

[amohammadkhan, 2015/08/20 00:20:13]

Yes it should be one here. But I was thinking maybe it is better to count it if
it is more than one for some reason. But in below, it could be more than one.

+      progress_all.at(0) += it->count;
+    }
+    EXPECT_TRUE(progress_bookmark.at(0) > 0);
+    EXPECT_TRUE(progress_all.at(0) > 0);

[Nicolas Zea, 2015/08/19 22:01:51]

should progress_all == progress_bookmark? Maybe that's a better condition (here
and below)

[amohammadkhan, 2015/08/20 00:20:13]

They are not equal because of having progress markers for other types.
progress_all is larger than progress_bookmark.

+
+    WriteTransaction wtrans(FROM_HERE, UNITTEST, directory());
+    MutableEntry A(&wtrans, GET_BY_ID, ids_.FromNumber(1));
+    A.PutIsUnsynced(true);
+    A.PutSpecifics(bookmark_data);
+    A.PutNonUniqueName("bookmark");
+  }
+
+  // Now sync without enabling bookmarks.
+  mock_server_->ExpectGetUpdatesRequestTypes(
+      Difference(context_->GetEnabledTypes(), ModelTypeSet(BOOKMARKS)));
+  ResetSession();
+  syncer_->NormalSyncShare(
+      Difference(context_->GetEnabledTypes(), ModelTypeSet(BOOKMARKS)),
+      &nudge_tracker_, session_.get());
+
+  {
+    // Nothing should have been committed as bookmarks is throttled.
+    histogram_tester.ExpectTotalCount("DataUse.Sync.Upload.Count", 0);
+    histogram_tester.ExpectTotalCount("DataUse.Sync.Upload.Bytes", 0);
+    histogram_tester.ExpectTotalCount("DataUse.Sync.Download.Count", 1);
+    histogram_tester.ExpectUniqueSample("DataUse.Sync.Download.Count",
+                                        BOOKMARKS, 1);
+
+    samples = histogram_tester.GetAllSamples("DataUse.Sync.Download.Bytes");
+    EXPECT_TRUE(samples.size() == 1);
+    EXPECT_TRUE(samples.at(0).min == BOOKMARKS);
+    EXPECT_TRUE(samples.at(0).count == download_bytes_bookmark);
+
+    samples = histogram_tester.GetAllSamples(
+        "DataUse.Sync.ProgressMarker.Bytes");
+    for(std::vector<base::Bucket>::iterator it = samples.begin();
+        it != samples.end(); ++it) {
+      if (it->min == BOOKMARKS)
+        progress_bookmark.at(1) += it->count;
+      progress_all.at(1) += it->count;
+    }
+    EXPECT_TRUE(progress_bookmark.at(1) == progress_bookmark.at(0));
+    EXPECT_TRUE(progress_all.at(1) > progress_all.at(0));
+  }
+
+  // Sync again with bookmarks enabled.
+  mock_server_->ExpectGetUpdatesRequestTypes(context_->GetEnabledTypes());
+  EXPECT_TRUE(SyncShareNudge());
+  {
+    // It should have been committed.
+    histogram_tester.ExpectTotalCount("DataUse.Sync.Upload.Count", 1);
+    histogram_tester.ExpectUniqueSample("DataUse.Sync.Upload.Count",
+                                        BOOKMARKS,1);
+    samples = histogram_tester.GetAllSamples("DataUse.Sync.Upload.Bytes");
+    EXPECT_TRUE(samples.size() == 1);
+    EXPECT_TRUE(samples.at(0).min == BOOKMARKS);
+    EXPECT_TRUE(samples.at(0).count >= 0);
+
+    samples = histogram_tester.GetAllSamples("DataUse.Sync.Download.Bytes");
+    EXPECT_TRUE(samples.size() == 1);
+    EXPECT_TRUE(samples.at(0).min == BOOKMARKS);
+    EXPECT_TRUE(samples.at(0).count == download_bytes_bookmark);
+
+    histogram_tester.ExpectTotalCount("DataUse.Sync.Download.Count", 1);
+
+    samples = histogram_tester.GetAllSamples(
+        "DataUse.Sync.ProgressMarker.Bytes");
+    for(std::vector<base::Bucket>::iterator it = samples.begin();
+        it != samples.end(); ++it) {
+      if (it->min == BOOKMARKS)
+        progress_bookmark.at(2) += it->count;
+      progress_all.at(2) += it->count;
+    }
+    EXPECT_TRUE(progress_bookmark.at(2) > progress_bookmark.at(1));
+    EXPECT_TRUE(progress_all.at(2) > progress_all.at(1));
+  }
+}
+
 // We use a macro so we can preserve the error location.
 #define VERIFY_ENTRY(id, is_unapplied, is_unsynced, prev_initialized, \
                      parent_id, version, server_version, id_fac, rtrans) \