Begin to separate the MetricsService logic for creating vs uploading logs

chrome/common/metrics/metrics_log_manager.cc

Index: chrome/common/metrics/metrics_log_manager.cc
diff --git a/chrome/common/metrics/metrics_log_manager.cc b/chrome/common/metrics/metrics_log_manager.cc
index 32fe51c8329c4b1cdb21855cb201a6745cff2338..bea7d08d2cef896dcf6b209a1922962ca9bca38a 100644
--- a/chrome/common/metrics/metrics_log_manager.cc
+++ b/chrome/common/metrics/metrics_log_manager.cc
@@ -14,28 +14,45 @@
 #include "base/string_util.h"
 #include "chrome/common/metrics/metrics_log_base.h"
 
-MetricsLogManager::MetricsLogManager() : max_ongoing_log_store_size_(0) {}
+MetricsLogManager::MetricsLogManager() : current_log_type_(INITIAL_LOG),
+                                         staged_log_type_(INITIAL_LOG),
+                                         max_ongoing_log_store_size_(0) {}
 
 MetricsLogManager::~MetricsLogManager() {}
 
-void MetricsLogManager::BeginLoggingWithLog(MetricsLogBase* log) {
+void MetricsLogManager::BeginLoggingWithLog(MetricsLogBase* log,
+                                            LogType log_type) {
   DCHECK(!current_log_.get());
   current_log_.reset(log);
+  current_log_type_ = log_type;
 }
 
-void MetricsLogManager::StageCurrentLogForUpload() {
+void MetricsLogManager::FinishCurrentLog() {
   DCHECK(current_log_.get());
   current_log_->CloseLog();
-  staged_log_.reset(current_log_.release());
-  CompressStagedLog();
+  std::string compressed_log;
+  CompressCurrentLog(&compressed_log);
+  if (!compressed_log.empty())
+    StoreLog(compressed_log, current_log_type_);
+  current_log_.reset();
+}
+
+void MetricsLogManager::StageNextLogForUpload() {
+  // Prioritize initial logs for uploading.
+  LogType log_type = unsent_initial_logs_.empty() ? ONGOING_LOG : INITIAL_LOG;

[Ilya Sherman, 2012/02/29 01:25:41]

nit: I'm not sure how much breaking out this variable helps readability, since
you only use it on the line below, where using unsent_initial_logs_.empty()
would also be pretty clear.

[stuartmorgan, 2012/02/29 13:26:15]

Yep, this was an artifact of a previous iteration. Removed.

+  std::vector<std::string>* source_list = (log_type == ONGOING_LOG) ?
+      &unsent_ongoing_logs_ : &unsent_initial_logs_;
+  DCHECK(!source_list->empty());
+  DCHECK(compressed_staged_log_text_.empty());
+  compressed_staged_log_text_ = source_list->back();

[Ilya Sherman, 2012/02/29 01:25:41]

Please use std::string::swap() here to avoid copying the log text, which can be
quite large.

[stuartmorgan, 2012/02/29 13:26:15]

Good tip; I wasn't familiar with swap(). (It's a bit awkward with both xml and
proto, so I added another temporary utility method to the SerializedLog struct.)

+  source_list->pop_back();
 }
 
 bool MetricsLogManager::has_staged_log() const {
-  return staged_log_.get() || !compressed_staged_log_text_.empty();
+  return !compressed_staged_log_text_.empty();
 }
 
 void MetricsLogManager::DiscardStagedLog() {
-  staged_log_.reset();
   compressed_staged_log_text_.clear();
 }
 
@@ -54,42 +71,43 @@ void MetricsLogManager::ResumePausedLog() {
   current_log_.reset(paused_log_.release());
 }
 
-void MetricsLogManager::StoreStagedLogAsUnsent(LogType log_type) {
+void MetricsLogManager::StoreStagedLogAsUnsent() {
   DCHECK(has_staged_log());
   // If compressing the log failed, there's nothing to store.
   if (compressed_staged_log_text_.empty())
     return;
 
-  if (log_type == INITIAL_LOG) {
-    unsent_initial_logs_.push_back(compressed_staged_log_text_);
-  } else {
-    // If it's too large, just note that and discard it.
-    if (max_ongoing_log_store_size_ &&
-        compressed_staged_log_text_.length() > max_ongoing_log_store_size_) {
-      UMA_HISTOGRAM_COUNTS(
-          "UMA.Large Accumulated Log Not Persisted",
-          static_cast<int>(compressed_staged_log_text_.length()));
-    } else {
-      unsent_ongoing_logs_.push_back(compressed_staged_log_text_);
-    }
-  }
+  StoreLog(compressed_staged_log_text_, staged_log_type_);
   DiscardStagedLog();
 }
 
-void MetricsLogManager::StageNextStoredLogForUpload() {
-  // Prioritize initial logs for uploading.
-  std::vector<std::string>* source_list = unsent_initial_logs_.empty() ?
-      &unsent_ongoing_logs_ : &unsent_initial_logs_;
-  DCHECK(!source_list->empty());
-  DCHECK(compressed_staged_log_text_.empty());
-  compressed_staged_log_text_ = source_list->back();
-  source_list->pop_back();
+void MetricsLogManager::StoreLog(const std::string& log_text,
+                                 LogType log_type) {
+  if (log_type == INITIAL_LOG) {
+    unsent_initial_logs_.push_back(log_text);
+  } else {
+    unsent_ongoing_logs_.push_back(log_text);
+  }

[Ilya Sherman, 2012/02/29 01:25:41]

If we can avoid copying the log text by using std::string::swap() here, that
would probably be better, since the log text can get quite large.

[stuartmorgan, 2012/02/29 13:26:15]

The semantics of the StoreLog method become unfortunate, but it's an internal
method, where non-obvious semantics are less of a concern, so the efficiency win
seems worthwhile. Done.

 }
 
 void MetricsLogManager::PersistUnsentLogs() {
   DCHECK(log_serializer_.get());
   if (!log_serializer_.get())
     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();
+         it != unsent_ongoing_logs_.end();) {
+      size_t log_size = it->length();
+      if (log_size > max_ongoing_log_store_size_) {
+        UMA_HISTOGRAM_COUNTS("UMA.Large Accumulated Log Not Persisted",
+                             static_cast<int>(log_size));
+        it = unsent_ongoing_logs_.erase(it);
+      } else {
+        ++it;
+      }
+    }
+  }
   log_serializer_->SerializeLogs(unsent_initial_logs_, INITIAL_LOG);
   log_serializer_->SerializeLogs(unsent_ongoing_logs_, ONGOING_LOG);
 }
@@ -102,17 +120,16 @@ void MetricsLogManager::LoadPersistedUnsentLogs() {
   log_serializer_->DeserializeLogs(ONGOING_LOG, &unsent_ongoing_logs_);
 }
 
-void MetricsLogManager::CompressStagedLog() {
-  int text_size = staged_log_->GetEncodedLogSize();
-  std::string staged_log_text;
+void MetricsLogManager::CompressCurrentLog(std::string* compressed_log) {
+  int text_size = current_log_->GetEncodedLogSize();
   DCHECK_GT(text_size, 0);
-  staged_log_->GetEncodedLog(WriteInto(&staged_log_text, text_size + 1),
-                             text_size);
+  std::string log_text;
+  current_log_->GetEncodedLog(WriteInto(&log_text, text_size + 1), text_size);
 
-  bool success = Bzip2Compress(staged_log_text, &compressed_staged_log_text_);
+  bool success = Bzip2Compress(log_text, compressed_log);
   if (success) {
     // Allow security-conscious users to see all metrics logs that we send.
-    DVLOG(1) << "METRICS LOG: " << staged_log_text;
+    DVLOG(1) << "METRICS LOG: " << log_text;
   } else {
     NOTREACHED() << "Failed to compress log for transmission.";
   }