diff options
-rw-r--r-- | sync/BUILD.gn | 1 | ||||
-rw-r--r-- | sync/engine/commit.cc | 18 | ||||
-rw-r--r-- | sync/engine/directory_update_handler.cc | 6 | ||||
-rw-r--r-- | sync/engine/process_updates_util.cc | 9 | ||||
-rw-r--r-- | sync/engine/syncer_unittest.cc | 110 | ||||
-rw-r--r-- | sync/sync.gyp | 1 | ||||
-rw-r--r-- | sync/util/data_type_histogram.cc | 13 | ||||
-rw-r--r-- | sync/util/data_type_histogram.h | 10 | ||||
-rw-r--r-- | tools/metrics/histograms/histograms.xml | 48 |
9 files changed, 211 insertions, 5 deletions
diff --git a/sync/BUILD.gn b/sync/BUILD.gn index bf9e471..22845d7 100644 --- a/sync/BUILD.gn +++ b/sync/BUILD.gn @@ -384,6 +384,7 @@ source_set("sync_core") { "syncable/write_transaction_info.h", "util/cryptographer.cc", "util/cryptographer.h", + "util/data_type_histogram.cc", "util/data_type_histogram.h", "util/encryptor.h", "util/extensions_activity.cc", diff --git a/sync/engine/commit.cc b/sync/engine/commit.cc index f607be4..743996d 100644 --- a/sync/engine/commit.cc +++ b/sync/engine/commit.cc @@ -4,6 +4,7 @@ #include "sync/engine/commit.h" +#include "base/metrics/sparse_histogram.h" #include "base/trace_event/trace_event.h" #include "sync/engine/commit_contribution.h" #include "sync/engine/commit_processor.h" @@ -13,6 +14,7 @@ #include "sync/internal_api/public/events/commit_request_event.h" #include "sync/internal_api/public/events/commit_response_event.h" #include "sync/sessions/sync_session.h" +#include "sync/util/data_type_histogram.h" namespace syncer { @@ -70,11 +72,19 @@ Commit* Commit::Init( enabled_types, commit_message); + int previous_message_size = message.ByteSize(); // Finally, serialize all our contributions. - for (std::map<ModelType, CommitContribution*>::const_iterator it = - contributions.begin(); - it != contributions.end(); ++it) { - it->second->AddToCommitMessage(&message); + for (const auto& contribution : contributions) { + contribution.second->AddToCommitMessage(&message); + int current_entry_size = message.ByteSize() - previous_message_size; + previous_message_size = message.ByteSize(); + int local_integer_model_type = ModelTypeToHistogramInt(contribution.first); + if (current_entry_size > 0) { + SyncRecordDatatypeBin("DataUse.Sync.Upload.Bytes", + local_integer_model_type, current_entry_size); + } + UMA_HISTOGRAM_SPARSE_SLOWLY("DataUse.Sync.Upload.Count", + local_integer_model_type); } // If we made it this far, then we've successfully prepared a commit message. diff --git a/sync/engine/directory_update_handler.cc b/sync/engine/directory_update_handler.cc index adcece7..5b2bac8 100644 --- a/sync/engine/directory_update_handler.cc +++ b/sync/engine/directory_update_handler.cc @@ -12,6 +12,7 @@ #include "sync/syncable/model_neutral_mutable_entry.h" #include "sync/syncable/syncable_model_neutral_write_transaction.h" #include "sync/syncable/syncable_write_transaction.h" +#include "sync/util/data_type_histogram.h" namespace syncer { @@ -46,6 +47,11 @@ SyncerError DirectoryUpdateHandler::ProcessGetUpdatesResponse( const SyncEntityList& applicable_updates, sessions::StatusController* status) { syncable::ModelNeutralWriteTransaction trans(FROM_HERE, SYNCER, dir_); + if (progress_marker.ByteSize() > 0) { + SyncRecordDatatypeBin("DataUse.Sync.ProgressMarker.Bytes", + ModelTypeToHistogramInt(type_), + progress_marker.ByteSize()); + } if (mutated_context.has_context()) { sync_pb::DataTypeContext local_context; dir_->GetDataTypeContext(&trans, type_, &local_context); diff --git a/sync/engine/process_updates_util.cc b/sync/engine/process_updates_util.cc index 9b428ef..e1931d7 100644 --- a/sync/engine/process_updates_util.cc +++ b/sync/engine/process_updates_util.cc @@ -5,6 +5,7 @@ #include "sync/engine/process_updates_util.h" #include "base/location.h" +#include "base/metrics/sparse_histogram.h" #include "sync/engine/syncer_proto_util.h" #include "sync/engine/syncer_types.h" #include "sync/engine/syncer_util.h" @@ -15,6 +16,7 @@ #include "sync/syncable/syncable_proto_util.h" #include "sync/syncable/syncable_util.h" #include "sync/util/cryptographer.h" +#include "sync/util/data_type_histogram.h" namespace syncer { @@ -303,6 +305,13 @@ void ProcessDownloadedUpdates( if (verify_result != VERIFY_SUCCESS && verify_result != VERIFY_UNDELETE) continue; ProcessUpdate(**update_it, dir->GetCryptographer(trans), trans); + if ((*update_it)->ByteSize() > 0) { + SyncRecordDatatypeBin("DataUse.Sync.Download.Bytes", + ModelTypeToHistogramInt(type), + (*update_it)->ByteSize()); + } + UMA_HISTOGRAM_SPARSE_SLOWLY("DataUse.Sync.Download.Count", + ModelTypeToHistogramInt(type)); } } diff --git a/sync/engine/syncer_unittest.cc b/sync/engine/syncer_unittest.cc index dcd326e..c630fe4 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,115 @@ 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<unsigned int> progress_bookmark(3, 0); + vector<unsigned int> progress_all(3, 0); + 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_EQ(samples.size(), 1u); + EXPECT_EQ(samples.at(0).min, BOOKMARKS); + EXPECT_GE(samples.at(0).count, 0); + download_bytes_bookmark = samples.at(0).count; + + samples = + histogram_tester.GetAllSamples("DataUse.Sync.ProgressMarker.Bytes"); + + for (const base::Bucket& bucket : samples) { + if (bucket.min == BOOKMARKS) + progress_bookmark.at(0) += bucket.count; + progress_all.at(0) += bucket.count; + } + EXPECT_GT(progress_bookmark.at(0), 0u); + EXPECT_GT(progress_all.at(0), 0u); + + 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_EQ(samples.size(), 1u); + EXPECT_EQ(samples.at(0).min, BOOKMARKS); + EXPECT_EQ(samples.at(0).count, download_bytes_bookmark); + + samples = + histogram_tester.GetAllSamples("DataUse.Sync.ProgressMarker.Bytes"); + for (const base::Bucket& bucket : samples) { + if (bucket.min == BOOKMARKS) + progress_bookmark.at(1) += bucket.count; + progress_all.at(1) += bucket.count; + } + EXPECT_EQ(progress_bookmark.at(1), progress_bookmark.at(0)); + EXPECT_GT(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_EQ(samples.size(), 1u); + EXPECT_EQ(samples.at(0).min, BOOKMARKS); + EXPECT_GE(samples.at(0).count, 0); + + samples = histogram_tester.GetAllSamples("DataUse.Sync.Download.Bytes"); + EXPECT_EQ(samples.size(), 1u); + EXPECT_EQ(samples.at(0).min, BOOKMARKS); + EXPECT_EQ(samples.at(0).count, download_bytes_bookmark); + + histogram_tester.ExpectTotalCount("DataUse.Sync.Download.Count", 1); + + samples = + histogram_tester.GetAllSamples("DataUse.Sync.ProgressMarker.Bytes"); + for (const base::Bucket& bucket : samples) { + if (bucket.min == BOOKMARKS) + progress_bookmark.at(2) += bucket.count; + progress_all.at(2) += bucket.count; + } + EXPECT_GT(progress_bookmark.at(2), progress_bookmark.at(1)); + EXPECT_GT(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) \ diff --git a/sync/sync.gyp b/sync/sync.gyp index 2ec6f35..a029b68 100644 --- a/sync/sync.gyp +++ b/sync/sync.gyp @@ -437,6 +437,7 @@ 'syncable/write_transaction_info.h', 'util/cryptographer.cc', 'util/cryptographer.h', + 'util/data_type_histogram.cc', 'util/data_type_histogram.h', 'util/encryptor.h', 'util/extensions_activity.cc', diff --git a/sync/util/data_type_histogram.cc b/sync/util/data_type_histogram.cc new file mode 100644 index 0000000..8c292b0 --- /dev/null +++ b/sync/util/data_type_histogram.cc @@ -0,0 +1,13 @@ +// Copyright 2015 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#include "sync/util/data_type_histogram.h" + +#include "base/metrics/sparse_histogram.h" + +void SyncRecordDatatypeBin(const std::string& name, int sample, int value) { + base::HistogramBase* histogram = base::SparseHistogram::FactoryGet( + name, base::HistogramBase::kUmaTargetedHistogramFlag); + histogram->AddCount(sample, value); +} diff --git a/sync/util/data_type_histogram.h b/sync/util/data_type_histogram.h index 0f7e6a9..20a6f8a 100644 --- a/sync/util/data_type_histogram.h +++ b/sync/util/data_type_histogram.h @@ -5,10 +5,18 @@ #ifndef SYNC_UTIL_DATA_TYPE_HISTOGRAM_H_ #define SYNC_UTIL_DATA_TYPE_HISTOGRAM_H_ -#include "base/metrics/histogram.h" +#include <string> + +#include "base/metrics/histogram_macros.h" #include "base/time/time.h" #include "sync/internal_api/public/base/model_type.h" +// This function adds |value| to |sample| bucket of histogram |name|. |value| +// should be greater or equal to 1 and |name| can be variable. DataTypes are +// mapped to proper |sample| bucket by using ModelTypeToHistogramInt() function. +// So different DataTypes play the role of different buckets in this histogram. +void SyncRecordDatatypeBin(const std::string& name, int sample, int value); + // For now, this just implements UMA_HISTOGRAM_LONG_TIMES. This can be adjusted // if we feel the min, max, or bucket count amount are not appropriate. #define SYNC_FREQ_HISTOGRAM(name, time) UMA_HISTOGRAM_CUSTOM_TIMES( \ diff --git a/tools/metrics/histograms/histograms.xml b/tools/metrics/histograms/histograms.xml index 8672fa5..2296111 100644 --- a/tools/metrics/histograms/histograms.xml +++ b/tools/metrics/histograms/histograms.xml @@ -5565,6 +5565,54 @@ http://cs/file:chrome/histograms.xml - but prefer this file for new entries. </summary> </histogram> +<histogram name="DataUse.Sync.Download.Bytes" enum="SyncModelTypes"> + <owner>amohammadkhan@chromium.org</owner> + <owner>bengr@chromium.org</owner> + <summary> + Number of downloaded bytes of different data types in Sync service for + received updates. It is updated when an update message is received from sync + server. + </summary> +</histogram> + +<histogram name="DataUse.Sync.Download.Count" enum="SyncModelTypes"> + <owner>amohammadkhan@chromium.org</owner> + <owner>bengr@chromium.org</owner> + <summary> + Number of downloaded entities of different data types in Sync service for + received updates. It is updated when an update message is received from sync + server. + </summary> +</histogram> + +<histogram name="DataUse.Sync.ProgressMarker.Bytes" enum="SyncModelTypes"> + <owner>amohammadkhan@chromium.org</owner> + <owner>bengr@chromium.org</owner> + <summary> + Number of downloaded bytes of ProgressMarker of different data types in Sync + service for received updates. It is updated when an update message is + received from sync server. + </summary> +</histogram> + +<histogram name="DataUse.Sync.Upload.Bytes" enum="SyncModelTypes"> + <owner>amohammadkhan@chromium.org</owner> + <owner>bengr@chromium.org</owner> + <summary> + Number of uploaded bytes of different data types in Sync service for sent + commits. Updated when a commit message is sent to sync server. + </summary> +</histogram> + +<histogram name="DataUse.Sync.Upload.Count" enum="SyncModelTypes"> + <owner>amohammadkhan@chromium.org</owner> + <owner>bengr@chromium.org</owner> + <summary> + Number of uploaded entities of different data types in Sync service for sent + commits. Updated when a commit message is sent to sync server. + </summary> +</histogram> + <histogram name="DefaultBrowser.InteractionResult" enum="MakeChromeDefaultResult"> <owner>gab@chromium.org</owner> |