Suffixed histograms for SB2.DatabaseKilobytes and SB2.PrefixSetKilobytes

chrome/browser/safe_browsing/safe_browsing_database.cc

 // 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 "chrome/browser/safe_browsing/safe_browsing_database.h"
 
 #include <algorithm>
 #include <iterator>
 
 #include "base/bind.h"
@@ skipping 1311 matching lines @@
     if (unwanted_software_store_)
       unwanted_software_store_->CancelUpdate();
     return;
   }
 
   if (download_store_) {
     int64 size_bytes = UpdateHashPrefixStore(
         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));
   }
 
   UpdatePrefixSetUrlStore(BrowseDBFilename(filename_base_),
                           browse_store_.get(),
                           PrefixSetId::BROWSE,
                           FAILURE_BROWSE_DATABASE_UPDATE_FINISH,
                           FAILURE_BROWSE_PREFIX_SET_WRITE,
                           true);
 
   UpdateWhitelistStore(CsdWhitelistDBFilename(filename_base_),
                        csd_whitelist_store_.get(),
                        SBWhitelistId::CSD);
   UpdateWhitelistStore(DownloadWhitelistDBFilename(filename_base_),
                        download_whitelist_store_.get(),
                        SBWhitelistId::DOWNLOAD);
 
   if (extension_blacklist_store_) {
     int64 size_bytes = UpdateHashPrefixStore(
         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));
   }
 
   if (side_effect_free_whitelist_store_) {
     UpdatePrefixSetUrlStore(SideEffectFreeWhitelistDBFilename(filename_base_),
                             side_effect_free_whitelist_store_.get(),
                             PrefixSetId::SIDE_EFFECT_FREE_WHITELIST,
                             FAILURE_SIDE_EFFECT_FREE_WHITELIST_UPDATE_FINISH,
                             FAILURE_SIDE_EFFECT_FREE_WHITELIST_PREFIX_SET_WRITE,
                             false);
@@ skipping 24 matching lines @@
   // Note: |builder| will not be empty.  The current data store implementation
   // stores all full-length hashes as both full and prefix hashes.
   PrefixSetBuilder builder;
   std::vector<SBAddFullHash> full_hashes;
   if (!store->FinishUpdate(&builder, &full_hashes)) {
     RecordFailure(FAILURE_WHITELIST_DATABASE_UPDATE_FINISH);
     WhitelistEverything(whitelist_id);
     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
 
   LoadWhitelist(full_hashes, whitelist_id);
 }
 
 int64 SafeBrowsingDatabaseNew::UpdateHashPrefixStore(
     const base::FilePath& store_filename,
     SafeBrowsingStore* store,
@@ skipping 67 matching lines @@
   }
 
   // Swap in the newly built filter.
   {
     scoped_ptr<WriteTransaction> txn = state_manager_.BeginWriteTransaction();
     txn->SwapPrefixSet(prefix_set_id, new_prefix_set.Pass());
   }
 
   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)) {
     UMA_HISTOGRAM_COUNTS("SB2.BuildReadKilobytes",
                          static_cast<int>(io_after.ReadTransferCount -
                                           io_before.ReadTransferCount) / 1024);
     UMA_HISTOGRAM_COUNTS("SB2.BuildWriteKilobytes",
                          static_cast<int>(io_after.WriteTransferCount -
                                           io_before.WriteTransferCount) / 1024);
     UMA_HISTOGRAM_COUNTS("SB2.BuildReadOperations",
                          static_cast<int>(io_after.ReadOperationCount -
                                           io_before.ReadOperationCount));
     UMA_HISTOGRAM_COUNTS("SB2.BuildWriteOperations",
                          static_cast<int>(io_after.WriteOperationCount -
                                           io_before.WriteOperationCount));
   }
 
   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);
 #endif
 }
 
 void SafeBrowsingDatabaseNew::UpdateIpBlacklistStore() {
   DCHECK(thread_checker_.CalledOnValidThread());
 
   // Note: prefixes will not be empty.  The current data store implementation
   // stores all full-length hashes as both full and prefix hashes.
   PrefixSetBuilder builder;
   std::vector<SBAddFullHash> full_hashes;
   if (!ip_blacklist_store_->FinishUpdate(&builder, &full_hashes)) {
     RecordFailure(FAILURE_IP_BLACKLIST_UPDATE_FINISH);
     LoadIpBlacklist(std::vector<SBAddFullHash>());  // Clear the list.
     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);
 }
 
 void SafeBrowsingDatabaseNew::HandleCorruptDatabase() {
   DCHECK(thread_checker_.CalledOnValidThread());
 
   // Reset the database after the current task has unwound (but only
   // reset once within the scope of a given task).
@@ skipping 122 matching lines @@
 
   const bool r11 =
       base::DeleteFile(UnwantedSoftwareDBFilename(filename_base_), false);
   if (!r11)
     RecordFailure(FAILURE_UNWANTED_SOFTWARE_PREFIX_SET_DELETE);
 
   return r1 && r2 && r3 && r4 && r5 && r6 && r7 && r8 && r9 && r10 && r11;
 }
 
 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;
 
   const base::FilePath prefix_set_filename = PrefixSetForFilename(db_filename);
 
   const base::TimeTicks before = base::TimeTicks::Now();
   const bool write_ok = prefix_set->WriteFile(prefix_set_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);
 
 #if defined(OS_MACOSX)
   base::mac::SetFileBackupExclusion(prefix_set_filename);
 #endif
 }
 
 void SafeBrowsingDatabaseNew::WhitelistEverything(SBWhitelistId whitelist_id) {
@@ skipping 86 matching lines @@
 bool SafeBrowsingDatabaseNew::IsCsdWhitelistKillSwitchOn() {
   scoped_ptr<ReadTransaction> txn = state_manager_.BeginReadTransaction();
   return txn->GetSBWhitelist(SBWhitelistId::CSD)->second;
 }
 
 SafeBrowsingDatabaseNew::PrefixGetHashCache*
 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.