From 2418cb8fbef8f2862286bb4abdcd4f8778aef5a8 Mon Sep 17 00:00:00 2001 From: Joshua Peraza Date: Tue, 24 Jul 2018 09:13:49 -0700 Subject: [PATCH] Make upload report metrics optional Bug: crashpad:30 Change-Id: I202e4571ee8dc8006550173c1cf0c735fae29103 Reviewed-on: https://chromium-review.googlesource.com/1148580 Reviewed-by: Mark Mentovai Commit-Queue: Joshua Peraza --- client/crash_report_database.cc | 3 ++- client/crash_report_database.h | 8 +++++++- client/crash_report_database_generic.cc | 11 ++++++++--- client/crash_report_database_mac.mm | 11 ++++++++--- client/crash_report_database_win.cc | 11 ++++++++--- client/prune_crash_reports_test.cc | 5 +++-- 6 files changed, 36 insertions(+), 13 deletions(-) diff --git a/client/crash_report_database.cc b/client/crash_report_database.cc index aa365beb..d300a8f9 100644 --- a/client/crash_report_database.cc +++ b/client/crash_report_database.cc @@ -69,7 +69,8 @@ CrashReportDatabase::UploadReport::UploadReport() reader_(std::make_unique()), database_(nullptr), attachment_readers_(), - attachment_map_() {} + attachment_map_(), + report_metrics_(false) {} CrashReportDatabase::UploadReport::~UploadReport() { if (database_) { diff --git a/client/crash_report_database.h b/client/crash_report_database.h index 9ceeddc3..d115c74b 100644 --- a/client/crash_report_database.h +++ b/client/crash_report_database.h @@ -178,6 +178,7 @@ class CrashReportDatabase { CrashReportDatabase* database_; std::vector> attachment_readers_; std::map attachment_map_; + bool report_metrics_; DISALLOW_COPY_AND_ASSIGN(UploadReport); }; @@ -326,11 +327,16 @@ class CrashReportDatabase { //! \param[in] uuid The unique identifier for the crash report record. //! \param[out] report A crash report record for the report to be uploaded. //! Only valid if this returns #kNoError. + //! \param[in] report_metrics If `false`, metrics will not be recorded for + //! this upload attempt when RecordUploadComplete() is called or \a report + //! is destroyed. Metadata for the upload attempt will still be recorded + //! in the database. //! //! \return The operation status code. virtual OperationStatus GetReportForUploading( const UUID& uuid, - std::unique_ptr* report) = 0; + std::unique_ptr* report, + bool report_metrics = true) = 0; //! \brief Records a successful upload for a report and updates the last //! upload attempt time as returned by diff --git a/client/crash_report_database_generic.cc b/client/crash_report_database_generic.cc index 6dc8e9e6..8e932374 100644 --- a/client/crash_report_database_generic.cc +++ b/client/crash_report_database_generic.cc @@ -180,7 +180,8 @@ class CrashReportDatabaseGeneric : public CrashReportDatabase { OperationStatus GetCompletedReports(std::vector* reports) override; OperationStatus GetReportForUploading( const UUID& uuid, - std::unique_ptr* report) override; + std::unique_ptr* report, + bool report_metrics) override; OperationStatus SkipReportUpload(const UUID& uuid, Metrics::CrashSkippedReason reason) override; OperationStatus DeleteReport(const UUID& uuid) override; @@ -455,7 +456,8 @@ OperationStatus CrashReportDatabaseGeneric::GetCompletedReports( OperationStatus CrashReportDatabaseGeneric::GetReportForUploading( const UUID& uuid, - std::unique_ptr* report) { + std::unique_ptr* report, + bool report_metrics) { INITIALIZATION_STATE_DCHECK_VALID(initialized_); auto upload_report = std::make_unique(); @@ -470,6 +472,7 @@ OperationStatus CrashReportDatabaseGeneric::GetReportForUploading( if (!upload_report->Initialize(path, this)) { return kFileSystemError; } + upload_report->report_metrics_ = report_metrics; report->reset(upload_report.release()); return kNoError; @@ -609,7 +612,9 @@ OperationStatus CrashReportDatabaseGeneric::RecordUploadAttempt( const std::string& id) { INITIALIZATION_STATE_DCHECK_VALID(initialized_); - Metrics::CrashUploadAttempted(successful); + if (report->report_metrics_) { + Metrics::CrashUploadAttempted(successful); + } time_t now = time(nullptr); report->id = id; diff --git a/client/crash_report_database_mac.mm b/client/crash_report_database_mac.mm index 79b876d2..3106dc2c 100644 --- a/client/crash_report_database_mac.mm +++ b/client/crash_report_database_mac.mm @@ -141,7 +141,8 @@ class CrashReportDatabaseMac : public CrashReportDatabase { OperationStatus GetCompletedReports(std::vector* reports) override; OperationStatus GetReportForUploading( const UUID& uuid, - std::unique_ptr* report) override; + std::unique_ptr* report, + bool report_metrics) override; OperationStatus SkipReportUpload(const UUID& uuid, Metrics::CrashSkippedReason reason) override; OperationStatus DeleteReport(const UUID& uuid) override; @@ -422,7 +423,8 @@ CrashReportDatabaseMac::GetCompletedReports( CrashReportDatabase::OperationStatus CrashReportDatabaseMac::GetReportForUploading( const UUID& uuid, - std::unique_ptr* report) { + std::unique_ptr* report, + bool report_metrics) { INITIALIZATION_STATE_DCHECK_VALID(initialized_); auto upload_report = std::make_unique(); @@ -444,6 +446,7 @@ CrashReportDatabaseMac::GetReportForUploading( upload_report->database_ = this; upload_report->lock_fd.reset(lock.release()); + upload_report->report_metrics_ = report_metrics; report->reset(upload_report.release()); return kNoError; } @@ -454,7 +457,9 @@ CrashReportDatabaseMac::RecordUploadAttempt(UploadReport* report, const std::string& id) { INITIALIZATION_STATE_DCHECK_VALID(initialized_); - Metrics::CrashUploadAttempted(successful); + if (report->report_metrics_) { + Metrics::CrashUploadAttempted(successful); + } DCHECK(report); DCHECK(successful || id.empty()); diff --git a/client/crash_report_database_win.cc b/client/crash_report_database_win.cc index e19e6058..89677706 100644 --- a/client/crash_report_database_win.cc +++ b/client/crash_report_database_win.cc @@ -594,7 +594,8 @@ class CrashReportDatabaseWin : public CrashReportDatabase { OperationStatus GetCompletedReports(std::vector* reports) override; OperationStatus GetReportForUploading( const UUID& uuid, - std::unique_ptr* report) override; + std::unique_ptr* report, + bool report_metrics) override; OperationStatus SkipReportUpload(const UUID& uuid, Metrics::CrashSkippedReason reason) override; OperationStatus DeleteReport(const UUID& uuid) override; @@ -731,7 +732,8 @@ OperationStatus CrashReportDatabaseWin::GetCompletedReports( OperationStatus CrashReportDatabaseWin::GetReportForUploading( const UUID& uuid, - std::unique_ptr* report) { + std::unique_ptr* report, + bool report_metrics) { INITIALIZATION_STATE_DCHECK_VALID(initialized_); std::unique_ptr metadata(AcquireMetadata()); @@ -749,6 +751,7 @@ OperationStatus CrashReportDatabaseWin::GetReportForUploading( if (!upload_report->Initialize(upload_report->file_path, this)) { return kFileSystemError; } + upload_report->report_metrics_ = report_metrics; report->reset(upload_report.release()); } @@ -761,7 +764,9 @@ OperationStatus CrashReportDatabaseWin::RecordUploadAttempt( const std::string& id) { INITIALIZATION_STATE_DCHECK_VALID(initialized_); - Metrics::CrashUploadAttempted(successful); + if (report->report_metrics_) { + Metrics::CrashUploadAttempted(successful); + } std::unique_ptr metadata(AcquireMetadata()); if (!metadata) diff --git a/client/prune_crash_reports_test.cc b/client/prune_crash_reports_test.cc index 2648dee2..01990d27 100644 --- a/client/prune_crash_reports_test.cc +++ b/client/prune_crash_reports_test.cc @@ -41,9 +41,10 @@ class MockDatabase : public CrashReportDatabase { MOCK_METHOD2(LookUpCrashReport, OperationStatus(const UUID&, Report*)); MOCK_METHOD1(GetPendingReports, OperationStatus(std::vector*)); MOCK_METHOD1(GetCompletedReports, OperationStatus(std::vector*)); - MOCK_METHOD2(GetReportForUploading, + MOCK_METHOD3(GetReportForUploading, OperationStatus(const UUID&, - std::unique_ptr*)); + std::unique_ptr*, + bool report_metrics)); MOCK_METHOD3(RecordUploadAttempt, OperationStatus(UploadReport*, bool, const std::string&)); MOCK_METHOD2(SkipReportUpload,