[fuchsia] Get histograms from child processes.

Collects histograms from child processes (GPU, renderer, etc.) before
sending metrics over the fuchsia.legacymetrics.MetricsRecorder
interface.

Implement upload batching, to break up large report calls into
smaller chunks that fit within the FIDL message limit.

Make the AdditionalMetricsCallback interface asynchronous, so that
async data collection may take place.

Bug: 1060768
Change-Id: I4d4ae5f168ecbeb65a8d9745e940fb05f68e2399
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2147683
Commit-Queue: Kevin Marshall <kmarshall@chromium.org>
Reviewed-by: Sergey Ulanov <sergeyu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#759033}

fuchsia/base/legacymetrics_client.cc

@@ -6,6 +6,7 @@
 
 #include <lib/fit/function.h>
 #include <lib/sys/cpp/component_context.h>
+#include <algorithm>
 #include <memory>
 #include <utility>
 #include <vector>
@@ -19,6 +20,8 @@
 
 namespace cr_fuchsia {
 
+constexpr size_t LegacyMetricsClient::kMaxBatchSize;
+
 LegacyMetricsClient::LegacyMetricsClient() = default;
 
 LegacyMetricsClient::~LegacyMetricsClient() {
@@ -54,18 +57,23 @@ void LegacyMetricsClient::SetReportAdditionalMetricsCallback(
 void LegacyMetricsClient::ScheduleNextReport() {
   DVLOG(1) << "Scheduling next report in " << report_interval_.InSeconds()
            << "seconds.";
-  timer_.Start(FROM_HERE, report_interval_, this, &LegacyMetricsClient::Report);
+  timer_.Start(FROM_HERE, report_interval_, this,
+               &LegacyMetricsClient::StartReport);
 }
 
-void LegacyMetricsClient::Report() {
-  DCHECK(metrics_recorder_);
-  DVLOG(1) << __func__ << "called.";
-
-  std::vector<fuchsia::legacymetrics::Event> events;
+void LegacyMetricsClient::StartReport() {
+  if (!report_additional_callback_) {
+    Report({});
+    return;
+  }
+  report_additional_callback_.Run(
+      base::BindOnce(&LegacyMetricsClient::Report, weak_factory_.GetWeakPtr()));
+}
 
-  // Add events from the additional metrics callback, if set.
-  if (report_additional_callback_)
-    report_additional_callback_.Run(&events);
+void LegacyMetricsClient::Report(
+    std::vector<fuchsia::legacymetrics::Event> events) {
+  DCHECK(metrics_recorder_);
+  DVLOG(1) << __func__ << " called.";
 
   // Include histograms.
   for (auto& histogram : GetLegacyMetricsDeltas()) {
@@ -88,10 +96,30 @@ void LegacyMetricsClient::Report() {
     return;
   }
 
-  metrics_recorder_->Record(std::move(events), [this]() {
-    VLOG(1) << "Report finished.";
+  DrainBuffer(std::move(events));
+}
+
+void LegacyMetricsClient::DrainBuffer(
+    std::vector<fuchsia::legacymetrics::Event> buffer) {
+  if (buffer.empty()) {
+    DVLOG(1) << "Buffer drained.";
     ScheduleNextReport();
-  });
+    return;
+  }
+
+  // Since ordering doesn't matter, we can efficiently drain |buffer| by
+  // repeatedly sending and truncating its tail.
+  const size_t batch_size = std::min(buffer.size(), kMaxBatchSize);
+  const size_t batch_start_idx = buffer.size() - batch_size;
+  std::vector<fuchsia::legacymetrics::Event> batch;
+  batch.resize(batch_size);
+  std::move(buffer.begin() + batch_start_idx, buffer.end(), batch.begin());
+  buffer.resize(buffer.size() - batch_size);
+
+  metrics_recorder_->Record(std::move(batch),
+                            [this, buffer = std::move(buffer)]() mutable {
+                              DrainBuffer(std::move(buffer));
+                            });
 }
 
 void LegacyMetricsClient::OnMetricsRecorderDisconnected(zx_status_t status) {

fuchsia/base/legacymetrics_client.h

@@ -25,11 +25,14 @@ namespace cr_fuchsia {
 // Must be constructed, used, and destroyed on the same sequence.
 class LegacyMetricsClient {
  public:
+  // Maximum number of Events to send to Record() at a time, so as to not exceed
+  // the 64KB FIDL maximum message size.
+  static constexpr size_t kMaxBatchSize = 50;
+
   using ReportAdditionalMetricsCallback = base::RepeatingCallback<void(
-      std::vector<fuchsia::legacymetrics::Event>*)>;
+      base::OnceCallback<void(std::vector<fuchsia::legacymetrics::Event>)>)>;
 
   LegacyMetricsClient();
-
   ~LegacyMetricsClient();
 
   explicit LegacyMetricsClient(const LegacyMetricsClient&) = delete;
@@ -39,17 +42,24 @@ class LegacyMetricsClient {
   // |report_interval|.
   void Start(base::TimeDelta report_interval);
 
-  // Sets a |callback| to be invoked just prior to reporting, allowing users to
-  // report additional custom metrics.
-  // Must be called before Start().
+  // Sets an asynchronous |callback| to be invoked just prior to reporting,
+  // allowing users to asynchronously gather and provide additional custom
+  // metrics. |callback| will receive the list of metrics when they are ready.
+  // Reporting is paused until |callback| is fulfilled.
+  // If used, then this method must be called before calling Start().
   void SetReportAdditionalMetricsCallback(
       ReportAdditionalMetricsCallback callback);
 
  private:
   void ScheduleNextReport();
-  void Report();
+  void StartReport();
+  void Report(std::vector<fuchsia::legacymetrics::Event> additional_metrics);
   void OnMetricsRecorderDisconnected(zx_status_t status);
 
+  // Incrementally sends the contents of |buffer| to |metrics_recorder|, and
+  // invokes |done_cb| when finished.
+  void DrainBuffer(std::vector<fuchsia::legacymetrics::Event> buffer);
+
   base::TimeDelta report_interval_;
   ReportAdditionalMetricsCallback report_additional_callback_;
   std::unique_ptr<LegacyMetricsUserActionRecorder> user_events_recorder_;
@@ -57,6 +67,10 @@ class LegacyMetricsClient {
   fuchsia::legacymetrics::MetricsRecorderPtr metrics_recorder_;
   base::RetainingOneShotTimer timer_;
   SEQUENCE_CHECKER(sequence_checker_);
+
+  // Prevents use-after-free if |report_additional_callback_| is invoked after
+  // |this| is destroyed.
+  base::WeakPtrFactory<LegacyMetricsClient> weak_factory_{this};
 };
 
 }  // namespace cr_fuchsia

fuchsia/base/legacymetrics_client_unittest.cc

@@ -102,14 +102,17 @@ TEST_F(LegacyMetricsClientTest, ReportIntervalBoundary) {
 }
 
 void PopulateAdditionalEvents(
-    std::vector<fuchsia::legacymetrics::Event>* events) {
+    base::OnceCallback<void(std::vector<fuchsia::legacymetrics::Event>)>
+        callback) {
   fuchsia::legacymetrics::ImplementationDefinedEvent impl_event;
   impl_event.set_name("baz");
 
   fuchsia::legacymetrics::Event event;
   event.set_impl_defined_event(std::move(impl_event));
 
-  events->emplace_back(std::move(event));
+  std::vector<fuchsia::legacymetrics::Event> events;
+  events.push_back(std::move(event));
+  std::move(callback).Run(std::move(events));
 }
 
 TEST_F(LegacyMetricsClientTest, AllTypes) {
@@ -184,5 +187,34 @@ TEST_F(LegacyMetricsClientTest, MetricsChannelDisconnected) {
   task_environment_.FastForwardBy(kReportInterval);
 }
 
+TEST_F(LegacyMetricsClientTest, Batching) {
+  client_.Start(kReportInterval);
+
+  // Log enough actions that the list will be split across multiple batches.
+  // Batches are read out in reverse order, so even though it is being logged
+  // first, it will be emitted in the final batch.
+  base::RecordComputedAction("batch2");
+
+  for (size_t i = 0; i < LegacyMetricsClient::kMaxBatchSize; ++i)
+    base::RecordComputedAction("batch1");
+
+  task_environment_.FastForwardBy(kReportInterval);
+  EXPECT_TRUE(test_recorder_.IsRecordInFlight());
+
+  // First batch.
+  auto events = test_recorder_.WaitForEvents();
+  EXPECT_EQ(LegacyMetricsClient::kMaxBatchSize, events.size());
+  for (const auto& event : events)
+    EXPECT_EQ(event.user_action_event().name(), "batch1");
+  test_recorder_.SendAck();
+
+  // Second batch (remainder).
+  events = test_recorder_.WaitForEvents();
+  EXPECT_EQ(1u, events.size());
+  for (const auto& event : events)
+    EXPECT_EQ(event.user_action_event().name(), "batch2");
+  test_recorder_.SendAck();
+}
+
 }  // namespace
 }  // namespace cr_fuchsia

fuchsia/engine/browser/web_engine_browser_main_parts.cc

@@ -5,12 +5,15 @@
 #include "fuchsia/engine/browser/web_engine_browser_main_parts.h"
 
 #include <utility>
+#include <vector>
 
 #include "base/bind_helpers.h"
 #include "base/command_line.h"
 #include "base/fuchsia/fuchsia_logging.h"
 #include "base/logging.h"
+#include "base/threading/thread_task_runner_handle.h"
 #include "content/public/browser/gpu_data_manager.h"
+#include "content/public/browser/histogram_fetcher.h"
 #include "content/public/browser/render_frame_host.h"
 #include "content/public/common/main_function_params.h"
 #include "fuchsia/base/legacymetrics_client.h"
@@ -29,6 +32,20 @@ namespace {
 constexpr base::TimeDelta kMetricsReportingInterval =
     base::TimeDelta::FromMinutes(1);
 
+constexpr base::TimeDelta kChildProcessHistogramFetchTimeout =
+    base::TimeDelta::FromSeconds(10);
+
+// Merge child process' histogram deltas into the browser process' histograms.
+void FetchHistogramsFromChildProcesses(
+    base::OnceCallback<void(std::vector<fuchsia::legacymetrics::Event>)>
+        done_cb) {
+  content::FetchHistogramsAsynchronously(
+      base::ThreadTaskRunnerHandle::Get(),
+      base::BindOnce(std::move(done_cb),
+                     std::vector<fuchsia::legacymetrics::Event>()),
+      kChildProcessHistogramFetchTimeout);
+}
+
 }  // namespace
 
 WebEngineBrowserMainParts::WebEngineBrowserMainParts(
@@ -74,6 +91,12 @@ void WebEngineBrowserMainParts::PreMainMessageLoopRun() {
           switches::kUseLegacyMetricsService)) {
     legacy_metrics_client_ =
         std::make_unique<cr_fuchsia::LegacyMetricsClient>();
+
+    // Add a hook to asynchronously pull metrics from child processes just prior
+    // to uploading.
+    legacy_metrics_client_->SetReportAdditionalMetricsCallback(
+        base::BindRepeating(&FetchHistogramsFromChildProcesses));
+
     legacy_metrics_client_->Start(kMetricsReportingInterval);
   }