Consolidate initial log logic behind feature flag

This CL is essentially a reland of crrev.com/c/2753547 which removes
the logic for the initial log (credits to asvitkine@). But this CL
gates the changes behind the feature flag
ConsolidateMetricsServiceInitialLogLogic.

This CL includes a few additional changes:

1.
- With the status quo logic, for initial logs, we trim right after
closing the initial log. So then an initial log that is over the max
size limit would not be sent. For non-initial logs, we do *not* trim
after closing the log. So then logs over the max size limit would be
attempted to be sent once. If that failed, we would then trim it.

- With the changes in crrev.com/c/2753547, we would trim after closing
every log. So then for initial logs, the logic stayed the same. But
for non-initial logs, we wouldn't attempt to send them at all if they
were over the max size. I think this is what was causing logs drop.

- With the changes in this CL, we do not trim at all after closing a
log (regardless of if it's an initial or non-initial log). So then an
initial log that is over the max size will be attempted to be sent
once, like non-initial logs. For non-initial logs the logic
essentially stays the same.


2. In this CL, after closing a log, we call
TrimAndPersistUnsentLogs(/*overwrite_in_memory_store=*/false). This
way, if we crash before sending the log that we just closed, we won't
lose it. But at the same time, we still give the log a chance to be
sent once if it's over the max size limit. This parameter was
introduced in crrev.com/c/3806465

3. Unit tests in components/metrics/metrics_service_unittest.cc are
now parameterized to test the behavior with both the feature enabled
and the feature disabled.


Bug: 1171830
Change-Id: Ifb3b2a0a3bc22c3c6f5e92d16f9140a78f3024fc
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3781383
Reviewed-by: Alexei Svitkine <asvitkine@chromium.org>
Commit-Queue: Luc Nguyen <lucnguyen@google.com>
Cr-Commit-Position: refs/heads/main@{#1032725}

components/metrics/metrics_service.cc

@@ -219,6 +219,12 @@ void RecordUserLogStoreState(UserLogStoreState state) {
 
 }  // namespace
 
+// Determines whether the initial log should use the same logic as subsequent
+// logs when building it.
+const base::Feature kConsolidateMetricsServiceInitialLogLogic = {
+    "ConsolidateMetricsServiceInitialLogLogic",
+    base::FEATURE_DISABLED_BY_DEFAULT};
+
 // static
 void MetricsService::RegisterPrefs(PrefRegistrySimple* registry) {
   CleanExitBeacon::RegisterPrefs(registry);
@@ -695,14 +701,18 @@ void MetricsService::FinishedInitTask() {
   DCHECK_EQ(INIT_TASK_SCHEDULED, state_);
   state_ = INIT_TASK_DONE;
 
-  // Create the initial log.
-  if (!initial_metrics_log_) {
-    initial_metrics_log_ = CreateLog(MetricsLog::ONGOING_LOG);
-    // Note: We explicitly do not call OnDidCreateMetricsLog() here, as this
-    // function would have already been called in Start() and this log will
-    // already contain any histograms logged there. OnDidCreateMetricsLog()
-    // will be called again after the initial log is closed, for the next log.
-    // TODO(crbug.com/1171830): Consider getting rid of |initial_metrics_log_|.
+  if (!base::FeatureList::IsEnabled(
+          kConsolidateMetricsServiceInitialLogLogic)) {
+    // Create the initial log.
+    if (!initial_metrics_log_) {
+      initial_metrics_log_ = CreateLog(MetricsLog::ONGOING_LOG);
+      // Note: We explicitly do not call OnDidCreateMetricsLog() here, as this
+      // function would have already been called in Start() and this log will
+      // already contain any histograms logged there. OnDidCreateMetricsLog()
+      // will be called again after the initial log is closed, for the next log.
+      // TODO(crbug.com/1171830): Consider getting rid of
+      // |initial_metrics_log_|.
+    }
   }
 
   rotation_scheduler_->InitTaskComplete();
@@ -832,9 +842,24 @@ void MetricsService::StartScheduledUpload() {
     return;
   }
 
+  if (base::FeatureList::IsEnabled(kConsolidateMetricsServiceInitialLogLogic)) {
+    // The first ongoing log should be collected prior to sending any unsent
+    // logs.
+    if (state_ == INIT_TASK_DONE) {
+      client_->CollectFinalMetricsForLog(
+          base::BindOnce(&MetricsService::OnFinalLogInfoCollectionDone,
+                         self_ptr_factory_.GetWeakPtr()));
+      return;
+    }
+  }
+
   // If there are unsent logs, send the next one. If not, start the asynchronous
   // process of finalizing the current log for upload.
-  if (state_ == SENDING_LOGS && has_unsent_logs()) {
+  bool send_unsent_logs =
+      base::FeatureList::IsEnabled(kConsolidateMetricsServiceInitialLogLogic)
+          ? has_unsent_logs()
+          : state_ == SENDING_LOGS && has_unsent_logs();
+  if (send_unsent_logs) {
     reporting_service_.Start();
     rotation_scheduler_->RotationFinished();
   } else {
@@ -847,6 +872,10 @@ void MetricsService::StartScheduledUpload() {
 
 void MetricsService::OnFinalLogInfoCollectionDone() {
   DVLOG(1) << "OnFinalLogInfoCollectionDone";
+  if (base::FeatureList::IsEnabled(kConsolidateMetricsServiceInitialLogLogic)) {
+    DCHECK(state_ >= INIT_TASK_DONE);
+    state_ = SENDING_LOGS;
+  }
 
   // Abort if metrics were turned off during the final info gathering.
   if (!recording_active()) {
@@ -855,13 +884,27 @@ void MetricsService::OnFinalLogInfoCollectionDone() {
     return;
   }
 
-  if (state_ == INIT_TASK_DONE) {
-    PrepareInitialMetricsLog();
-  } else {
-    DCHECK_EQ(SENDING_LOGS, state_);
+  if (base::FeatureList::IsEnabled(kConsolidateMetricsServiceInitialLogLogic)) {
     CloseCurrentLog();
     OpenNewLog();
+    // Trim and store unsent logs, including the log that was just closed, so
+    // that they're not lost in case of a crash before upload time. However, the
+    // in-memory log store is unchanged. I.e., logs that are trimmed will still
+    // be available in memory. This is to give the log that was just created a
+    // chance to be sent in case it is trimmed. After uploading (whether
+    // successful or not), the log store is trimmed and stored again, and at
+    // that time, the in-memory log store will be updated.
+    log_store()->TrimAndPersistUnsentLogs(/*overwrite_in_memory_store=*/false);
+  } else {
+    if (state_ == INIT_TASK_DONE) {
+      PrepareInitialMetricsLog();
+    } else {
+      DCHECK_EQ(SENDING_LOGS, state_);
+      CloseCurrentLog();
+      OpenNewLog();
+    }
   }
+
   reporting_service_.Start();
   rotation_scheduler_->RotationFinished();
   HandleIdleSinceLastTransmission(true);

components/metrics/metrics_service.h

@@ -59,6 +59,9 @@ class MetricsRotationScheduler;
 class MetricsServiceClient;
 class MetricsStateManager;
 
+// Exposed in the header file for tests.
+extern const base::Feature kConsolidateMetricsServiceInitialLogLogic;
+
 // See metrics_service.cc for a detailed description.
 class MetricsService : public base::HistogramFlattener {
  public: