Suffixed histograms for SB2.DatabaseKilobytes and SB2.PrefixSetKilobytes

chrome/browser/safe_browsing/safe_browsing_database.cc

Index: chrome/browser/safe_browsing/safe_browsing_database.cc
diff --git a/chrome/browser/safe_browsing/safe_browsing_database.cc b/chrome/browser/safe_browsing/safe_browsing_database.cc
index 0f11795edb63e2dafcbbedf451ae1771a38d25a5..6359b7037d41b6b3d8a1acc6ad7aa9b09115bb74 100644
--- a/chrome/browser/safe_browsing/safe_browsing_database.cc
+++ b/chrome/browser/safe_browsing/safe_browsing_database.cc
@@ -1329,7 +1329,7 @@ void SafeBrowsingDatabaseNew::UpdateFinished(bool update_succeeded) {
         DownloadDBFilename(filename_base_),
         download_store_.get(),
         FAILURE_DOWNLOAD_DATABASE_UPDATE_FINISH);
-    UMA_HISTOGRAM_COUNTS("SB2.DownloadDatabaseKilobytes",
+    UMA_HISTOGRAM_COUNTS("SB2.DatabaseKilobytes.Download",
                          static_cast<int>(size_bytes / 1024));
   }
 
@@ -1352,7 +1352,7 @@ void SafeBrowsingDatabaseNew::UpdateFinished(bool update_succeeded) {
         ExtensionBlacklistDBFilename(filename_base_),
         extension_blacklist_store_.get(),
         FAILURE_EXTENSION_BLACKLIST_UPDATE_FINISH);
-    UMA_HISTOGRAM_COUNTS("SB2.ExtensionBlacklistKilobytes",
+    UMA_HISTOGRAM_COUNTS("SB2.DatabaseKilobytes.ExtensionBlacklist",
                          static_cast<int>(size_bytes / 1024));
   }
 
@@ -1397,6 +1397,10 @@ void SafeBrowsingDatabaseNew::UpdateWhitelistStore(
     return;
   }
 
+  const int64 file_size = GetFileSizeOrZero(store_filename);
+  RecordSBWhitelistCountsHistogram("SB2.DatabaseKilobytes", whitelist_id,
+                                   static_cast<int>(file_size / 1024));
+
 #if defined(OS_MACOSX)
   base::mac::SetFileBackupExclusion(store_filename);
 #endif
