Upload UMA data using protocol buffers.

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 bbddf601eb77710ae0e2ea2fae460a83c747425f..da375fe6730f50a97d859a546c9186c804bb9943 100644
--- a/chrome/browser/metrics/metrics_log_serializer.cc
+++ b/chrome/browser/metrics/metrics_log_serializer.cc
@@ -4,6 +4,8 @@
 
 #include "chrome/browser/metrics/metrics_log_serializer.h"
 
+#include <utility>
+
 #include "base/base64.h"
 #include "base/md5.h"
 #include "base/metrics/histogram.h"
@@ -12,10 +14,12 @@
 #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.
-static const size_t kMaxInitialLogsPersisted = 20;
+const size_t kMaxInitialLogsPersisted = 20;

[jar (doing other things), 2012/02/23 01:59:18]

We should really have a style guide opinion for using anonymous namespaces, vs
static, when declaring variables.

IMO, the static keyword is VERY helpful in locating static state variables, and
is probably preferable even if we use an anonymous namespace (In contrast,
anonymous namespaces are required for keeping classes from being advertised
globally, and hence very useful in that case).

Do you know of a style guide request to the contrary?

[ian fette, 2012/02/23 02:08:43]

"Unnamed namespaces are allowed and even encouraged in .cc files, to avoid
runtime naming conflicts:" according to the google style guide...

[jar (doing other things), 2012/02/23 03:03:06]

You missed my point (or perhaps I stated it poorly):

I'd like to see more explicit use of static linkage.

I don't *mind* using an anonymous namespace, and in fact, it is necessary for
classes (for which "static" linkage cannot be specified!).



On 2012/02/23 02:08:43, ian fette wrote:

[Ilya Sherman, 2012/02/24 02:10:06]

