SQL: Add optional per-DB logging for BuiltInRecovery

Currently, we only have logging which provides the sum of _all_
attempts to recovery using BuiltInRecovery

Bug: 1385500
Change-Id: I1894ef85b5c291f9b80e2573d4d14698c230cae5
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4917012
Commit-Queue: Austin Sullivan <asully@chromium.org>
Reviewed-by: Evan Stade <estade@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1206716}

sql/recovery.cc

@@ -23,6 +23,7 @@
 #include "base/logging.h"
 #include "base/metrics/histogram_functions.h"
 #include "base/notreached.h"
+#include "base/strings/strcat.h"
 #include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
 #include "base/types/pass_key.h"
@@ -66,13 +67,16 @@ bool BuiltInRecovery::ShouldAttemptRecovery(Database* database,
 }
 
 // static
-SqliteResultCode BuiltInRecovery::RecoverDatabase(Database* database,
-                                                  Strategy strategy) {
+SqliteResultCode BuiltInRecovery::RecoverDatabase(
+    Database* database,
+    Strategy strategy,
+    std::string database_uma_name) {
   if (!BuiltInRecovery::IsSupported()) {
     return SqliteResultCode::kAbort;
   }
 
-  auto recovery = BuiltInRecovery(database, strategy);
+  auto recovery =
+      BuiltInRecovery(database, strategy, std::move(database_uma_name));
   return recovery.RecoverAndReplaceDatabase();
 }
 
@@ -81,7 +85,8 @@ bool BuiltInRecovery::RecoverIfPossible(
     Database* database,
     int extended_error,
     Strategy strategy,
-    const base::Feature* const use_builtin_recovery_if_supported_flag) {
+    const base::Feature* const use_builtin_recovery_if_supported_flag,
+    std::string database_uma_name) {
   // If `BuiltInRecovery` is supported at all, check the flag for this specific
   // database, provided by the feature team.
   bool use_builtin_recovery =
@@ -104,7 +109,8 @@ bool BuiltInRecovery::RecoverIfPossible(
 
   if (use_builtin_recovery) {
     CHECK(BuiltInRecovery::IsSupported());
-    auto result = BuiltInRecovery::RecoverDatabase(database, strategy);
+    auto result =
+        BuiltInRecovery::RecoverDatabase(database, strategy, database_uma_name);
     if (!IsSqliteSuccessCode(result)) {
       DLOG(ERROR) << "Database recovery failed with result code: " << result;
     }
@@ -124,8 +130,11 @@ bool BuiltInRecovery::RecoverIfPossible(
   return true;
 }
 
-BuiltInRecovery::BuiltInRecovery(Database* database, Strategy strategy)
+BuiltInRecovery::BuiltInRecovery(Database* database,
+                                 Strategy strategy,
+                                 std::string database_uma_name)
     : strategy_(strategy),
+      database_uma_name_(std::move(database_uma_name)),
       db_(database),
       recover_db_(sql::DatabaseOptions{
           .exclusive_locking = false,
@@ -161,6 +170,14 @@ BuiltInRecovery::~BuiltInRecovery() {
   UmaHistogramSqliteResult("Sql.Recovery.ResultCode",
                            static_cast<int>(sqlite_result_code_));
 
+  if (!database_uma_name_.empty()) {
+    base::UmaHistogramEnumeration(
+        base::StrCat({"Sql.Recovery.Result.", database_uma_name_}), result_);
+    UmaHistogramSqliteResult(
+        base::StrCat({"Sql.Recovery.ResultCode.", database_uma_name_}),
+        static_cast<int>(sqlite_result_code_));
+  }
+
   if (db_) {
     if (result_ == Result::kSuccess) {
       // Poison the original handle, but don't raze the database.

sql/recovery.h

@@ -107,6 +107,8 @@ class COMPONENT_EXPORT(SQL) BuiltInRecovery {
   // recovered according to `strategy`. After attempting recovery, the database
   // can be re-opened and assumed to be free of corruption.
   //
+  // `database_uma_name` is used to log UMA specific to the given database.
+  //
   // It is not considered an error if some or all of the data cannot be
   // recovered due to database corruption, so it is possible that some records
   // could not be salvaged from the corrupted database.
@@ -124,8 +126,10 @@ class COMPONENT_EXPORT(SQL) BuiltInRecovery {
   //
   // Returns a SQLite error code specifying whether the database was
   // successfully recovered.
-  [[nodiscard]] static SqliteResultCode RecoverDatabase(Database* database,
-                                                        Strategy strategy);
+  [[nodiscard]] static SqliteResultCode RecoverDatabase(
+      Database* database,
+      Strategy strategy,
+      std::string database_uma_name = std::string());
 
   // Similar to `RecoverDatabase()` above, but with a few key differences:
   //   - Uses `BuiltInRecovery` or the legacy `Recovery` to recover the
@@ -138,7 +142,7 @@ class COMPONENT_EXPORT(SQL) BuiltInRecovery {
   //   - Must only be called from within a database error callback.
   //   - Includes the option to pass a per-database feature flag indicating
   //     whether `BuiltInRecovery` should be used to recover this database, if
-  //     it's supported.
+  //     it's supported. A per-database UMA may optionally be logged, as well.
   //
   // Recommended usage from within a database error callback:
   //
@@ -146,7 +150,8 @@ class COMPONENT_EXPORT(SQL) BuiltInRecovery {
   //  if (sql::BuiltInRecovery::RecoverIfPossible(
   //          &db, extended_error,
   //          sql::BuiltInRecovery::Strategy::kRecoverWithMetaVersionOrRaze,
-  //          &features::kMyFeatureTeamShouldUseBuiltInRecoveryIfSupported)) {
+  //          &features::kMyFeatureTeamShouldUseBuiltInRecoveryIfSupported,
+  //          "MyFeatureDatabase")) {
   //    // Recovery was attempted. The database handle has been poisoned and the
   //    // error callback has been reset.
   //
@@ -158,13 +163,16 @@ class COMPONENT_EXPORT(SQL) BuiltInRecovery {
       int extended_error,
       Strategy strategy,
       const base::Feature* const use_builtin_recovery_if_supported_flag =
-          nullptr);
+          nullptr,
+      std::string database_uma_name = std::string());
 
   BuiltInRecovery(const BuiltInRecovery&) = delete;
   BuiltInRecovery& operator=(const BuiltInRecovery&) = delete;
 
  private:
-  BuiltInRecovery(Database* database, Strategy strategy);
+  BuiltInRecovery(Database* database,
+                  Strategy strategy,
+                  std::string database_uma_name);
   ~BuiltInRecovery();
 
   // Entry point.
@@ -185,6 +193,10 @@ class COMPONENT_EXPORT(SQL) BuiltInRecovery {
 
   const Strategy strategy_;
 
+  // If non-empty, UMA will be logged with the result of the recovery for this
+  // specific database.
+  const std::string database_uma_name_;
+
   // Result of the recovery. This value must be set to something other than
   // `kUnknown` before this object is destroyed.
   Result result_ = Result::kUnknown;

sql/recovery_unittest.cc

@@ -19,6 +19,7 @@
 #include "base/functional/callback_helpers.h"
 #include "base/path_service.h"
 #include "base/ranges/algorithm.h"
+#include "base/strings/strcat.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/test/bind.h"
 #include "base/test/gtest_util.h"
@@ -26,6 +27,7 @@
 #include "base/test/scoped_feature_list.h"
 #include "sql/database.h"
 #include "sql/meta_table.h"
+#include "sql/sql_features.h"
 #include "sql/sqlite_result_code.h"
 #include "sql/sqlite_result_code_values.h"
 #include "sql/statement.h"
@@ -1166,6 +1168,36 @@ TEST_P(SqlRecoveryTest, RecoverIfPossibleWithClosedDatabase) {
                       std::move(run_recovery));
 }
 
+TEST_P(SqlRecoveryTest, RecoverIfPossibleWithPerDatabaseUma) {
+  auto run_recovery = base::BindLambdaForTesting([&]() {
+    EXPECT_TRUE(BuiltInRecovery::RecoverIfPossible(
+        &db_, SQLITE_CORRUPT, BuiltInRecovery::Strategy::kRecoverOrRaze,
+        &features::kUseBuiltInRecoveryIfSupported, "MyFeatureDatabase"));
+  });
+
+  TestRecoverDatabase(db_, db_path_, /*with_meta=*/false,
+                      std::move(run_recovery));
+
+  if (UseBuiltIn()) {
+    // Log to the overall histograms.
+    histogram_tester_.ExpectUniqueSample(kRecoveryResultHistogramName,
+                                         BuiltInRecovery::Result::kSuccess,
+                                         /*expected_bucket_count=*/1);
+    histogram_tester_.ExpectUniqueSample(kRecoveryResultCodeHistogramName,
+                                         SqliteLoggedResultCode::kNoError,
+                                         /*expected_bucket_count=*/1);
+    // And the histograms for this specific feature.
+    histogram_tester_.ExpectUniqueSample(
+        base::StrCat({kRecoveryResultHistogramName, ".MyFeatureDatabase"}),
+        BuiltInRecovery::Result::kSuccess,
+        /*expected_bucket_count=*/1);
+    histogram_tester_.ExpectUniqueSample(
+        base::StrCat({kRecoveryResultCodeHistogramName, ".MyFeatureDatabase"}),
+        SqliteLoggedResultCode::kNoError,
+        /*expected_bucket_count=*/1);
+  }
+}
+
 TEST_P(SqlRecoveryTest, RecoverDatabaseWithView) {
   db_.Close();
   sql::Database db({.enable_views_discouraged = true});