Read/write Odrefresh metrics in XML format am: 82f62eedb2
Original change: https://android-review.googlesource.com/c/platform/art/+/2101702
Change-Id: I2c315d38f9665570c17287adb1d056e29d4dc15d
Signed-off-by: Automerger Merge Worker <android-build-automerger-merge-worker@system.gserviceaccount.com>
diff --git a/odrefresh/odr_metrics.cc b/odrefresh/odr_metrics.cc
index 4bddb17..33368b2 100644
--- a/odrefresh/odr_metrics.cc
+++ b/odrefresh/odr_metrics.cc
@@ -119,6 +119,7 @@
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_);
@@ -132,15 +133,17 @@
}
void OdrMetrics::WriteToFile(const std::string& path, const OdrMetrics* metrics) {
- OdrMetricsRecord record;
+ OdrMetricsRecord record{};
if (!metrics->ToRecord(&record)) {
LOG(ERROR) << "Attempting to report metrics without a compilation trigger.";
return;
}
- // Preserve order from frameworks/proto_logging/stats/atoms.proto in metrics file written.
- std::ofstream ofs(path);
- ofs << record;
+ const android::base::Result<void>& result = record.WriteToFile(path);
+ if (!result.ok()) {
+ LOG(ERROR) << "Failed to report metrics to file: " << path
+ << ", error: " << result.error().message();
+ }
}
} // namespace odrefresh
diff --git a/odrefresh/odr_metrics_record.cc b/odrefresh/odr_metrics_record.cc
index fc135d3..a5b4707 100644
--- a/odrefresh/odr_metrics_record.cc
+++ b/odrefresh/odr_metrics_record.cc
@@ -14,59 +14,110 @@
* limitations under the License.
*/
+#include "android-base/logging.h"
#include "odr_metrics_record.h"
+#include "tinyxml2.h"
#include <iosfwd>
-#include <istream>
-#include <ostream>
-#include <streambuf>
#include <string>
namespace art {
namespace odrefresh {
-std::istream& operator>>(std::istream& is, OdrMetricsRecord& record) {
- // Block I/O related exceptions
- auto saved_exceptions = is.exceptions();
- is.exceptions(std::ios_base::iostate {});
+namespace {
+android::base::Result<int64_t> ReadInt64(tinyxml2::XMLElement* parent, const char* name) {
+ tinyxml2::XMLElement* element = parent->FirstChildElement(name);
+ if (element == nullptr) {
+ return Errorf("Expected Odrefresh metric {} not found", name);
+ }
- // The order here matches the field order of MetricsRecord.
- is >> record.art_apex_version >> std::ws;
- is >> record.trigger >> std::ws;
- is >> record.stage_reached >> std::ws;
- is >> record.status >> std::ws;
- is >> record.primary_bcp_compilation_seconds >> std::ws;
- is >> record.secondary_bcp_compilation_seconds >> std::ws;
- is >> record.system_server_compilation_seconds >> std::ws;
- is >> record.cache_space_free_start_mib >> std::ws;
- is >> record.cache_space_free_end_mib >> std::ws;
-
- // Restore I/O related exceptions
- is.exceptions(saved_exceptions);
- return is;
+ int64_t metric;
+ tinyxml2::XMLError result = element->QueryInt64Text(&metric);
+ if (result == tinyxml2::XML_SUCCESS) {
+ return metric;
+ } else {
+ return Errorf("Odrefresh metric {} is not an int64", name);
+ }
}
-std::ostream& operator<<(std::ostream& os, const OdrMetricsRecord& record) {
- static const char kSpace = ' ';
+android::base::Result<int32_t> ReadInt32(tinyxml2::XMLElement* parent, const char* name) {
+ tinyxml2::XMLElement* element = parent->FirstChildElement(name);
+ if (element == nullptr) {
+ return Errorf("Expected Odrefresh metric {} not found", name);
+ }
- // Block I/O related exceptions
- auto saved_exceptions = os.exceptions();
- os.exceptions(std::ios_base::iostate {});
+ int32_t metric;
+ tinyxml2::XMLError result = element->QueryIntText(&metric);
+ if (result == tinyxml2::XML_SUCCESS) {
+ return metric;
+ } else {
+ return Errorf("Odrefresh metric {} is not an int32", name);
+ }
+}
+
+template <typename T>
+void AddMetric(tinyxml2::XMLElement* parent, const char* name, const T& value) {
+ parent->InsertNewChildElement(name)->SetText(value);
+}
+} // namespace
+
+android::base::Result<void> OdrMetricsRecord::ReadFromFile(const std::string& filename) {
+ tinyxml2::XMLDocument xml_document;
+ tinyxml2::XMLError result = xml_document.LoadFile(filename.data());
+ if (result != tinyxml2::XML_SUCCESS) {
+ return android::base::Error() << xml_document.ErrorStr();
+ }
+
+ tinyxml2::XMLElement* metrics = xml_document.FirstChildElement("odrefresh_metrics");
+ if (metrics == nullptr) {
+ return Errorf("odrefresh_metrics element not found in {}", filename);
+ }
+
+ odrefresh_metrics_version = OR_RETURN(ReadInt32(metrics, "odrefresh_metrics_version"));
+ if (odrefresh_metrics_version != kOdrefreshMetricsVersion) {
+ return Errorf("odrefresh_metrics_version {} is different than expected ({})",
+ odrefresh_metrics_version,
+ kOdrefreshMetricsVersion);
+ }
+
+ art_apex_version = OR_RETURN(ReadInt64(metrics, "art_apex_version"));
+ trigger = OR_RETURN(ReadInt32(metrics, "trigger"));
+ stage_reached = OR_RETURN(ReadInt32(metrics, "stage_reached"));
+ status = OR_RETURN(ReadInt32(metrics, "status"));
+ primary_bcp_compilation_seconds = OR_RETURN(
+ ReadInt32(metrics, "primary_bcp_compilation_seconds"));
+ secondary_bcp_compilation_seconds = OR_RETURN(
+ ReadInt32(metrics, "secondary_bcp_compilation_seconds"));
+ system_server_compilation_seconds = OR_RETURN(
+ ReadInt32(metrics, "system_server_compilation_seconds"));
+ cache_space_free_start_mib = OR_RETURN(ReadInt32(metrics, "cache_space_free_start_mib"));
+ cache_space_free_end_mib = OR_RETURN(ReadInt32(metrics, "cache_space_free_end_mib"));
+ return {};
+}
+
+android::base::Result<void> OdrMetricsRecord::WriteToFile(const std::string& filename) const {
+ tinyxml2::XMLDocument xml_document;
+ tinyxml2::XMLElement* metrics = xml_document.NewElement("odrefresh_metrics");
+ xml_document.InsertEndChild(metrics);
// The order here matches the field order of MetricsRecord.
- os << record.art_apex_version << kSpace;
- os << record.trigger << kSpace;
- os << record.stage_reached << kSpace;
- os << record.status << kSpace;
- os << record.primary_bcp_compilation_seconds << kSpace;
- os << record.secondary_bcp_compilation_seconds << kSpace;
- os << record.system_server_compilation_seconds << kSpace;
- os << record.cache_space_free_start_mib << kSpace;
- os << record.cache_space_free_end_mib << std::endl;
+ AddMetric(metrics, "odrefresh_metrics_version", odrefresh_metrics_version);
+ AddMetric(metrics, "art_apex_version", art_apex_version);
+ AddMetric(metrics, "trigger", trigger);
+ AddMetric(metrics, "stage_reached", stage_reached);
+ AddMetric(metrics, "status", status);
+ AddMetric(metrics, "primary_bcp_compilation_seconds", primary_bcp_compilation_seconds);
+ AddMetric(metrics, "secondary_bcp_compilation_seconds", secondary_bcp_compilation_seconds);
+ AddMetric(metrics, "system_server_compilation_seconds", system_server_compilation_seconds);
+ AddMetric(metrics, "cache_space_free_start_mib", cache_space_free_start_mib);
+ AddMetric(metrics, "cache_space_free_end_mib", cache_space_free_end_mib);
- // Restore I/O related exceptions
- os.exceptions(saved_exceptions);
- return os;
+ tinyxml2::XMLError result = xml_document.SaveFile(filename.data(), /*compact=*/true);
+ if (result == tinyxml2::XML_SUCCESS) {
+ return {};
+ } else {
+ return android::base::Error() << xml_document.ErrorStr();
+ }
}
} // namespace odrefresh
diff --git a/odrefresh/odr_metrics_record.h b/odrefresh/odr_metrics_record.h
index 9dd51a6..1bd7faa 100644
--- a/odrefresh/odr_metrics_record.h
+++ b/odrefresh/odr_metrics_record.h
@@ -20,16 +20,23 @@
#include <cstdint>
#include <iosfwd> // For forward-declaration of std::string.
+#include "android-base/result.h"
+#include "tinyxml2.h"
+
namespace art {
namespace odrefresh {
// Default location for storing metrics from odrefresh.
-constexpr const char* kOdrefreshMetricsFile = "/data/misc/odrefresh/odrefresh-metrics.txt";
+constexpr const char* kOdrefreshMetricsFile = "/data/misc/odrefresh/odrefresh-metrics.xml";
+
+// Initial OdrefreshMetrics version
+static constexpr int32_t kOdrefreshMetricsVersion = 1;
// MetricsRecord is a simpler container for Odrefresh metric values reported to statsd. The order
// and types of fields here mirror definition of `OdrefreshReported` in
// frameworks/proto_logging/stats/atoms.proto.
struct OdrMetricsRecord {
+ int32_t odrefresh_metrics_version;
int64_t art_apex_version;
int32_t trigger;
int32_t stage_reached;
@@ -39,24 +46,16 @@
int32_t system_server_compilation_seconds;
int32_t cache_space_free_start_mib;
int32_t cache_space_free_end_mib;
+
+ // Reads a `MetricsRecord` from an XML file.
+ // Returns an error if the XML document was not found or parsed correctly.
+ android::base::Result<void> ReadFromFile(const std::string& filename);
+
+ // Writes a `MetricsRecord` to an XML file.
+ // Returns an error if the XML document was not saved correctly.
+ android::base::Result<void> WriteToFile(const std::string& filename) const;
};
-// Read a `MetricsRecord` from an `istream`.
-//
-// This method blocks istream related exceptions, the caller should check `is.fail()` is false after
-// calling.
-//
-// Returns `is`.
-std::istream& operator>>(std::istream& is, OdrMetricsRecord& record);
-
-// Write a `MetricsRecord` to an `ostream`.
-//
-// This method blocks ostream related exceptions, the caller should check `os.fail()` is false after
-// calling.
-//
-// Returns `os`
-std::ostream& operator<<(std::ostream& os, const OdrMetricsRecord& record);
-
} // namespace odrefresh
} // namespace art
diff --git a/odrefresh/odr_metrics_record_test.cc b/odrefresh/odr_metrics_record_test.cc
index dd739d6..d2cc59f 100644
--- a/odrefresh/odr_metrics_record_test.cc
+++ b/odrefresh/odr_metrics_record_test.cc
@@ -20,6 +20,7 @@
#include <fstream>
+#include "android-base/result-gmock.h"
#include "base/common_art_test.h"
namespace art {
@@ -27,36 +28,30 @@
class OdrMetricsRecordTest : public CommonArtTest {};
+using android::base::testing::Ok;
+using android::base::testing::HasError;
+using android::base::testing::WithMessage;
+
TEST_F(OdrMetricsRecordTest, HappyPath) {
- const OdrMetricsRecord expected {
- .art_apex_version = 0x01233456'789abcde,
- .trigger = 0x01020304,
- .stage_reached = 0x11121314,
- .status = 0x21222324,
- .primary_bcp_compilation_seconds = 0x31323334,
- .secondary_bcp_compilation_seconds = 0x41424344,
- .system_server_compilation_seconds = 0x51525354,
- .cache_space_free_start_mib = 0x61626364,
- .cache_space_free_end_mib = 0x71727374
- };
+ const OdrMetricsRecord expected{.odrefresh_metrics_version = 0x1,
+ .art_apex_version = 0x01233456'789abcde,
+ .trigger = 0x01020304,
+ .stage_reached = 0x11121314,
+ .status = 0x21222324,
+ .primary_bcp_compilation_seconds = 0x31323334,
+ .secondary_bcp_compilation_seconds = 0x41424344,
+ .system_server_compilation_seconds = 0x51525354,
+ .cache_space_free_start_mib = 0x61626364,
+ .cache_space_free_end_mib = 0x71727374};
ScratchDir dir(/*keep_files=*/false);
- std::string file_path = dir.GetPath() + "/metrics-record.txt";
-
- {
- std::ofstream ofs(file_path);
- ofs << expected;
- ASSERT_FALSE(ofs.fail());
- ofs.close();
- }
+ std::string file_path = dir.GetPath() + "/metrics-record.xml";
+ ASSERT_THAT(expected.WriteToFile(file_path), Ok());
OdrMetricsRecord actual {};
- {
- std::ifstream ifs(file_path);
- ifs >> actual;
- ASSERT_TRUE(ifs.eof());
- }
+ ASSERT_THAT(actual.ReadFromFile(file_path), Ok());
+ ASSERT_EQ(expected.odrefresh_metrics_version, actual.odrefresh_metrics_version);
ASSERT_EQ(expected.art_apex_version, actual.art_apex_version);
ASSERT_EQ(expected.trigger, actual.trigger);
ASSERT_EQ(expected.stage_reached, actual.stage_reached);
@@ -71,42 +66,81 @@
TEST_F(OdrMetricsRecordTest, EmptyInput) {
ScratchDir dir(/*keep_files=*/false);
- std::string file_path = dir.GetPath() + "/metrics-record.txt";
+ std::string file_path = dir.GetPath() + "/metrics-record.xml";
- std::ifstream ifs(file_path);
- OdrMetricsRecord record;
- ifs >> record;
-
- ASSERT_TRUE(ifs.fail());
- ASSERT_TRUE(!ifs);
+ OdrMetricsRecord record{};
+ ASSERT_THAT(record.ReadFromFile(file_path), testing::Not(Ok()));
}
-TEST_F(OdrMetricsRecordTest, ClosedInput) {
+TEST_F(OdrMetricsRecordTest, UnexpectedInput) {
ScratchDir dir(/*keep_files=*/false);
- std::string file_path = dir.GetPath() + "/metrics-record.txt";
-
- std::ifstream ifs(file_path);
- ifs.close();
-
- OdrMetricsRecord record;
- ifs >> record;
-
- ASSERT_TRUE(ifs.fail());
- ASSERT_TRUE(!ifs);
-}
-
-TEST_F(OdrMetricsRecordTest, ClosedOutput) {
- ScratchDir dir(/*keep_files=*/false);
- std::string file_path = dir.GetPath() + "/metrics-record.txt";
+ std::string file_path = dir.GetPath() + "/metrics-record.xml";
std::ofstream ofs(file_path);
+ ofs << "<not_odrefresh_metrics></not_odrefresh_metrics>";
ofs.close();
- OdrMetricsRecord record {};
- ofs << record;
+ OdrMetricsRecord record{};
+ ASSERT_THAT(
+ record.ReadFromFile(file_path),
+ HasError(WithMessage("odrefresh_metrics element not found in " + file_path)));
+}
- ASSERT_TRUE(ofs.fail());
- ASSERT_TRUE(!ofs.good());
+TEST_F(OdrMetricsRecordTest, ExpectedElementNotFound) {
+ ScratchDir dir(/*keep_files=*/false);
+ std::string file_path = dir.GetPath() + "/metrics-record.xml";
+
+ std::ofstream ofs(file_path);
+ ofs << "<odrefresh_metrics>";
+ ofs << "<not_valid_metric>25</not_valid_metric>";
+ ofs << "</odrefresh_metrics>";
+ ofs.close();
+
+ OdrMetricsRecord record{};
+ ASSERT_THAT(
+ record.ReadFromFile(file_path),
+ HasError(WithMessage("Expected Odrefresh metric odrefresh_metrics_version not found")));
+}
+
+TEST_F(OdrMetricsRecordTest, UnexpectedOdrefreshMetricsVersion) {
+ ScratchDir dir(/*keep_files=*/false);
+ std::string file_path = dir.GetPath() + "/metrics-record.xml";
+
+ std::ofstream ofs(file_path);
+ ofs << "<odrefresh_metrics>";
+ ofs << "<odrefresh_metrics_version>0</odrefresh_metrics_version>";
+ ofs << "</odrefresh_metrics>";
+ ofs.close();
+
+ OdrMetricsRecord record{};
+ ASSERT_THAT(record.ReadFromFile(file_path),
+ HasError(WithMessage("odrefresh_metrics_version 0 is different than expected (1)")));
+}
+
+TEST_F(OdrMetricsRecordTest, UnexpectedType) {
+ ScratchDir dir(/*keep_files=*/false);
+ std::string file_path = dir.GetPath() + "/metrics-record.xml";
+
+ std::ofstream ofs(file_path);
+ ofs << "<odrefresh_metrics>";
+ ofs << "<odrefresh_metrics_version>" << kOdrefreshMetricsVersion
+ << "</odrefresh_metrics_version>";
+ ofs << "<art_apex_version>81966764218039518</art_apex_version>";
+ ofs << "<trigger>16909060</trigger>";
+ ofs << "<stage_reached>286397204</stage_reached>";
+ ofs << "<status>abcd</status>"; // It should be an int32.
+ ofs << "<primary_bcp_compilation_seconds>825373492</primary_bcp_compilation_seconds>";
+ ofs << "<secondary_bcp_compilation_seconds>1094861636</secondary_bcp_compilation_seconds>";
+ ofs << "<system_server_compilation_seconds>1364349780</system_server_compilation_seconds>";
+ ofs << "<cache_space_free_start_mib>1633837924</cache_space_free_start_mib>";
+ ofs << "<cache_space_free_end_mib>1903326068</cache_space_free_end_mib>";
+ ofs << "</odrefresh_metrics>";
+ ofs.close();
+
+ OdrMetricsRecord record{};
+ ASSERT_THAT(
+ record.ReadFromFile(file_path),
+ HasError(WithMessage("Odrefresh metric status is not an int32")));
}
} // namespace odrefresh
diff --git a/odrefresh/odr_metrics_test.cc b/odrefresh/odr_metrics_test.cc
index 4519f00..4f73aef 100644
--- a/odrefresh/odr_metrics_test.cc
+++ b/odrefresh/odr_metrics_test.cc
@@ -24,6 +24,7 @@
#include <memory>
#include <string>
+#include "android-base/result-gmock.h"
#include "base/common_art_test.h"
namespace art {
@@ -35,7 +36,7 @@
CommonArtTest::SetUp();
scratch_dir_ = std::make_unique<ScratchDir>();
- metrics_file_path_ = scratch_dir_->GetPath() + "/metrics.txt";
+ metrics_file_path_ = scratch_dir_->GetPath() + "/metrics.xml";
cache_directory_ = scratch_dir_->GetPath() + "/dir";
mkdir(cache_directory_.c_str(), S_IRWXU);
}
@@ -207,11 +208,8 @@
EXPECT_EQ(0, snap.cache_space_free_end_mib);
}
- OdrMetricsRecord on_disk;
- std::ifstream ifs(GetMetricsFilePath());
- EXPECT_TRUE(ifs);
- ifs >> on_disk;
- EXPECT_TRUE(ifs);
+ 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);
}
diff --git a/odrefresh/odr_statslog_android.cc b/odrefresh/odr_statslog_android.cc
index 7db348e..831d1d1 100644
--- a/odrefresh/odr_statslog_android.cc
+++ b/odrefresh/odr_statslog_android.cc
@@ -103,16 +103,11 @@
bool ReadValues(const char* metrics_file,
/*out*/ OdrMetricsRecord* record,
/*out*/ std::string* error_msg) {
- std::ifstream ifs(metrics_file);
- if (!ifs) {
- *error_msg = android::base::StringPrintf(
- "metrics file '%s' could not be opened: %s", metrics_file, strerror(errno));
- return false;
- }
-
- ifs >> *record;
- if (!ifs) {
- *error_msg = "file parsing error.";
+ const android::base::Result<void>& result = record->ReadFromFile(metrics_file);
+ if (!result.ok()) {
+ *error_msg = android::base::StringPrintf("Unable to open or parse metrics file %s (error: %s)",
+ metrics_file,
+ result.error().message().data());
return false;
}