chromium/src
0
Fork 0
Code Activity

sync: Define histogram ints for model types

Browse Source 
The common practice of using enums directly when counting histograms
works only if the enums' values never change.  Unfortunately, we insert
into the middle of the list of ModelTypes all the time.

Rather than change the way we manage the list of model type enums, we
decided to separate the model types values from those used when
reporting histograms.  This change defines a mapping from model types to
integers.  These integers should be kept in sync with integer to label
mapping defined in histograms.xml.

This mapping is often similar to, but does not necessarily match, the
int values of the enums.

BUG=190015


Review URL: https://chromiumcodereview.appspot.com/12820010

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@188314 0039d316-1c4b-4281-b951-d872f2087c98
This commit is contained in:
rlarocque@chromium.org
2013-03-15 09:13:55 +00:00
parent 1c12a3440e
commit fccfe67dfb
10 changed files with 103 additions and 13 deletions

3
chrome/browser/sync/glue/bookmark_model_associator.cc

@ -706,7 +706,8 @@ void BookmarkModelAssociator::CheckModelSyncState() const {
if (base::StringToInt64(version_str, &native_version) &&
native_version != trans.GetModelVersion(syncer::BOOKMARKS)) {
UMA_HISTOGRAM_ENUMERATION("Sync.LocalModelOutOfSync",
syncer::BOOKMARKS, syncer::MODEL_TYPE_COUNT);
ModelTypeToHistogramInt(syncer::BOOKMARKS),
syncer::MODEL_TYPE_COUNT);
// Clear version on bookmark model so that we only report error once.
bookmark_model_->DeleteNodeMetaInfo(bookmark_model_->root_node(),
kBookmarkTransactionVersionKey);

4
chrome/browser/sync/glue/data_type_controller.cc

@ -5,6 +5,7 @@
#include "chrome/browser/sync/glue/chrome_report_unrecoverable_error.h"
#include "chrome/browser/sync/glue/data_type_controller.h"
#include "sync/internal_api/public/base/model_type.h"
#include "sync/util/data_type_histogram.h"
namespace browser_sync {
@ -32,7 +33,8 @@ void DataTypeController::RecordUnrecoverableError(
<< ModelTypeToString(type()) << " "
<< message << " at location "
<< from_here.ToString();
UMA_HISTOGRAM_ENUMERATION("Sync.DataTypeRunFailures", type(),
UMA_HISTOGRAM_ENUMERATION("Sync.DataTypeRunFailures",
ModelTypeToHistogramInt(type()),
syncer::MODEL_TYPE_COUNT);
// TODO(sync): remove this once search engines triggers less errors, such as

3
chrome/browser/sync/glue/frontend_data_type_controller.cc

@ -264,7 +264,8 @@ void FrontendDataTypeController::RecordAssociationTime(base::TimeDelta time) {
void FrontendDataTypeController::RecordStartFailure(StartResult result) {
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
UMA_HISTOGRAM_ENUMERATION("Sync.DataTypeStartFailures", type(),
UMA_HISTOGRAM_ENUMERATION("Sync.DataTypeStartFailures",
ModelTypeToHistogramInt(type()),
syncer::MODEL_TYPE_COUNT);
#define PER_DATA_TYPE_MACRO(type_str) \
UMA_HISTOGRAM_ENUMERATION("Sync." type_str "StartFailure", result, \

4
chrome/browser/sync/glue/model_association_manager.cc

@ -11,7 +11,7 @@
#include "base/logging.h"
#include "base/message_loop.h"
#include "base/metrics/histogram.h"
#include "sync/internal_api/public/base/model_type.h"
using content::BrowserThread;
using syncer::ModelTypeSet;
@ -300,7 +300,7 @@ void ModelAssociationManager::AppendToFailedDatatypesAndLogError(
LOG(ERROR) << "Failed to associate models for "
<< syncer::ModelTypeToString(error.type());
UMA_HISTOGRAM_ENUMERATION("Sync.ConfigureFailed",
error.type(),
ModelTypeToHistogramInt(error.type()),
syncer::MODEL_TYPE_COUNT);
}

3
chrome/browser/sync/glue/non_frontend_data_type_controller.cc

@ -333,7 +333,8 @@ void NonFrontendDataTypeController::RecordAssociationTime(
void NonFrontendDataTypeController::RecordStartFailure(StartResult result) {
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
UMA_HISTOGRAM_ENUMERATION("Sync.DataTypeStartFailures", type(),
UMA_HISTOGRAM_ENUMERATION("Sync.DataTypeStartFailures",
ModelTypeToHistogramInt(type()),
syncer::MODEL_TYPE_COUNT);
#define PER_DATA_TYPE_MACRO(type_str) \
UMA_HISTOGRAM_ENUMERATION("Sync." type_str "StartFailure", result, \

3
chrome/browser/sync/glue/non_ui_data_type_controller.cc

@ -258,7 +258,8 @@ void NonUIDataTypeController::RecordAssociationTime(base::TimeDelta time) {
void NonUIDataTypeController::RecordStartFailure(StartResult result) {
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
UMA_HISTOGRAM_ENUMERATION("Sync.DataTypeStartFailures", type(),
UMA_HISTOGRAM_ENUMERATION("Sync.DataTypeStartFailures",
ModelTypeToHistogramInt(type()),
syncer::MODEL_TYPE_COUNT);
#define PER_DATA_TYPE_MACRO(type_str) \
UMA_HISTOGRAM_ENUMERATION("Sync." type_str "StartFailure", result, \

3
chrome/browser/sync/glue/ui_data_type_controller.cc

@ -316,7 +316,8 @@ void UIDataTypeController::RecordAssociationTime(base::TimeDelta time) {
void UIDataTypeController::RecordStartFailure(StartResult result) {
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
UMA_HISTOGRAM_ENUMERATION("Sync.DataTypeStartFailures", type(),
UMA_HISTOGRAM_ENUMERATION("Sync.DataTypeStartFailures",
ModelTypeToHistogramInt(type()),
syncer::MODEL_TYPE_COUNT);
#define PER_DATA_TYPE_MACRO(type_str) \
UMA_HISTOGRAM_ENUMERATION("Sync." type_str "StartFailure", result, \

10
sync/internal_api/public/base/model_type.h

@ -49,11 +49,6 @@ enum ModelType {
// can be represented in the protocol using a specific Message type in the
// EntitySpecifics protocol buffer.
//
// WARNING: Modifying the order of these types or inserting a new type above
// these will affect numerous histograms that rely on the enum values being
// consistent. When adding a new type, add it to the end of the user model
// types section, but before the proxy types.
//
// A bookmark folder or a bookmark URL object.
BOOKMARKS,
FIRST_USER_MODEL_TYPE = BOOKMARKS, // Declared 2nd, for debugger prettiness.
@ -244,6 +239,11 @@ FullModelTypeSet ToFullModelTypeSet(ModelTypeSet in);
// the name of |model_type|.
SYNC_EXPORT const char* ModelTypeToString(ModelType model_type);
// Some histograms take an integer parameter that represents a model type.
// The mapping from ModelType to integer is defined here. It should match
// the mapping from integer to labels defined in histograms.xml.
SYNC_EXPORT int ModelTypeToHistogramInt(ModelType model_type);
// Handles all model types, and not just real ones.
//
// Caller takes ownership of returned value.

65
sync/syncable/model_type.cc

@ -449,6 +449,71 @@ const char* ModelTypeToString(ModelType model_type) {
return "INVALID";
}
// The normal rules about histograms apply here. Always append to the bottom of
// the list, and be careful to not reuse integer values that have already been
// assigned. Don't forget to update histograms.xml when you make changes to
// this list.
int ModelTypeToHistogramInt(ModelType model_type) {
switch (model_type) {
case UNSPECIFIED:
return 0;
case TOP_LEVEL_FOLDER:
return 1;
case BOOKMARKS:
return 2;
case PREFERENCES:
return 3;
case PASSWORDS:
return 4;
case AUTOFILL_PROFILE:
return 5;
case AUTOFILL:
return 6;
case THEMES:
return 7;
case TYPED_URLS:
return 8;
case EXTENSIONS:
return 9;
case SEARCH_ENGINES:
return 10;
case SESSIONS:
return 11;
case APPS:
return 12;
case APP_SETTINGS:
return 13;
case EXTENSION_SETTINGS:
return 14;
case APP_NOTIFICATIONS:
return 15;
case HISTORY_DELETE_DIRECTIVES:
return 16;
case NIGORI:
return 17;
case DEVICE_INFO:
return 18;
case EXPERIMENTS:
return 19;
case SYNCED_NOTIFICATIONS:
return 20;
case PRIORITY_PREFERENCES:
return 21;
case DICTIONARY:
return 22;
case FAVICON_IMAGES:
return 23;
case FAVICON_TRACKING:
return 24;
case PROXY_TABS:
return 25;
// Silence a compiler warning.
case MODEL_TYPE_COUNT:
return 0;
}
return 0;
}
base::StringValue* ModelTypeToValue(ModelType model_type) {
if (model_type >= FIRST_REAL_MODEL_TYPE) {
return new base::StringValue(ModelTypeToString(model_type));

18
sync/syncable/model_type_unittest.cc

@ -86,5 +86,23 @@ TEST_F(ModelTypeTest, ModelTypeOfInvalidSpecificsFieldNumber) {
EXPECT_EQ(UNSPECIFIED, GetModelTypeFromSpecificsFieldNumber(0));
}
TEST_F(ModelTypeTest, ModelTypeHistogramMapping) {
std::set<int> histogram_values;
ModelTypeSet all_types = ModelTypeSet::All();
for (ModelTypeSet::Iterator it = all_types.First(); it.Good(); it.Inc()) {
SCOPED_TRACE(ModelTypeToString(it.Get()));
int histogram_value = ModelTypeToHistogramInt(it.Get());
EXPECT_TRUE(histogram_values.insert(histogram_value).second)
<< "Expected histogram values to be unique";
// This is not necessary for the mapping to be valid, but most instances of
// UMA_HISTOGRAM that use this mapping specify MODEL_TYPE_COUNT as the
// maximum possible value. If you break this assumption, you should update
// those histograms.
EXPECT_LT(histogram_value, MODEL_TYPE_COUNT);
}
}
} // namespace
} // namespace syncer