Add Android fingerprint to metrics logs

chrome/browser/metrics/metrics_log.cc

Index: chrome/browser/metrics/metrics_log.cc
diff --git a/chrome/browser/metrics/metrics_log.cc b/chrome/browser/metrics/metrics_log.cc
index 8025882c45940c551c5167d4ec958d705e3ed940..27e197fcb553cfb7c0b431213fca904106e06abf 100644
--- a/chrome/browser/metrics/metrics_log.cc
+++ b/chrome/browser/metrics/metrics_log.cc
@@ -45,6 +45,10 @@
 #include "ui/gfx/screen.h"
 #include "webkit/plugins/webplugininfo.h"
 
+#if defined(OS_ANDROID)
+#include "base/android/build_info.h"
+#endif
+
 #define OPEN_ELEMENT_FOR_SCOPE(name) ScopedElement scoped_element(this, name)
 
 #if defined(OS_WIN)
@@ -325,8 +329,10 @@ void MetricsLog::RecordIncrementalStabilityElements(
 
   OPEN_ELEMENT_FOR_SCOPE("profile");
   WriteCommonEventAttributes();
-
   WriteInstallElement();
+#if defined(OS_ANDROID)
+  WriteOSInfoElement();
+#endif

[Ilya Sherman, 2012/10/01 23:01:38]

I don't think it's appropriate to add the OS info as part of recording
incremental stability data.  For the protocol buffer uploads, this data should
already be included as part of RecordEnvironmentProto(), which is called
immediately before RecordIncrementalStabilityElements() in the MetricsService.

[gone, 2012/10/02 18:24:24]

With nilesh's comment as context, does your comment about RecordEnvironmentProto
still hold?  MetricsService::StopRecording() does seem to send the proto
immediately before the XML, meaning that we probably get the fingerprint twice
on m18...  It'd be nice to confirm this somehow, though.

[Ilya Sherman, 2012/10/02 21:37:45]

TL;DR: You can ignore my previous comment w.r.t. the XML uploads, as long as the
changes only affect Android.

To clarify:  There are no protocol buffer uploads in m18; just XML ones.  In
m21+, there are both XML and protocol buffer uploads, which upload more or less
the same data.  The XML uploading mechanism is deprecated, and will soon be
removed.  Adding a new field to the XML uploads requires server-side changes for
parsing the XML, which just isn't worth at this point.

So, there are two questions: (1) What, if anything, should we upstream for the
XML uploads?  (2) How should we add the fingerprints to the protocol buffer
uploads?

For (1), I don't have a strong preference for whether or not we upstream some
Android-only code; but we definitely shouldn't add any new XML fields (tags). 
If you want to upstream the changes currently in Android, that's fine, but in
that case please leave the fingerprint as part of the OS name.

[gone, 2012/10/02 23:10:00]

If that's the case then we really shouldn't be forking it on the Android side;
I'm removing the diff from our downstream master branch.  The diff in the m18
release branch isn't being upstreamed and will continue overriding the OS
version in the XML.

For (2) I'd still lean towards adding the fingerprint as a separate field in the
protobuf definition.  Apparently the only thing we need to change after adding
the new field is a dremel query or two.

 
   {
     OPEN_ELEMENT_FOR_SCOPE("stability");  // Minimal set of stability elements.
@@ -648,7 +654,6 @@ void MetricsLog::RecordEnvironment(
 
   OPEN_ELEMENT_FOR_SCOPE("profile");
   WriteCommonEventAttributes();
-
   WriteInstallElement();
 
   // Write the XML version.
@@ -675,13 +680,7 @@ void MetricsLog::RecordEnvironment(
 #endif
   }
 
-  {
-    // Write the XML version.
-    // We'll write the protobuf version in RecordEnvironmentProto().
-    OPEN_ELEMENT_FOR_SCOPE("os");
-    WriteAttribute("name", base::SysInfo::OperatingSystemName());
-    WriteAttribute("version", base::SysInfo::OperatingSystemVersion());
-  }
+  WriteOSInfoElement();
 
   {
     OPEN_ELEMENT_FOR_SCOPE("gpu");
@@ -776,6 +775,10 @@ void MetricsLog::RecordEnvironmentProto(
 #endif
   os->set_name(os_name);
   os->set_version(base::SysInfo::OperatingSystemVersion());
+#if defined(OS_ANDROID)
+  os->set_fingerprint(
+      base::android::BuildInfo::GetInstance()->android_build_fp());
+#endif
 
   const content::GPUInfo& gpu_info =
       GpuDataManager::GetInstance()->GetGPUInfo();
@@ -894,6 +897,17 @@ void MetricsLog::WriteProfileMetrics(const std::string& profileidhash,
   }
 }
 
+void MetricsLog::WriteOSInfoElement() {
+  // Write the XML version.

[Ilya Sherman, 2012/10/01 23:01:38]

As I mentioned above, I don't think you should move this code.  However, if you
do, you should document why this method only writes the XML version, and
probably name it accordingly.

[gone, 2012/10/02 23:45:10]

Undid the change.  We'll go with the protobuf on master.

+  OPEN_ELEMENT_FOR_SCOPE("os");
+  WriteAttribute("name", base::SysInfo::OperatingSystemName());
+  WriteAttribute("version", base::SysInfo::OperatingSystemVersion());
+#if defined(OS_ANDROID)
+  WriteAttribute("fingerprint",
+      base::android::BuildInfo::GetInstance()->android_build_fp());

[Ilya Sherman, 2012/10/01 23:01:38]

I don't recommend trying to add this field to the XML-based uploads, as that
pipeline is deprecated.  The only reason to add this field to the XML uploads is
if you need it like yesterday for M18-based Android builds... but then there's
not much point to upstreaming the change to the main chrome/ repository.

[gone, 2012/10/02 23:45:10]

Removed.

+#endif
+}
+
 void MetricsLog::RecordOmniboxOpenedURL(const AutocompleteLog& log) {
   DCHECK(!locked());