Add a minimum byte count to metrics log serialization

chrome/browser/metrics/metrics_log_serializer.cc

Index: chrome/browser/metrics/metrics_log_serializer.cc
diff --git a/chrome/browser/metrics/metrics_log_serializer.cc b/chrome/browser/metrics/metrics_log_serializer.cc
index 3369f85a7a630d2fb4048732e554a3b5d0a8c320..cb6862982e13597697c745f4ae855ca4d2b9fb6c 100644
--- a/chrome/browser/metrics/metrics_log_serializer.cc
+++ b/chrome/browser/metrics/metrics_log_serializer.cc
@@ -14,21 +14,8 @@
 
 namespace {
 
-// The number of "initial" logs we're willing to save, and hope to send during
-// a future Chrome session.  Initial logs contain crash stats, and are pretty
-// small.
-const size_t kMaxInitialLogsPersisted = 20;
-
-// The number of ongoing logs we're willing to save persistently, and hope to
-// send during a this or future sessions.  Note that each log may be pretty
-// large, as presumably the related "initial" log wasn't sent (probably nothing
-// was, as the user was probably off-line).  As a result, the log probably kept
-// accumulating while the "initial" log was stalled, and couldn't be sent.  As a
-// result, we don't want to save too many of these mega-logs.
-// A "standard shutdown" will create a small log, including just the data that
-// was not yet been transmitted, and that is normal (to have exactly one
-// ongoing_log_ at startup).
-const size_t kMaxOngoingLogsPersisted = 8;
+// The number of bytes each of initial and ongoing logs that will be stored.
+const size_t kMaxStorageBytesPerLogType = 300000;
 
 // We append (2) more elements to persisted lists: the size of the list and a
 // checksum of the elements.
@@ -75,17 +62,14 @@ void MetricsLogSerializer::SerializeLogs(
   DCHECK(local_state);
   const char* pref_xml = NULL;
   const char* pref_proto = NULL;
-  size_t max_store_count = 0;
   switch (log_type) {
     case MetricsLogManager::INITIAL_LOG:
       pref_xml = prefs::kMetricsInitialLogsXml;
       pref_proto = prefs::kMetricsInitialLogsProto;
-      max_store_count = kMaxInitialLogsPersisted;
       break;
     case MetricsLogManager::ONGOING_LOG:
       pref_xml = prefs::kMetricsOngoingLogsXml;
       pref_proto = prefs::kMetricsOngoingLogsProto;
-      max_store_count = kMaxOngoingLogsPersisted;
       break;
     default:
       NOTREACHED();
@@ -94,11 +78,11 @@ void MetricsLogSerializer::SerializeLogs(
 
   // Write the XML version.
   ListPrefUpdate update_xml(local_state, pref_xml);
-  WriteLogsToPrefList(logs, true, max_store_count, update_xml.Get());
+  WriteLogsToPrefList(logs, true, update_xml.Get());
 
   // Write the protobuf version.
   ListPrefUpdate update_proto(local_state, pref_proto);
-  WriteLogsToPrefList(logs, false, max_store_count, update_proto.Get());
+  WriteLogsToPrefList(logs, false, update_proto.Get());
 }
 
 void MetricsLogSerializer::DeserializeLogs(
@@ -131,12 +115,24 @@ void MetricsLogSerializer::DeserializeLogs(
 void MetricsLogSerializer::WriteLogsToPrefList(
     const std::vector<MetricsLogManager::SerializedLog>& local_list,
     bool is_xml,
-    size_t max_list_size,
     base::ListValue* list) {
   list->Clear();
-  size_t start = 0;
-  if (local_list.size() > max_list_size)
-    start = local_list.size() - max_list_size;
+
+  // Keep the most recent logs, up to the size limit.
+  size_t start = local_list.size();
+  size_t bytes_used = 0;
+  for (std::vector<MetricsLogManager::SerializedLog>::const_reverse_iterator
+       it = local_list.rbegin(); it != local_list.rend(); ++it) {
+    // TODO(isherman): Always uses XML length so both formats of a given log
+    // will be saved; switch to proto once that's the primary format.
+    size_t log_size = it->xml.length();
+    bytes_used += log_size;
+    if (bytes_used > kMaxStorageBytesPerLogType)
+      break;
+    --start;
+  }
+  // Logs big enough that not even one fits in the allowed size should never
+  // have reached this point, so at least one should always be written.

[Ilya Sherman, 2012/03/01 22:58:21]

nit: This comment makes it sound like the line below should be DCHECK_LT.

[jar (doing other things), 2012/03/02 17:47:00]

In fact... I don't see why you are sure this is true.  The limit you set is not
enforced anywhere else (the constant is local to this file), so if I set it to
(for example) 1, then I'm pretty sure your comment would be violated.

IF you have an expectation that some other constant is related to your 300000
value, you should use that constant, and not have an independently defined
value.  To put it another way: I should be able to change values of the
constants, and things should just work (I shouldn't have to adjust several
things in sync).

   DCHECK_LE(start, local_list.size());
   if (local_list.size() <= start)
     return;