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_functions.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"
 
+namespace arc {
+
 namespace {
 
-const int kRequestProcessListPeriodInMinutes = 5;
-const char kArcProcessNamePrefix[] = "org.chromium.arc.";
-const char kGmsProcessNamePrefix[] = "com.google.android.gms";
-const char kBootProgressEnableScreen[] = "boot_progress_enable_screen";
+constexpr base::TimeDelta kUmaMinTime = base::TimeDelta::FromMilliseconds(1);
+constexpr int kUmaNumBuckets = 50;
+
+constexpr base::TimeDelta kRequestProcessListPeriod =
+    base::TimeDelta::FromMinutes(5);
+constexpr char kArcProcessNamePrefix[] = "org.chromium.arc.";
+constexpr char kGmsProcessNamePrefix[] = "com.google.android.gms";
+constexpr 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 ".FirstBoot";
+    case mojom::BootType::FIRST_BOOT_AFTER_UPDATE:
+      return ".FirstBootAfterUpdate";
+    case mojom::BootType::REGULAR_BOOT:
+      return ".RegularBoot";
+  }
+  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 19 matching lines @@
 
 void ArcMetricsService::OnInstanceClosed() {
   VLOG(2) << "Close metrics service.";
   DCHECK(CalledOnValidThread());
   if (binding_.is_bound())
     binding_.Unbind();
 }
 
 void ArcMetricsService::OnProcessInstanceReady() {
   VLOG(2) << "Start updating process list.";
-  timer_.Start(FROM_HERE,
-               base::TimeDelta::FromMinutes(kRequestProcessListPeriodInMinutes),
-               this, &ArcMetricsService::RequestProcessList);
+  timer_.Start(FROM_HERE, kRequestProcessListPeriod, this,
+               &ArcMetricsService::RequestProcessList);
 }
 
 void ArcMetricsService::OnProcessInstanceClosed() {
   VLOG(2) << "Stop updating process list.";
   timer_.Stop();
 }
 
 void ArcMetricsService::RequestProcessList() {
   mojom::ProcessInstance* process_instance = ARC_GET_INSTANCE_FOR_METHOD(
       arc_bridge_service()->process(), RequestProcessList);
@@ skipping 49 matching lines @@
   if (!binding_.is_bound()) {
     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) {
+    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;
-    base::TimeDelta elapsed_time = base::TimeDelta::FromMilliseconds(
+    const std::string name =
+        "Arc." + event->event + BootTypeToString(boot_type);

[dcheng, 2017/05/26 20:53:45]

It would be nice not to build the temporary BootTypeToString() object twice.

[Yusuke Sato, 2017/05/26 21:34:53]

Done.

+    const 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::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);
+    // TODO(yusukes): Define kUmaMaxTime and always use 60s.
+    const base::TimeDelta max_time = base::TimeDelta::FromSeconds(
+        boot_type == mojom::BootType::UNKNOWN ? 30 : 60);
+    base::UmaHistogramCustomTimes(name, elapsed_time, kUmaMinTime, max_time,

[dcheng, 2017/05/26 20:53:46]

FWIW, there is a macro to help with groups of UMAs like this, but I don't know
if it's better or worse: see STATIC_HISTOGRAM_POINTER_GROUP

[Yusuke Sato, 2017/05/26 21:34:53]

Thanks for the pointer, but for this CL, let me follow the recommendation from
Ilya.

[Ilya Sherman, 2017/05/26 21:56:31]

I'd actually forgotten that macro exists =P  My take on it is that it adds a lot
of mental overhead for a small amount of performance gain (by caching the
histogram pointers).  I'd only recommend it over one of the options in
//base/metrics/histogram_functions.h for unusually performance-sensitive code.

+                                  kUmaNumBuckets);
+    if (event->event.compare(kBootProgressEnableScreen) == 0) {
+      base::UmaHistogramCustomTimes(
+          "Arc.AndroidBootTime" + BootTypeToString(boot_type), elapsed_time,
+          kUmaMinTime, max_time, kUmaNumBuckets);
+    }
   }
 }
 
 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