chromium/src
0
Fork 0
Code Activity

Remove old sync datatype histograms

Browse Source 
This affects:
Sync.*AssociationTime
Sync.*ConfigureFailure

Both groups are rarely consumed these days and have competing metrics
that are more interesting (although not identical).

Bug: 995514
Change-Id: I44f7b1e18db9eebe1d8172a2b2f2ce2fa75e3ee2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1937370
Reviewed-by: Jesse Doherty <jwd@chromium.org>
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#720146}
This commit is contained in:
Mikel Astiz
2019-11-29 09:09:26 +00:00
committed by Commit Bot
parent 0bc256884f
commit 3c13d4966b
13 changed files with 6 additions and 240 deletions

1
components/sync/BUILD.gn

@ -560,7 +560,6 @@ source_set("unit_tests") {
"base/bind_to_task_runner_unittest.cc",
"base/cancelation_signal_unittest.cc",
"base/client_tag_hash_unittest.cc",
"base/data_type_histogram_unittest.cc",
"base/enum_set_unittest.cc",
"base/immutable_unittest.cc",
"base/model_type_unittest.cc",

143
components/sync/base/data_type_histogram.h

@ -21,147 +21,4 @@ void SyncRecordModelTypeMemoryHistogram(syncer::ModelType model_type,
void SyncRecordModelTypeCountHistogram(syncer::ModelType model_type,
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.
//
// Example usage (ignoring newlines necessary for multiline macro):
// std::vector<syncer::ModelType> types = GetEntryTypes();
// for (int i = 0; i < types.size(); ++i) {
// #define PER_DATA_TYPE_MACRO(type_str)
// UMA_HISTOGRAM_ENUMERATION("Sync." type_str "StartFailures",
// error, max_error);
// SYNC_DATA_TYPE_HISTOGRAM(types[i]);
// #undef PER_DATA_TYPE_MACRO
// }
//
// TODO(zea): Once visual studio supports proper variadic argument replacement
// in macros, pass in the histogram method directly as a parameter.
// See http://connect.microsoft.com/VisualStudio/feedback/details/380090/
// variadic-macro-replacement#details
// When adding a new datatype in the switch below, also update the SyncModelType
// and SyncModelTypeByMacro histogram suffixes in histograms.xml.
#define SYNC_DATA_TYPE_HISTOGRAM(datatype) \
do { \
switch (datatype) { \
case ::syncer::BOOKMARKS: \
PER_DATA_TYPE_MACRO("Bookmarks"); \
break; \
case ::syncer::PREFERENCES: \
PER_DATA_TYPE_MACRO("Preferences"); \
break; \
case ::syncer::PASSWORDS: \
PER_DATA_TYPE_MACRO("Passwords"); \
break; \
case ::syncer::AUTOFILL_PROFILE: \
PER_DATA_TYPE_MACRO("AutofillProfiles"); \
break; \
case ::syncer::AUTOFILL: \
PER_DATA_TYPE_MACRO("Autofill"); \
break; \
case ::syncer::AUTOFILL_WALLET_DATA: \
PER_DATA_TYPE_MACRO("AutofillWallet"); \
break; \
case ::syncer::AUTOFILL_WALLET_METADATA: \
PER_DATA_TYPE_MACRO("AutofillWalletMetadata"); \
break; \
case ::syncer::THEMES: \
PER_DATA_TYPE_MACRO("Themes"); \
break; \
case ::syncer::TYPED_URLS: \
PER_DATA_TYPE_MACRO("TypedUrls"); \
break; \
case ::syncer::EXTENSIONS: \
PER_DATA_TYPE_MACRO("Extensions"); \
break; \
case ::syncer::SEARCH_ENGINES: \
PER_DATA_TYPE_MACRO("SearchEngines"); \
break; \
case ::syncer::SESSIONS: \
PER_DATA_TYPE_MACRO("Sessions"); \
break; \
case ::syncer::APPS: \
PER_DATA_TYPE_MACRO("Apps"); \
break; \
case ::syncer::APP_SETTINGS: \
PER_DATA_TYPE_MACRO("AppSettings"); \
break; \
case ::syncer::EXTENSION_SETTINGS: \
PER_DATA_TYPE_MACRO("ExtensionSettings"); \
break; \
case ::syncer::HISTORY_DELETE_DIRECTIVES: \
PER_DATA_TYPE_MACRO("HistoryDeleteDirectives"); \
break; \
case ::syncer::DICTIONARY: \
PER_DATA_TYPE_MACRO("Dictionary"); \
break; \
case ::syncer::FAVICON_IMAGES: \
PER_DATA_TYPE_MACRO("FaviconImages"); \
break; \
case ::syncer::FAVICON_TRACKING: \
PER_DATA_TYPE_MACRO("FaviconTracking"); \
break; \
case ::syncer::DEVICE_INFO: \
PER_DATA_TYPE_MACRO("DeviceInfo"); \
break; \
case ::syncer::PRIORITY_PREFERENCES: \
PER_DATA_TYPE_MACRO("PriorityPreferences"); \
break; \
case ::syncer::SUPERVISED_USER_SETTINGS: \
PER_DATA_TYPE_MACRO("ManagedUserSetting"); \
break; \
case ::syncer::APP_LIST: \
PER_DATA_TYPE_MACRO("AppList"); \
break; \
case ::syncer::SUPERVISED_USER_WHITELISTS: \
PER_DATA_TYPE_MACRO("ManagedUserWhitelist"); \
break; \
case ::syncer::ARC_PACKAGE: \
PER_DATA_TYPE_MACRO("ArcPackage"); \
break; \
case ::syncer::PRINTERS: \
PER_DATA_TYPE_MACRO("Printers"); \
break; \
case ::syncer::READING_LIST: \
PER_DATA_TYPE_MACRO("ReadingList"); \
break; \
case ::syncer::USER_CONSENTS: \
PER_DATA_TYPE_MACRO("UserConsents"); \
break; \
case ::syncer::USER_EVENTS: \
PER_DATA_TYPE_MACRO("UserEvents"); \
break; \
case ::syncer::PROXY_TABS: \
PER_DATA_TYPE_MACRO("Tabs"); \
break; \
case ::syncer::NIGORI: \
PER_DATA_TYPE_MACRO("Nigori"); \
break; \
case ::syncer::DEPRECATED_EXPERIMENTS: \
PER_DATA_TYPE_MACRO("Experiments"); \
break; \
case ::syncer::SEND_TAB_TO_SELF: \
PER_DATA_TYPE_MACRO("SendTabToSelf"); \
break; \
case ::syncer::SECURITY_EVENTS: \
PER_DATA_TYPE_MACRO("SecurityEvents"); \
break; \
case ::syncer::WEB_APPS: \
PER_DATA_TYPE_MACRO("WebApps"); \
break; \
case ::syncer::WIFI_CONFIGURATIONS: \
PER_DATA_TYPE_MACRO("WifiConfigurations"); \
break; \
case ::syncer::OS_PREFERENCES: \
PER_DATA_TYPE_MACRO("OsPreferences"); \
break; \
case ::syncer::OS_PRIORITY_PREFERENCES: \
PER_DATA_TYPE_MACRO("OsPriorityPreferences"); \
break; \
default: \
NOTREACHED() << "Unknown datatype " \
<< ::syncer::ModelTypeToString(datatype); \
} \
} while (0)
#endif // COMPONENTS_SYNC_BASE_DATA_TYPE_HISTOGRAM_H_

45
components/sync/base/data_type_histogram_unittest.cc

@ -1,45 +0,0 @@
// Copyright (c) 2012 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 "components/sync/base/data_type_histogram.h"
#include "testing/gtest/include/gtest/gtest.h"
namespace syncer {
namespace {
class DataTypeHistogramTest : public testing::Test {};
// Create a histogram of type LOCAL_HISTOGRAM_COUNTS for each model type.
// Nothing should break.
TEST(DataTypeHistogramTest, BasicCount) {
for (int i = FIRST_REAL_MODEL_TYPE; i <= LAST_REAL_MODEL_TYPE; ++i) {
ModelType type = ModelTypeFromInt(i);
#define PER_DATA_TYPE_MACRO(type_str) \
LOCAL_HISTOGRAM_COUNTS("BasicCountPrefix" type_str "Suffix", 1);
SYNC_DATA_TYPE_HISTOGRAM(type);
#undef PER_DATA_TYPE_MACRO
}
}
// Create a histogram of type UMA_HISTOGRAM_ENUMERATION for each model type.
// Nothing should break.
TEST(DataTypeHistogramTest, BasicEnum) {
enum HistTypes {
TYPE_1,
TYPE_2,
TYPE_COUNT,
};
for (int i = FIRST_REAL_MODEL_TYPE; i <= LAST_REAL_MODEL_TYPE; ++i) {
ModelType type = ModelTypeFromInt(i);
#define PER_DATA_TYPE_MACRO(type_str) \
UMA_HISTOGRAM_ENUMERATION("BasicEnumPrefix" type_str "Suffix", \
(i % 2 ? TYPE_1 : TYPE_2), TYPE_COUNT);
SYNC_DATA_TYPE_HISTOGRAM(type);
#undef PER_DATA_TYPE_MACRO
}
}
} // namespace
} // namespace syncer

6
components/sync/driver/async_directory_type_controller.cc

@ -209,14 +209,8 @@ void AsyncDirectoryTypeController::StartDone(
void AsyncDirectoryTypeController::RecordStartFailure(ConfigureResult result) {
DCHECK(CalledOnValidThread());
// TODO(wychen): enum uma should be strongly typed. crbug.com/661401
UMA_HISTOGRAM_ENUMERATION("Sync.DataTypeStartFailures2",
ModelTypeHistogramValue(type()));
#define PER_DATA_TYPE_MACRO(type_str) \
UMA_HISTOGRAM_ENUMERATION("Sync." type_str "ConfigureFailure", result, \
MAX_CONFIGURE_RESULT);
SYNC_DATA_TYPE_HISTOGRAM(type());
#undef PER_DATA_TYPE_MACRO
}
void AsyncDirectoryTypeController::DisableImpl(const SyncError& error) {

4
components/sync/driver/async_directory_type_controller_unittest.cc

@ -76,7 +76,6 @@ class SharedChangeProcessorMock : public SharedChangeProcessor {
MOCK_METHOD1(SyncModelHasUserCreatedNodes, bool(bool*));
MOCK_METHOD0(CryptoReadyIfNecessary, bool());
MOCK_CONST_METHOD1(GetDataTypeContext, bool(std::string*));
MOCK_METHOD1(RecordAssociationTime, void(base::TimeDelta time));
void SetConnectReturn(base::WeakPtr<SyncableService> service) {
connect_return_ = service;
@ -221,7 +220,6 @@ class SyncAsyncDirectoryTypeControllerTest : public testing::Test {
EXPECT_CALL(*change_processor_, GetAllSyncDataReturnError(_, _))
.WillOnce(Return(SyncError()));
EXPECT_CALL(*change_processor_, GetSyncCount()).WillOnce(Return(0));
EXPECT_CALL(*change_processor_, RecordAssociationTime(_));
}
void SetActivateExpectations(DataTypeController::ConfigureResult result) {
@ -284,7 +282,6 @@ TEST_F(SyncAsyncDirectoryTypeControllerTest, StartFirstRun) {
.WillOnce(DoAll(SetArgPointee<0>(false), Return(true)));
EXPECT_CALL(*change_processor_, GetAllSyncDataReturnError(_, _))
.WillOnce(Return(SyncError()));
EXPECT_CALL(*change_processor_, RecordAssociationTime(_));
SetActivateExpectations(DataTypeController::OK_FIRST_RUN);
EXPECT_EQ(DataTypeController::NOT_RUNNING, non_ui_dtc_->state());
Start();
@ -319,7 +316,6 @@ TEST_F(SyncAsyncDirectoryTypeControllerTest, StartAssociationFailed) {
.WillOnce(DoAll(SetArgPointee<0>(true), Return(true)));
EXPECT_CALL(*change_processor_, GetAllSyncDataReturnError(_, _))
.WillOnce(Return(SyncError()));
EXPECT_CALL(*change_processor_, RecordAssociationTime(_));
SetStartFailExpectations(DataTypeController::ASSOCIATION_FAILED);
// Set up association to fail with an association failed error.
EXPECT_EQ(DataTypeController::NOT_RUNNING, non_ui_dtc_->state());

1
components/sync/driver/data_type_controller.cc

@ -4,7 +4,6 @@
#include "components/sync/driver/data_type_controller.h"
#include "components/sync/base/data_type_histogram.h"
#include "components/sync/syncable/user_share.h"
namespace syncer {

11
components/sync/driver/model_type_controller.cc

@ -250,17 +250,6 @@ void ModelTypeController::ReportModelError(SyncError::ErrorType error_type,
const ModelError& error) {
DCHECK(CalledOnValidThread());
// TODO(crbug.com/890729): This is obviously misplaced/misnamed as we report
// run-time failures as well. Rename the histogram to ConfigureResult and
// report it only after startup (also for success).
if (state_ != NOT_RUNNING) {
#define PER_DATA_TYPE_MACRO(type_str) \
UMA_HISTOGRAM_ENUMERATION("Sync." type_str "ConfigureFailure", \
UNRECOVERABLE_ERROR, MAX_CONFIGURE_RESULT);
SYNC_DATA_TYPE_HISTOGRAM(type());
#undef PER_DATA_TYPE_MACRO
}
switch (state_) {
case MODEL_LOADED:
// Fall through. Before state_ is flipped to RUNNING, we treat it as a

10
components/sync/driver/shared_change_processor.cc

@ -7,7 +7,6 @@
#include <utility>
#include "base/threading/sequenced_task_runner_handle.h"
#include "components/sync/base/data_type_histogram.h"
#include "components/sync/driver/generic_change_processor.h"
#include "components/sync/driver/generic_change_processor_factory.h"
#include "components/sync/driver/shared_change_processor_ref.h"
@ -98,7 +97,6 @@ void SharedChangeProcessor::StartAssociation(
{
SyncDataList initial_sync_data;
base::TimeTicks start_time = base::TimeTicks::Now();
SyncError error = GetAllSyncDataReturnError(type_, &initial_sync_data);
if (error.IsSet()) {
local_merge_result.set_error(error);
@ -114,7 +112,6 @@ void SharedChangeProcessor::StartAssociation(
type_, initial_sync_data, std::unique_ptr<SyncChangeProcessor>(
new SharedChangeProcessorRef(this)),
std::unique_ptr<SyncErrorFactory>(new SharedChangeProcessorRef(this)));
RecordAssociationTime(base::TimeTicks::Now() - start_time);
if (local_merge_result.error().IsSet()) {
start_done.Run(DataTypeController::ASSOCIATION_FAILED, local_merge_result,
syncer_merge_result);
@ -297,13 +294,6 @@ SyncError SharedChangeProcessor::CreateAndUploadError(
}
}
void SharedChangeProcessor::RecordAssociationTime(base::TimeDelta time) {
#define PER_DATA_TYPE_MACRO(type_str) \
UMA_HISTOGRAM_TIMES("Sync." type_str "AssociationTime", time);
SYNC_DATA_TYPE_HISTOGRAM(type_);
#undef PER_DATA_TYPE_MACRO
}
void SharedChangeProcessor::StopLocalService() {
if (local_service_)
local_service_->StopSyncing(type_);

3
components/sync/driver/shared_change_processor.h

@ -123,9 +123,6 @@ class SharedChangeProcessor
virtual ~SharedChangeProcessor();
private:
// Record association time.
virtual void RecordAssociationTime(base::TimeDelta time);
// Monitor lock for this object. All methods that interact with the change
// processor must aquire this lock and check whether we're disconnected or
// not. Once disconnected, all attempted changes to or loads from the change

14
components/sync/model_impl/syncable_service_based_bridge.cc

@ -12,7 +12,6 @@
#include "base/location.h"
#include "base/trace_event/memory_usage_estimator.h"
#include "components/sync/base/client_tag_hash.h"
#include "components/sync/base/data_type_histogram.h"
#include "components/sync/model/mutable_data_batch.h"
#include "components/sync/model/sync_change.h"
#include "components/sync/model/sync_error_factory.h"
@ -533,8 +532,6 @@ base::Optional<ModelError> SyncableServiceBasedBridge::StartSyncableService() {
DCHECK(!syncable_service_started_);
DCHECK(change_processor()->IsTrackingMetadata());
const base::TimeTicks start_time = base::TimeTicks::Now();
// Sync enabled, so exercise MergeDataAndStartSyncing() immediately, since
// this function is reached only if sync is starting already.
SyncDataList initial_sync_data;
@ -560,8 +557,6 @@ base::Optional<ModelError> SyncableServiceBasedBridge::StartSyncableService() {
std::make_unique<SyncErrorFactoryImpl>(type_))
.error());
RecordAssociationTime(base::TimeTicks::Now() - start_time);
if (!merge_error) {
syncable_service_started_ = true;
}
@ -680,13 +675,4 @@ void SyncableServiceBasedBridge::ReportErrorIfSet(
}
}
void SyncableServiceBasedBridge::RecordAssociationTime(
base::TimeDelta time) const {
// This mimics the implementation in SharedChangeProcessor.
#define PER_DATA_TYPE_MACRO(type_str) \
UMA_HISTOGRAM_TIMES("Sync." type_str "AssociationTime", time);
SYNC_DATA_TYPE_HISTOGRAM(type_);
#undef PER_DATA_TYPE_MACRO
}
} // namespace syncer

1
components/sync/model_impl/syncable_service_based_bridge.h

@ -96,7 +96,6 @@ class SyncableServiceBasedBridge : public ModelTypeSyncBridge {
const base::Optional<ModelError>& error,
std::unique_ptr<ModelTypeStore::RecordList> record_list);
void ReportErrorIfSet(const base::Optional<ModelError>& error);
void RecordAssociationTime(base::TimeDelta time) const;
const ModelType type_;
SyncableService* const syncable_service_;

1
docs/sync/model_api.md

@ -278,7 +278,6 @@ the next client restart.
[GetUserSelectableTypeInfo].
* Add to the `SyncModelTypes` enum in [`enums.xml`][enums] and to the
`SyncModelType` suffix in [`histograms.xml`][histograms].
* Add to the [`SYNC_DATA_TYPE_HISTOGRAM`][DataTypeHistogram] macro.
[protocol]: https://cs.chromium.org/chromium/src/components/sync/protocol/
[ModelType]: https://cs.chromium.org/chromium/src/components/sync/base/model_type.h

6
tools/metrics/histograms/histograms.xml

@ -148198,6 +148198,9 @@ should be kept until we use this API. -->
<histogram base="true" name="Sync.AssociationTime" units="ms"
expires_after="2019-10-01">
<obsolete>
Deprecated in M80.
</obsolete>
<owner>jkrcal@chromium.org</owner>
<owner>mastiz@chromium.org</owner>
<summary>Time taken during association of this data type.</summary>
@ -148652,6 +148655,9 @@ should be kept until we use this API. -->
<histogram base="true" name="Sync.ConfigureFailure" enum="SyncConfigureResult"
expires_after="2020-04-01">
<obsolete>
Deprecated in M80.
</obsolete>
<owner>jkrcal@chromium.org</owner>
<owner>mastiz@chromium.org</owner>
<summary>