@@ -1484,15 +1488,7 @@ void SafeBrowsingDatabaseNew::UpdatePrefixSetUrlStore(
 
   UMA_HISTOGRAM_LONG_TIMES("SB2.BuildFilter", base::TimeTicks::Now() - before);
 
-  {
-    // Persist the prefix set to disk. Do not grab the lock to avoid contention
-    // while writing to disk. This is safe as only this thread can ever modify
-    // |state_manager_|'s prefix sets anyways.
-    scoped_ptr<ReadTransaction> txn =
-        state_manager_.BeginReadTransactionNoLockOnMainThread();
-    WritePrefixSet(db_filename, txn->GetPrefixSet(prefix_set_id),
-                   write_failure_type);
-  }
+  WritePrefixSet(db_filename, prefix_set_id, write_failure_type);
 
   // Gather statistics.
   if (got_counters && metric->GetIOCounters(&io_after)) {
@@ -1511,8 +1507,8 @@ void SafeBrowsingDatabaseNew::UpdatePrefixSetUrlStore(
   }
 
   const int64 file_size = GetFileSizeOrZero(db_filename);
-  UMA_HISTOGRAM_COUNTS("SB2.DatabaseKilobytes",
-                       static_cast<int>(file_size / 1024));
+  RecordPrefixSetCountsHistogram("SB2.DatabaseKilobytes", prefix_set_id,
+                                 static_cast<int>(file_size / 1024));

[Scott Hess - ex-Googler, 2014/12/18 23:57:55]

I find the prefix-set references misleading, here.  Really this seems like it
should be a store-id, some of which happen to have prefix sets associated with
them.  If that were the case, then it would be easy to pull the entire 3 lines
into a histogram helper for each of the points where it's happening.

It would also unify this helper with the whitelist helper.

[gab, 2014/12/19 15:25:54]

Right but I was trying to avoid introducing yet another enum which would have an
ID for every store (we already have an enum with an ID for every list and the
only difference would be that lists MALWARE/PHISHING are in the same BROWSE
store...).

And it felt that re-using the upcoming (and already piped through the relevant
calls) PrefixSetId/SBWhitelistId was natural (sure we could add a
PrefixSetIdToStoreId() helper to alleviate more ID piping, but I felt the
complexity of two different histogram helper was less than the complexity of yet
another store ID as it is centralized to a single use-case whereas a StoreId
enum is more global and invasive).

 
 #if defined(OS_MACOSX)
   base::mac::SetFileBackupExclusion(db_filename);
@@ -1532,8 +1528,15 @@ void SafeBrowsingDatabaseNew::UpdateIpBlacklistStore() {
     return;
   }
 
+  const base::FilePath ip_blacklist_filename =
+      IpBlacklistDBFilename(filename_base_);
+
+  const int64 file_size = GetFileSizeOrZero(ip_blacklist_filename);
+  UMA_HISTOGRAM_COUNTS("SB2.DatabaseKilobytes.IPBlacklist",
+                       static_cast<int>(file_size / 1024));
+
 #if defined(OS_MACOSX)
-  base::mac::SetFileBackupExclusion(IpBlacklistDBFilename(filename_base_));
+  base::mac::SetFileBackupExclusion(ip_blacklist_filename);
 #endif
 
   LoadIpBlacklist(full_hashes);
@@ -1676,10 +1679,17 @@ bool SafeBrowsingDatabaseNew::Delete() {
 }
 
 void SafeBrowsingDatabaseNew::WritePrefixSet(const base::FilePath& db_filename,
-                                             const PrefixSet* prefix_set,
+                                             PrefixSetId prefix_set_id,
                                              FailureType write_failure_type) {
   DCHECK(thread_checker_.CalledOnValidThread());
 
+  // Do not grab the lock to avoid contention while writing to disk. This is
+  // safe as only this thread can ever modify |state_manager_|'s prefix sets
+  // anyways.
+  scoped_ptr<ReadTransaction> txn =
+      state_manager_.BeginReadTransactionNoLockOnMainThread();
+  const PrefixSet* prefix_set = txn->GetPrefixSet(prefix_set_id);
+
   if (!prefix_set)
     return;
 
@@ -1690,8 +1700,8 @@ void SafeBrowsingDatabaseNew::WritePrefixSet(const base::FilePath& db_filename,
   UMA_HISTOGRAM_TIMES("SB2.PrefixSetWrite", base::TimeTicks::Now() - before);
 
   const int64 file_size = GetFileSizeOrZero(prefix_set_filename);
-  UMA_HISTOGRAM_COUNTS("SB2.PrefixSetKilobytes",
-                       static_cast<int>(file_size / 1024));
+  RecordPrefixSetCountsHistogram("SB2.PrefixSetKilobytes", prefix_set_id,
+                                 static_cast<int>(file_size / 1024));
 
   if (!write_ok)
     RecordFailure(write_failure_type);
@@ -1798,3 +1808,50 @@ SafeBrowsingDatabaseNew::GetUnsynchronizedPrefixGetHashCacheForTesting() {
   scoped_ptr<ReadTransaction> txn = state_manager_.BeginReadTransaction();
   return txn->prefix_gethash_cache();
 }
+
+void SafeBrowsingDatabaseNew::RecordPrefixSetCountsHistogram(
+    const std::string& histogram_name_base,
+    PrefixSetId prefix_set_id,
+    int sample) {
+  std::string histogram_name_full(histogram_name_base);
+  switch (prefix_set_id) {
+    case PrefixSetId::BROWSE:
+      histogram_name_full.append(".Browse");
+      break;
+    case PrefixSetId::SIDE_EFFECT_FREE_WHITELIST:
+      histogram_name_full.append(".SideEffectFreeWhitelist");
+      break;
+    case PrefixSetId::UNWANTED_SOFTWARE:
+      histogram_name_full.append(".UnwantedSoftware");
+      break;
+  }
+
+  // Histogram properties as in UMA_HISTOGRAM_COUNTS macro.
+  base::HistogramBase* histogram_pointer = base::Histogram::FactoryGet(
+      histogram_name_full, 1, 1000000, 50,
+      base::HistogramBase::kUmaTargetedHistogramFlag);
+
+  histogram_pointer->Add(sample);
+}
+
+void SafeBrowsingDatabaseNew::RecordSBWhitelistCountsHistogram(
+    const std::string& histogram_name_base,
+    SBWhitelistId whitelist_id,
+    int sample) {
+  std::string histogram_name_full(histogram_name_base);
+  switch (whitelist_id) {
+    case SBWhitelistId::CSD:
+      histogram_name_full.append(".CSD");
+      break;
+    case SBWhitelistId::DOWNLOAD:
+      histogram_name_full.append(".DownloadWhitelist");
+      break;
+  }
+
+  // Histogram properties as in UMA_HISTOGRAM_COUNTS macro.
+  base::HistogramBase* histogram_pointer = base::Histogram::FactoryGet(
+      histogram_name_full, 1, 1000000, 50,
+      base::HistogramBase::kUmaTargetedHistogramFlag);
+
+  histogram_pointer->Add(sample);
+}

[Scott Hess - ex-Googler, 2014/12/18 23:57:55]

I think these might usefully use a helper on the order of:
   void HistogramHelper(name_base, name_suffix, sample);

[gab, 2014/12/19 15:25:53]

It felt to me like that avoiding the readability overhead of one more
indirection level was worth just inlining the Get()->Add(sample) without much
harm.