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 <bsimonnet@chromium.org>
Tested-by: Bertrand Simonnet <bsimonnet@chromium.org>
Reviewed-by: Nathan Bullock <nathanbullock@google.com>
Commit-Queue: Bertrand Simonnet <bsimonnet@chromium.org>
diff --git a/metrics/metrics_daemon.cc b/metrics/metrics_daemon.cc
index 4a69d8b..4ea899f 100644
--- a/metrics/metrics_daemon.cc
+++ b/metrics/metrics_daemon.cc
@@ -191,6 +191,7 @@
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 @@
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 24db112..a90f3e6 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 @@
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 0f1047f..99892bf 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 @@
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 dd49d1f..92c9e10 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::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 0b087de..ebbb54f 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 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 @@
// Private constructor for use in unit testing.
UploadService(SystemProfileSetter* setter,
+ MetricsLibraryInterface* metrics_lib,
const std::string& server,
bool testing);
@@ -134,6 +138,7 @@
MetricsLog* GetOrCreateCurrentLog();
scoped_ptr<SystemProfileSetter> system_profile_setter_;
+ MetricsLibraryInterface* metrics_lib_;
base::HistogramSnapshotManager histogram_snapshot_manager_;
scoped_ptr<Sender> sender_;
int failed_upload_count_;
diff --git a/metrics/uploader/upload_service_test.cc b/metrics/uploader/upload_service_test.cc
index ca90e85..d04cc72 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 @@
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 @@
SenderMock* sender_;
SystemProfileCache cache_;
UploadService upload_service_;
+ MetricsLibraryMock metrics_lib_;
scoped_ptr<base::AtExitManager> exit_manager_;
};
@@ -126,7 +129,8 @@
}
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.