ARC: Add BootType parameter to ReportBootProgress() in metrics.mojom

components/arc/metrics/arc_metrics_service.cc

Index: components/arc/metrics/arc_metrics_service.cc
diff --git a/components/arc/metrics/arc_metrics_service.cc b/components/arc/metrics/arc_metrics_service.cc
index 2f391c598382e16dd158f43f3196667d7627bd53..d9b4969ff47d7ae06444c98b6c52173abc1e968d 100644
--- a/components/arc/metrics/arc_metrics_service.cc
+++ b/components/arc/metrics/arc_metrics_service.cc
@@ -14,6 +14,15 @@
 #include "chromeos/dbus/session_manager_client.h"
 #include "components/arc/arc_bridge_service.h"
 
+#define RECORD_ARC_UMA(suffix, elapsed_time, max_seconds)                      \
+  do {                                                                         \
+    UMA_HISTOGRAM_CUSTOM_TIMES("Arc.AndroidBootTime" suffix, (elapsed_time),   \

[cywang, 2017/05/25 04:26:06]

You seem to use "UNKNOWN" for backward compatibility, isn't it more clear to
have  "Arc.AndroidBootTime2" instead to prevent from mixing up the counts of old
and new boot times in the future?

[Yusuke Sato, 2017/05/25 05:57:07]

So the current idea is to keep "Arc.AndroidBootTime" for now for backward
compatibility purposes, and also introduce "Arc.AndroidBootTime.<SUFFIX>".

It does the same for Arc.boot_progesss* too. For example, this keeps
"Arc.boot_progesss_enable_screen" and introduces
"Arc.boot_progesss_enable_screen.<SUFFIX>".

Adding "2" is fine for the former, but I'm not really sure where to insert "2"
for the latter. Inserting "2" might make this code (and the xml) more
complicated actually...

As we chatted offline, let me ask the xml owner about this first.

+                               base::TimeDelta::FromMilliseconds(1),           \
+                               base::TimeDelta::FromSeconds(max_seconds), 50); \
+  } while (false)
+
+namespace arc {
+
 namespace {
 
 const int kRequestProcessListPeriodInMinutes = 5;

[Luis Héctor Chávez, 2017/05/25 16:00:35]

not your fault, but can these be constexpr? Can this in particular be a
base::TimeDelta?

[Yusuke Sato, 2017/05/25 18:21:13]

Done.

@@ -21,9 +30,22 @@ const char kArcProcessNamePrefix[] = "org.chromium.arc.";
 const char kGmsProcessNamePrefix[] = "com.google.android.gms";
 const char kBootProgressEnableScreen[] = "boot_progress_enable_screen";
 
-}  // namespace
+std::string BootTypeToString(mojom::BootType boot_type) {
+  switch (boot_type) {
+    case mojom::BootType::UNKNOWN:
+      return "";  // for backward compatibility.
+    case mojom::BootType::FIRST_BOOT:
+      return ".FIRST_BOOT";
+    case mojom::BootType::FIRST_BOOT_AFTER_UPDATE:
+      return ".FIRST_BOOT_AFTER_UPDATE";
+    case mojom::BootType::REGULAR_BOOT:
+      return ".REGULAR_BOOT";
+  }
+  NOTREACHED();
+  return "";
+}
 
-namespace arc {
+}  // namespace
 
 ArcMetricsService::ArcMetricsService(ArcBridgeService* bridge_service)
     : ArcService(bridge_service),
@@ -137,25 +159,50 @@ void ArcMetricsService::OnArcStartTimeRetrieved(
 
 void ArcMetricsService::ReportBootProgress(
     std::vector<mojom::BootProgressEventPtr> events) {
+  // TODO(yusukes): Replace this with NOTREACHED().
+  ReportBootProgressWithType(std::move(events), mojom::BootType::UNKNOWN);
+}
+
+void ArcMetricsService::ReportBootProgressWithType(
+    std::vector<mojom::BootProgressEventPtr> events,
+    mojom::BootType boot_type) {
   DCHECK(CalledOnValidThread());
+  // TODO(yusukes): Return immediately with with LOG(ERROR) when |boot_type| is
+  // UNKNOWN. Once the container is updated, we'll never see the boot type.
   int64_t arc_start_time_in_ms =
       (arc_start_time_ - base::TimeTicks()).InMilliseconds();
   for (const auto& event : events) {
     VLOG(2) << "Report boot progress event:" << event->event << "@"
             << event->uptimeMillis;
-    std::string title = "Arc." + event->event;
+    // TODO(yusukes): Always use 60.
+    const int max_seconds = (boot_type == mojom::BootType::UNKNOWN) ? 30 : 60;
+    std::string title = "Arc." + event->event + BootTypeToString(boot_type);
     base::TimeDelta elapsed_time = base::TimeDelta::FromMilliseconds(
         event->uptimeMillis - arc_start_time_in_ms);
     // Note: This leaks memory, which is expected behavior.
     base::HistogramBase* histogram = base::Histogram::FactoryTimeGet(
         title, base::TimeDelta::FromMilliseconds(1),
-        base::TimeDelta::FromSeconds(30), 50,
+        base::TimeDelta::FromSeconds(max_seconds), 50,
         base::HistogramBase::kUmaTargetedHistogramFlag);
     histogram->AddTime(elapsed_time);
-    if (event->event.compare(kBootProgressEnableScreen) == 0)
-      UMA_HISTOGRAM_CUSTOM_TIMES("Arc.AndroidBootTime", elapsed_time,
-                                 base::TimeDelta::FromMilliseconds(1),
-                                 base::TimeDelta::FromSeconds(30), 50);
+    if (event->event.compare(kBootProgressEnableScreen) != 0)
+      continue;
+    switch (boot_type) {

[cywang, 2017/05/25 04:26:06]

std::string name = "Arc.AndroidBootTime2" + BootTypeToString(boot_type);
UMA_HISTOGRAM_CUSTOM_TIMES(name, elapsed_time,
                           base::TimeDelta::FromMilliseconds(1),
                           base::TimeDelta::FromSeconds(max_seconds), 50);

[Yusuke Sato, 2017/05/25 05:37:56]

IIUC, the macro UMA_HISTOGRAM_CUSTOM_TIMES requires a string literal. I checked
the existing code with git grep, and it seemed like all
UMA_HISTOGRAM_CUSTOM_TIMES invocations were with string literals.

+      case mojom::BootType::UNKNOWN:
+        // For backward compatibility, use "".
+        // TODO(yusukes): Replace this with NOTREACHED().
+        RECORD_ARC_UMA("", elapsed_time, max_seconds);
+        break;
+      case mojom::BootType::FIRST_BOOT:
+        RECORD_ARC_UMA(".FIRST_BOOT", elapsed_time, max_seconds);
+        break;
+      case mojom::BootType::FIRST_BOOT_AFTER_UPDATE:
+        RECORD_ARC_UMA(".FIRST_BOOT_AFTER_UPDATE", elapsed_time, max_seconds);
+        break;
+      case mojom::BootType::REGULAR_BOOT:
+        RECORD_ARC_UMA(".REGULAR_BOOT", elapsed_time, max_seconds);
+        break;
+    }
   }
 }