I think the relevant style guide rule is this one: [
http://www.chromium.org/developers/coding-style#TOC-Unnamed-Namespaces ].  That
said, this is mostly a rule that I've picked up implicitly from reviewers of my
code: Since we have to use anonymous namespaces to force static linkage for
classes, we might as well be consistent and use them in all cases where we'd
like to force static linkage.  (This is assuming that I understand correctly,
and that declaring a constant in an anonymous namespace effectively forces
static linkage for it.)

 
 // 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
@@ -26,15 +30,14 @@ static const size_t kMaxInitialLogsPersisted = 20;
 // 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).
-static const size_t kMaxOngoingLogsPersisted = 8;
+const size_t kMaxOngoingLogsPersisted = 8;
 
 // We append (2) more elements to persisted lists: the size of the list and a
 // checksum of the elements.
-static const size_t kChecksumEntryCount = 2;
+const size_t kChecksumEntryCount = 2;
 
-namespace {
-// TODO(ziadh): This is here temporarily for a side experiment. Remove later
-// on.
+// 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.
@@ -43,17 +46,23 @@ enum LogStoreStatus {
 };
 
 MetricsLogSerializer::LogReadStatus MakeRecallStatusHistogram(
-    MetricsLogSerializer::LogReadStatus status) {
-  UMA_HISTOGRAM_ENUMERATION("PrefService.PersistentLogRecall", status,
-                            MetricsLogSerializer::END_RECALL_STATUS);
+    MetricsLogSerializer::LogReadStatus status,
+    bool is_xml) {
+  if (is_xml) {
+    UMA_HISTOGRAM_ENUMERATION("PrefService.PersistentLogRecall",
+                              status, MetricsLogSerializer::END_RECALL_STATUS);
+  } else {
+    UMA_HISTOGRAM_ENUMERATION("PrefService.PersistentLogRecallProtobufs",
+                              status, MetricsLogSerializer::END_RECALL_STATUS);
+  }
   return status;
 }
 
-// TODO(ziadh): Remove this when done with experiment.
 void MakeStoreStatusHistogram(LogStoreStatus status) {
   UMA_HISTOGRAM_ENUMERATION("PrefService.PersistentLogStore2", status,
                             END_STORE_STATUS);
 }
+
 }  // namespace
 
 
@@ -61,40 +70,84 @@ MetricsLogSerializer::MetricsLogSerializer() {}
 
 MetricsLogSerializer::~MetricsLogSerializer() {}
 
-void MetricsLogSerializer::SerializeLogs(const std::vector<std::string>& logs,
-                                         MetricsLogManager::LogType log_type) {
+void MetricsLogSerializer::SerializeLogs(
+    const std::vector<std::pair<std::string, std::string> >& logs,

[jar (doing other things), 2012/02/23 01:59:18]

This makes it clearer (reading the code) what the pairs of strings are.  Each
one is a serialized log.  The first is the xml.  The second is the protobuf
style.  

Pairs are bad for this (compared to a simple class with two named slots),
because that mapping needs to be held in your head, and is easy to forget (hard
to read).

This is almost throw-away code (soon(?) to transition to protobuf only), so I'm
guessing that is why you wrote it this way.  I need to read it and see how it
comes out.

Perhaps this would be a lot cleaner if you had two arrays, rather than array of
pairs.  You would then have the risk that the pairs of arrays would get
misaligned... but you should have that anyway since either attempted upload may
independently fail.  Hence, the number of logs that need sending from each list
should probably be different (after servers reject an upload).

+    MetricsLogManager::LogType log_type) {
   PrefService* local_state = g_browser_process->local_state();
   DCHECK(local_state);
-  const char* pref = NULL;
+  const char* pref_xml = NULL;
+  const char* pref_proto = NULL;
   size_t max_store_count = 0;
   switch (log_type) {
     case MetricsLogManager::INITIAL_LOG:
-      pref = prefs::kMetricsInitialLogs;
+      pref_xml = prefs::kMetricsInitialLogsXml;
+      pref_proto = prefs::kMetricsInitialLogsProto;
       max_store_count = kMaxInitialLogsPersisted;
       break;
     case MetricsLogManager::ONGOING_LOG:
-      pref = prefs::kMetricsOngoingLogs;
+      pref_xml = prefs::kMetricsOngoingLogsXml;
+      pref_proto = prefs::kMetricsOngoingLogsProto;
       max_store_count = kMaxOngoingLogsPersisted;
       break;
     default:
       NOTREACHED();
       return;
   };
-  ListPrefUpdate update(local_state, pref);
-  ListValue* pref_list = update.Get();
-  WriteLogsToPrefList(logs, max_store_count, pref_list);
+
+  std::vector<std::string> logs_xml(logs.size());
+  std::vector<std::string> logs_proto(logs.size());
+  for (size_t i = 0; i < logs.size(); ++i) {
+    logs_xml[i] = logs[i].first;

[jar (doing other things), 2012/02/23 01:59:18]

These logs get to be big.... so this copying of strings will be expensive, and
perchance undesirable.

Why don't you just keep these separate, and get to avoid this step?

[ian fette, 2012/02/23 02:08:43]

std::strings are copy on write. Creating this sort of copy is cheap IIRC...

[ian fette, 2012/02/23 03:02:53]

actually, rsleevi reminds me that after GCC 4.6, std::string is no longer COW...
so yeah, this could be bad...

[jar (doing other things), 2012/02/23 03:03:06]

As noted by Sleevi, this copy-on-write semantic (rendering strings thread-unsafe
once copied) was peculiar to an older version of GCC, and never supported by
other vendors, such as MS and MSVC.

These strings are very large... and we need to handle them efficiently.

On 2012/02/23 02:08:43, ian fette wrote:

[Ryan Sleevi, 2012/02/23 03:28:06]

The relevant item from C++11 that I mentioned was
http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2008/n2668.htm

libc++ implements strings as CoW, as ian notes, but has moved away from it, and
C++11 explicitly forbids it. STLport (Android) and Dinkumware (MSVC) never
supported CoW. Clang's libc++ (to avoid the LGPL of libstdc++ from GCC) has one
of it's main bullet points that it uses the short string optimization rather
than CoW.

The Sutter reference I mentioned is
http://www.gotw.ca/publications/optimizations.htm

[Ilya Sherman, 2012/02/24 02:10:06]

No longer copying the strings needlessly.

+    logs_proto[i] = logs[i].second;
+  }
+
+  // Write the XML version.
+  ListPrefUpdate update_xml(local_state, pref_xml);
+  ListValue* pref_list_xml = update_xml.Get();
+  WriteLogsToPrefList(logs_xml, max_store_count, pref_list_xml);
+
+  // Write the protobuf version.
+  ListPrefUpdate update_proto(local_state, pref_proto);
+  ListValue* pref_list_proto = update_proto.Get();
+  WriteLogsToPrefList(logs_proto, max_store_count, pref_list_proto);
 }
 
-void MetricsLogSerializer::DeserializeLogs(MetricsLogManager::LogType log_type,
-                                           std::vector<std::string>* logs) {
+void MetricsLogSerializer::DeserializeLogs(
+    MetricsLogManager::LogType log_type,
+    std::vector<std::pair<std::string, std::string> >* logs) {
   DCHECK(logs);
   PrefService* local_state = g_browser_process->local_state();
   DCHECK(local_state);
 
-  const char* pref = (log_type == MetricsLogManager::INITIAL_LOG) ?
-      prefs::kMetricsInitialLogs : prefs::kMetricsOngoingLogs;
-  const ListValue* unsent_logs = local_state->GetList(pref);
-  ReadLogsFromPrefList(*unsent_logs, logs);
+  const char* pref_xml;
+  const char* pref_proto;
+  if (log_type == MetricsLogManager::INITIAL_LOG) {
+    pref_xml = prefs::kMetricsInitialLogsXml;
+    pref_proto = prefs::kMetricsInitialLogsProto;
+  } else {
+    pref_xml = prefs::kMetricsOngoingLogsXml;
+    pref_proto = prefs::kMetricsOngoingLogsProto;
+  }
+
+  const ListValue* unsent_logs_xml = local_state->GetList(pref_xml);
+  const ListValue* unsent_logs_proto = local_state->GetList(pref_proto);
+  std::vector<std::string> logs_xml;
+  std::vector<std::string> logs_proto;
+  if (ReadLogsFromPrefList(*unsent_logs_xml, true, &logs_xml) != RECALL_SUCCESS)
+    return;
+  if (ReadLogsFromPrefList(*unsent_logs_proto, false, &logs_proto) !=
+          RECALL_SUCCESS)
+    return;
+
+  if (logs_xml.size() != logs_proto.size()) {
+    MakeRecallStatusHistogram(XML_PROTO_MISMATCH, false);
+    return;
+  }
+
+  logs->resize(logs_xml.size());
+  for (size_t i = 0; i < logs->size(); ++i) {
+    (*logs)[i] = std::make_pair(logs_xml[i], logs_proto[i]);
+  }
 }
 
 // static
@@ -106,7 +159,7 @@ void MetricsLogSerializer::WriteLogsToPrefList(
   size_t start = 0;
   if (local_list.size() > kMaxLocalListSize)
     start = local_list.size() - kMaxLocalListSize;
-  DCHECK(start <= local_list.size());
+  DCHECK_LE(start, local_list.size());
   if (local_list.size() <= start)
     return;
 
@@ -140,23 +193,24 @@ void MetricsLogSerializer::WriteLogsToPrefList(
 // static
 MetricsLogSerializer::LogReadStatus MetricsLogSerializer::ReadLogsFromPrefList(
     const ListValue& list,
+    bool is_xml,
     std::vector<std::string>* local_list) {
   DCHECK(local_list->empty());
   if (list.GetSize() == 0)
-    return MakeRecallStatusHistogram(LIST_EMPTY);
+    return MakeRecallStatusHistogram(LIST_EMPTY, is_xml);
   if (list.GetSize() < 3)
-    return MakeRecallStatusHistogram(LIST_SIZE_TOO_SMALL);
+    return MakeRecallStatusHistogram(LIST_SIZE_TOO_SMALL, is_xml);
 
   // The size is stored at the beginning of the list.
   int size;
   bool valid = (*list.begin())->GetAsInteger(&size);
   if (!valid)
-    return MakeRecallStatusHistogram(LIST_SIZE_MISSING);
+    return MakeRecallStatusHistogram(LIST_SIZE_MISSING, is_xml);
 
   // Account for checksum and size included in the list.
   if (static_cast<unsigned int>(size) !=
       list.GetSize() - kChecksumEntryCount) {
-    return MakeRecallStatusHistogram(LIST_SIZE_CORRUPTION);
+    return MakeRecallStatusHistogram(LIST_SIZE_CORRUPTION, is_xml);
   }
 
   base::MD5Context ctx;
@@ -168,14 +222,14 @@ MetricsLogSerializer::LogReadStatus MetricsLogSerializer::ReadLogsFromPrefList(
     valid = (*it)->GetAsString(&encoded_log);
     if (!valid) {
       local_list->clear();
-      return MakeRecallStatusHistogram(LOG_STRING_CORRUPTION);
+      return MakeRecallStatusHistogram(LOG_STRING_CORRUPTION, is_xml);
     }
 
     base::MD5Update(&ctx, encoded_log);
 
     if (!base::Base64Decode(encoded_log, &decoded_log)) {
       local_list->clear();
-      return MakeRecallStatusHistogram(DECODE_FAIL);
+      return MakeRecallStatusHistogram(DECODE_FAIL, is_xml);
     }
     local_list->push_back(decoded_log);
   }
@@ -188,11 +242,11 @@ MetricsLogSerializer::LogReadStatus MetricsLogSerializer::ReadLogsFromPrefList(
   valid = (*(list.end() - 1))->GetAsString(&recovered_md5);
   if (!valid) {
     local_list->clear();
-    return MakeRecallStatusHistogram(CHECKSUM_STRING_CORRUPTION);
+    return MakeRecallStatusHistogram(CHECKSUM_STRING_CORRUPTION, is_xml);
   }
   if (recovered_md5 != base::MD5DigestToBase16(digest)) {
     local_list->clear();
-    return MakeRecallStatusHistogram(CHECKSUM_CORRUPTION);
+    return MakeRecallStatusHistogram(CHECKSUM_CORRUPTION, is_xml);
   }
-  return MakeRecallStatusHistogram(RECALL_SUCCESS);
+  return MakeRecallStatusHistogram(RECALL_SUCCESS, is_xml);
 }