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

components/arc/metrics/arc_metrics_service.cc

 // Copyright 2016 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 "components/arc/metrics/arc_metrics_service.h"
 
 #include <string>
 #include <utility>
 
 #include "base/logging.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/strings/string_util.h"
 #include "chromeos/dbus/dbus_thread_manager.h"
 #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.

 const char kArcProcessNamePrefix[] = "org.chromium.arc.";
 const char kGmsProcessNamePrefix[] = "com.google.android.gms";
 const char kBootProgressEnableScreen[] = "boot_progress_enable_screen";
 
+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
 
-namespace arc {
-
 ArcMetricsService::ArcMetricsService(ArcBridgeService* bridge_service)
     : ArcService(bridge_service),
       binding_(this),
       process_observer_(this),
       weak_ptr_factory_(this) {
   arc_bridge_service()->metrics()->AddObserver(this);
   arc_bridge_service()->process()->AddObserver(&process_observer_);
 }
 
 ArcMetricsService::~ArcMetricsService() {
@@ skipping 92 matching lines @@
     mojom::MetricsHostPtr host_ptr;
     binding_.Bind(mojo::MakeRequest(&host_ptr));
     instance->Init(std::move(host_ptr));
   }
   arc_start_time_ = arc_start_time;
   VLOG(2) << "ARC start @" << arc_start_time_;
 }
 
 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;
+    }
   }
 }
 
 ArcMetricsService::ProcessObserver::ProcessObserver(
     ArcMetricsService* arc_metrics_service)
     : arc_metrics_service_(arc_metrics_service) {}
 
 ArcMetricsService::ProcessObserver::~ProcessObserver() = default;
 
 void ArcMetricsService::ProcessObserver::OnInstanceReady() {
   arc_metrics_service_->OnProcessInstanceReady();
 }
 
 void ArcMetricsService::ProcessObserver::OnInstanceClosed() {
   arc_metrics_service_->OnProcessInstanceClosed();
 }
 
 }  // namespace arc