From ac6c01b5752ecb1aa1da0ea613740cf6825bb72e Mon Sep 17 00:00:00 2001 From: Scott Graham Date: Mon, 26 Sep 2016 15:06:19 -0700 Subject: [PATCH] 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 --- client/crash_report_database.h | 7 +++- client/crash_report_database_mac.mm | 13 +++++-- client/crash_report_database_test.cc | 9 +++-- client/crash_report_database_win.cc | 14 ++++++-- client/prune_crash_reports_test.cc | 3 +- handler/crash_report_upload_thread.cc | 16 ++++++--- util/misc/metrics.cc | 24 ++++++++++++- util/misc/metrics.h | 49 ++++++++++++++++++++++++++- 8 files changed, 119 insertions(+), 16 deletions(-) diff --git a/client/crash_report_database.h b/client/crash_report_database.h index 7fef8a2e..3768cb23 100644 --- a/client/crash_report_database.h +++ b/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. //! diff --git a/client/crash_report_database_mac.mm b/client/crash_report_database_mac.mm index 861a699e..f9c783eb 100644 --- a/client/crash_report_database_mac.mm +++ b/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; } diff --git a/client/crash_report_database_test.cc b/client/crash_report_database_test.cc index 200619d2..72804643 100644 --- a/client/crash_report_database_test.cc +++ b/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 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)); diff --git a/client/crash_report_database_win.cc b/client/crash_report_database_win.cc index 5a7db2d1..4fccd3fd 100644 --- a/client/crash_report_database_win.cc +++ b/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 upload_report(report); std::unique_ptr 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(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; } diff --git a/client/prune_crash_reports_test.cc b/client/prune_crash_reports_test.cc index 4e7c3da5..7772d20d 100644 --- a/client/prune_crash_reports_test.cc +++ b/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&)); }; diff --git a/handler/crash_report_upload_thread.cc b/handler/crash_report_upload_thread.cc index 99c76b77..23dda482 100644 --- a/handler/crash_report_upload_thread.cc +++ b/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; } } diff --git a/util/misc/metrics.cc b/util/misc/metrics.cc index 6967398d..db7718b7 100644 --- a/util/misc/metrics.cc +++ b/util/misc/metrics.cc @@ -35,11 +35,19 @@ enum class ExceptionProcessingState { void ExceptionProcessing(ExceptionProcessingState state) { UMA_HISTOGRAM_COUNTS("Crashpad.ExceptionEncountered", - static_cast(state)); + static_cast(state)); } } // namespace +// static +void Metrics::CrashReportPending(PendingReportReason reason) { + UMA_HISTOGRAM_ENUMERATION( + "Crashpad.CrashReportPending", + static_cast(reason), + static_cast(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(successful)); +} + +// static +void Metrics::CrashUploadSkipped(CrashSkippedReason reason) { + UMA_HISTOGRAM_ENUMERATION( + "Crashpad.CrashUpload.Skipped", + static_cast(reason), + static_cast(CrashSkippedReason::kMaxValue)); +} + // static void Metrics::ExceptionCaptureResult(CaptureResult result) { ExceptionProcessing(ExceptionProcessingState::kFinished); diff --git a/util/misc/metrics.h b/util/misc/metrics.h index 64fb1af3..18666bec 100644 --- a/util/misc/metrics.h +++ b/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,