From e4fa61e05e17d759dae5a724d67c34dafa3d706a Mon Sep 17 00:00:00 2001 From: Bertrand SIMONNET Date: Wed, 18 Feb 2015 09:38:55 -0800 Subject: [PATCH] metrics: don't upload metrics when metrics are disabled The uploader should only send metrics samples when the metrics are enabled. The uploader daemon is still started when the metrics are disabled so that: * When we enable the metrics, we don't require a restart of metrics_daemon to start uploading metrics. * The metrics file is truncated periodically and avoid taking too much space on long running system with metrics disabled. BUG=chromium:459636 TEST=unittests TEST=`test_that -b gizmo gizmo platform_MetricsUploader` works TEST=manual: uploader does not upload metrics if metrics are disabled. CQ-DEPEND=CL:250980 Change-Id: I9f5da3457066a183c5791b5488e985b7ab13b6e1 Reviewed-on: https://chromium-review.googlesource.com/250822 Trybot-Ready: Bertrand Simonnet Tested-by: Bertrand Simonnet Reviewed-by: Nathan Bullock Commit-Queue: Bertrand Simonnet --- metrics/metrics_daemon.cc | 3 ++- metrics/metrics_library.h | 3 ++- metrics/metrics_library_mock.h | 4 ++++ metrics/uploader/upload_service.cc | 12 +++++++++++- metrics/uploader/upload_service.h | 5 +++++ metrics/uploader/upload_service_test.cc | 8 ++++++-- 6 files changed, 30 insertions(+), 5 deletions(-) diff --git a/metrics/metrics_daemon.cc b/metrics/metrics_daemon.cc index 4a69d8b4c..4ea899f51 100644 --- a/metrics/metrics_daemon.cc +++ b/metrics/metrics_daemon.cc @@ -191,6 +191,7 @@ int MetricsDaemon::Run() { void MetricsDaemon::RunUploaderTest() { upload_service_.reset(new UploadService(new SystemProfileCache(true, config_root_), + metrics_lib_, server_)); upload_service_->Init(upload_interval_, metrics_file_); upload_service_->UploadEvent(); @@ -341,7 +342,7 @@ int MetricsDaemon::OnInit() { if (IsOnOfficialBuild()) { LOG(INFO) << "uploader enabled"; upload_service_.reset( - new UploadService(new SystemProfileCache(), server_)); + new UploadService(new SystemProfileCache(), metrics_lib_, server_)); upload_service_->Init(upload_interval_, metrics_file_); } else { LOG(INFO) << "uploader disabled on non-official build"; diff --git a/metrics/metrics_library.h b/metrics/metrics_library.h index 24db112d5..a90f3e61a 100644 --- a/metrics/metrics_library.h +++ b/metrics/metrics_library.h @@ -19,6 +19,7 @@ class MetricsLibraryInterface { public: virtual void Init() = 0; + virtual bool AreMetricsEnabled() = 0; virtual bool SendToUMA(const std::string& name, int sample, int min, int max, int nbuckets) = 0; virtual bool SendEnumToUMA(const std::string& name, int sample, int max) = 0; @@ -40,7 +41,7 @@ class MetricsLibrary : public MetricsLibraryInterface { bool IsGuestMode(); // Returns whether or not metrics collection is enabled. - bool AreMetricsEnabled(); + bool AreMetricsEnabled() override; // Sends histogram data to Chrome for transport to UMA and returns // true on success. This method results in the equivalent of an diff --git a/metrics/metrics_library_mock.h b/metrics/metrics_library_mock.h index 0f1047f2e..99892bfa0 100644 --- a/metrics/metrics_library_mock.h +++ b/metrics/metrics_library_mock.h @@ -13,6 +13,8 @@ class MetricsLibraryMock : public MetricsLibraryInterface { public: + bool metrics_enabled_ = true; + MOCK_METHOD0(Init, void()); MOCK_METHOD5(SendToUMA, bool(const std::string& name, int sample, int min, int max, int nbuckets)); @@ -20,6 +22,8 @@ class MetricsLibraryMock : public MetricsLibraryInterface { int max)); MOCK_METHOD2(SendSparseToUMA, bool(const std::string& name, int sample)); MOCK_METHOD1(SendUserActionToUMA, bool(const std::string& action)); + + bool AreMetricsEnabled() override {return metrics_enabled_;}; }; #endif // METRICS_METRICS_LIBRARY_MOCK_H_ diff --git a/metrics/uploader/upload_service.cc b/metrics/uploader/upload_service.cc index dd49d1f67..92c9e1061 100644 --- a/metrics/uploader/upload_service.cc +++ b/metrics/uploader/upload_service.cc @@ -26,17 +26,20 @@ const int UploadService::kMaxFailedUpload = 10; UploadService::UploadService(SystemProfileSetter* setter, + MetricsLibraryInterface* metrics_lib, const std::string& server) : system_profile_setter_(setter), + metrics_lib_(metrics_lib), histogram_snapshot_manager_(this), sender_(new HttpSender(server)), testing_(false) { } UploadService::UploadService(SystemProfileSetter* setter, + MetricsLibraryInterface* metrics_lib, const std::string& server, bool testing) - : UploadService(setter, server) { + : UploadService(setter, metrics_lib, server) { testing_ = testing; } @@ -94,6 +97,13 @@ void UploadService::UploadEvent() { void UploadService::SendStagedLog() { CHECK(staged_log_) << "staged_log_ must exist to be sent"; + // If metrics are not enabled, discard the log and exit. + if (!metrics_lib_->AreMetricsEnabled()) { + LOG(INFO) << "Metrics disabled. Don't upload metrics samples."; + staged_log_.reset(); + return; + } + std::string log_text; staged_log_->GetEncodedLog(&log_text); if (!sender_->Send(log_text, base::SHA1HashString(log_text))) { diff --git a/metrics/uploader/upload_service.h b/metrics/uploader/upload_service.h index 0b087de12..ebbb54f2a 100644 --- a/metrics/uploader/upload_service.h +++ b/metrics/uploader/upload_service.h @@ -10,6 +10,8 @@ #include "base/metrics/histogram_base.h" #include "base/metrics/histogram_flattener.h" #include "base/metrics/histogram_snapshot_manager.h" + +#include "metrics/metrics_library.h" #include "metrics/uploader/metrics_log.h" #include "metrics/uploader/sender.h" #include "metrics/uploader/system_profile_cache.h" @@ -55,6 +57,7 @@ class SystemProfileSetter; class UploadService : public base::HistogramFlattener { public: explicit UploadService(SystemProfileSetter* setter, + MetricsLibraryInterface* metrics_lib, const std::string& server); void Init(const base::TimeDelta& upload_interval, @@ -99,6 +102,7 @@ class UploadService : public base::HistogramFlattener { // Private constructor for use in unit testing. UploadService(SystemProfileSetter* setter, + MetricsLibraryInterface* metrics_lib, const std::string& server, bool testing); @@ -134,6 +138,7 @@ class UploadService : public base::HistogramFlattener { MetricsLog* GetOrCreateCurrentLog(); scoped_ptr system_profile_setter_; + MetricsLibraryInterface* metrics_lib_; base::HistogramSnapshotManager histogram_snapshot_manager_; scoped_ptr sender_; int failed_upload_count_; diff --git a/metrics/uploader/upload_service_test.cc b/metrics/uploader/upload_service_test.cc index ca90e85ea..d04cc72c4 100644 --- a/metrics/uploader/upload_service_test.cc +++ b/metrics/uploader/upload_service_test.cc @@ -12,6 +12,7 @@ #include "components/metrics/proto/chrome_user_metrics_extension.pb.h" #include "components/metrics/proto/histogram_event.pb.h" #include "components/metrics/proto/system_profile.pb.h" +#include "metrics/metrics_library_mock.h" #include "metrics/serialization/metric_sample.h" #include "metrics/uploader/metrics_log.h" #include "metrics/uploader/mock/mock_system_profile_setter.h" @@ -26,7 +27,8 @@ class UploadServiceTest : public testing::Test { protected: UploadServiceTest() : cache_(true, "/"), - upload_service_(new MockSystemProfileSetter(), kMetricsServer, true), + upload_service_(new MockSystemProfileSetter(), &metrics_lib_, + kMetricsServer, true), exit_manager_(new base::AtExitManager()) { sender_ = new SenderMock; upload_service_.sender_.reset(sender_); @@ -52,6 +54,7 @@ class UploadServiceTest : public testing::Test { SenderMock* sender_; SystemProfileCache cache_; UploadService upload_service_; + MetricsLibraryMock metrics_lib_; scoped_ptr exit_manager_; }; @@ -126,7 +129,8 @@ TEST_F(UploadServiceTest, EmptyLogsAreNotSent) { } TEST_F(UploadServiceTest, LogEmptyByDefault) { - UploadService upload_service(new MockSystemProfileSetter(), kMetricsServer); + UploadService upload_service(new MockSystemProfileSetter(), &metrics_lib_, + kMetricsServer); // current_log_ should be initialized later as it needs AtExitManager to exit // in order to gather system information from SysInfo.