Add UMA metrics for the linux distro.

chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc

 // Copyright (c) 2013 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/chrome_browser_main_extra_parts_metrics.h"
 
 #include <string>
 
 #include "base/bind.h"
 #include "base/command_line.h"
 #include "base/cpu.h"
+#include "base/linux_util.h"

[Tom Anderson, 2017/05/23 22:12:13]

Guard this include with #if defined(OS_LINUX) && !defined(OS_CHROMEOS) since the
only code that uses it has those guards

[timbrown, 2017/05/24 00:40:55]

Ah, I missed those sections. Done.

 #include "base/macros.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/metrics/sparse_histogram.h"
 #include "base/sys_info.h"
 #include "base/threading/sequenced_worker_pool.h"
 #include "base/time/time.h"
 #include "build/build_config.h"
 #include "chrome/browser/about_flags.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/chrome_browser_main.h"
@@ skipping 31 matching lines @@
 
 #if defined(OS_WIN)
 #include "base/win/windows_version.h"
 #include "chrome/browser/metrics/chrome_metrics_service_accessor.h"
 #include "chrome/browser/shell_integration_win.h"
 #include "chrome/installer/util/google_update_settings.h"
 #endif  // defined(OS_WIN)
 
 namespace {
 
+enum UMALinuxDistro {

[Tom Anderson, 2017/05/23 22:12:13]

Please follow the practices here:
https://chromium.googlesource.com/chromium/src.git/+/HEAD/tools/metrics/histo...

[timbrown, 2017/05/24 00:40:55]

Done.

+  UMA_LINUX_DISTRO_UNKNOWN,
+  UMA_LINUX_DISTRO_UBUNTU_OTHER,
+  UMA_LINUX_DISTRO_UBUNTU_14_04,
+  UMA_LINUX_DISTRO_UBUNTU_16_04,
+  UMA_LINUX_DISTRO_UBUNTU_16_10,
+  UMA_LINUX_DISTRO_UBUNTU_17_04,
+  UMA_LINUX_DISTRO_DEBIAN_OTHER,
+  UMA_LINUX_DISTRO_DEBIAN_8,
+  UMA_LINUX_DISTRO_OPENSUSE_OTHER,
+  UMA_LINUX_DISTRO_OPENSUSE_LEAP_42_2,
+  UMA_LINUX_DISTRO_FEDORA_OTHER,
+  UMA_LINUX_DISTRO_FEDORA_24,
+  UMA_LINUX_DISTRO_FEDORA_25,
+};
+
 enum UMALinuxGlibcVersion {
   UMA_LINUX_GLIBC_NOT_PARSEABLE,
   UMA_LINUX_GLIBC_UNKNOWN,
   UMA_LINUX_GLIBC_2_11,
   // To log newer versions, just update tools/metrics/histograms/histograms.xml.
 };
 
 enum UMALinuxWindowManager {
   UMA_LINUX_WINDOW_MANAGER_OTHER,
   UMA_LINUX_WINDOW_MANAGER_BLACKBOX,
@@ skipping 92 matching lines @@
                             bluetooth_utility::BLUETOOTH_AVAILABILITY_COUNT);
 #endif   // defined(OS_MACOSX)
 
   // Record whether Chrome is the default browser or not.
   shell_integration::DefaultWebClientState default_state =
       shell_integration::GetDefaultBrowser();
   UMA_HISTOGRAM_ENUMERATION("DefaultBrowser.State", default_state,
                             shell_integration::NUM_DEFAULT_STATES);
 }
 
