Read/write Odrefresh metrics in XML format

Test: atest ArtGtestsTargetChroot

Bug: 175048705
Change-Id: Ie921dbe362b8caeef406277c0eb44497bb864391

6 files changed, 201 insertions, 121 deletions

diff --git a/odrefresh/odr_metrics.cc b/odrefresh/odr_metrics.cc
index 4bddb17bd8..33368b2a1d 100644
--- a/odrefresh/odr_metrics.cc
+++ b/odrefresh/odr_metrics.cc
@@ -119,6 +119,7 @@ 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_);
@@ -132,15 +133,17 @@ bool OdrMetrics::ToRecord(/*out*/OdrMetricsRecord* record) const {
 }
 
 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 fc135d3399..a5b4707f17 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);
+  }
+}
+
+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);
+  }
+
+  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
 
-std::ostream& operator<<(std::ostream& os, const OdrMetricsRecord& record) {
-  static const char kSpace = ' ';
+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();
+  }
 
-  // Block I/O related exceptions
-  auto saved_exceptions = os.exceptions();
-  os.exceptions(std::ios_base::iostate {});
+  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;
-
-  // Restore I/O related exceptions
-  os.exceptions(saved_exceptions);
-  return os;
+  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);
+
+  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 9dd51a63cc..1bd7faa0e0 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,23 +46,15 @@ struct OdrMetricsRecord {
   int32_t system_server_compilation_seconds;
   int32_t cache_space_free_start_mib;
   int32_t cache_space_free_end_mib;
-};
 
-// 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);
+  // 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;
+};
 
 }  // namespace odrefresh
 }  // namespace art
diff --git a/odrefresh/odr_metrics_record_test.cc b/odrefresh/odr_metrics_record_test.cc
index dd739d68f2..d2cc59f541 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 @@ namespace odrefresh {
 
 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, HappyPath) {
 
 TEST_F(OdrMetricsRecordTest, EmptyInput) {
   ScratchDir dir(/*keep_files=*/false);
-  std::string file_path = dir.GetPath() + "/metrics-record.txt";
-
-  std::ifstream ifs(file_path);
-  OdrMetricsRecord record;
-  ifs >> record;
+  std::string file_path = dir.GetPath() + "/metrics-record.xml";
 
-  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::string file_path = dir.GetPath() + "/metrics-record.xml";
 
-  std::ifstream ifs(file_path);
-  ifs.close();
+  std::ofstream ofs(file_path);
+  ofs << "<not_odrefresh_metrics></not_odrefresh_metrics>";
+  ofs.close();
 
-  OdrMetricsRecord record;
-  ifs >> record;
+  OdrMetricsRecord record{};
+  ASSERT_THAT(
+      record.ReadFromFile(file_path),
+      HasError(WithMessage("odrefresh_metrics element not found in " + file_path)));
+}
+
+TEST_F(OdrMetricsRecordTest, ExpectedElementNotFound) {
+  ScratchDir dir(/*keep_files=*/false);
+  std::string file_path = dir.GetPath() + "/metrics-record.xml";
 
-  ASSERT_TRUE(ifs.fail());
-  ASSERT_TRUE(!ifs);
+  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, ClosedOutput) {
+TEST_F(OdrMetricsRecordTest, UnexpectedOdrefreshMetricsVersion) {
   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 << "<odrefresh_metrics>";
+  ofs << "<odrefresh_metrics_version>0</odrefresh_metrics_version>";
+  ofs << "</odrefresh_metrics>";
   ofs.close();
 
-  OdrMetricsRecord record {};
-  ofs << record;
+  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();
 
-  ASSERT_TRUE(ofs.fail());
-  ASSERT_TRUE(!ofs.good());
+  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 4519f00363..4f73aef0ce 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 @@ class OdrMetricsTest : public CommonArtTest {
     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 @@ TEST_F(OdrMetricsTest, CacheSpaceValuesAreUpdated) {
     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 7db348e633..831d1d1315 100644
--- a/odrefresh/odr_statslog_android.cc
+++ b/odrefresh/odr_statslog_android.cc
@@ -103,16 +103,11 @@ int32_t TranslateTrigger(int32_t art_metrics_trigger) {
 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;
   }