[sql] Track uploads of diagnostic data to prevent duplication.

components/history/core/browser/thumbnail_database.cc

Index: components/history/core/browser/thumbnail_database.cc
diff --git a/components/history/core/browser/thumbnail_database.cc b/components/history/core/browser/thumbnail_database.cc
index 35afa12319fc9a021b6c7c3d7dfd30971e35cf5f..78dc8582df6e7bc31292072a881ebc5fbf60b551 100644
--- a/components/history/core/browser/thumbnail_database.cc
+++ b/components/history/core/browser/thumbnail_database.cc
@@ -9,9 +9,7 @@
 
 #include "base/bind.h"
 #include "base/debug/alias.h"
-#include "base/debug/dump_without_crashing.h"
 #include "base/files/file_util.h"
-#include "base/format_macros.h"
 #include "base/memory/ref_counted_memory.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/rand_util.h"
@@ -121,164 +119,26 @@ void RecordInvalidStructure(InvalidStructureType invalid_type) {
                             invalid_type, STRUCTURE_EVENT_MAX);
 }
 
-// Attempt to pass 2000 bytes of |debug_info| into a crash dump.
-void DumpWithoutCrashing2000(const std::string& debug_info) {
-  char debug_buf[2000];
-  base::strlcpy(debug_buf, debug_info.c_str(), arraysize(debug_buf));
-  base::debug::Alias(&debug_buf);
-
-  base::debug::DumpWithoutCrashing();
-}
-
-void ReportCorrupt(sql::Connection* db, size_t startup_kb) {
-  // Buffer for accumulating debugging info about the error.  Place
-  // more-relevant information earlier, in case things overflow the
-  // fixed-size buffer.
-  std::string debug_info;
-
-  base::StringAppendF(&debug_info, "SQLITE_CORRUPT, integrity_check:\n");
-
-  // Check files up to 8M to keep things from blocking too long.
-  const size_t kMaxIntegrityCheckSize = 8192;
-  if (startup_kb > kMaxIntegrityCheckSize) {
-    base::StringAppendF(&debug_info, "too big %" PRIuS "\n", startup_kb);
-  } else {
-    std::vector<std::string> messages;
-
-    const base::TimeTicks before = base::TimeTicks::Now();
-    db->FullIntegrityCheck(&messages);
-    base::StringAppendF(&debug_info, "# %" PRIx64 " ms, %" PRIuS " records\n",
-                        (base::TimeTicks::Now() - before).InMilliseconds(),
-                        messages.size());
-
-    // SQLite returns up to 100 messages by default, trim deeper to
-    // keep close to the 2000-character size limit for dumping.
-    //
-    // TODO(shess): If the first 20 tend to be actionable, test if
-    // passing the count to integrity_check makes it exit earlier.  In
-    // that case it may be possible to greatly ease the size
-    // restriction.
-    const size_t kMaxMessages = 20;
-    for (size_t i = 0; i < kMaxMessages && i < messages.size(); ++i) {
-      base::StringAppendF(&debug_info, "%s\n", messages[i].c_str());
-    }
-  }
-
-  DumpWithoutCrashing2000(debug_info);
-}
-
-void ReportError(sql::Connection* db, int error) {
-  // Buffer for accumulating debugging info about the error.  Place
-  // more-relevant information earlier, in case things overflow the
-  // fixed-size buffer.
-  std::string debug_info;
-
-  // The error message from the failed operation.
-  base::StringAppendF(&debug_info, "db error: %d/%s\n",
-                      db->GetErrorCode(), db->GetErrorMessage());
-
-  // System errno information.
-  base::StringAppendF(&debug_info, "errno: %d\n", db->GetLastErrno());
-
-  // SQLITE_ERROR reports seem to be attempts to upgrade invalid
-  // schema, try to log that info.
-  if (error == SQLITE_ERROR) {
-    const char* kVersionSql = "SELECT value FROM meta WHERE key = 'version'";
-    if (db->IsSQLValid(kVersionSql)) {
-      sql::Statement statement(db->GetUniqueStatement(kVersionSql));
-      if (statement.Step()) {
-        debug_info += "version: ";
-        debug_info += statement.ColumnString(0);
-        debug_info += '\n';
-      } else if (statement.Succeeded()) {
-        debug_info += "version: none\n";
-      } else {
-        debug_info += "version: error\n";
-      }
-    } else {
-      debug_info += "version: invalid\n";
-    }
-
-    debug_info += "schema:\n";
-
-    // sqlite_master has columns:
-    //   type - "index" or "table".
-    //   name - name of created element.
-    //   tbl_name - name of element, or target table in case of index.
-    //   rootpage - root page of the element in database file.
-    //   sql - SQL to create the element.
-    // In general, the |sql| column is sufficient to derive the other
-    // columns.  |rootpage| is not interesting for debugging, without
-    // the contents of the database.  The COALESCE is because certain
-    // automatic elements will have a |name| but no |sql|,
-    const char* kSchemaSql = "SELECT COALESCE(sql, name) FROM sqlite_master";
-    sql::Statement statement(db->GetUniqueStatement(kSchemaSql));
-    while (statement.Step()) {
-      debug_info += statement.ColumnString(0);
-      debug_info += '\n';
-    }
-    if (!statement.Succeeded())
-      debug_info += "error\n";
-  }
-
-  // TODO(shess): Think of other things to log.  Not logging the
-  // statement text because the backtrace should suffice in most
-  // cases.  The database schema is a possibility, but the
-  // likelihood of recursive error callbacks makes that risky (same
-  // reasoning applies to other data fetched from the database).
-
-  DumpWithoutCrashing2000(debug_info);
-}
-
-// TODO(shess): If this proves out, perhaps lift the code out to
-// chrome/browser/diagnostics/sqlite_diagnostics.{h,cc}.
+// TODO(shess): If this proves out, move it all into sql::Connection to be
+// shared.
 void GenerateDiagnostics(sql::Connection* db,
-                         size_t startup_kb,
-                         int extended_error) {
-  int error = (extended_error & 0xFF);
-
-  // Infrequently report information about the error up to the crash
-  // server.
-  static const uint64 kReportsPerMillion = 50000;
-
+                         int extended_error,
+                         sql::Statement* stmt) {
   // Since some/most errors will not resolve themselves, only report
   // once per Chrome run.
   static bool reported = false;
   if (reported)
     return;
+  reported = true;
 
+  // Only pass 5% of new reports to prevent a thundering herd of dumps.
+  // TODO(shess): If this could be related to the time in the channel, then the
+  // rate could ramp up over time.  Perhaps could remember the timestamp the
+  // first time upload is considered, and ramp up 1% per day?
+  static const uint64 kReportsPerMillion = 50000;
   uint64 rand = base::RandGenerator(1000000);
-  if (error == SQLITE_CORRUPT) {
-    // Once the database is known to be corrupt, it will generate a
-    // stream of errors until someone fixes it, so give one chance.
-    // Set first in case of errors in generating the report.
-    reported = true;
-
-    // Corrupt cases currently dominate, report them very infrequently.
-    static const uint64 kCorruptReportsPerMillion = 10000;
-    if (rand < kCorruptReportsPerMillion)
-      ReportCorrupt(db, startup_kb);
-  } else if (error == SQLITE_READONLY) {
-    // SQLITE_READONLY appears similar to SQLITE_CORRUPT - once it
-    // is seen, it is almost guaranteed to be seen again.
-    reported = true;
-
-    if (rand < kReportsPerMillion)
-      ReportError(db, extended_error);
-  } else {
-    // Only set the flag when making a report.  This should allow
-    // later (potentially different) errors in a stream of errors to
-    // be reported.
-    //
-    // TODO(shess): Would it be worthwile to audit for which cases
-    // want once-only handling?  Sqlite.Error.Thumbnail shows
-    // CORRUPT and READONLY as almost 95% of all reports on these
-    // channels, so probably easier to just harvest from the field.
-    if (rand < kReportsPerMillion) {
-      reported = true;
-      ReportError(db, extended_error);
-    }
-  }
+  if (rand <= kReportsPerMillion)
+    db->ReportDiagnosticInfo(extended_error, stmt);

[Scott Hess - ex-Googler, 2015/10/19 22:23:41]

I considered pushing this into sql::Connection, but the most obvious place was
the intent-to-report function, which would then require adaptation for testing
purposes.  Since I'm not even certain this is the right solution, I decided to
leave it near where it already was.

Previously, the goal of this threshold was to keep the daily load approachable. 
Now it has changed to spread the incoming bump over time.  I think it is
probably not perfect for this, but it is probably Good Enough For Now.  My major
concern is actually that the report rate will be too low, but I still need to
grind through my backlog of data from this, so there's time to tweak going
forward.

 }
 
 // NOTE(shess): Schema modifications must consider initial creation in