+void RecordLinuxDistro() {
+#if defined(OS_LINUX) && !defined(OS_CHROMEOS)
+  const std::string distro = base::GetLinuxDistro();

[Tom Anderson, 2017/05/23 22:12:13]

Does this call block?  (GetLinuxDistro caches its result.  Has it been called
before this call?)

[timbrown, 2017/05/24 00:40:55]

Good catch. GetLinuxDistro() is called previously, but only as part of a task.
So I guess there is no guarantee that it has completed (and cached) by this
point.

I have therefore made the call to RecordLinuxDistro() a task too.

Done.

+
+  UMALinuxDistro distro_result = UMA_LINUX_DISTRO_UNKNOWN;
+
+  const char ubuntu[] = "Ubuntu ";

[Tom Anderson, 2017/05/23 22:12:13]

constants should have a k prefix and should be named CamelCase:
eg: kDistroUbuntu

[timbrown, 2017/05/24 00:40:55]

Acknowledged.

+  const char openSuse[] = "openSUSE ";
+  const char debian[] = "Debian GNU/Linux ";
+  const char fedora[] = "Fedora release ";
+  if (distro.compare(0, strlen(ubuntu), ubuntu) == 0) {

[Tom Anderson, 2017/05/23 22:12:13]

I think it's better to use base::SplitString() on the whitespaces once before
the conditionals.  Then you can avoid substr, strlen, etc.  And you can do
direct string comparison using std::string::operator==

[timbrown, 2017/05/24 00:40:55]

Good idea. It's a little longer now with guards on the vector size etc, but I
think it's more readable. Done.

+    // Ubuntu uses point releases for the same version, so we need to do
+    // prefix matching.
+    distro_result = UMA_LINUX_DISTRO_UBUNTU_OTHER;
+    const std::string version = distro.substr(strlen(ubuntu), 5);
+    if (version == "14.04") {
+      distro_result = UMA_LINUX_DISTRO_UBUNTU_14_04;
+    } else if (version == "16.04") {
+      distro_result = UMA_LINUX_DISTRO_UBUNTU_16_04;
+    } else if (version == "16.10") {
+      distro_result = UMA_LINUX_DISTRO_UBUNTU_16_10;
+    } else if (version == "17.04") {
+      distro_result = UMA_LINUX_DISTRO_UBUNTU_17_04;
+    }
+  } else if (distro.compare(0, strlen(openSuse), openSuse) == 0) {
+    distro_result = UMA_LINUX_DISTRO_OPENSUSE_OTHER;
+    const std::string version = distro.substr(strlen(openSuse));
+    if (version == "Leap 42.2") {
+      distro_result = UMA_LINUX_DISTRO_OPENSUSE_LEAP_42_2;
+    }
+  } else if (distro.compare(0, strlen(debian), debian) == 0) {
+    // Debian uses point releases for the same version, so we need to do
+    // prefix matching.
+    distro_result = UMA_LINUX_DISTRO_DEBIAN_OTHER;
+    const std::string version = distro.substr(strlen(debian));
+    if (version[0] == '8') {
+      distro_result = UMA_LINUX_DISTRO_DEBIAN_8;
+    }
+  } else if (distro.compare(0, strlen(fedora), fedora) == 0) {
+    distro_result = UMA_LINUX_DISTRO_FEDORA_OTHER;
+    const std::string version = distro.substr(strlen(debian));
+    if (version == "24") {
+      distro_result = UMA_LINUX_DISTRO_FEDORA_24;
+    } else if (distro == "25") {
+      distro_result = UMA_LINUX_DISTRO_FEDORA_25;
+    }
+  }
+
+  UMA_HISTOGRAM_SPARSE_SLOWLY("Linux.Distro", distro_result);

[Tom Anderson, 2017/05/23 22:12:13]

UMA_HISTOGRAM_SPARSE_SLOWLY => UMA_HISTOGRAM_ENUMERATION

[timbrown, 2017/05/24 00:40:55]

rkaplow: Do you agree? From the comments here
https://cs.chromium.org/chromium/src/base/metrics/histogram_macros.h?&l=243 it
seemed like a reasonable choice, but the benefits and drawbacks are still not
really clear to me.

+#endif
+}
+
 void RecordLinuxGlibcVersion() {
 #if defined(OS_LINUX) && !defined(OS_CHROMEOS)
   base::Version version(gnu_get_libc_version());
 
   UMALinuxGlibcVersion glibc_version_result = UMA_LINUX_GLIBC_NOT_PARSEABLE;
   if (version.IsValid() && version.components().size() == 2) {
     glibc_version_result = UMA_LINUX_GLIBC_UNKNOWN;
     uint32_t glibc_major_version = version.components()[0];
     uint32_t glibc_minor_version = version.components()[1];
     if (glibc_major_version == 2) {
@@ skipping 185 matching lines @@
 #if defined(__clang__)
                                                             "Enabled"
 #else
                                                             "Disabled"
 #endif
                                                             );
 #endif
 }
 
 void ChromeBrowserMainExtraPartsMetrics::PostBrowserStart() {
+  RecordLinuxDistro();
   RecordLinuxGlibcVersion();
 #if defined(USE_X11) && !defined(OS_CHROMEOS)
   UMA_HISTOGRAM_ENUMERATION("Linux.WindowManager",
                             GetLinuxWindowManager(),
                             UMA_LINUX_WINDOW_MANAGER_COUNT);
 #endif
 
 #if defined(USE_OZONE) || defined(USE_X11)
   // The touch event state for X11 and Ozone based event sub-systems are based
   // on device scans that happen asynchronously. So we may need to attach an
@@ skipping 57 matching lines @@
   }
 }
 
 namespace chrome {
 
 void AddMetricsExtraParts(ChromeBrowserMainParts* main_parts) {
   main_parts->AddParts(new ChromeBrowserMainExtraPartsMetrics());
 }
 
 }  // namespace chrome