Add a minimum byte count to metrics log serialization

chrome/browser/metrics/metrics_log_serializer.cc

 // Copyright (c) 2011 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/metrics/metrics_log_serializer.h"
 
 #include "base/base64.h"
 #include "base/md5.h"
 #include "base/metrics/histogram.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/prefs/pref_service.h"
 #include "chrome/browser/prefs/scoped_user_pref_update.h"
 #include "chrome/common/pref_names.h"
 
 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.
 const size_t kChecksumEntryCount = 2;
 
 // TODO(isherman): Remove this histogram once it's confirmed that there are no
 // encoding failures for protobuf logs.
 enum LogStoreStatus {
   STORE_SUCCESS,    // Successfully presisted log.
   ENCODE_FAIL,      // Failed to encode log.
@@ skipping 26 matching lines @@
 
 MetricsLogSerializer::~MetricsLogSerializer() {}
 
 void MetricsLogSerializer::SerializeLogs(
     const std::vector<MetricsLogManager::SerializedLog>& logs,
     MetricsLogManager::LogType log_type) {
   PrefService* local_state = g_browser_process->local_state();
   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();
       return;
   };
 
   // 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(
     MetricsLogManager::LogType log_type,
     std::vector<MetricsLogManager::SerializedLog>* logs) {
   DCHECK(logs);
   PrefService* local_state = g_browser_process->local_state();
   DCHECK(local_state);
 
   const char* pref_xml;
@@ skipping 12 matching lines @@
     // In order to try to keep the data sent to both servers roughly in sync,
     // only read the protobuf data if we read the XML data successfully.
     ReadLogsFromPrefList(*unsent_logs_proto, false, logs);
   }
 }
 
 // static
 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.
     if (!base::Base64Encode(value, &encoded_log)) {
       MakeStoreStatusHistogram(ENCODE_FAIL);
       list->Clear();
       return;
     }
     base::MD5Update(&ctx, encoded_log);
     list->Append(Value::CreateStringValue(encoded_log));
@@ skipping 73 matching lines @@
   if (!valid) {
     local_list->clear();
     return MakeRecallStatusHistogram(CHECKSUM_STRING_CORRUPTION, is_xml);
   }
   if (recovered_md5 != base::MD5DigestToBase16(digest)) {
     local_list->clear();
     return MakeRecallStatusHistogram(CHECKSUM_CORRUPTION, is_xml);
   }
   return MakeRecallStatusHistogram(RECALL_SUCCESS, is_xml);
 }