diff options
-rw-r--r-- | odrefresh/odr_metrics.cc | 53 | ||||
-rw-r--r-- | odrefresh/odr_metrics.h | 43 | ||||
-rw-r--r-- | odrefresh/odr_metrics_test.cc | 139 | ||||
-rw-r--r-- | odrefresh/odr_statslog_android.cc | 2 | ||||
-rw-r--r-- | odrefresh/odrefresh.cc | 28 | ||||
-rw-r--r-- | odrefresh/odrefresh.h | 2 | ||||
-rw-r--r-- | odrefresh/odrefresh_main.cc | 13 |
7 files changed, 116 insertions, 164 deletions
diff --git a/odrefresh/odr_metrics.cc b/odrefresh/odr_metrics.cc index 57a0241609..b59ed717a8 100644 --- a/odrefresh/odr_metrics.cc +++ b/odrefresh/odr_metrics.cc @@ -36,7 +36,7 @@ namespace art { namespace odrefresh { OdrMetrics::OdrMetrics(const std::string& cache_directory, const std::string& metrics_file) - : cache_directory_(cache_directory), metrics_file_(metrics_file), status_(Status::kOK) { + : cache_directory_(cache_directory), metrics_file_(metrics_file) { DCHECK(StartsWith(metrics_file_, "/")); // Remove existing metrics file if it exists. @@ -56,14 +56,19 @@ OdrMetrics::OdrMetrics(const std::string& cache_directory, const std::string& me } OdrMetrics::~OdrMetrics() { - cache_space_free_end_mib_ = GetFreeSpaceMiB(cache_directory_); + CaptureSpaceFreeEnd(); - // Log metrics only if odrefresh detected a reason to compile. - if (trigger_.has_value()) { + // Log metrics only if this is explicitly enabled (typically when compilation was done or an error + // occurred). + if (enabled_) { WriteToFile(metrics_file_, this); } } +void OdrMetrics::CaptureSpaceFreeEnd() { + cache_space_free_end_mib_ = GetFreeSpaceMiB(cache_directory_); +} + void OdrMetrics::SetCompilationTime(int32_t millis) { switch (stage_) { case Stage::kPrimaryBootClasspath: @@ -102,12 +107,6 @@ void OdrMetrics::SetDex2OatResult(const ExecResult& dex2oat_result) { } } -void OdrMetrics::SetStage(Stage stage) { - if (status_ == Status::kOK) { - stage_ = stage; - } -} - int32_t OdrMetrics::GetFreeSpaceMiB(const std::string& path) { static constexpr uint32_t kBytesPerMiB = 1024 * 1024; static constexpr uint64_t kNominalMaximumCacheBytes = 1024 * kBytesPerMiB; @@ -134,29 +133,23 @@ int32_t OdrMetrics::GetFreeSpaceMiB(const std::string& path) { return static_cast<int32_t>(free_space_mib); } -bool OdrMetrics::ToRecord(/*out*/OdrMetricsRecord* record) const { - if (!trigger_.has_value()) { - return false; - } - record->odrefresh_metrics_version = kOdrefreshMetricsVersion; - record->art_apex_version = art_apex_version_; - record->trigger = static_cast<uint32_t>(trigger_.value()); - record->stage_reached = static_cast<uint32_t>(stage_); - record->status = static_cast<uint32_t>(status_); - record->cache_space_free_start_mib = cache_space_free_start_mib_; - record->cache_space_free_end_mib = cache_space_free_end_mib_; - record->primary_bcp_compilation_millis = primary_bcp_compilation_millis_; - record->secondary_bcp_compilation_millis = secondary_bcp_compilation_millis_; - record->system_server_compilation_millis = system_server_compilation_millis_; - return true; +OdrMetricsRecord OdrMetrics::ToRecord() const { + return { + .odrefresh_metrics_version = kOdrefreshMetricsVersion, + .art_apex_version = art_apex_version_, + .trigger = static_cast<int32_t>(trigger_), + .stage_reached = static_cast<int32_t>(stage_), + .status = static_cast<int32_t>(status_), + .cache_space_free_start_mib = cache_space_free_start_mib_, + .cache_space_free_end_mib = cache_space_free_end_mib_, + .primary_bcp_compilation_millis = primary_bcp_compilation_millis_, + .secondary_bcp_compilation_millis = secondary_bcp_compilation_millis_, + .system_server_compilation_millis = system_server_compilation_millis_, + }; } void OdrMetrics::WriteToFile(const std::string& path, const OdrMetrics* metrics) { - OdrMetricsRecord record{}; - if (!metrics->ToRecord(&record)) { - LOG(ERROR) << "Attempting to report metrics without a compilation trigger."; - return; - } + OdrMetricsRecord record = metrics->ToRecord(); const android::base::Result<void>& result = record.WriteToFile(path); if (!result.ok()) { diff --git a/odrefresh/odr_metrics.h b/odrefresh/odr_metrics.h index afe3efd348..4a29ede548 100644 --- a/odrefresh/odr_metrics.h +++ b/odrefresh/odr_metrics.h @@ -58,6 +58,8 @@ class OdrMetrics final { kTimeLimitExceeded = 5, kStagingFailed = 6, kInstallFailed = 7, + // Failed to access the dalvik-cache directory due to lack of permission. + kDalvikCachePermissionDenied = 8, }; // Enumeration describing the cause of compilation (if any) in odrefresh. @@ -75,20 +77,17 @@ class OdrMetrics final { const std::string& metrics_file = kOdrefreshMetricsFile); ~OdrMetrics(); + // Enables/disables metrics writing. + void SetEnabled(bool value) { enabled_ = value; } + // Gets the ART APEX that metrics are being collected on behalf of. - int64_t GetArtApexVersion() const { - return art_apex_version_; - } + int64_t GetArtApexVersion() const { return art_apex_version_; } // Sets the ART APEX that metrics are being collected on behalf of. - void SetArtApexVersion(int64_t version) { - art_apex_version_ = version; - } + void SetArtApexVersion(int64_t version) { art_apex_version_ = version; } // Gets the ART APEX last update time in milliseconds. - int64_t GetArtApexLastUpdateMillis() const { - return art_apex_last_update_millis_; - } + int64_t GetArtApexLastUpdateMillis() const { return art_apex_last_update_millis_; } // Sets the ART APEX last update time in milliseconds. void SetArtApexLastUpdateMillis(int64_t last_update_millis) { @@ -97,31 +96,27 @@ class OdrMetrics final { // Gets the trigger for metrics collection. The trigger is the reason why odrefresh considers // compilation necessary. - Trigger GetTrigger() const { - return trigger_.has_value() ? trigger_.value() : Trigger::kUnknown; - } + Trigger GetTrigger() const { return trigger_; } // Sets the trigger for metrics collection. The trigger is the reason why odrefresh considers // compilation necessary. Only call this method if compilation is necessary as the presence // of a trigger means we will try to record and upload metrics. - void SetTrigger(const Trigger trigger) { - trigger_ = trigger; - } + void SetTrigger(const Trigger trigger) { trigger_ = trigger; } // Sets the execution status of the current odrefresh processing stage. - void SetStatus(const Status status) { - status_ = status; - } + void SetStatus(const Status status) { status_ = status; } // Sets the current odrefresh processing stage. - void SetStage(Stage stage); + void SetStage(Stage stage) { stage_ = stage; } // Sets the result of the current dex2oat invocation. void SetDex2OatResult(const ExecResult& dex2oat_result); - // Record metrics into an OdrMetricsRecord. - // returns true on success, false if instance is not valid (because the trigger value is not set). - bool ToRecord(/*out*/OdrMetricsRecord* record) const; + // Captures the current free space as the end free space. + void CaptureSpaceFreeEnd(); + + // Records metrics into an OdrMetricsRecord. + OdrMetricsRecord ToRecord() const; private: OdrMetrics(const OdrMetrics&) = delete; @@ -135,9 +130,11 @@ class OdrMetrics final { const std::string cache_directory_; const std::string metrics_file_; + bool enabled_ = false; + int64_t art_apex_version_ = 0; int64_t art_apex_last_update_millis_ = 0; - std::optional<Trigger> trigger_ = {}; // metrics are only logged if compilation is triggered. + Trigger trigger_ = Trigger::kUnknown; Stage stage_ = Stage::kUnknown; Status status_ = Status::kUnknown; diff --git a/odrefresh/odr_metrics_test.cc b/odrefresh/odr_metrics_test.cc index f222caaf2e..7bc6f6becd 100644 --- a/odrefresh/odr_metrics_test.cc +++ b/odrefresh/odr_metrics_test.cc @@ -15,21 +15,24 @@ */ #include "odr_metrics.h" -#include "base/casts.h" -#include "odr_metrics_record.h" #include <unistd.h> +#include <chrono> #include <fstream> #include <memory> #include <string> +#include <thread> -#include "android-base/result-gmock.h" +#include "base/casts.h" #include "base/common_art_test.h" +#include "odr_metrics_record.h" namespace art { namespace odrefresh { +using std::chrono_literals::operator""ms; // NOLINT + class OdrMetricsTest : public CommonArtTest { public: void SetUp() override { @@ -50,14 +53,6 @@ class OdrMetricsTest : public CommonArtTest { return OS::FileExists(path); } - bool RemoveMetricsFile() const { - const char* path = metrics_file_path_.c_str(); - if (OS::FileExists(path)) { - return unlink(path) == 0; - } - return true; - } - const std::string GetCacheDirectory() const { return cache_directory_; } const std::string GetMetricsFilePath() const { return metrics_file_path_; } @@ -67,59 +62,23 @@ class OdrMetricsTest : public CommonArtTest { std::string cache_directory_; }; -TEST_F(OdrMetricsTest, ToRecordFailsIfNotTriggered) { - { - OdrMetrics metrics(GetCacheDirectory(), GetMetricsFilePath()); - OdrMetricsRecord record {}; - EXPECT_FALSE(metrics.ToRecord(&record)); - } - - { - OdrMetrics metrics(GetCacheDirectory(), GetMetricsFilePath()); - metrics.SetArtApexVersion(99); - metrics.SetStage(OdrMetrics::Stage::kCheck); - metrics.SetStatus(OdrMetrics::Status::kNoSpace); - OdrMetricsRecord record {}; - EXPECT_FALSE(metrics.ToRecord(&record)); - } -} - -TEST_F(OdrMetricsTest, ToRecordSucceedsIfTriggered) { - OdrMetrics metrics(GetCacheDirectory(), GetMetricsFilePath()); - metrics.SetArtApexVersion(99); - metrics.SetTrigger(OdrMetrics::Trigger::kApexVersionMismatch); - metrics.SetStage(OdrMetrics::Stage::kCheck); - metrics.SetStatus(OdrMetrics::Status::kNoSpace); - - OdrMetricsRecord record{}; - EXPECT_TRUE(metrics.ToRecord(&record)); - - EXPECT_EQ(99, record.art_apex_version); - EXPECT_EQ(OdrMetrics::Trigger::kApexVersionMismatch, - enum_cast<OdrMetrics::Trigger>(record.trigger)); - EXPECT_EQ(OdrMetrics::Stage::kCheck, enum_cast<OdrMetrics::Stage>(record.stage_reached)); - EXPECT_EQ(OdrMetrics::Status::kNoSpace, enum_cast<OdrMetrics::Status>(record.status)); -} - -TEST_F(OdrMetricsTest, MetricsFileIsNotCreatedIfNotTriggered) { - EXPECT_TRUE(RemoveMetricsFile()); - +TEST_F(OdrMetricsTest, MetricsFileIsNotCreatedIfNotEnabled) { // Metrics file is (potentially) written in OdrMetrics destructor. { OdrMetrics metrics(GetCacheDirectory(), GetMetricsFilePath()); metrics.SetArtApexVersion(99); + metrics.SetTrigger(OdrMetrics::Trigger::kApexVersionMismatch); metrics.SetStage(OdrMetrics::Stage::kCheck); metrics.SetStatus(OdrMetrics::Status::kNoSpace); } EXPECT_FALSE(MetricsFileExists()); } -TEST_F(OdrMetricsTest, NoMetricsFileIsCreatedIfTriggered) { - EXPECT_TRUE(RemoveMetricsFile()); - +TEST_F(OdrMetricsTest, MetricsFileIsCreatedIfEnabled) { // Metrics file is (potentially) written in OdrMetrics destructor. { OdrMetrics metrics(GetCacheDirectory(), GetMetricsFilePath()); + metrics.SetEnabled(true); metrics.SetArtApexVersion(101); metrics.SetTrigger(OdrMetrics::Trigger::kDexFilesChanged); metrics.SetStage(OdrMetrics::Stage::kCheck); @@ -128,20 +87,6 @@ TEST_F(OdrMetricsTest, NoMetricsFileIsCreatedIfTriggered) { EXPECT_TRUE(MetricsFileExists()); } -TEST_F(OdrMetricsTest, StageDoesNotAdvancedAfterFailure) { - OdrMetrics metrics(GetCacheDirectory(), GetMetricsFilePath()); - metrics.SetArtApexVersion(1999); - metrics.SetTrigger(OdrMetrics::Trigger::kMissingArtifacts); - metrics.SetStage(OdrMetrics::Stage::kCheck); - metrics.SetStatus(OdrMetrics::Status::kNoSpace); - metrics.SetStage(OdrMetrics::Stage::kComplete); - - OdrMetricsRecord record{}; - EXPECT_TRUE(metrics.ToRecord(&record)); - - EXPECT_EQ(OdrMetrics::Stage::kCheck, enum_cast<OdrMetrics::Stage>(record.stage_reached)); -} - TEST_F(OdrMetricsTest, TimeValuesAreRecorded) { OdrMetrics metrics(GetCacheDirectory(), GetMetricsFilePath()); metrics.SetArtApexVersion(1999); @@ -150,68 +95,54 @@ TEST_F(OdrMetricsTest, TimeValuesAreRecorded) { metrics.SetStatus(OdrMetrics::Status::kOK); // Primary boot classpath compilation time. - OdrMetricsRecord record{}; { metrics.SetStage(OdrMetrics::Stage::kPrimaryBootClasspath); ScopedOdrCompilationTimer timer(metrics); - sleep(2u); + std::this_thread::sleep_for(100ms); } - EXPECT_TRUE(metrics.ToRecord(&record)); - EXPECT_EQ(OdrMetrics::Stage::kPrimaryBootClasspath, - enum_cast<OdrMetrics::Stage>(record.stage_reached)); - EXPECT_NE(0, record.primary_bcp_compilation_millis); - EXPECT_GT(10'000, record.primary_bcp_compilation_millis); - EXPECT_EQ(0, record.secondary_bcp_compilation_millis); - EXPECT_EQ(0, record.system_server_compilation_millis); + OdrMetricsRecord record = metrics.ToRecord(); + EXPECT_EQ(enum_cast<OdrMetrics::Stage>(record.stage_reached), + OdrMetrics::Stage::kPrimaryBootClasspath); + EXPECT_GT(record.primary_bcp_compilation_millis, 0); + EXPECT_LT(record.primary_bcp_compilation_millis, 300); + EXPECT_EQ(record.secondary_bcp_compilation_millis, 0); + EXPECT_EQ(record.system_server_compilation_millis, 0); // Secondary boot classpath compilation time. { metrics.SetStage(OdrMetrics::Stage::kSecondaryBootClasspath); ScopedOdrCompilationTimer timer(metrics); - sleep(2u); + std::this_thread::sleep_for(100ms); } - EXPECT_TRUE(metrics.ToRecord(&record)); + record = metrics.ToRecord(); EXPECT_EQ(OdrMetrics::Stage::kSecondaryBootClasspath, enum_cast<OdrMetrics::Stage>(record.stage_reached)); - EXPECT_NE(0, record.primary_bcp_compilation_millis); - EXPECT_NE(0, record.secondary_bcp_compilation_millis); - EXPECT_GT(10'000, record.secondary_bcp_compilation_millis); - EXPECT_EQ(0, record.system_server_compilation_millis); + EXPECT_GT(record.primary_bcp_compilation_millis, 0); + EXPECT_GT(record.secondary_bcp_compilation_millis, 0); + EXPECT_LT(record.secondary_bcp_compilation_millis, 300); + EXPECT_EQ(record.system_server_compilation_millis, 0); // system_server classpath compilation time. { metrics.SetStage(OdrMetrics::Stage::kSystemServerClasspath); ScopedOdrCompilationTimer timer(metrics); - sleep(2u); + std::this_thread::sleep_for(100ms); } - EXPECT_TRUE(metrics.ToRecord(&record)); + record = metrics.ToRecord(); EXPECT_EQ(OdrMetrics::Stage::kSystemServerClasspath, enum_cast<OdrMetrics::Stage>(record.stage_reached)); - EXPECT_NE(0, record.primary_bcp_compilation_millis); - EXPECT_NE(0, record.secondary_bcp_compilation_millis); - EXPECT_NE(0, record.system_server_compilation_millis); - EXPECT_GT(10'000, record.system_server_compilation_millis); + EXPECT_GT(record.primary_bcp_compilation_millis, 0); + EXPECT_GT(record.secondary_bcp_compilation_millis, 0); + EXPECT_GT(record.system_server_compilation_millis, 0); + EXPECT_LT(record.system_server_compilation_millis, 300); } TEST_F(OdrMetricsTest, CacheSpaceValuesAreUpdated) { - OdrMetricsRecord snap {}; - snap.cache_space_free_start_mib = -1; - snap.cache_space_free_end_mib = -1; - { - OdrMetrics metrics(GetCacheDirectory(), GetMetricsFilePath()); - metrics.SetArtApexVersion(1999); - metrics.SetTrigger(OdrMetrics::Trigger::kMissingArtifacts); - metrics.SetStage(OdrMetrics::Stage::kCheck); - metrics.SetStatus(OdrMetrics::Status::kOK); - EXPECT_TRUE(metrics.ToRecord(&snap)); - EXPECT_NE(0, snap.cache_space_free_start_mib); - EXPECT_EQ(0, snap.cache_space_free_end_mib); - } - - OdrMetricsRecord on_disk{}; - EXPECT_THAT(on_disk.ReadFromFile(GetMetricsFilePath()), android::base::testing::Ok()); - EXPECT_EQ(snap.cache_space_free_start_mib, on_disk.cache_space_free_start_mib); - EXPECT_NE(0, on_disk.cache_space_free_end_mib); + OdrMetrics metrics(GetCacheDirectory(), GetMetricsFilePath()); + metrics.CaptureSpaceFreeEnd(); + OdrMetricsRecord record = metrics.ToRecord(); + EXPECT_GT(record.cache_space_free_start_mib, 0); + EXPECT_GT(record.cache_space_free_end_mib, 0); } } // namespace odrefresh diff --git a/odrefresh/odr_statslog_android.cc b/odrefresh/odr_statslog_android.cc index ee173d4b3d..f8ed4f8e0d 100644 --- a/odrefresh/odr_statslog_android.cc +++ b/odrefresh/odr_statslog_android.cc @@ -77,6 +77,8 @@ int32_t TranslateStatus(int32_t art_metrics_status) { return metrics::statsd::ODREFRESH_REPORTED__STATUS__STATUS_STAGING_FAILED; case OdrMetrics::Status::kInstallFailed: return metrics::statsd::ODREFRESH_REPORTED__STATUS__STATUS_INSTALL_FAILED; + case OdrMetrics::Status::kDalvikCachePermissionDenied: + return metrics::statsd::ODREFRESH_REPORTED__STATUS__STATUS_DALVIK_CACHE_PERMISSION_DENIED; } LOG(ERROR) << "Unknown status value: " << art_metrics_status; diff --git a/odrefresh/odrefresh.cc b/odrefresh/odrefresh.cc index bed694cc5e..cf68aa2199 100644 --- a/odrefresh/odrefresh.cc +++ b/odrefresh/odrefresh.cc @@ -1241,7 +1241,7 @@ bool OnDeviceRefresh::CheckSystemServerArtifactsAreUpToDate( } Result<void> OnDeviceRefresh::CleanupArtifactDirectory( - const std::vector<std::string>& artifacts_to_keep) const { + OdrMetrics& metrics, const std::vector<std::string>& artifacts_to_keep) const { const std::string& artifact_dir = config_.GetArtifactDirectory(); std::unordered_set<std::string> artifact_set{artifacts_to_keep.begin(), artifacts_to_keep.end()}; @@ -1259,7 +1259,9 @@ Result<void> OnDeviceRefresh::CleanupArtifactDirectory( // undefined behavior; entries.push_back(entry); } - if (ec) { + if (ec && ec.value() != ENOENT) { + metrics.SetStatus(ec.value() == EPERM ? OdrMetrics::Status::kDalvikCachePermissionDenied : + OdrMetrics::Status::kIoError); return Errorf("Failed to iterate over entries in the artifact directory: {}", ec.message()); } @@ -1269,6 +1271,7 @@ Result<void> OnDeviceRefresh::CleanupArtifactDirectory( if (!ContainsElement(artifact_set, path)) { LOG(INFO) << "Removing " << path; if (unlink(path.c_str()) != 0) { + metrics.SetStatus(OdrMetrics::Status::kIoError); return ErrnoErrorf("Failed to remove file {}", QuotePath(path)); } } @@ -1276,6 +1279,7 @@ Result<void> OnDeviceRefresh::CleanupArtifactDirectory( // Neither a regular file nor a directory. Unexpected file type. LOG(INFO) << "Removing " << path; if (unlink(path.c_str()) != 0) { + metrics.SetStatus(OdrMetrics::Status::kIoError); return ErrnoErrorf("Failed to remove file {}", QuotePath(path)); } } @@ -1336,7 +1340,11 @@ OnDeviceRefresh::CheckArtifactsAreUpToDate(OdrMetrics& metrics, auto cleanup_and_compile_all = [&, this]() { compilation_options->compile_boot_classpath_for_isas = config_.GetBootClasspathIsas(); compilation_options->system_server_jars_to_compile = AllSystemServerJars(); - return RemoveArtifactsDirectory() ? ExitCode::kCompilationRequired : ExitCode::kCleanupFailed; + if (!RemoveArtifactsDirectory()) { + metrics.SetStatus(OdrMetrics::Status::kIoError); + return ExitCode::kCleanupFailed; + } + return ExitCode::kCompilationRequired; }; std::optional<std::vector<apex::ApexInfo>> apex_info_list = GetApexInfoList(); @@ -1410,7 +1418,7 @@ OnDeviceRefresh::CheckArtifactsAreUpToDate(OdrMetrics& metrics, checked_artifacts.push_back(cache_info_filename_); } - Result<void> result = CleanupArtifactDirectory(checked_artifacts); + Result<void> result = CleanupArtifactDirectory(metrics, checked_artifacts); if (!result.ok()) { LOG(ERROR) << result.error(); return ExitCode::kCleanupFailed; @@ -1497,6 +1505,7 @@ WARN_UNUSED bool OnDeviceRefresh::CompileBootClasspathArtifacts( args.emplace_back("--runtime-arg"); args.emplace_back(Concatenate({"-Xbootclasspath:", android::base::Join(jars_to_compile, ":")})); if (!AddBootClasspathFds(args, readonly_files_raii, jars_to_compile)) { + metrics.SetStatus(OdrMetrics::Status::kIoError); return false; } @@ -1654,6 +1663,7 @@ WARN_UNUSED bool OnDeviceRefresh::CompileSystemServerArtifacts( auto bcp_jars = android::base::Split(config_.GetBootClasspath(), ":"); if (!AddBootClasspathFds(args, readonly_files_raii, bcp_jars)) { + metrics.SetStatus(OdrMetrics::Status::kIoError); return false; } std::string unused_error_msg; @@ -1729,10 +1739,18 @@ WARN_UNUSED ExitCode OnDeviceRefresh::Compile(OdrMetrics& metrics, const char* staging_dir = nullptr; metrics.SetStage(OdrMetrics::Stage::kPreparation); + if (!EnsureDirectoryExists(config_.GetArtifactDirectory())) { + LOG(ERROR) << "Failed to prepare artifact directory"; + metrics.SetStatus(errno == EPERM ? OdrMetrics::Status::kDalvikCachePermissionDenied : + OdrMetrics::Status::kIoError); + return ExitCode::kCleanupFailed; + } + if (config_.GetRefresh()) { Result<void> result = RefreshExistingArtifacts(); if (!result.ok()) { LOG(ERROR) << "Failed to refresh existing artifacts: " << result.error(); + metrics.SetStatus(OdrMetrics::Status::kIoError); return ExitCode::kCleanupFailed; } } @@ -1741,6 +1759,7 @@ WARN_UNUSED ExitCode OnDeviceRefresh::Compile(OdrMetrics& metrics, Result<void> result = WriteCacheInfo(); if (!result.ok()) { LOG(ERROR) << result.error(); + metrics.SetStatus(OdrMetrics::Status::kIoError); return ExitCode::kCleanupFailed; } @@ -1850,6 +1869,7 @@ WARN_UNUSED ExitCode OnDeviceRefresh::Compile(OdrMetrics& metrics, } metrics.SetStage(OdrMetrics::Stage::kComplete); + metrics.SetStatus(OdrMetrics::Status::kOK); return ExitCode::kCompilationSuccess; } diff --git a/odrefresh/odrefresh.h b/odrefresh/odrefresh.h index e48567fced..b14aa41260 100644 --- a/odrefresh/odrefresh.h +++ b/odrefresh/odrefresh.h @@ -112,7 +112,7 @@ class OnDeviceRefresh final { // Removes files that are not in the list. android::base::Result<void> CleanupArtifactDirectory( - const std::vector<std::string>& artifacts_to_keep) const; + OdrMetrics& metrics, const std::vector<std::string>& artifacts_to_keep) const; // Loads artifacts to memory and writes them back. This is a workaround for old versions of // odsign, which encounters "file exists" error when it adds existing artifacts to fs-verity. This diff --git a/odrefresh/odrefresh_main.cc b/odrefresh/odrefresh_main.cc index c85115ce3c..09f8f56950 100644 --- a/odrefresh/odrefresh_main.cc +++ b/odrefresh/odrefresh_main.cc @@ -274,10 +274,17 @@ int main(int argc, char** argv) { CompilationOptions compilation_options; if (action == "--check") { // Fast determination of whether artifacts are up to date. - return odr.CheckArtifactsAreUpToDate(metrics, &compilation_options); + ExitCode exit_code = odr.CheckArtifactsAreUpToDate(metrics, &compilation_options); + // Normally, `--check` should not write metrics. If compilation is not required, there's no need + // to write metrics; if compilation is required, `--compile` will write metrics. Therefore, + // `--check` should only write metrics when things went wrong. + metrics.SetEnabled(exit_code != ExitCode::kOkay && exit_code != ExitCode::kCompilationRequired); + return exit_code; } else if (action == "--compile") { - const ExitCode exit_code = odr.CheckArtifactsAreUpToDate(metrics, &compilation_options); + ExitCode exit_code = odr.CheckArtifactsAreUpToDate(metrics, &compilation_options); if (exit_code != ExitCode::kCompilationRequired) { + // No compilation required, so only write metrics when things went wrong. + metrics.SetEnabled(exit_code != ExitCode::kOkay); return exit_code; } OdrCompilationLog compilation_log; @@ -285,6 +292,8 @@ int main(int argc, char** argv) { LOG(INFO) << "Compilation skipped because it was attempted recently"; return ExitCode::kOkay; } + // Compilation required, so always write metrics. + metrics.SetEnabled(true); ExitCode compile_result = odr.Compile(metrics, compilation_options); compilation_log.Log(metrics.GetArtApexVersion(), metrics.GetArtApexLastUpdateMillis(), |