MetricsService: Send a hash of the UMA log in a header.

chrome/common/metrics/metrics_log_manager.cc

Index: chrome/common/metrics/metrics_log_manager.cc
===================================================================
--- chrome/common/metrics/metrics_log_manager.cc	(revision 228712)
+++ chrome/common/metrics/metrics_log_manager.cc	(working copy)
@@ -7,9 +7,32 @@
 #include <algorithm>
 
 #include "base/metrics/histogram.h"
+#include "base/sha1.h"

[Ilya Sherman, 2013/10/16 18:26:14]

It's probably not a big deal, but I wonder if something like CRC would be more
appropriate than SHA-1.  SHA-1 seems like a somewhat strange choice in general,
since it's a cryptographic hash function, but not really considered to be
cryptographically secure anymore.

 #include "base/strings/string_util.h"
 #include "chrome/common/metrics/metrics_log_base.h"
 
+bool MetricsLogManager::SerializedLog::IsEmpty() const {
+  return log_text.empty();
+}
+
+void MetricsLogManager::SerializedLog::Clear() {
+  log_text.clear();
+  log_hash.clear();
+}
+
+void MetricsLogManager::SerializedLog::Swap(
+    MetricsLogManager::SerializedLog* other) {
+  log_text.swap(other->log_text);
+  log_hash.swap(other->log_hash);
+}
+
+void MetricsLogManager::SerializedLog::UpdateHash() {
+  if (log_text.empty())
+    log_hash.clear();
+  else
+    log_hash = base::SHA1HashString(log_text);
+}

[Ilya Sherman, 2013/10/16 18:26:14]

It would be nice if the struct guaranteed that the hash were always up to date,
i.e. if the struct were a class such that the only way to modify log_text were
to call a setter that updates the hash as well.

[Alexei Svitkine (slow), 2013/10/16 19:31:46]

Done.

+
 MetricsLogManager::MetricsLogManager()
     : current_log_type_(NO_LOG),
       paused_log_type_(NO_LOG),
@@ -32,9 +55,9 @@
   DCHECK(current_log_.get());
   DCHECK(current_log_type_ != NO_LOG);
   current_log_->CloseLog();
-  std::string compressed_log;
+  SerializedLog compressed_log;
   CompressCurrentLog(&compressed_log);
-  if (!compressed_log.empty())
+  if (!compressed_log.IsEmpty())
     StoreLog(&compressed_log, current_log_type_, NORMAL_STORE);
   current_log_.reset();
   current_log_type_ = NO_LOG;
@@ -42,7 +65,7 @@
 
 void MetricsLogManager::StageNextLogForUpload() {
   // Prioritize initial logs for uploading.
-  std::vector<std::string>* source_list =
+  std::vector<SerializedLog>* source_list =
       unsent_initial_logs_.empty() ? &unsent_ongoing_logs_
                                    : &unsent_initial_logs_;
   LogType source_type = (source_list == &unsent_ongoing_logs_) ? ONGOING_LOG
@@ -50,9 +73,9 @@
   // CHECK, rather than DCHECK, because swap()ing with an empty list causes
   // hard-to-identify crashes much later.
   CHECK(!source_list->empty());
-  DCHECK(staged_log_text_.empty());
+  DCHECK(staged_log_.IsEmpty());
   DCHECK(staged_log_type_ == NO_LOG);
-  staged_log_text_.swap(source_list->back());
+  staged_log_.Swap(&source_list->back());
   staged_log_type_ = source_type;
   source_list->pop_back();
 
@@ -67,11 +90,11 @@
 }
 
 bool MetricsLogManager::has_staged_log() const {
-  return !staged_log_text().empty();
+  return !staged_log_.IsEmpty();
 }
 
 void MetricsLogManager::DiscardStagedLog() {
-  staged_log_text_.clear();
+  staged_log_.Clear();
   staged_log_type_ = NO_LOG;
 }
 
@@ -101,22 +124,22 @@
   DCHECK(has_staged_log());
 
   // If compressing the log failed, there's nothing to store.
-  if (staged_log_text_.empty())
+  if (staged_log_.IsEmpty())
     return;
 
-  StoreLog(&staged_log_text_, staged_log_type_, store_type);
+  StoreLog(&staged_log_, staged_log_type_, store_type);
   DiscardStagedLog();
 }
 
-void MetricsLogManager::StoreLog(std::string* log_text,
+void MetricsLogManager::StoreLog(SerializedLog* log,
                                  LogType log_type,
                                  StoreType store_type) {
   DCHECK(log_type != NO_LOG);
-  std::vector<std::string>* destination_list =
+  std::vector<SerializedLog>* destination_list =
       (log_type == INITIAL_LOG) ? &unsent_initial_logs_
                                 : &unsent_ongoing_logs_;
-  destination_list->push_back(std::string());
-  destination_list->back().swap(*log_text);
+  destination_list->push_back(SerializedLog());
+  destination_list->back().Swap(log);
 
   if (store_type == PROVISIONAL_STORE) {
     last_provisional_store_index_ = destination_list->size() - 1;
@@ -127,7 +150,7 @@
 void MetricsLogManager::DiscardLastProvisionalStore() {
   if (last_provisional_store_index_ == -1)
     return;
-  std::vector<std::string>* source_list =
+  std::vector<SerializedLog>* source_list =
       (last_provisional_store_type_ == ONGOING_LOG) ? &unsent_ongoing_logs_
                                                     : &unsent_initial_logs_;
   DCHECK_LT(static_cast<unsigned int>(last_provisional_store_index_),
@@ -142,9 +165,9 @@
     return;
   // Remove any ongoing logs that are over the serialization size limit.
   if (max_ongoing_log_store_size_) {
-    for (std::vector<std::string>::iterator it = unsent_ongoing_logs_.begin();
+    for (std::vector<SerializedLog>::iterator it = unsent_ongoing_logs_.begin();
          it != unsent_ongoing_logs_.end();) {
-      size_t log_size = it->length();
+      size_t log_size = it->log_text.length();
       if (log_size > max_ongoing_log_store_size_) {
         UMA_HISTOGRAM_COUNTS("UMA.Large Accumulated Log Not Persisted",
                              static_cast<int>(log_size));
@@ -166,6 +189,7 @@
   log_serializer_->DeserializeLogs(ONGOING_LOG, &unsent_ongoing_logs_);
 }
 
-void MetricsLogManager::CompressCurrentLog(std::string* compressed_log) {
-  current_log_->GetEncodedLog(compressed_log);
+void MetricsLogManager::CompressCurrentLog(SerializedLog* compressed_log) {
+  current_log_->GetEncodedLog(&compressed_log->log_text);
+  compressed_log->UpdateHash();
 }