@@ -549,7 +409,6 @@ void RecoverDatabaseOrRaze(sql::Connection* db, const base::FilePath& db_path) {
 
 void DatabaseErrorCallback(sql::Connection* db,
                            const base::FilePath& db_path,
-                           size_t startup_kb,
                            HistoryBackendClient* backend_client,
                            int extended_error,
                            sql::Statement* stmt) {
@@ -558,7 +417,7 @@ void DatabaseErrorCallback(sql::Connection* db,
   // see how to reach that.
 
   if (backend_client && backend_client->ShouldReportDatabaseError()) {
-    GenerateDiagnostics(db, startup_kb, extended_error);
+    GenerateDiagnostics(db, extended_error, stmt);

[Scott Hess - ex-Googler, 2015/10/19 22:23:41]

Removed |startup_kb| because DbPath() lets it just query the size as needed. 
Previously |stmt| wasn't logged on the theory that the backtrace would provide
it - I've found that to be just annoying enough to decode sometimes that I think
it was a false savings.

   }
 
   // Attempt to recover corrupt databases.
@@ -1205,14 +1064,9 @@ bool ThumbnailDatabase::RetainDataForPageUrls(
 
 sql::InitStatus ThumbnailDatabase::OpenDatabase(sql::Connection* db,
                                                 const base::FilePath& db_name) {
-  size_t startup_kb = 0;
-  int64 size_64;
-  if (base::GetFileSize(db_name, &size_64))
-    startup_kb = static_cast<size_t>(size_64 / 1024);
-
   db->set_histogram_tag("Thumbnail");
   db->set_error_callback(base::Bind(&DatabaseErrorCallback,
-                                    db, db_name, startup_kb, backend_client_));
+                                    db, db_name, backend_client_));
 
   // Thumbnails db now only stores favicons, so we don't need that big a page
   // size or cache.