Merge SP1A.210624.001

Change-Id: I3bf08300e39810c6d0486ad966137ad9d42af7b1

9 files changed, 688 insertions, 106 deletions

diff --git a/runtime/Android.bp b/runtime/Android.bp
index c78bc4c68f..0059144974 100644
--- a/runtime/Android.bp
+++ b/runtime/Android.bp
@@ -725,6 +725,7 @@ art_cc_test {
         "jni/java_vm_ext_test.cc",
         "jni/jni_internal_test.cc",
         "method_handles_test.cc",
+        "metrics/reporter_test.cc",
         "mirror/dex_cache_test.cc",
         "mirror/method_type_test.cc",
         "mirror/object_test.cc",
diff --git a/runtime/metrics/reporter.cc b/runtime/metrics/reporter.cc
index 4cf1ba53b7..2398ea9bee 100644
--- a/runtime/metrics/reporter.cc
+++ b/runtime/metrics/reporter.cc
@@ -16,6 +16,11 @@
 
 #include "reporter.h"
 
+#include <algorithm>
+
+#include <android-base/parseint.h>
+
+#include "base/flags.h"
 #include "runtime.h"
 #include "runtime_options.h"
 #include "statsd.h"
@@ -27,31 +32,41 @@
 namespace art {
 namespace metrics {
 
-std::unique_ptr<MetricsReporter> MetricsReporter::Create(ReportingConfig config, Runtime* runtime) {
+std::unique_ptr<MetricsReporter> MetricsReporter::Create(
+    const ReportingConfig& config, Runtime* runtime) {
   // We can't use std::make_unique here because the MetricsReporter constructor is private.
   return std::unique_ptr<MetricsReporter>{new MetricsReporter{std::move(config), runtime}};
 }
 
-MetricsReporter::MetricsReporter(ReportingConfig config, Runtime* runtime)
-    : config_{std::move(config)}, runtime_{runtime} {}
+MetricsReporter::MetricsReporter(const ReportingConfig& config, Runtime* runtime)
+    : config_{config},
+      runtime_{runtime},
+      startup_reported_{false},
+      report_interval_index_{0} {}
 
 MetricsReporter::~MetricsReporter() { MaybeStopBackgroundThread(); }
 
-bool MetricsReporter::IsPeriodicReportingEnabled() const {
-  return config_.periodic_report_seconds.has_value();
-}
-
-void MetricsReporter::SetReportingPeriod(unsigned int period_seconds) {
-  DCHECK(!thread_.has_value()) << "The reporting period should not be changed after the background "
+void MetricsReporter::ReloadConfig(const ReportingConfig& config) {
+  DCHECK(!thread_.has_value()) << "The config cannot be reloaded after the background "
                                   "reporting thread is started.";
+  config_ = config;
+}
 
-  config_.periodic_report_seconds = period_seconds;
+bool MetricsReporter::IsMetricsReportingEnabled(const SessionData& session_data) const {
+  return session_data.session_id % config_.reporting_num_mods < config_.reporting_mods;
 }
 
 bool MetricsReporter::MaybeStartBackgroundThread(SessionData session_data) {
   CHECK(!thread_.has_value());
+
+  session_data_ = session_data;
+  LOG_STREAM(DEBUG) << "Received session metadata: " << session_data_.session_id;
+
+  if (!IsMetricsReportingEnabled(session_data_)) {
+    return false;
+  }
+
   thread_.emplace(&MetricsReporter::BackgroundThreadRun, this);
-  messages_.SendMessage(BeginSessionMessage{session_data});
   return true;
 }
 
@@ -59,11 +74,12 @@ void MetricsReporter::MaybeStopBackgroundThread() {
   if (thread_.has_value()) {
     messages_.SendMessage(ShutdownRequestedMessage{});
     thread_->join();
+    thread_.reset();
   }
 }
 
 void MetricsReporter::NotifyStartupCompleted() {
-  if (thread_.has_value()) {
+  if (ShouldReportAtStartup() && thread_.has_value()) {
     messages_.SendMessage(StartupCompletedMessage{});
   }
 }
@@ -113,39 +129,34 @@ void MetricsReporter::BackgroundThreadRun() {
 
   while (running) {
     messages_.SwitchReceive(
-        [&](BeginSessionMessage message) {
-          LOG_STREAM(DEBUG) << "Received session metadata";
-          session_data_ = message.session_data;
-        },
         [&]([[maybe_unused]] ShutdownRequestedMessage message) {
-          LOG_STREAM(DEBUG) << "Shutdown request received";
+          LOG_STREAM(DEBUG) << "Shutdown request received " << session_data_.session_id;
           running = false;
 
-          // Do one final metrics report, if enabled.
-          if (config_.report_metrics_on_shutdown) {
-            ReportMetrics();
-          }
+          ReportMetrics();
         },
         [&](RequestMetricsReportMessage message) {
-          LOG_STREAM(DEBUG) << "Explicit report request received";
+          LOG_STREAM(DEBUG) << "Explicit report request received " << session_data_.session_id;
           ReportMetrics();
           if (message.synchronous) {
             thread_to_host_messages_.SendMessage(ReportCompletedMessage{});
           }
         },
         [&]([[maybe_unused]] TimeoutExpiredMessage message) {
-          LOG_STREAM(DEBUG) << "Timer expired, reporting metrics";
+          LOG_STREAM(DEBUG) << "Timer expired, reporting metrics " << session_data_.session_id;
 
           ReportMetrics();
-
           MaybeResetTimeout();
         },
         [&]([[maybe_unused]] StartupCompletedMessage message) {
-          LOG_STREAM(DEBUG) << "App startup completed, reporting metrics";
+          LOG_STREAM(DEBUG) << "App startup completed, reporting metrics "
+              << session_data_.session_id;
           ReportMetrics();
+          startup_reported_ = true;
+          MaybeResetTimeout();
         },
         [&](CompilationInfoMessage message) {
-          LOG_STREAM(DEBUG) << "Compilation info received";
+          LOG_STREAM(DEBUG) << "Compilation info received " << session_data_.session_id;
           session_data_.compilation_reason = message.compilation_reason;
           session_data_.compiler_filter = message.compiler_filter;
         });
@@ -154,17 +165,21 @@ void MetricsReporter::BackgroundThreadRun() {
   if (attached) {
     runtime_->DetachCurrentThread();
   }
-  LOG_STREAM(DEBUG) << "Metrics reporting thread terminating";
+  LOG_STREAM(DEBUG) << "Metrics reporting thread terminating " << session_data_.session_id;
 }
 
 void MetricsReporter::MaybeResetTimeout() {
-  if (config_.periodic_report_seconds.has_value()) {
-    messages_.SetTimeout(SecondsToMs(config_.periodic_report_seconds.value()));
+  if (ShouldContinueReporting()) {
+    messages_.SetTimeout(SecondsToMs(GetNextPeriodSeconds()));
   }
 }
 
+const ArtMetrics* MetricsReporter::GetMetrics() {
+  return runtime_->GetMetrics();
+}
+
 void MetricsReporter::ReportMetrics() {
-  ArtMetrics* metrics{runtime_->GetMetrics()};
+  const ArtMetrics* metrics = GetMetrics();
 
   if (!session_started_) {
     for (auto& backend : backends_) {
@@ -178,17 +193,122 @@ void MetricsReporter::ReportMetrics() {
   }
 }
 
-ReportingConfig ReportingConfig::FromRuntimeArguments(const RuntimeArgumentMap& args) {
-  using M = RuntimeArgumentMap;
+bool MetricsReporter::ShouldReportAtStartup() const {
+  return IsMetricsReportingEnabled(session_data_) &&
+      config_.period_spec.has_value() &&
+      config_.period_spec->report_startup_first;
+}
+
+bool MetricsReporter::ShouldContinueReporting() const {
+  bool result =
+      // Only if the reporting is enabled
+      IsMetricsReportingEnabled(session_data_) &&
+      // and if we have period spec
+      config_.period_spec.has_value() &&
+      // and the periods are non empty
+      !config_.period_spec->periods_seconds.empty() &&
+      // and we already reported startup or not required to report startup
+      (startup_reported_ || !config_.period_spec->report_startup_first) &&
+      // and we still have unreported intervals or we are asked to report continuously.
+      (config_.period_spec->continuous_reporting ||
+              (report_interval_index_ < config_.period_spec->periods_seconds.size()));
+  return result;
+}
+
+uint32_t MetricsReporter::GetNextPeriodSeconds() {
+  DCHECK(ShouldContinueReporting());
+
+  // The index is either the current report_interval_index or the last index
+  // if we are in continuous mode and reached the end.
+  uint32_t index = std::min(
+      report_interval_index_,
+      static_cast<uint32_t>(config_.period_spec->periods_seconds.size() - 1));
+
+  uint32_t result = config_.period_spec->periods_seconds[index];
+
+  // Advance the index if we didn't get to the end.
+  if (report_interval_index_ < config_.period_spec->periods_seconds.size()) {
+    report_interval_index_++;
+  }
+  return result;
+}
+
+ReportingConfig ReportingConfig::FromFlags(bool is_system_server) {
+  std::optional<std::string> spec_str = is_system_server
+      ? gFlags.MetricsReportingSpecSystemServer.GetValueOptional()
+      : gFlags.MetricsReportingSpec.GetValueOptional();
+
+  std::optional<ReportingPeriodSpec> period_spec = std::nullopt;
+
+  if (spec_str.has_value()) {
+    std::string error;
+    period_spec = ReportingPeriodSpec::Parse(spec_str.value(), &error);
+    if (!period_spec.has_value()) {
+      LOG(ERROR) << "Failed to create metrics reporting spec from: " << spec_str.value()
+          << " with error: " << error;
+    }
+  }
+
+  uint32_t reporting_num_mods = gFlags.MetricsReportingNumMods();
+  uint32_t reporting_mods = gFlags.MetricsReportingMods();
+  if (reporting_mods > reporting_num_mods) {
+    LOG(ERROR) << "Invalid metrics reporting mods: " << reporting_mods
+        << " num modes=" << reporting_num_mods;
+    reporting_mods = 0;
+  }
+
   return {
-      .dump_to_logcat = args.Exists(M::WriteMetricsToLog),
-      .dump_to_statsd = args.GetOrDefault(M::WriteMetricsToStatsd),
-      .dump_to_file = args.GetOptional(M::WriteMetricsToFile),
-      .report_metrics_on_shutdown = !args.Exists(M::DisableFinalMetricsReport),
-      .periodic_report_seconds = args.GetOptional(M::MetricsReportingPeriod),
+      .dump_to_logcat = gFlags.MetricsWriteToLogcat(),
+      .dump_to_file = gFlags.MetricsWriteToFile.GetValueOptional(),
+      .dump_to_statsd = gFlags.MetricsWriteToStatsd(),
+      .period_spec = period_spec,
+      .reporting_num_mods = reporting_num_mods,
+      .reporting_mods = reporting_mods,
   };
 }
 
+std::optional<ReportingPeriodSpec> ReportingPeriodSpec::Parse(
+    const std::string& spec_str, std::string* error_msg) {
+  *error_msg = "";
+  if (spec_str.empty()) {
+    *error_msg = "Invalid empty spec.";
+    return std::nullopt;
+  }
+
+  // Split the string. Each element is separated by comma.
+  std::vector<std::string> elems;
+  Split(spec_str, ',', &elems);
+
+  // Check the startup marker (front) and the continuous one (back).
+  std::optional<ReportingPeriodSpec> spec = std::make_optional(ReportingPeriodSpec());
+  spec->spec = spec_str;
+  spec->report_startup_first = elems.front() == "S";
+  spec->continuous_reporting = elems.back() == "*";
+
+  // Compute the indices for the period values.
+  size_t start_interval_idx = spec->report_startup_first ? 1 : 0;
+  size_t end_interval_idx = spec->continuous_reporting ? (elems.size() - 1) : elems.size();
+
+  // '*' needs a numeric interval before in order to be valid.
+  if (spec->continuous_reporting &&
+      end_interval_idx == start_interval_idx) {
+    *error_msg = "Invalid period value in spec: " + spec_str;
+    return std::nullopt;
+  }
+
+  // Parse the periods.
+  for (size_t i = start_interval_idx; i < end_interval_idx; i++) {
+    uint32_t period;
+    if (!android::base::ParseUint(elems[i], &period)) {
+        *error_msg = "Invalid period value in spec: " + spec_str;
+        return std::nullopt;
+    }
+    spec->periods_seconds.push_back(period);
+  }
+
+  return spec;
+}
+
 }  // namespace metrics
 }  // namespace art
 
diff --git a/runtime/metrics/reporter.h b/runtime/metrics/reporter.h
index 3ad55b090f..4db5df163f 100644
--- a/runtime/metrics/reporter.h
+++ b/runtime/metrics/reporter.h
@@ -26,9 +26,47 @@
 namespace art {
 namespace metrics {
 
+/**
+ * Encapsulates the specification of the metric reporting periods.
+ *
+ * The period spec follows the following regex: "(S,)?(\d+,)*\*?"
+ * with the following semantics:
+ *   "S"         - will only report at startup.
+ *
+ *   "S,1,1"     - will report startup, than 1 second later, then another
+ *                 second later.
+ *
+ *   "S,1,2,4  " - will report at Startup time, then 1 seconds later,
+ *                 then 2, then finally 4 seconds later. After that, the
+ *                 reporting will stop.
+ *
+ *   "S,1,2,4,*" - same as above, but after the final 4s period, the
+ *                 reporting will continue every other 4s.
+ *                 '*' is an indication we should report continuously
+ *                 every N seconds, where N is the last period.
+ *
+ *   "2,*"       - will report every 2 seconds
+ *
+ * Note that "", "*", or "S,*" are not valid specs, and 'S' can only occur
+ * in the beginning.
+ */
+struct ReportingPeriodSpec {
+  static std::optional<ReportingPeriodSpec> Parse(
+      const std::string& spec_str, std::string* error_msg);
+
+  // The original spec.
+  std::string spec;
+  // The intervals when we should report.
+  std::vector<uint32_t> periods_seconds;
+  // Whether or not the reporting is continuous (contains a '*').
+  bool continuous_reporting{false};
+  // Whether or not the reporting should start after startup event (starts with an 'S').
+  bool report_startup_first{false};
+};
+
 // Defines the set of options for how metrics reporting happens.
 struct ReportingConfig {
-  static ReportingConfig FromRuntimeArguments(const RuntimeArgumentMap& args);
+  static ReportingConfig FromFlags(bool is_system_server = false);
 
   // Causes metrics to be written to the log, which makes them show up in logcat.
   bool dump_to_logcat{false};
@@ -39,22 +77,24 @@ struct ReportingConfig {
   // If set, provides a file name to enable metrics logging to a file.
   std::optional<std::string> dump_to_file;
 
-  // Indicates whether to report the final state of metrics on shutdown.
-  //
-  // Note that reporting only happens if some output, such as logcat, is enabled.
-  bool report_metrics_on_shutdown{true};
+  // The reporting period configuration.
+  std::optional<ReportingPeriodSpec> period_spec;
 
-  // If set, metrics will be reported every time this many seconds elapses.
-  std::optional<unsigned int> periodic_report_seconds;
+  // The mods that should report metrics. Together with reporting_num_mods, they
+  // dictate what percentage of the runtime execution will report metrics.
+  // If the `session_id (a random number) % reporting_num_mods < reporting_mods`
+  // then the runtime session will report metrics.
+  uint32_t reporting_mods{0};
+  uint32_t reporting_num_mods{100};
 };
 
 // MetricsReporter handles periodically reporting ART metrics.
 class MetricsReporter {
  public:
   // Creates a MetricsReporter instance that matches the options selected in ReportingConfig.
-  static std::unique_ptr<MetricsReporter> Create(ReportingConfig config, Runtime* runtime);
+  static std::unique_ptr<MetricsReporter> Create(const ReportingConfig& config, Runtime* runtime);
 
-  ~MetricsReporter();
+  virtual ~MetricsReporter();
 
   // Creates and runs the background reporting thread.
   //
@@ -70,26 +110,31 @@ class MetricsReporter {
   // completes.
   void NotifyStartupCompleted();
 
-  bool IsPeriodicReportingEnabled() const;
-
-  // Changes the reporting period.
-  //
-  // This function is not thread safe and may only be called before the background reporting thread
-  // has been started.
-  void SetReportingPeriod(unsigned int period_seconds);
-
   // Requests a metrics report
   //
   // If synchronous is set to true, this function will block until the report has completed.
   void RequestMetricsReport(bool synchronous = true);
 
+  // Reloads the metrics config from the given value.
+  // Can only be called before starting the background thread.
+  void ReloadConfig(const ReportingConfig& config);
+
   void SetCompilationInfo(CompilationReason compilation_reason,
                           CompilerFilter::Filter compiler_filter);
 
   static constexpr const char* kBackgroundThreadName = "Metrics Background Reporting Thread";
 
+ protected:
+  // Returns the metrics to be reported.
+  // This exists only for testing purposes so that we can verify reporting with minimum
+  // runtime interference.
+  virtual const ArtMetrics* GetMetrics();
+
+  MetricsReporter(const ReportingConfig& config, Runtime* runtime);
+
  private:
-  MetricsReporter(ReportingConfig config, Runtime* runtime);
+  // Whether or not we should reporting metrics according to the sampling rate.
+  bool IsMetricsReportingEnabled(const SessionData& session_data) const;
 
   // The background reporting thread main loop.
   void BackgroundThreadRun();
@@ -100,10 +145,26 @@ class MetricsReporter {
   // Outputs the current state of the metrics to the destination set by config_.
   void ReportMetrics();
 
+  // Whether or not we should wait for startup before reporting for the first time.
+  bool ShouldReportAtStartup() const;
+
+  // Whether or not we should continue reporting (either because we still
+  // have periods to report, or because we are in continuous mode).
+  bool ShouldContinueReporting() const;
+
+  // Returns the next reporting period.
+  // Must be called only if ShouldContinueReporting() is true.
+  uint32_t GetNextPeriodSeconds();
+
   ReportingConfig config_;
   Runtime* runtime_;
   std::vector<std::unique_ptr<MetricsBackend>> backends_;
   std::optional<std::thread> thread_;
+  // Whether or not we reported the startup event.
+  bool startup_reported_;
+  // The index into period_spec.periods_seconds which tells the next delay in
+  // seconds for the next reporting.
+  uint32_t report_interval_index_;
 
   // A message indicating that the reporting thread should shut down.
   struct ShutdownRequestedMessage {};
@@ -111,12 +172,6 @@ class MetricsReporter {
   // A message indicating that app startup has completed.
   struct StartupCompletedMessage {};
 
-  // A message marking the beginning of a metrics logging session.
-  //
-  // The primary purpose of this is to pass the session metadata from the Runtime to the metrics
-  // backends.
-  struct BeginSessionMessage{ SessionData session_data; };
-
   // A message requesting an explicit metrics report.
   //
   // The synchronous field specifies whether the reporting thread will send a message back when
@@ -132,7 +187,6 @@ class MetricsReporter {
 
   MessageQueue<ShutdownRequestedMessage,
                StartupCompletedMessage,
-               BeginSessionMessage,
                RequestMetricsReportMessage,
                CompilationInfoMessage>
       messages_;
@@ -144,6 +198,8 @@ class MetricsReporter {
 
   SessionData session_data_{};
   bool session_started_{false};
+
+  friend class MetricsReporterTest;
 };
 
 }  // namespace metrics
diff --git a/runtime/metrics/reporter_test.cc b/runtime/metrics/reporter_test.cc
new file mode 100644
index 0000000000..8c2a581ac1
--- /dev/null
+++ b/runtime/metrics/reporter_test.cc
@@ -0,0 +1,426 @@
+/*
+ * Copyright (C) 2021 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#include "reporter.h"
+
+#include "gtest/gtest.h"
+
+#include "common_runtime_test.h"
+#include "base/metrics/metrics.h"
+
+#pragma clang diagnostic push
+#pragma clang diagnostic error "-Wconversion"
+
+namespace art {
+namespace metrics {
+
+// Helper class to verify the metrics reporter.
+// The functionality is identical to the MetricsReporter with the exception of
+// the metrics source. Instead of taking its metrics from the current Runtime,
+// this class will keep its own copy so that it does not get interference from
+// other runtime setup logic.
+class MockMetricsReporter : public MetricsReporter {
+ protected:
+  MockMetricsReporter(const ReportingConfig& config, Runtime* runtime) :
+      MetricsReporter(config, runtime),
+      art_metrics_(new ArtMetrics()) {}
+
+  const ArtMetrics* GetMetrics() override {
+    return art_metrics_.get();
+  }
+
+  std::unique_ptr<ArtMetrics> art_metrics_;
+
+  friend class MetricsReporterTest;
+};
+
+// A test backend which keeps track of all metrics reporting.
+class TestBackend : public MetricsBackend {
+ public:
+  struct Report {
+    uint64_t timestamp_millis;
+    SafeMap<DatumId, uint64_t> data;
+  };
+
+  void BeginSession(const SessionData& session_data) override {
+    session_data_ = session_data;
+  }
+
+  void BeginReport(uint64_t timestamp_millis) override {
+    current_report_.reset(new Report());
+    current_report_->timestamp_millis = timestamp_millis;
+  }
+
+  void ReportCounter(DatumId counter_type, uint64_t value) override {
+    current_report_->data.Put(counter_type, value);
+  }
+
+  void ReportHistogram(DatumId histogram_type ATTRIBUTE_UNUSED,
+                       int64_t low_value ATTRIBUTE_UNUSED,
+                       int64_t high_value ATTRIBUTE_UNUSED,
+                       const std::vector<uint32_t>& buckets ATTRIBUTE_UNUSED) override {
+    // TODO: nothing yet. We should implement and test histograms as well.
+  }
+
+  void EndReport() override {
+    reports_.push_back(*current_report_);
+    current_report_ = nullptr;
+  }
+
+  const std::vector<Report>& GetReports() {
+    return reports_;
+  }
+
+ private:
+  SessionData session_data_;
+  std::vector<Report> reports_;
+  std::unique_ptr<Report> current_report_;
+};
+
+// The actual metrics test class
+class MetricsReporterTest : public CommonRuntimeTest {
+ protected:
+  void SetUp() override {
+    // Do the normal setup.
+    CommonRuntimeTest::SetUp();
+
+    // We need to start the runtime in order to run threads.
+    Thread::Current()->TransitionFromSuspendedToRunnable();
+    bool started = runtime_->Start();
+    CHECK(started);
+  }
+
+  // Configures the metric reporting.
+  void SetupReporter(const char* period_spec,
+                     uint32_t session_id = 1,
+                     uint32_t reporting_mods = 100) {
+    ReportingConfig config;
+    if (period_spec != nullptr) {
+      std::string error;
+      config.reporting_mods = reporting_mods;
+      config.period_spec = ReportingPeriodSpec::Parse(period_spec, &error);
+      ASSERT_TRUE(config.period_spec.has_value());
+    }
+
+    reporter_.reset(new MockMetricsReporter(std::move(config), Runtime::Current()));
+    backend_ = new TestBackend();
+    reporter_->backends_.emplace_back(backend_);
+
+    session_data_ = metrics::SessionData::CreateDefault();
+    session_data_.session_id = session_id;
+  }
+
+  void TearDown() override {
+    reporter_ = nullptr;
+    backend_ = nullptr;
+  }
+
+  bool ShouldReportAtStartup() {
+    return reporter_->ShouldReportAtStartup();
+  }
+
+  bool ShouldContinueReporting() {
+    return reporter_->ShouldContinueReporting();
+  }
+
+  uint32_t GetNextPeriodSeconds() {
+    return reporter_->GetNextPeriodSeconds();
+  }
+
+  void ReportMetrics() {
+    reporter_->ReportMetrics();
+  }
+
+  void NotifyStartupCompleted() {
+    reporter_->NotifyStartupCompleted();
+  }
+
+  // Starts the reporting thread and adds some metrics if necessary.
+  bool MaybeStartBackgroundThread(bool add_metrics) {
+    // TODO: set session_data.compilation_reason and session_data.compiler_filter
+    bool result = reporter_->MaybeStartBackgroundThread(session_data_);
+    if (add_metrics) {
+      reporter_->art_metrics_->JitMethodCompileCount()->Add(1);
+      reporter_->art_metrics_->ClassVerificationCount()->Add(2);
+    }
+    return result;
+  }
+
+  // Right now we either:
+  //   1) don't add metrics (with_metrics = false)
+  //   2) or always add the same metrics (see MaybeStartBackgroundThread)
+  // So we can write a global verify method.
+  void VerifyReports(uint32_t size, bool with_metrics) {
+    // TODO: we should iterate through all the other metrics to make sure they were not
+    // reported. However, we don't have an easy to use iteration mechanism over metrics yet.
+    // We should ads one
+    ASSERT_EQ(backend_->GetReports().size(), size);
+    for (auto report : backend_->GetReports()) {
+      ASSERT_EQ(report.data.Get(DatumId::kClassVerificationCount), with_metrics ? 2u : 0u);
+      ASSERT_EQ(report.data.Get(DatumId::kJitMethodCompileCount), with_metrics ? 1u : 0u);
+    }
+  }
+
+  // Sleeps until the backend received the give number of reports.
+  void WaitForReport(uint32_t report_count, uint32_t sleep_period_ms) {
+    while (true) {
+      if (backend_->GetReports().size() == report_count) {
+        return;
+      }
+      usleep(sleep_period_ms * 1000);
+    }
+  }
+
+ private:
+  std::unique_ptr<MockMetricsReporter> reporter_;
+  TestBackend* backend_;
+  metrics::SessionData session_data_;
+};
+
+// Verifies startup reporting.
+TEST_F(MetricsReporterTest, StartupOnly) {
+  SetupReporter("S");
+
+  // Verify startup conditions
+  ASSERT_TRUE(ShouldReportAtStartup());
+  ASSERT_FALSE(ShouldContinueReporting());
+
+  // Start the thread and notify the startup. This will advance the state.
+  MaybeStartBackgroundThread(/*add_metrics=*/ true);
+
+  NotifyStartupCompleted();
+  WaitForReport(/*report_count=*/ 1, /*sleep_period_ms=*/ 50);
+  VerifyReports(/*size=*/ 1, /*with_metrics*/ true);
+
+  // We still should not report at period.
+  ASSERT_FALSE(ShouldContinueReporting());
+}
+
+// LARGE TEST: This test takes 1s to run.
+// Verifies startup reporting, followed by a fixed, one time only reporting.
+TEST_F(MetricsReporterTest, StartupAndPeriod) {
+  SetupReporter("S,1");
+
+  // Verify startup conditions
+  ASSERT_TRUE(ShouldReportAtStartup());
+  ASSERT_FALSE(ShouldContinueReporting());
+
+  // Start the thread and notify the startup. This will advance the state.
+  MaybeStartBackgroundThread(/*add_metrics=*/ true);
+  NotifyStartupCompleted();
+
+  // We're waiting for 2 reports: the startup one, and the 1s one.
+  WaitForReport(/*report_count=*/ 2, /*sleep_period_ms=*/ 500);
+  VerifyReports(/*size=*/ 2, /*with_metrics*/ true);
+
+  // We should not longer report at period.
+  ASSERT_FALSE(ShouldContinueReporting());
+}
+
+// LARGE TEST: This takes take 2s to run.
+// Verifies startup reporting, followed by continuous reporting.
+TEST_F(MetricsReporterTest, StartupAndPeriodContinuous) {
+  SetupReporter("S,1,*");
+
+  // Verify startup conditions
+  ASSERT_TRUE(ShouldReportAtStartup());
+  ASSERT_FALSE(ShouldContinueReporting());
+
+  // Start the thread and notify the startup. This will advance the state.
+  MaybeStartBackgroundThread(/*add_metrics=*/ true);
+  NotifyStartupCompleted();
+
+  // We're waiting for 3 reports: the startup one, and the 1s one.
+  WaitForReport(/*report_count=*/ 3, /*sleep_period_ms=*/ 500);
+  VerifyReports(/*size=*/ 3, /*with_metrics*/ true);
+
+  // We should keep reporting at period.
+  ASSERT_TRUE(ShouldContinueReporting());
+}
+
+// LARGE TEST: This test takes 1s to run.
+// Verifies a fixed, one time only reporting.
+TEST_F(MetricsReporterTest, OneTime) {
+  SetupReporter("1");
+
+  // Verify startup conditions
+  ASSERT_FALSE(ShouldReportAtStartup());
+  ASSERT_TRUE(ShouldContinueReporting());
+
+  // Start the thread and notify the startup. This will advance the state.
+  MaybeStartBackgroundThread(/*add_metrics=*/ true);
+
+  // We're waiting for 1 report
+  WaitForReport(/*report_count=*/ 1, /*sleep_period_ms=*/ 500);
+  VerifyReports(/*size=*/ 1, /*with_metrics*/ true);
+
+  // We should not longer report at period.
+  ASSERT_FALSE(ShouldContinueReporting());
+}
+
+// LARGE TEST: This takes take 5s to run.
+// Verifies a sequence of reporting, at different interval of times.
+TEST_F(MetricsReporterTest, PeriodContinuous) {
+  SetupReporter("1,2,*");
+
+  // Verify startup conditions
+  ASSERT_FALSE(ShouldReportAtStartup());
+  ASSERT_TRUE(ShouldContinueReporting());
+
+  // Start the thread and notify the startup. This will advance the state.
+  MaybeStartBackgroundThread(/*add_metrics=*/ true);
+  NotifyStartupCompleted();
+
+  // We're waiting for 2 reports: the startup one, and the 1s one.
+  WaitForReport(/*report_count=*/ 3, /*sleep_period_ms=*/ 500);
+  VerifyReports(/*size=*/ 3, /*with_metrics*/ true);
+
+  // We should keep reporting at period.
+  ASSERT_TRUE(ShouldContinueReporting());
+}
+
+// LARGE TEST: This test takes 1s to run.
+// Verifies reporting when no metrics where recorded.
+TEST_F(MetricsReporterTest, NoMetrics) {
+  SetupReporter("1");
+
+  // Verify startup conditions
+  ASSERT_FALSE(ShouldReportAtStartup());
+  ASSERT_TRUE(ShouldContinueReporting());
+
+  // Start the thread and notify the startup. This will advance the state.
+  MaybeStartBackgroundThread(/*add_metrics=*/ false);
+
+  // We're waiting for 1 report
+  WaitForReport(/*report_count=*/ 1, /*sleep_period_ms=*/ 500);
+  VerifyReports(/*size=*/ 1, /*with_metrics*/ false);
+
+  // We should not longer report at period.
+  ASSERT_FALSE(ShouldContinueReporting());
+}
+
+// Verify we don't start reporting if the sample rate is set to 0.
+TEST_F(MetricsReporterTest, SampleRateDisable) {
+  SetupReporter("1", /*session_id=*/ 1, /*reporting_mods=*/ 0);
+
+  // The background thread should not start.
+  ASSERT_FALSE(MaybeStartBackgroundThread(/*add_metrics=*/ false));
+
+  ASSERT_FALSE(ShouldReportAtStartup());
+  ASSERT_FALSE(ShouldContinueReporting());
+}
+
+// Verify we don't start reporting if the sample rate is low and the session does
+// not meet conditions.
+TEST_F(MetricsReporterTest, SampleRateDisable24) {
+  SetupReporter("1", /*session_id=*/ 125, /*reporting_mods=*/ 24);
+
+  // The background thread should not start.
+  ASSERT_FALSE(MaybeStartBackgroundThread(/*add_metrics=*/ false));
+
+  ASSERT_FALSE(ShouldReportAtStartup());
+  ASSERT_FALSE(ShouldContinueReporting());
+}
+
+// Verify we start reporting if the sample rate and the session meet
+// reporting conditions
+TEST_F(MetricsReporterTest, SampleRateEnable50) {
+  SetupReporter("1", /*session_id=*/ 125, /*reporting_mods=*/ 50);
+
+  // The background thread should not start.
+  ASSERT_TRUE(MaybeStartBackgroundThread(/*add_metrics=*/ false));
+
+  ASSERT_FALSE(ShouldReportAtStartup());
+  ASSERT_TRUE(ShouldContinueReporting());
+}
+
+// Verify we start reporting if the sample rate and the session meet
+// reporting conditions
+TEST_F(MetricsReporterTest, SampleRateEnableAll) {
+  SetupReporter("1", /*session_id=*/ 1099, /*reporting_mods=*/ 100);
+
+  // The background thread should not start.
+  ASSERT_TRUE(MaybeStartBackgroundThread(/*add_metrics=*/ false));
+
+  ASSERT_FALSE(ShouldReportAtStartup());
+  ASSERT_TRUE(ShouldContinueReporting());
+}
+
+
+// Test class for period spec parsing
+class ReportingPeriodSpecTest : public testing::Test {
+ public:
+  void VerifyFalse(const std::string& spec_str) {
+    Verify(spec_str, false, false, false, {});
+  }
+
+  void VerifyTrue(
+      const std::string& spec_str,
+      bool startup_first,
+      bool continuous,
+      std::vector<uint32_t> periods) {
+    Verify(spec_str, true, startup_first, continuous, periods);
+  }
+
+  void Verify(
+      const std::string& spec_str,
+      bool valid,
+      bool startup_first,
+      bool continuous,
+      std::vector<uint32_t> periods) {
+    std::string error_msg;
+    std::optional<ReportingPeriodSpec> spec = ReportingPeriodSpec::Parse(spec_str, &error_msg);
+
+    ASSERT_EQ(valid, spec.has_value()) << spec_str;
+    if (valid) {
+        ASSERT_EQ(spec->spec, spec_str) << spec_str;
+        ASSERT_EQ(spec->report_startup_first, startup_first) << spec_str;
+        ASSERT_EQ(spec->continuous_reporting, continuous) << spec_str;
+        ASSERT_EQ(spec->periods_seconds, periods) << spec_str;
+    }
+  }
+};
+
+TEST_F(ReportingPeriodSpecTest, ParseTestsInvalid) {
+  VerifyFalse("");
+  VerifyFalse("*");
+  VerifyFalse("S,*");
+  VerifyFalse("foo");
+  VerifyFalse("-1");
+  VerifyFalse("1,S");
+  VerifyFalse("*,1");
+  VerifyFalse("1,2,3,-1,3");
+  VerifyFalse("1,*,2");
+  VerifyFalse("1,S,2");
+}
+
+TEST_F(ReportingPeriodSpecTest, ParseTestsValid) {
+  VerifyTrue("S", true, false, {});
+  VerifyTrue("S,1", true, false, {1});
+  VerifyTrue("S,1,2,3,4", true, false, {1, 2, 3, 4});
+  VerifyTrue("S,1,*", true, true, {1});
+  VerifyTrue("S,1,2,3,4,*", true, true, {1, 2, 3, 4});
+
+  VerifyTrue("1", false, false, {1});
+  VerifyTrue("1,2,3,4", false, false, {1, 2, 3, 4});
+  VerifyTrue("1,*", false, true, {1});
+  VerifyTrue("1,2,3,4,*", false, true, {1, 2, 3, 4});
+}
+
+}  // namespace metrics
+}  // namespace art
+
+#pragma clang diagnostic pop  // -Wconversion
diff --git a/runtime/oat_file.cc b/runtime/oat_file.cc
index 8a5ad63b5d..49d5e61013 100644
--- a/runtime/oat_file.cc
+++ b/runtime/oat_file.cc
@@ -803,20 +803,18 @@ bool OatFileBase::Setup(int zip_fd,
     const bool valid_magic = DexFileLoader::IsMagicValid(dex_file_pointer);
     if (UNLIKELY(!valid_magic)) {
       *error_msg = StringPrintf("In oat file '%s' found OatDexFile #%zu for '%s' with invalid "
-                                    "dex file magic '%s'",
+                                    "dex file magic",
                                 GetLocation().c_str(),
                                 i,
-                                dex_file_location.c_str(),
-                                dex_file_pointer);
+                                dex_file_location.c_str());
       return false;
     }
     if (UNLIKELY(!DexFileLoader::IsVersionAndMagicValid(dex_file_pointer))) {
       *error_msg = StringPrintf("In oat file '%s' found OatDexFile #%zu for '%s' with invalid "
-                                    "dex file version '%s'",
+                                    "dex file version",
                                 GetLocation().c_str(),
                                 i,
-                                dex_file_location.c_str(),
-                                dex_file_pointer);
+                                dex_file_location.c_str());
       return false;
     }
     const DexFile::Header* header = reinterpret_cast<const DexFile::Header*>(dex_file_pointer);
@@ -1603,6 +1601,12 @@ class OatFileBackedByVdex final : public OatFileBase {
       for (const uint8_t* dex_file_start = vdex_file->GetNextDexFileData(nullptr, i);
            dex_file_start != nullptr;
            dex_file_start = vdex_file->GetNextDexFileData(dex_file_start, ++i)) {
+        if (UNLIKELY(!DexFileLoader::IsVersionAndMagicValid(dex_file_start))) {
+          *error_msg =
+              StringPrintf("In vdex file '%s' found dex file with invalid dex file version",
+                           dex_location.c_str());
+          return nullptr;
+        }
         // Create the OatDexFile and add it to the owning container.
         std::string location = DexFileLoader::GetMultiDexLocation(i, dex_location.c_str());
         std::string canonical_location = DexFileLoader::GetDexCanonicalLocation(location.c_str());
diff --git a/runtime/parsed_options.cc b/runtime/parsed_options.cc
index 7699b25311..c85c951deb 100644
--- a/runtime/parsed_options.cc
+++ b/runtime/parsed_options.cc
@@ -412,25 +412,6 @@ std::unique_ptr<RuntimeParser> ParsedOptions::MakeParser(bool ignore_unrecognize
           .IntoKey(M::CorePlatformApiPolicy)
       .Define("-Xuse-stderr-logger")
           .IntoKey(M::UseStderrLogger)
-      .Define("-Xwrite-metrics-to-log")
-          .WithHelp("Enables writing ART metrics to logcat")
-          .IntoKey(M::WriteMetricsToLog)
-      .Define("-Xwrite-metrics-to-statsd=_")
-          .WithType<bool>()
-          .WithValueMap({{"false", false}, {"true", true}})
-          .WithHelp("Enables writing ART metrics to statsd")
-          .IntoKey(M::WriteMetricsToStatsd)
-      .Define("-Xwrite-metrics-to-file=_")
-          .WithHelp("Enables writing ART metrics to the given file")
-          .WithType<std::string>()
-          .IntoKey(M::WriteMetricsToFile)
-      .Define("-Xdisable-final-metrics-report")
-          .WithHelp("Disables reporting metrics when ART shuts down")
-          .IntoKey(M::DisableFinalMetricsReport)
-      .Define("-Xmetrics-reporting-period=_")
-          .WithHelp("The time in seconds between metrics reports")
-          .WithType<unsigned int>()
-          .IntoKey(M::MetricsReportingPeriod)
       .Define("-Xonly-use-system-oat-files")
           .IntoKey(M::OnlyUseTrustedOatFiles)
       .Define("-Xverifier-logging-threshold=_")
diff --git a/runtime/runtime.cc b/runtime/runtime.cc
index 74a563e23a..370ee02790 100644
--- a/runtime/runtime.cc
+++ b/runtime/runtime.cc
@@ -1088,16 +1088,17 @@ void Runtime::InitNonZygoteOrPostFork(
   GetMetrics()->Reset();
 
   if (metrics_reporter_ != nullptr) {
-    if (IsSystemServer() && !metrics_reporter_->IsPeriodicReportingEnabled()) {
-      // For system server, we don't get startup metrics, so make sure we have periodic reporting
-      // enabled.
-      //
-      // Note that this does not override the command line argument if one is given.
-      metrics_reporter_->SetReportingPeriod(kOneHourInSeconds);
-    }
+    // Now that we know if we are an app or system server, reload the metrics reporter config
+    // in case there are any difference.
+    metrics::ReportingConfig metrics_config =
+        metrics::ReportingConfig::FromFlags(is_system_server);
+
+    metrics_reporter_->ReloadConfig(metrics_config);
 
     metrics::SessionData session_data{metrics::SessionData::CreateDefault()};
-    session_data.session_id = GetRandomNumber<int64_t>(0, std::numeric_limits<int64_t>::max());
+    // Start the session id from 1 to avoid clashes with the default value.
+    // (better for debugability)
+    session_data.session_id = GetRandomNumber<int64_t>(1, std::numeric_limits<int64_t>::max());
     // TODO: set session_data.compilation_reason and session_data.compiler_filter
     metrics_reporter_->MaybeStartBackgroundThread(session_data);
   }
@@ -1838,7 +1839,7 @@ bool Runtime::Init(RuntimeArgumentMap&& runtime_options_in) {
   // Class-roots are setup, we can now finish initializing the JniIdManager.
   GetJniIdManager()->Init(self);
 
-  InitMetrics(runtime_options);
+  InitMetrics();
 
   // Runtime initialization is largely done now.
   // We load plugins first since that can modify the runtime state slightly.
@@ -1938,8 +1939,8 @@ bool Runtime::Init(RuntimeArgumentMap&& runtime_options_in) {
   return true;
 }
 
-void Runtime::InitMetrics(const RuntimeArgumentMap& runtime_options) {
-  auto metrics_config = metrics::ReportingConfig::FromRuntimeArguments(runtime_options);
+void Runtime::InitMetrics() {
+  metrics::ReportingConfig metrics_config = metrics::ReportingConfig::FromFlags();
   metrics_reporter_ = metrics::MetricsReporter::Create(metrics_config, this);
 }
 
diff --git a/runtime/runtime.h b/runtime/runtime.h
index 9026e3dad8..5bea321fb3 100644
--- a/runtime/runtime.h
+++ b/runtime/runtime.h
@@ -1028,7 +1028,7 @@ class Runtime {
       SHARED_TRYLOCK_FUNCTION(true, Locks::mutator_lock_);
   void InitNativeMethods() REQUIRES(!Locks::mutator_lock_);
   void RegisterRuntimeNativeMethods(JNIEnv* env);
-  void InitMetrics(const RuntimeArgumentMap& runtime_options);
+  void InitMetrics();
 
   void StartDaemonThreads();
   void StartSignalCatcher();
diff --git a/runtime/runtime_options.def b/runtime/runtime_options.def
index d1b37e5e4f..b73f5c1f7e 100644
--- a/runtime/runtime_options.def
+++ b/runtime/runtime_options.def
@@ -191,11 +191,4 @@ RUNTIME_OPTIONS_KEY (bool,                PerfettoHprof,                  false)
 // This is to enable/disable Perfetto Java Heap Stack Profiling
 RUNTIME_OPTIONS_KEY (bool,                PerfettoJavaHeapStackProf,      false)
 
-// Metrics configuration settings
-RUNTIME_OPTIONS_KEY (Unit,                WriteMetricsToLog)
-RUNTIME_OPTIONS_KEY (bool,                WriteMetricsToStatsd,           true)
-RUNTIME_OPTIONS_KEY (std::string,         WriteMetricsToFile)
-RUNTIME_OPTIONS_KEY (Unit,                DisableFinalMetricsReport)
-RUNTIME_OPTIONS_KEY (unsigned int,        MetricsReportingPeriod)
-
 #undef RUNTIME_OPTIONS_KEY