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"
@@ skipping 12 matching lines @@
 // 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 must be reached
+// before the count-based limits above will be enforced. This ensures that a
+// reasonable amount of history will be stored even if there is a long series
+// of very small logs.
+const size_t kMinStorageBytesPerLogType = 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.
   COMPRESS_FAIL,    // Failed to compress log.
@@ skipping 44 matching lines @@
       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, max_store_count, kMinStorageBytesPerLogType,
+                      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, max_store_count, kMinStorageBytesPerLogType,
+                      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 list_length_limit,
+    size_t byte_limit,
     base::ListValue* list) {
+  // One of the limit arguments must be non-zero.
+  DCHECK(list_length_limit > 0 || byte_limit > 0);
+
   list->Clear();
+  if (local_list.size() == 0)
+    return;
+
   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)
-    return;
+  // If there are too many logs, keep the most recent logs up to the length
+  // limit, and at least to the minimum number of bytes.
+  if (local_list.size() > list_length_limit) {
+    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();
+      if (bytes_used >= byte_limit &&
+          (local_list.size() - start) >= list_length_limit)

[jar (doing other things), 2012/05/15 18:27:40]

You probably want ">" rather than ">=" now.

Currently, if list_length_limit == 0, then we could break on the first pass...
and get to line 170, and violate the DCHECK.

I guess this gets back to the meaning of "limit".  

The alternative (in my other comment) is to remove the DCHECK, and allow a call
where we sometimes choose to not persist any logs.  I think that is a very
plausible scenario on a low memory system anyway.

[stuartmorgan, 2012/05/15 19:02:58]

No, because it's an &&. To satisfy both conditions on the first pass, both
limits would have to be 0, and that's a DCHECK at the start of the method.
Since we always have at least one log to save, this would guarantee loss of
metrics on every single shutdown (or app termination on mobile). I'm having a
hard time imagining a device with so little persistant storage that we would
consider always throwing away metrics in normal use, so I'd rather not design
for it.

+        break;
+      bytes_used += log_size;
+      --start;
+    }
+  }
+  DCHECK_LT(start, local_list.size());
 
   // 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;
   for (std::vector<MetricsLogManager::SerializedLog>::const_iterator it =
            local_list.begin() + start;
        it != local_list.end(); ++it) {
@@ skipping 83 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);
 }