Extensions: Logging value and db repair to UMA in different values.

The extensions database repair UMA values added in r358722 combine
success/failures when deleting an invalid value with deleting/repairing
the entire database. This made it impossible to determine the percentage
of corrupted db's that can be repaired, vs. those that needed to be
deleted. This CL splits the stats into separate value and database
entries.

Review URL: https://codereview.chromium.org/1563573003

Cr-Commit-Position: refs/heads/master@{#368158}

4 files changed, 61 insertions, 35 deletions

diff --git a/extensions/browser/value_store/leveldb_value_store.cc b/extensions/browser/value_store/leveldb_value_store.cc
index 9849842..db3e30a 100644
--- a/extensions/browser/value_store/leveldb_value_store.cc
+++ b/extensions/browser/value_store/leveldb_value_store.cc
@@ -36,11 +36,20 @@ const char kRestoredDuringOpen[] = "Database corruption repaired during open";
 // UMA values used when recovering from a corrupted leveldb.
 // Do not change/delete these values as you will break reporting for older
 // copies of Chrome. Only add new values to the end.
-enum LevelDBCorruptionRecoveryValue {
-  LEVELDB_RESTORE_DELETE_SUCCESS = 0,
-  LEVELDB_RESTORE_DELETE_FAILURE,
-  LEVELDB_RESTORE_REPAIR_SUCCESS,
-  LEVELDB_RESTORE_MAX
+enum LevelDBDatabaseCorruptionRecoveryValue {
+  LEVELDB_DB_RESTORE_DELETE_SUCCESS = 0,
+  LEVELDB_DB_RESTORE_DELETE_FAILURE,
+  LEVELDB_DB_RESTORE_REPAIR_SUCCESS,
+  LEVELDB_DB_RESTORE_MAX
+};
+
+// UMA values used when recovering from a corrupted leveldb.
+// Do not change/delete these values as you will break reporting for older
+// copies of Chrome. Only add new values to the end.
+enum LevelDBValueCorruptionRecoveryValue {
+  LEVELDB_VALUE_RESTORE_DELETE_SUCCESS,
+  LEVELDB_VALUE_RESTORE_DELETE_FAILURE,
+  LEVELDB_VALUE_RESTORE_MAX
 };
 
 // Scoped leveldb snapshot which releases the snapshot on destruction.
@@ -80,7 +89,8 @@ LeveldbValueStore::LeveldbValueStore(const std::string& uma_client_name,
     : db_path_(db_path),
       db_unrecoverable_(false),
       open_histogram_(nullptr),
-      restore_histogram_(nullptr) {
+      db_restore_histogram_(nullptr),
+      value_restore_histogram_(nullptr) {
   DCHECK_CURRENTLY_ON(BrowserThread::FILE);
 
   open_options_.max_open_files = 0;  // Use minimum.
@@ -96,9 +106,14 @@ LeveldbValueStore::LeveldbValueStore(const std::string& uma_client_name,
       "Extensions.Database.Open." + uma_client_name, 1,
       leveldb_env::LEVELDB_STATUS_MAX, leveldb_env::LEVELDB_STATUS_MAX + 1,
       base::Histogram::kUmaTargetedHistogramFlag);
-  restore_histogram_ = base::LinearHistogram::FactoryGet(
-      "Extensions.Database.Restore." + uma_client_name, 1, LEVELDB_RESTORE_MAX,
-      LEVELDB_RESTORE_MAX + 1, base::Histogram::kUmaTargetedHistogramFlag);
+  db_restore_histogram_ = base::LinearHistogram::FactoryGet(
+      "Extensions.Database.Database.Restore." + uma_client_name, 1,
+      LEVELDB_DB_RESTORE_MAX, LEVELDB_DB_RESTORE_MAX + 1,
+      base::Histogram::kUmaTargetedHistogramFlag);
+  value_restore_histogram_ = base::LinearHistogram::FactoryGet(
+      "Extensions.Database.Value.Restore." + uma_client_name, 1,
+      LEVELDB_VALUE_RESTORE_MAX, LEVELDB_VALUE_RESTORE_MAX + 1,
+      base::Histogram::kUmaTargetedHistogramFlag);
   base::trace_event::MemoryDumpManager::GetInstance()->RegisterDumpProvider(
       this, "LeveldbValueStore", base::ThreadTaskRunnerHandle::Get());
 }
@@ -196,10 +211,10 @@ ValueStore::ReadResult LeveldbValueStore::Get() {
     scoped_ptr<base::Value> value =
         json_reader.ReadToValue(it->value().ToString());
     if (!value) {
-      return MakeReadResult(Status(
-          CORRUPTION,
-          Delete(key).ok() ? RESTORE_REPAIR_SUCCESS : RESTORE_DELETE_FAILURE,
-          kInvalidJson));
+      return MakeReadResult(
+          Status(CORRUPTION, Delete(key).ok() ? VALUE_RESTORE_DELETE_SUCCESS
+                                              : VALUE_RESTORE_DELETE_FAILURE,
+                 kInvalidJson));
     }
     settings->SetWithoutPathExpansion(key, std::move(value));
   }
@@ -360,14 +375,20 @@ ValueStore::BackingStoreRestoreStatus LeveldbValueStore::LogRestoreStatus(
     case RESTORE_NONE:
       NOTREACHED();
       break;
-    case RESTORE_DELETE_SUCCESS:
-      restore_histogram_->Add(LEVELDB_RESTORE_DELETE_SUCCESS);
+    case DB_RESTORE_DELETE_SUCCESS:
+      db_restore_histogram_->Add(LEVELDB_DB_RESTORE_DELETE_SUCCESS);
+      break;
+    case DB_RESTORE_DELETE_FAILURE:
+      db_restore_histogram_->Add(LEVELDB_DB_RESTORE_DELETE_FAILURE);
+      break;
+    case DB_RESTORE_REPAIR_SUCCESS:
+      db_restore_histogram_->Add(LEVELDB_DB_RESTORE_REPAIR_SUCCESS);
       break;
-    case RESTORE_DELETE_FAILURE:
-      restore_histogram_->Add(LEVELDB_RESTORE_DELETE_FAILURE);
+    case VALUE_RESTORE_DELETE_SUCCESS:
+      value_restore_histogram_->Add(LEVELDB_VALUE_RESTORE_DELETE_SUCCESS);
       break;
-    case RESTORE_REPAIR_SUCCESS:
-      restore_histogram_->Add(LEVELDB_RESTORE_REPAIR_SUCCESS);
+    case VALUE_RESTORE_DELETE_FAILURE:
+      value_restore_histogram_->Add(LEVELDB_VALUE_RESTORE_DELETE_FAILURE);
       break;
   }
   return restore_status;
@@ -381,9 +402,9 @@ ValueStore::BackingStoreRestoreStatus LeveldbValueStore::FixCorruption(
     // Deleting involves writing to the log, so it's possible to have a
     // perfectly OK database but still have a delete fail.
     if (s.ok())
-      return LogRestoreStatus(RESTORE_REPAIR_SUCCESS);
+      return LogRestoreStatus(VALUE_RESTORE_DELETE_SUCCESS);
     else if (s.IsIOError())
-      return LogRestoreStatus(RESTORE_DELETE_FAILURE);
+      return LogRestoreStatus(VALUE_RESTORE_DELETE_FAILURE);
     // Any other kind of failure triggers a db repair.
   }
 
@@ -402,16 +423,16 @@ ValueStore::BackingStoreRestoreStatus LeveldbValueStore::FixCorruption(
 
   leveldb::DB* db = nullptr;
   if (s.ok()) {
-    restore_status = RESTORE_REPAIR_SUCCESS;
+    restore_status = DB_RESTORE_REPAIR_SUCCESS;
     s = leveldb::DB::Open(open_options_, db_path_.AsUTF8Unsafe(), &db);
   }
 
   if (!s.ok()) {
     if (DeleteDbFile()) {
-      restore_status = RESTORE_DELETE_SUCCESS;
+      restore_status = DB_RESTORE_DELETE_SUCCESS;
       s = leveldb::DB::Open(open_options_, db_path_.AsUTF8Unsafe(), &db);
     } else {
-      restore_status = RESTORE_DELETE_FAILURE;
+      restore_status = DB_RESTORE_DELETE_FAILURE;
     }
   }
 
@@ -423,14 +444,14 @@ ValueStore::BackingStoreRestoreStatus LeveldbValueStore::FixCorruption(
   if (s.ok() && key) {
     s = Delete(*key);
     if (s.ok()) {
-      restore_status = RESTORE_REPAIR_SUCCESS;
+      restore_status = VALUE_RESTORE_DELETE_SUCCESS;
     } else if (s.IsIOError()) {
-      restore_status = RESTORE_DELETE_FAILURE;
+      restore_status = VALUE_RESTORE_DELETE_FAILURE;
     } else {
       db_.reset(db);
       if (!DeleteDbFile())
         db_unrecoverable_ = true;
-      restore_status = RESTORE_DELETE_FAILURE;
+      restore_status = DB_RESTORE_DELETE_FAILURE;
     }
   }
 
@@ -448,7 +469,7 @@ ValueStore::Status LeveldbValueStore::EnsureDbIsOpen() {
 
   if (db_unrecoverable_) {
     return ValueStore::Status(ValueStore::CORRUPTION,
-                              ValueStore::RESTORE_DELETE_FAILURE,
+                              ValueStore::DB_RESTORE_DELETE_FAILURE,
                               "Database corrupted");
   }
 
@@ -461,7 +482,7 @@ ValueStore::Status LeveldbValueStore::EnsureDbIsOpen() {
     db_.reset(db);
   } else if (ldb_status.IsCorruption()) {
     status.restore_status = FixCorruption(nullptr);
-    if (status.restore_status != RESTORE_DELETE_FAILURE) {
+    if (status.restore_status != DB_RESTORE_DELETE_FAILURE) {
       status.code = OK;
       status.message = kRestoredDuringOpen;
     }
diff --git a/extensions/browser/value_store/leveldb_value_store.h b/extensions/browser/value_store/leveldb_value_store.h
index 52ea667..058e282 100644
--- a/extensions/browser/value_store/leveldb_value_store.h
+++ b/extensions/browser/value_store/leveldb_value_store.h
@@ -115,7 +115,8 @@ class LeveldbValueStore : public ValueStore,
   // Database is corrupt - restoration failed.
   bool db_unrecoverable_;
   base::HistogramBase* open_histogram_;
-  base::HistogramBase* restore_histogram_;
+  base::HistogramBase* db_restore_histogram_;
+  base::HistogramBase* value_restore_histogram_;
 
   DISALLOW_COPY_AND_ASSIGN(LeveldbValueStore);
 };
diff --git a/extensions/browser/value_store/leveldb_value_store_unittest.cc b/extensions/browser/value_store/leveldb_value_store_unittest.cc
index 6aeda43..cf53224 100644
--- a/extensions/browser/value_store/leveldb_value_store_unittest.cc
+++ b/extensions/browser/value_store/leveldb_value_store_unittest.cc
@@ -134,7 +134,7 @@ TEST_F(LeveldbValueStoreUnitTest, RestoreDoesMinimumNecessary) {
   ValueStore::ReadResult result = store()->Get();
   ASSERT_FALSE(result->status().ok());
   ASSERT_EQ(ValueStore::CORRUPTION, result->status().code);
-  ASSERT_EQ(ValueStore::RESTORE_REPAIR_SUCCESS,
+  ASSERT_EQ(ValueStore::VALUE_RESTORE_DELETE_SUCCESS,
             result->status().restore_status);
 
   // We should still have all valid pairs present in the database.
@@ -183,7 +183,7 @@ TEST_F(LeveldbValueStoreUnitTest, RestoreFullDatabase) {
 
   // We couldn't recover anything, but we should be in a sane state again.
   ValueStore::ReadResult result = store()->Get();
-  ASSERT_EQ(ValueStore::RESTORE_REPAIR_SUCCESS,
+  ASSERT_EQ(ValueStore::DB_RESTORE_REPAIR_SUCCESS,
             result->status().restore_status);
   EXPECT_TRUE(result->status().ok());
   EXPECT_EQ(0u, result->settings().size());
diff --git a/extensions/browser/value_store/value_store.h b/extensions/browser/value_store/value_store.h
index 8ca110f..b6aa0ba 100644
--- a/extensions/browser/value_store/value_store.h
+++ b/extensions/browser/value_store/value_store.h
@@ -47,11 +47,15 @@ class ValueStore {
     // No restore attempted.
     RESTORE_NONE,
     // Corrupted backing store successfully deleted.
-    RESTORE_DELETE_SUCCESS,
+    DB_RESTORE_DELETE_SUCCESS,
     // Corrupted backing store cannot be deleted.
-    RESTORE_DELETE_FAILURE,
+    DB_RESTORE_DELETE_FAILURE,
     // Corrupted backing store successfully repaired.
-    RESTORE_REPAIR_SUCCESS,
+    DB_RESTORE_REPAIR_SUCCESS,
+    // Corrupted value successfully deleted.
+    VALUE_RESTORE_DELETE_SUCCESS,
+    // Corrupted value cannot be deleted.
+    VALUE_RESTORE_DELETE_FAILURE,
   };
 
   // The status (result) of an operation on a ValueStore.