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..d8b76d6f733bb5fd34252d6d1c2feab61821ed1e 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,12 @@ 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, kMaxStorageBytesPerLogType, 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, kMaxStorageBytesPerLogType,
+                      update_proto.Get());
 }
 
 void MetricsLogSerializer::DeserializeLogs(
@@ -131,25 +116,48 @@ void MetricsLogSerializer::DeserializeLogs(
 void MetricsLogSerializer::WriteLogsToPrefList(
     const std::vector<MetricsLogManager::SerializedLog>& local_list,
     bool is_xml,
-    size_t max_list_size,
+    size_t max_bytes,
     base::ListValue* list) {
   list->Clear();
-  size_t start = 0;
-  if (local_list.size() > max_list_size)
-    start = local_list.size() - max_list_size;
-  DCHECK_LE(start, local_list.size());
-  if (local_list.size() <= start)
+
+  // Keep the most recent logs, up to the size limit.

[jar (doing other things), 2012/05/05 01:09:31]

Although your proposal makes a lot of sense for mobile, where memory is
constrained, and there is a great potential for users to restart regularly
(generating small logs), I don't think this is a good solution at all for other
platforms.  The problem is that logs continue to rise in size, as does memory,
and this system seems guaranteed to impact desktop reporting IMO.  You may
(currently) be willing to live with that loss... but I don't think we can do
that to our current user base.

I'd rather see an ifdef style of discard patching, which will not impact the
current approach.  I wouldn't mind changing things to only discard if both
thresholds are exceeded (both too many files, and too many bytes).  That would
indeed seem to be a nice  improvement for non-mobile, as well as satisfying your
needs for mobile (at least in teh use case you described, with many many little
logs).

I do however suspect that you really want to just discard data when your memory
footprint is exceeded (too many bytes) so you may choose to do so on your
platform.

Am I missing something?

+  size_t start = local_list.size();
+  size_t bytes_used = 0;
+  std::set<size_t> skip_indexes;
+  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();
+    // If a log is individually larger than the max, skip it.
+    if (log_size > max_bytes) {
+      DLOG(WARNING) << "Individual UMA log was discarded as too large: "
+                    << log_size << " bytes";
+      skip_indexes.insert(start - 1);
+    } else {
+      bytes_used += log_size;
+      if (bytes_used > max_bytes)
+        break;
+    }
+    --start;
+  }
+  if (local_list.size() == 0 ||
+      skip_indexes.size() == local_list.size() - start)
     return;
+  DCHECK_LT(start, local_list.size());
 
   // Store size at the beginning of the list.
-  list->Append(Value::CreateIntegerValue(local_list.size() - start));
+  list->Append(Value::CreateIntegerValue(local_list.size() - start -
+                                         skip_indexes.size()));
 
   base::MD5Context ctx;
   base::MD5Init(&ctx);
   std::string encoded_log;
+  size_t i = start;
   for (std::vector<MetricsLogManager::SerializedLog>::const_iterator it =
-           local_list.begin() + start;
-       it != local_list.end(); ++it) {
+           local_list.begin() + start; it != local_list.end(); ++it, ++i) {
+    if (skip_indexes.count(i))
+      continue;
     const std::string& value = is_xml ? it->xml : it->proto;
     // We encode the compressed log as Value::CreateStringValue() expects to
     // take a valid UTF8 string.