From 1f14655b27c3e33ee1e93cd52b2ad9ab79018630 Mon Sep 17 00:00:00 2001 From: Bertrand SIMONNET Date: Wed, 19 Aug 2015 18:13:02 -0700 Subject: [PATCH] metricsd: Don't crash when some metadata is missing. Instead of crashing when the build target id is missing, simply print a useful warning and discard the log. BUG: 23351227 Change-Id: I3abf3063d6440b07103db29938eec5071ea8f60b --- metricsd/uploader/metrics_log.cc | 4 ++-- metricsd/uploader/metrics_log.h | 2 +- metricsd/uploader/system_profile_cache.cc | 11 +++++++---- metricsd/uploader/system_profile_cache.h | 2 +- metricsd/uploader/system_profile_setter.h | 2 +- metricsd/uploader/upload_service.cc | 17 ++++++++++------- 6 files changed, 22 insertions(+), 16 deletions(-) diff --git a/metricsd/uploader/metrics_log.cc b/metricsd/uploader/metrics_log.cc index 5f4c5991e..1f16ca15d 100644 --- a/metricsd/uploader/metrics_log.cc +++ b/metricsd/uploader/metrics_log.cc @@ -48,6 +48,6 @@ void MetricsLog::IncrementUncleanShutdownCount() { stability->set_unclean_system_shutdown_count(current + 1); } -void MetricsLog::PopulateSystemProfile(SystemProfileSetter* profile_setter) { - profile_setter->Populate(uma_proto()); +bool MetricsLog::PopulateSystemProfile(SystemProfileSetter* profile_setter) { + return profile_setter->Populate(uma_proto()); } diff --git a/metricsd/uploader/metrics_log.h b/metricsd/uploader/metrics_log.h index 50fed8938..5e090701b 100644 --- a/metricsd/uploader/metrics_log.h +++ b/metricsd/uploader/metrics_log.h @@ -39,7 +39,7 @@ class MetricsLog : public metrics::MetricsLogBase { void IncrementUncleanShutdownCount(); // Populate the system profile with system information using setter. - void PopulateSystemProfile(SystemProfileSetter* setter); + bool PopulateSystemProfile(SystemProfileSetter* setter); private: FRIEND_TEST(UploadServiceTest, LogContainsAggregatedValues); diff --git a/metricsd/uploader/system_profile_cache.cc b/metricsd/uploader/system_profile_cache.cc index 35910d7dd..7dd0323a7 100644 --- a/metricsd/uploader/system_profile_cache.cc +++ b/metricsd/uploader/system_profile_cache.cc @@ -75,7 +75,7 @@ bool SystemProfileCache::Initialize() { if (!base::SysInfo::GetLsbReleaseValue("BRILLO_BUILD_TARGET_ID", &profile_.build_target_id)) { - LOG(ERROR) << "Could not initialize system profile."; + LOG(ERROR) << "BRILLO_BUILD_TARGET_ID is not set in /etc/lsb-release."; return false; } @@ -109,11 +109,12 @@ bool SystemProfileCache::InitializeOrCheck() { return initialized_ || Initialize(); } -void SystemProfileCache::Populate( +bool SystemProfileCache::Populate( metrics::ChromeUserMetricsExtension* metrics_proto) { CHECK(metrics_proto); - CHECK(InitializeOrCheck()) - << "failed to initialize system information."; + if (not InitializeOrCheck()) { + return false; + } // The client id is hashed before being sent. metrics_proto->set_client_id( @@ -132,6 +133,8 @@ void SystemProfileCache::Populate( metrics::SystemProfileProto_BrilloDeviceData* device_data = profile_proto->mutable_brillo(); device_data->set_build_target_id(profile_.build_target_id); + + return true; } std::string SystemProfileCache::GetPersistentGUID( diff --git a/metricsd/uploader/system_profile_cache.h b/metricsd/uploader/system_profile_cache.h index c53a18ec7..ac80b47ff 100644 --- a/metricsd/uploader/system_profile_cache.h +++ b/metricsd/uploader/system_profile_cache.h @@ -52,7 +52,7 @@ class SystemProfileCache : public SystemProfileSetter { SystemProfileCache(bool testing, const std::string& config_root); // Populates the ProfileSystem protobuf with system information. - void Populate(metrics::ChromeUserMetricsExtension* metrics_proto) override; + bool Populate(metrics::ChromeUserMetricsExtension* metrics_proto) override; // Converts a string representation of the channel to a // SystemProfileProto_Channel diff --git a/metricsd/uploader/system_profile_setter.h b/metricsd/uploader/system_profile_setter.h index cd311a45f..bd3ff42c8 100644 --- a/metricsd/uploader/system_profile_setter.h +++ b/metricsd/uploader/system_profile_setter.h @@ -27,7 +27,7 @@ class SystemProfileSetter { public: virtual ~SystemProfileSetter() {} // Populates the protobuf with system informations. - virtual void Populate(metrics::ChromeUserMetricsExtension* profile_proto) = 0; + virtual bool Populate(metrics::ChromeUserMetricsExtension* profile_proto) = 0; }; #endif // METRICS_UPLOADER_SYSTEM_PROFILE_SETTER_H_ diff --git a/metricsd/uploader/upload_service.cc b/metricsd/uploader/upload_service.cc index 63b5789fe..23356307b 100644 --- a/metricsd/uploader/upload_service.cc +++ b/metricsd/uploader/upload_service.cc @@ -73,7 +73,6 @@ void UploadService::StartNewLog() { CHECK(!staged_log_) << "the staged log should be discarded before starting " "a new metrics log"; MetricsLog* log = new MetricsLog(); - log->PopulateSystemProfile(system_profile_setter_.get()); current_log_.reset(log); } @@ -97,13 +96,12 @@ void UploadService::UploadEvent() { // Previous upload successful, reading metrics sample from the file. ReadMetrics(); GatherHistograms(); - - // No samples found. Exit to avoid sending an empty log. - if (!current_log_) - return; - StageCurrentLog(); - SendStagedLog(); + + // If a log is available for upload, upload it. + if (staged_log_) { + SendStagedLog(); + } } void UploadService::SendStagedLog() { @@ -225,6 +223,11 @@ void UploadService::StageCurrentLog() { staged_log_.swap(current_log_); staged_log_->CloseLog(); + if (!staged_log_->PopulateSystemProfile(system_profile_setter_.get())) { + LOG(WARNING) << "Error while adding metadata to the log. Discarding the " + << "log."; + staged_log_.reset(); + } failed_upload_count_ = 0; }