Add metrics for tracking uploads

Three new metrics:
- counting upload success/failure;
- enum tracking the reason upload was skipped;
- enum describing how an upload got to the pending state.

R=mark@chromium.org, asvitkine@chromium.org
BUG=crashpad:100

Change-Id: I5e0cbc1ac3424e974f3a51560e5cdad484ffc038
Reviewed-on: https://chromium-review.googlesource.com/388855
Reviewed-by: Mark Mentovai <mark@chromium.org>

client/crash_report_database.h

@@ -24,6 +24,7 @@
 #include "base/files/file_path.h"
 #include "base/macros.h"
 #include "util/file/file_io.h"
+#include "util/misc/metrics.h"
 #include "util/misc/uuid.h"
 
 namespace crashpad {
@@ -326,9 +327,13 @@ class CrashReportDatabase {
   //! crash generation is still enabled in the product.
   //!
   //! \param[in] uuid The unique identifier for the crash report record.
+  //! \param[in] reason The reason the report upload is being skipped for
+  //!     metrics tracking purposes.
   //!
   //! \return The operation status code.
-  virtual OperationStatus SkipReportUpload(const UUID& uuid) = 0;
+  virtual OperationStatus SkipReportUpload(
+      const UUID& uuid,
+      Metrics::CrashSkippedReason reason) = 0;
 
   //! \brief Deletes a crash report file and its associated metadata.
   //!

client/crash_report_database_mac.mm

@@ -140,7 +140,8 @@ class CrashReportDatabaseMac : public CrashReportDatabase {
   OperationStatus RecordUploadAttempt(const Report* report,
                                       bool successful,
                                       const std::string& id) override;
-  OperationStatus SkipReportUpload(const UUID& uuid) override;
+  OperationStatus SkipReportUpload(const UUID& uuid,
+                                   Metrics::CrashSkippedReason reason) override;
   OperationStatus DeleteReport(const UUID& uuid) override;
   OperationStatus RequestUpload(const UUID& uuid) override;
 
@@ -359,6 +360,7 @@ CrashReportDatabaseMac::FinishedWritingCrashReport(NewReport* report,
     return kFileSystemError;
   }
 
+  Metrics::CrashReportPending(Metrics::PendingReportReason::kNewlyCreated);
   Metrics::CrashReportSize(report->handle);
 
   return kNoError;
@@ -451,6 +453,8 @@ CrashReportDatabaseMac::RecordUploadAttempt(const Report* report,
                                             const std::string& id) {
   INITIALIZATION_STATE_DCHECK_VALID(initialized_);
 
+  Metrics::CrashUploadAttempted(successful);
+
   DCHECK(report);
   DCHECK(successful || id.empty());
 
@@ -502,9 +506,12 @@ CrashReportDatabaseMac::RecordUploadAttempt(const Report* report,
 }
 
 CrashReportDatabase::OperationStatus CrashReportDatabaseMac::SkipReportUpload(
-    const UUID& uuid) {
+    const UUID& uuid,
+    Metrics::CrashSkippedReason reason) {
   INITIALIZATION_STATE_DCHECK_VALID(initialized_);
 
+  Metrics::CrashUploadSkipped(reason);
+
   base::FilePath report_path = LocateCrashReport(uuid);
   if (report_path.empty())
     return kReportNotFound;
@@ -597,6 +604,8 @@ CrashReportDatabase::OperationStatus CrashReportDatabaseMac::RequestUpload(
     return kFileSystemError;
   }
 
+  Metrics::CrashReportPending(Metrics::PendingReportReason::kUserInitiated);
+
   return kNoError;
 }
 

client/crash_report_database_test.cc

@@ -433,7 +433,8 @@ TEST_F(CrashReportDatabaseTest, GetCompletedAndNotUploadedReports) {
 
   // Skip upload for one report.
   EXPECT_EQ(CrashReportDatabase::kNoError,
-            db()->SkipReportUpload(report_3_uuid));
+            db()->SkipReportUpload(
+                report_3_uuid, Metrics::CrashSkippedReason::kUploadsDisabled));
 
   pending.clear();
   EXPECT_EQ(CrashReportDatabase::kNoError, db()->GetPendingReports(&pending));
@@ -608,7 +609,8 @@ TEST_F(CrashReportDatabaseTest, RequestUpload) {
 
   // Skipped report gets back to pending state after RequestUpload is called.
   EXPECT_EQ(CrashReportDatabase::kNoError,
-            db()->SkipReportUpload(report_1_uuid));
+            db()->SkipReportUpload(
+                report_1_uuid, Metrics::CrashSkippedReason::kUploadsDisabled));
 
   std::vector<CrashReportDatabase::Report> pending_reports;
   CrashReportDatabase::OperationStatus os =
@@ -643,7 +645,8 @@ TEST_F(CrashReportDatabaseTest, RequestUpload) {
   // Explicitly requested reports will not have upload_explicitly_requested bit
   // after getting skipped.
   EXPECT_EQ(CrashReportDatabase::kNoError,
-            db()->SkipReportUpload(report_1_uuid));
+            db()->SkipReportUpload(
+                report_1_uuid, Metrics::CrashSkippedReason::kUploadsDisabled));
   CrashReportDatabase::Report report;
   EXPECT_EQ(CrashReportDatabase::kNoError,
             db()->LookUpCrashReport(report_1_uuid, &report));

client/crash_report_database_win.cc

@@ -589,7 +589,8 @@ class CrashReportDatabaseWin : public CrashReportDatabase {
   OperationStatus RecordUploadAttempt(const Report* report,
                                       bool successful,
                                       const std::string& id) override;
-  OperationStatus SkipReportUpload(const UUID& uuid) override;
+  OperationStatus SkipReportUpload(const UUID& uuid,
+                                   Metrics::CrashSkippedReason reason) override;
   OperationStatus DeleteReport(const UUID& uuid) override;
   OperationStatus RequestUpload(const UUID& uuid) override;
 
@@ -679,6 +680,7 @@ OperationStatus CrashReportDatabaseWin::FinishedWritingCrashReport(
                                     ReportState::kPending));
   *uuid = scoped_report->uuid;
 
+  Metrics::CrashReportPending(Metrics::PendingReportReason::kNewlyCreated);
   Metrics::CrashReportSize(handle.get());
 
   return kNoError;
@@ -774,6 +776,8 @@ OperationStatus CrashReportDatabaseWin::RecordUploadAttempt(
     const std::string& id) {
   INITIALIZATION_STATE_DCHECK_VALID(initialized_);
 
+  Metrics::CrashUploadAttempted(successful);
+
   // Take ownership, allocated in GetReportForUploading.
   std::unique_ptr<const Report> upload_report(report);
   std::unique_ptr<Metadata> metadata(AcquireMetadata());
@@ -826,9 +830,13 @@ OperationStatus CrashReportDatabaseWin::DeleteReport(const UUID& uuid) {
   return kNoError;
 }
 
-OperationStatus CrashReportDatabaseWin::SkipReportUpload(const UUID& uuid) {
+OperationStatus CrashReportDatabaseWin::SkipReportUpload(
+    const UUID& uuid,
+    Metrics::CrashSkippedReason reason) {
   INITIALIZATION_STATE_DCHECK_VALID(initialized_);
 
+  Metrics::CrashUploadSkipped(reason);
+
   std::unique_ptr<Metadata> metadata(AcquireMetadata());
   if (!metadata)
     return kDatabaseError;
@@ -885,6 +893,8 @@ OperationStatus CrashReportDatabaseWin::RequestUpload(const UUID& uuid) {
   report_disk->upload_explicitly_requested = true;
   report_disk->state = ReportState::kPending;
 
+  Metrics::CrashReportPending(Metrics::PendingReportReason::kUserInitiated);
+
   return kNoError;
 }
 

client/prune_crash_reports_test.cc

@@ -44,7 +44,8 @@ class MockDatabase : public CrashReportDatabase {
                OperationStatus(const UUID&, const Report**));
   MOCK_METHOD3(RecordUploadAttempt,
                OperationStatus(const Report*, bool, const std::string&));
-  MOCK_METHOD1(SkipReportUpload, OperationStatus(const UUID&));
+  MOCK_METHOD2(SkipReportUpload,
+               OperationStatus(const UUID&, Metrics::CrashSkippedReason));
   MOCK_METHOD1(DeleteReport, OperationStatus(const UUID&));
   MOCK_METHOD1(RequestUpload, OperationStatus(const UUID&));
 };

handler/crash_report_upload_thread.cc

@@ -28,6 +28,7 @@
 #include "snapshot/minidump/process_snapshot_minidump.h"
 #include "snapshot/module_snapshot.h"
 #include "util/file/file_reader.h"
+#include "util/misc/metrics.h"
 #include "util/misc/uuid.h"
 #include "util/net/http_body.h"
 #include "util/net/http_multipart_builder.h"
@@ -196,7 +197,8 @@ void CrashReportUploadThread::ProcessPendingReport(
     // Don’t attempt an upload if there’s no URL to upload to. Allow upload if
     // it has been explicitly requested by the user, otherwise, respect the
     // upload-enabled state stored in the database’s settings.
-    database_->SkipReportUpload(report.uuid);
+    database_->SkipReportUpload(report.uuid,
+                                Metrics::CrashSkippedReason::kUploadsDisabled);
     return;
   }
 
@@ -217,7 +219,8 @@ void CrashReportUploadThread::ProcessPendingReport(
         // attempt to upload the report.
         const int kUploadAttemptIntervalSeconds = 60 * 60;  // 1 hour
         if (now - last_upload_attempt_time < kUploadAttemptIntervalSeconds) {
-          database_->SkipReportUpload(report.uuid);
+          database_->SkipReportUpload(
+              report.uuid, Metrics::CrashSkippedReason::kUploadThrottled);
           return;
         }
       } else {
@@ -228,7 +231,8 @@ void CrashReportUploadThread::ProcessPendingReport(
         // accept it and don’t attempt to upload the report.
         const int kBackwardsClockTolerance = 60 * 60 * 24;  // 1 day
         if (last_upload_attempt_time - now < kBackwardsClockTolerance) {
-          database_->SkipReportUpload(report.uuid);
+          database_->SkipReportUpload(
+              report.uuid, Metrics::CrashSkippedReason::kUnexpectedTime);
           return;
         }
       }
@@ -250,7 +254,8 @@ void CrashReportUploadThread::ProcessPendingReport(
     case CrashReportDatabase::kDatabaseError:
       // In these cases, SkipReportUpload() might not work either, but it’s best
       // to at least try to get the report out of the way.
-      database_->SkipReportUpload(report.uuid);
+      database_->SkipReportUpload(report.uuid,
+                                  Metrics::CrashSkippedReason::kDatabaseError);
       return;
 
     case CrashReportDatabase::kCannotRequestUpload:
@@ -274,7 +279,8 @@ void CrashReportUploadThread::ProcessPendingReport(
       // TODO(mark): Deal with retries properly: don’t call SkipReportUplaod()
       // if the result was kRetry and the report hasn’t already been retried
       // too many times.
-      database_->SkipReportUpload(report.uuid);
+      database_->SkipReportUpload(report.uuid,
+                                  Metrics::CrashSkippedReason::kUploadFailed);
       break;
   }
 }

util/misc/metrics.cc

@@ -35,11 +35,19 @@ enum class ExceptionProcessingState {
 
 void ExceptionProcessing(ExceptionProcessingState state) {
   UMA_HISTOGRAM_COUNTS("Crashpad.ExceptionEncountered",
-                       static_cast<int>(state));
+                       static_cast<int32_t>(state));
 }
 
 }  // namespace
 
+// static
+void Metrics::CrashReportPending(PendingReportReason reason) {
+  UMA_HISTOGRAM_ENUMERATION(
+      "Crashpad.CrashReportPending",
+      static_cast<int32_t>(reason),
+      static_cast<int32_t>(PendingReportReason::kMaxValue));
+}
+
 // static
 void Metrics::CrashReportSize(FileHandle file) {
   const FileOffset size = LoggingFileSizeByHandle(file);
@@ -47,6 +55,20 @@ void Metrics::CrashReportSize(FileHandle file) {
       "Crashpad.CrashReportSize", size, 0, 20 * 1024 * 1024, 50);
 }
 
+// static
+void Metrics::CrashUploadAttempted(bool successful) {
+  UMA_HISTOGRAM_COUNTS("Crashpad.CrashUpload.AttemptSuccessful",
+                       static_cast<int32_t>(successful));
+}
+
+// static
+void Metrics::CrashUploadSkipped(CrashSkippedReason reason) {
+  UMA_HISTOGRAM_ENUMERATION(
+      "Crashpad.CrashUpload.Skipped",
+      static_cast<int32_t>(reason),
+      static_cast<int32_t>(CrashSkippedReason::kMaxValue));
+}
+
 // static
 void Metrics::ExceptionCaptureResult(CaptureResult result) {
   ExceptionProcessing(ExceptionProcessingState::kFinished);

util/misc/metrics.h

@@ -30,12 +30,59 @@ namespace crashpad {
 //! Chromium's base, they allow integration with its metrics system.
 class Metrics {
  public:
+  //! \brief Values for CrashReportPending(). These are used as metrics
+  //!     enumeration values, so new values should always be added at the end.
+  enum class PendingReportReason : int32_t {
+    //! \brief A report was newly created and is ready for upload.
+    kNewlyCreated = 0,
+
+    //! \brief The user manually requested the report be uploaded.
+    kUserInitiated = 1,
+
+    //! \brief The number of values in this enumeration; not a valid value.
+    kMaxValue
+  };
+
+  //! \brief Reports when a crash upload has entered the pending state.
+  static void CrashReportPending(PendingReportReason reason);
+
   //! \brief Reports the size of a crash report file in bytes. Should be called
   //!     when a new report is written to disk.
   static void CrashReportSize(FileHandle file);
 
+  //! \brief Reports on a crash upload attempt, and if it succeeded.
+  static void CrashUploadAttempted(bool successful);
+
+  //! \brief Values for CrashUploadSkipped(). These are used as metrics
+  //!     enumeration values, so new values should always be added at the end.
+  enum class CrashSkippedReason : int32_t {
+    //! \brief Crash uploading is disabled.
+    kUploadsDisabled = 0,
+
+    //! \brief There was another upload too recently, so this one was throttled.
+    kUploadThrottled = 1,
+
+    //! \brief The report had an unexpected timestamp.
+    kUnexpectedTime = 2,
+
+    //! \brief The database reported an error, likely due to a filesystem
+    //!     problem.
+    kDatabaseError = 3,
+
+    //! \brief The upload of the crash failed during communication with the
+    //!     server.
+    kUploadFailed = 4,
+
+    //! \brief The number of values in this enumeration; not a valid value.
+    kMaxValue
+  };
+
+  //! \brief Reports when a report is moved to the completed state in the
+  //!     database, without the report being uploadad.
+  static void CrashUploadSkipped(CrashSkippedReason reason);
+
   //! \brief The result of capturing an exception. These are used as metrics
-  //!     enumeration values so new values should always be added at the end.
+  //!     enumeration values, so new values should always be added at the end.
   enum class CaptureResult : int32_t {
     //! \brief The exception capture succeeded normally.
     kSuccess = 0,