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, 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(
     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,
     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;
 
   // Store size at the beginning of the list.
   list->Append(Value::CreateIntegerValue(local_list.size() - start));
 
   base::MD5Context ctx;
   base::MD5Init(&ctx);
   std::string encoded_log;
@@ skipping 86 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);
 }