Refactor limits to be part of UnsentLogStore.

This CL refactors UnsentLogStore to make more clear when logs are dropped.


Bug: b/283126298
Change-Id: I764e02ecddc2ceb206e5861629d7bd10b1d1b9cb
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4559731
Reviewed-by: Robert Kaplow <rkaplow@chromium.org>
Commit-Queue: Jong Ahn <jongahn@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1152127}

chrome/browser/metrics/per_user_state_manager_chromeos.cc

@@ -525,9 +525,7 @@ void PerUserStateManagerChromeOS::AssignUserLogStore() {
   SetUserLogStore(std::make_unique<UnsentLogStore>(
       std::make_unique<UnsentLogStoreMetricsImpl>(), GetCurrentUserPrefs(),
       prefs::kMetricsUserMetricLogs, prefs::kMetricsUserMetricLogsMetadata,
-      storage_limits_.min_ongoing_log_queue_count,
-      storage_limits_.min_ongoing_log_queue_size,
-      storage_limits_.max_ongoing_log_size, signing_key_,
+      storage_limits_.ongoing_log_queue_limits, signing_key_,
       // |logs_event_manager| will be set by the metrics service directly in
       // MetricsLogStore::SetAlternateOngoingLogStore().
       /*logs_event_manager=*/nullptr));

chrome/browser/metrics/per_user_state_manager_chromeos_unittest.cc

@@ -193,9 +193,21 @@ class PerUserStateManagerChromeOSTest : public testing::Test {
 
  protected:
   void SetUp() override {
-    storage_limits_.min_ongoing_log_queue_count = 5;
-    storage_limits_.min_ongoing_log_queue_size = 10000;
-    storage_limits_.max_ongoing_log_size = 0;
+    // Limits to ensure at least some logs will be persisted for the tests.
+    storage_limits_ = {
+        // Log store that can hold up to 5 logs. Set so that logs are not
+        // dropped in the tests.
+        .initial_log_queue_limits =
+            UnsentLogStore::UnsentLogStoreLimits{
+                .min_log_count = 5,
+            },
+        // Log store that can hold up to 5 logs. Set so that logs are not
+        // dropped in the tests.
+        .ongoing_log_queue_limits =
+            UnsentLogStore::UnsentLogStoreLimits{
+                .min_log_count = 5,
+            },
+    };
 
     test_user_manager_ = std::make_unique<ash::FakeChromeUserManager>();
 

chromecast/metrics/cast_metrics_service_client_unittest.cc

@@ -63,26 +63,36 @@ TEST_F(CastMetricsServiceClientTest, UsesDelegateToGetStorageLimits) {
   FakeCastMetricsServiceDelegate delegate;
   CastMetricsServiceClient client(&delegate, nullptr, nullptr);
 
+  // Set arbitrary limits to ensure the limits propagate to the client
+  // correctly.
   ::metrics::MetricsLogStore::StorageLimits expected_limits = {
-      /*min_initial_log_queue_count=*/10,
-      /*min_initial_log_queue_size=*/2000,
-      /*min_ongoing_log_queue_count=*/30,
-      /*min_ongoing_log_queue_size=*/4000,
-      /*max_ongoing_log_size=*/5000,
+      .initial_log_queue_limits =
+          ::metrics::UnsentLogStore::UnsentLogStoreLimits{
+              .min_log_count = 10,
+              .min_queue_size_bytes = 2000,
+          },
+      .ongoing_log_queue_limits =
+          ::metrics::UnsentLogStore::UnsentLogStoreLimits{
+              .min_log_count = 30,
+              .min_queue_size_bytes = 4000,
+              .max_log_size_bytes = 5000,
+          },
   };
   delegate.SetStorageLimits(expected_limits);
   ::metrics::MetricsLogStore::StorageLimits actual_limits =
       client.GetStorageLimits();
-  EXPECT_EQ(actual_limits.min_initial_log_queue_count,
-            expected_limits.min_initial_log_queue_count);
-  EXPECT_EQ(actual_limits.min_initial_log_queue_size,
-            expected_limits.min_initial_log_queue_size);
-  EXPECT_EQ(actual_limits.min_ongoing_log_queue_count,
-            expected_limits.min_ongoing_log_queue_count);
-  EXPECT_EQ(actual_limits.min_ongoing_log_queue_size,
-            expected_limits.min_ongoing_log_queue_size);
-  EXPECT_EQ(actual_limits.max_ongoing_log_size,
-            expected_limits.max_ongoing_log_size);
+  EXPECT_EQ(actual_limits.initial_log_queue_limits.min_log_count,
+            expected_limits.initial_log_queue_limits.min_log_count);
+  EXPECT_EQ(actual_limits.initial_log_queue_limits.min_queue_size_bytes,
+            expected_limits.initial_log_queue_limits.min_queue_size_bytes);
+  EXPECT_EQ(actual_limits.initial_log_queue_limits.max_log_size_bytes,
+            expected_limits.initial_log_queue_limits.max_log_size_bytes);
+  EXPECT_EQ(actual_limits.ongoing_log_queue_limits.min_log_count,
+            expected_limits.ongoing_log_queue_limits.min_log_count);
+  EXPECT_EQ(actual_limits.ongoing_log_queue_limits.min_queue_size_bytes,
+            expected_limits.ongoing_log_queue_limits.min_queue_size_bytes);
+  EXPECT_EQ(actual_limits.ongoing_log_queue_limits.max_log_size_bytes,
+            expected_limits.ongoing_log_queue_limits.max_log_size_bytes);
 }
 
 }  // namespace

components/metrics/metrics_log_store.cc

@@ -25,22 +25,23 @@ MetricsLogStore::MetricsLogStore(PrefService* local_state,
                                  MetricsLogsEventManager* logs_event_manager)
     : unsent_logs_loaded_(false),
       logs_event_manager_(logs_event_manager),
-      initial_log_queue_(std::make_unique<UnsentLogStoreMetricsImpl>(),
-                         local_state,
-                         prefs::kMetricsInitialLogs,
-                         prefs::kMetricsInitialLogsMetadata,
-                         storage_limits.min_initial_log_queue_count,
-                         storage_limits.min_initial_log_queue_size,
-                         0,  // Each individual initial log can be any size.
-                         signing_key,
-                         logs_event_manager),
+      initial_log_queue_(
+          std::make_unique<UnsentLogStoreMetricsImpl>(),
+          local_state,
+          prefs::kMetricsInitialLogs,
+          prefs::kMetricsInitialLogsMetadata,
+          UnsentLogStore::UnsentLogStoreLimits{
+              storage_limits.initial_log_queue_limits.min_log_count,
+              storage_limits.initial_log_queue_limits.min_queue_size_bytes,
+              // Each individual initial log can be any size.
+              /*max_log_size_bytes=*/0},
+          signing_key,
+          logs_event_manager),
       ongoing_log_queue_(std::make_unique<UnsentLogStoreMetricsImpl>(),
                          local_state,
                          prefs::kMetricsOngoingLogs,
                          prefs::kMetricsOngoingLogsMetadata,
-                         storage_limits.min_ongoing_log_queue_count,
-                         storage_limits.min_ongoing_log_queue_size,
-                         storage_limits.max_ongoing_log_size,
+                         storage_limits.ongoing_log_queue_limits,
                          signing_key,
                          logs_event_manager) {}
 

components/metrics/metrics_log_store.h

@@ -39,25 +39,14 @@ class MetricsServiceClient;
 class MetricsLogStore : public LogStore {
  public:
   // Configurable limits for ensuring and restricting local log storage.
-  //
-  // |min_{initial,ongoing}_log_queue_count| are the minimum numbers of unsent
-  // logs that UnsentLogStore must persist before deleting old logs.
-  //
-  // |min_{initial,ongoing}_log_queue_size| are the minimum numbers of bytes in
-  // total across all logs within the initial or ongoing log queue that
-  // UnsentLogStore must persist before deleting old logs.
-  //
-  // If both |min_..._log_queue_count| and |min_..._log_queue_size| are 0, then
-  // this LogStore won't persist unsent logs to local storage.
-  //
-  // |max_ongoing_log_size| is the maximum size of any individual ongoing log.
-  // When set to 0, no limits are imposed, i.e. individual logs can be any size.
   struct StorageLimits {
-    size_t min_initial_log_queue_count = 0;
-    size_t min_initial_log_queue_size = 0;
-    size_t min_ongoing_log_queue_count = 0;
-    size_t min_ongoing_log_queue_size = 0;
-    size_t max_ongoing_log_size = 0;
+    // Log store limits for |initial_log_queue_|. See
+    // comments at //components/metrics/unsent_log_store.h for more details.
+    UnsentLogStore::UnsentLogStoreLimits initial_log_queue_limits;
+
+    // Log store limits for |ongoing_log_queue_|.See
+    // comments at //components/metrics/unsent_log_store.h for more details.
+    UnsentLogStore::UnsentLogStoreLimits ongoing_log_queue_limits;
   };
 
   // Constructs a MetricsLogStore that persists data into |local_state|.

components/metrics/metrics_log_store_unittest.cc

@@ -24,9 +24,10 @@ class TestUnsentLogStore : public UnsentLogStore {
                        service,
                        kTestPrefName,
                        nullptr,
-                       /*min_log_count=*/3,
-                       /*min_log_bytes=*/1,
-                       /*max_log_size=*/0,
+                       // Set to 3 so logs are not dropped in the test.
+                       UnsentLogStore::UnsentLogStoreLimits{
+                           .min_log_count = 3,
+                       },
                        /*signing_key=*/std::string(),
                        /*logs_event_manager=*/nullptr) {}
   ~TestUnsentLogStore() override = default;
@@ -203,7 +204,7 @@ TEST_F(MetricsLogStoreTest, StoreStagedInitialLog) {
 
 TEST_F(MetricsLogStoreTest, LargeLogDiscarding) {
   // Set the size threshold very low, to verify that it's honored.
-  client_.set_max_ongoing_log_size(1);
+  client_.set_max_ongoing_log_size_bytes(1);
   MetricsLogStore log_store(&pref_service_, client_.GetStorageLimits(),
                             /*signing_key=*/std::string(),
                             /*logs_event_manager=*/nullptr);

components/metrics/metrics_reporting_service.cc

@@ -31,7 +31,8 @@ MetricsReportingService::MetricsReportingService(
     MetricsLogsEventManager* logs_event_manager_)
     : ReportingService(client,
                        local_state,
-                       client->GetStorageLimits().max_ongoing_log_size,
+                       client->GetStorageLimits()
+                           .ongoing_log_queue_limits.max_log_size_bytes,
                        logs_event_manager_),
       metrics_log_store_(local_state,
                          client->GetStorageLimits(),

components/metrics/metrics_service_client.cc

@@ -19,19 +19,24 @@
 
 namespace metrics {
 
-const base::FeatureParam<int> kMaxLogQueueBytes{
+// TODO(b/282078734): Names "max_*" are the original names when the experiment
+// first started. These should be renamed after the experiment is over.
+const base::FeatureParam<int> kMinLogQueueBytes{
     &features::kStructuredMetrics, "max_log_queue_bytes",
     300 * 1024  // 300 KiB
 };
 
-const base::FeatureParam<int> kMaxOngoingLogQueueCount{
+const base::FeatureParam<int> kMinOngoingLogQueueCount{
     &features::kStructuredMetrics, "max_ongoing_log_queue_count", 8};
 
 namespace {
 
 // The minimum time in seconds between consecutive metrics report uploads.
 constexpr int kMetricsUploadIntervalSecMinimum = 20;
 
+// Initial logs can be of any size.
+constexpr size_t kMaxInitialLogSize = 0;
+
 // If a metrics log upload fails, and the transmission is over this byte count,
 // then we will discard the log, and not try to retransmit it. We also don't
 // persist the log to the prefs for transmission during the next chrome session
@@ -43,9 +48,8 @@ constexpr size_t kMaxOngoingLogSize = 1024 * 1024;  // 1 MiB
 constexpr size_t kMaxOngoingLogSize = 100 * 1024;  // 100 KiB
 #endif  // BUILDFLAG(IS_CHROMEOS)
 
-// The minimum number of "initial" logs to save, and hope to send during a
-// future Chrome session. Initial logs contain crash stats, and are pretty
-// small.
+// The minimum number of "initial" logs to save before logs are dropped. Initial
+// logs contain crash stats, and are pretty small.
 constexpr size_t kMinInitialLogQueueCount = 20;
 
 }  // namespace
@@ -150,17 +154,22 @@ MetricsServiceClient::AddOnClonedInstallDetectedCallback(
 }
 
 MetricsLogStore::StorageLimits MetricsServiceClient::GetStorageLimits() const {
-  // TODO(b/283126298): Rename min_* variable names to max_* to more accurately
-  // reflect what the variable names represent.
   return {
-      /*min_initial_log_queue_count=*/kMinInitialLogQueueCount,
-      /*min_initial_log_queue_size=*/
-      static_cast<size_t>(kMaxLogQueueBytes.Get()),
-      /*min_ongoing_log_queue_count=*/
-      static_cast<size_t>(kMaxOngoingLogQueueCount.Get()),
-      /*min_ongoing_log_queue_size=*/
-      static_cast<size_t>(kMaxLogQueueBytes.Get()),
-      /*max_ongoing_log_size=*/kMaxOngoingLogSize,
+      .initial_log_queue_limits =
+          UnsentLogStore::UnsentLogStoreLimits{
+              .min_log_count = kMinInitialLogQueueCount,
+              .min_queue_size_bytes =
+                  static_cast<size_t>(kMinLogQueueBytes.Get()),
+              .max_log_size_bytes = static_cast<size_t>(kMaxInitialLogSize),
+          },
+      .ongoing_log_queue_limits =
+          UnsentLogStore::UnsentLogStoreLimits{
+              .min_log_count =
+                  static_cast<size_t>(kMinOngoingLogQueueCount.Get()),
+              .min_queue_size_bytes =
+                  static_cast<size_t>(kMinLogQueueBytes.Get()),
+              .max_log_size_bytes = static_cast<size_t>(kMaxOngoingLogSize),
+          },
   };
 }
 

components/metrics/metrics_service_client.h

@@ -40,14 +40,17 @@ namespace structured {
 class StructuredMetricsService;
 }
 
-// The maximum capacity (bytes) of the queue for logs to be persisted. This
-// will be applied to both log queues (initial/ongoing). This ensures that a
-// reasonable amount of history will be stored even if there is a long series of
-// very small logs.
-extern const base::FeatureParam<int> kMaxLogQueueBytes;
-
-// The maximum number of ongoing logs to persist in the queue to send during
-// this or future sessions.
+// The minimum number bytes of the queue to be persisted before logs are
+// dropped. This will be applied to both log queues (initial/ongoing). This
+// ensures that a reasonable amount of history will be stored even if there is a
+// long series of very small logs.
+//
+// Refer to //components/metrics/unsent_log_store.h for more details on when
+// logs are dropped.
+extern const base::FeatureParam<int> kMinLogQueueBytes;
+
+// The minimum number of ongoing logs to persist in the queue before logs are
+// dropped.
 //
 // Note that each ongoing log may be pretty large, since "initial" logs must
 // first be sent before any ongoing logs are transmitted. "Initial" logs will
@@ -58,7 +61,10 @@ extern const base::FeatureParam<int> kMaxLogQueueBytes;
 // A "standard shutdown" will create a small log, including just the data that
 // was not yet been transmitted, and that is normal (to have exactly one
 // ongoing_log_ at startup).
-extern const base::FeatureParam<int> kMaxOngoingLogQueueCount;
+//
+// Refer to //components/metrics/unsent_log_store.h for more details on when
+// logs are dropped.
+extern const base::FeatureParam<int> kMinOngoingLogQueueCount;
 
 // An abstraction of operations that depend on the embedder's (e.g. Chrome)
 // environment.

components/metrics/metrics_service_observer_unittest.cc

@@ -229,7 +229,7 @@ TEST_F(MetricsServiceObserverTest, TrimLargeLog) {
   // Set the max log size to be 1 byte so that pretty much all logs will be
   // trimmed. We don't set it to 0 bytes because that is a special value that
   // represents no max size.
-  client.set_max_ongoing_log_size(1);
+  client.set_max_ongoing_log_size_bytes(1);
 
   MetricsService service(GetMetricsStateManager(), &client, local_state());
 
@@ -272,11 +272,11 @@ TEST_F(MetricsServiceObserverTest, TrimLargeLog) {
 TEST_F(MetricsServiceObserverTest, TrimLongLogList) {
   TestMetricsServiceClient client;
 
-  // Set the mininimum log count to 1 and minimum log size to 1 byte. This
+  // Set the minimum log count to 1 and minimum log size to 1 byte. This
   // essentially means that the log store, when trimming logs, will only keep
   // the most recent one. I.e., after storing one log, it will trim the rest
   // due to having stored enough logs.
-  client.set_min_ongoing_log_queue_size(1);
+  client.set_min_ongoing_log_queue_size_bytes(1);
   client.set_min_ongoing_log_queue_count(1);
 
   MetricsService service(GetMetricsStateManager(), &client, local_state());
@@ -444,9 +444,7 @@ TEST_F(MetricsServiceObserverTest, UmaLogType) {
     auto alternate_ongoing_log_store = std::make_unique<UnsentLogStore>(
         std::make_unique<UnsentLogStoreMetricsImpl>(), local_state(),
         prefs::kMetricsOngoingLogs, prefs::kMetricsOngoingLogsMetadata,
-        storage_limits.min_ongoing_log_queue_count,
-        storage_limits.min_ongoing_log_queue_size,
-        storage_limits.max_ongoing_log_size, client.GetUploadSigningKey(),
+        storage_limits.ongoing_log_queue_limits, client.GetUploadSigningKey(),
         // |logs_event_manager| will be set by |test_log_store| directly in
         // MetricsLogStore::SetAlternateOngoingLogStore().
         /*logs_event_manager=*/nullptr);

components/metrics/metrics_service_unittest.cc

@@ -64,9 +64,10 @@ class TestUnsentLogStore : public UnsentLogStore {
                        service,
                        kTestPrefName,
                        nullptr,
-                       /*min_log_count=*/3,
-                       /*min_log_bytes=*/1,
-                       /*max_log_size=*/0,
+                       // Set to 3 so logs are not dropped in the test.
+                       UnsentLogStore::UnsentLogStoreLimits{
+                           .min_log_count = 3,
+                       },
                        /*signing_key=*/std::string(),
                        /*logs_event_manager=*/nullptr) {}
   ~TestUnsentLogStore() override = default;

components/metrics/structured/reporting/structured_metrics_reporting_service.cc

@@ -14,18 +14,16 @@ namespace metrics::structured::reporting {
 StructuredMetricsReportingService::StructuredMetricsReportingService(
     MetricsServiceClient* client,
     PrefService* local_state,
-    const StorageLimits& storage_limits)
+    const UnsentLogStore::UnsentLogStoreLimits& storage_limits)
     : ReportingService(client,
                        local_state,
-                       storage_limits.max_log_size,
+                       storage_limits.max_log_size_bytes,
                        /*logs_event_manager=*/nullptr),
       log_store_(std::make_unique<StructuredMetricsLogMetrics>(),
                  local_state,
                  prefs::kLogStoreName,
                  /* metadata_pref_name=*/nullptr,
-                 storage_limits.min_log_queue_count,
-                 storage_limits.min_log_queue_size,
-                 storage_limits.max_log_size,
+                 storage_limits,
                  client->GetUploadSigningKey(),
                  /* logs_event_manager=*/nullptr) {}
 

components/metrics/structured/reporting/structured_metrics_reporting_service.h

@@ -17,19 +17,13 @@ class MetricsServiceClient;
 
 namespace metrics::structured::reporting {
 
-// The limiting parameters of the log store and reporting service.
-struct StorageLimits {
-  size_t min_log_queue_count = 0;
-  size_t min_log_queue_size = 0;
-  size_t max_log_size = 0;
-};
-
 // A service that uploads Structured Metrics logs to the UMA server.
 class StructuredMetricsReportingService : public metrics::ReportingService {
  public:
-  StructuredMetricsReportingService(MetricsServiceClient* client,
-                                    PrefService* local_state,
-                                    const StorageLimits& storage_limits);
+  StructuredMetricsReportingService(
+      MetricsServiceClient* client,
+      PrefService* local_state,
+      const UnsentLogStore::UnsentLogStoreLimits& storage_limits);
 
   void StoreLog(const std::string& serialized_log,
                 metrics::MetricsLogsEventManager::CreateReason reason);