Move metric reporting config to flags
This will enable us to enable periodic reporting according
to the properties set in the device config.
As part of this CL, enable cmdline only flags for thing that
do not make sense to read from system properties.
Test: gtest
Bug: 170149255
Change-Id: I99bae25d89cf3a17906b4d3c671e5c63e9a3c180
diff --git a/libartbase/base/flags.cc b/libartbase/base/flags.cc
index 4b7e780..aaa7661 100644
--- a/libartbase/base/flags.cc
+++ b/libartbase/base/flags.cc
@@ -84,10 +84,11 @@
}
template <typename Value>
-Flag<Value>::Flag(const std::string& name, Value default_value) :
+Flag<Value>::Flag(const std::string& name, Value default_value, FlagType type) :
FlagBase(GenerateCmdLineArgName(name),
GenerateSysPropName(name),
- GeneratePhenotypeName(name)),
+ GeneratePhenotypeName(name),
+ type),
initialized_{false},
default_{default_value} {
ALL_FLAGS.push_front(this);
@@ -100,7 +101,12 @@
template <typename Value>
void Flag<Value>::Reload() {
- // The cmdline flags are loaded by the parsed_options infra.
+ initialized_ = true;
+
+ // The cmdline flags are loaded by the parsed_options infra. No action needed here.
+ if (type_ == FlagType::kCmdlineOnly) {
+ return;
+ }
// Load system properties.
from_system_property_ = std::nullopt;
@@ -116,8 +122,6 @@
if (server_config != kUndefinedValue) {
ParseValue(server_config, &from_server_setting_);
}
-
- initialized_ = true;
}
template <typename Value>
@@ -131,8 +135,16 @@
template <typename Value>
void Flag<Value>::Dump(std::ostream& oss) const {
- std::pair<Value, std::string> valueLoc = GetValueLocation();
- oss << "value: " << std::get<0>(valueLoc) << " (from " << std::get<1>(valueLoc) << ")";
+ std::pair<Value, FlagOrigin> valueOrigin = GetValueAndOrigin();
+ std::string origin;
+ switch (std::get<1>(valueOrigin)) {
+ case FlagOrigin::kDefaultValue: origin = "default_value"; break;
+ case FlagOrigin::kCmdlineArg: origin = "cmdline_arg"; break;
+ case FlagOrigin::kSystemProperty: origin = "system_property"; break;
+ case FlagOrigin::kServerSetting: origin = "server_setting"; break;
+ }
+
+ oss << "value: " << std::get<0>(valueOrigin) << " (from " << origin << ")";
oss << "\n default: " << default_;
oss << "\n " << command_line_argument_name_ << ": ";
diff --git a/libartbase/base/flags.h b/libartbase/base/flags.h
index 973a5da..e71bb4a 100644
--- a/libartbase/base/flags.h
+++ b/libartbase/base/flags.h
@@ -43,6 +43,15 @@
namespace art {
+// Enum representing the type of the ART flag.
+enum class FlagType {
+ // A flag that only looks at the cmdline argument to retrieve its value.
+ kCmdlineOnly,
+ // A flag that also looks at system properties and device config
+ // (phenotype properties) when retrieving its value.
+ kDeviceConfig,
+};
+
// FlagMetaBase handles automatically adding flags to the command line parser. It is parameterized
// by all supported flag types. In general, this should be treated as though it does not exist and
// FlagBase, which is already specialized to the types we support, should be used instead.
@@ -51,10 +60,12 @@
public:
FlagMetaBase(const std::string&& command_line_argument_name,
const std::string&& system_property_name,
- const std::string&& server_setting_name) :
+ const std::string&& server_setting_name,
+ FlagType type) :
command_line_argument_name_(command_line_argument_name),
system_property_name_(system_property_name),
- server_setting_name_(server_setting_name) {}
+ server_setting_name_(server_setting_name),
+ type_(type) {}
virtual ~FlagMetaBase() {}
template <typename Builder>
@@ -124,6 +135,7 @@
const std::string command_line_argument_name_;
const std::string system_property_name_;
const std::string server_setting_name_;
+ FlagType type_;
};
using FlagBase = FlagMetaBase<bool, int, std::string>;
@@ -133,13 +145,21 @@
class FlagsTests;
+// Describes the possible origins of a flag value.
+enum class FlagOrigin {
+ kDefaultValue,
+ kCmdlineArg,
+ kSystemProperty,
+ kServerSetting,
+};
+
// This class defines a flag with a value of a particular type.
template <typename Value>
class Flag : public FlagBase {
public:
// Create a new Flag. The name parameter is used to generate the names from the various parameter
// sources. See the documentation on the Flags struct for an example.
- explicit Flag(const std::string& name, Value default_value = {});
+ Flag(const std::string& name, Value default_value, FlagType type);
virtual ~Flag();
@@ -151,26 +171,41 @@
// 3) cmdline flag
// 4) default value
ALWAYS_INLINE Value GetValue() const {
- return std::get<0>(GetValueLocation());
+ return std::get<0>(GetValueAndOrigin());
}
ALWAYS_INLINE Value operator()() const {
return GetValue();
}
- // Returns the value and the location of that value for the given flag.
- ALWAYS_INLINE std::pair<Value, std::string> GetValueLocation() const {
+ // Return the value of the flag as optional.
+ //
+ // Returns the value of the flag if and only if the flag is set via
+ // a server side setting, system property or a cmdline arg.
+ // Otherwise it returns nullopt (meaning this never returns the default value).
+ //
+ // This is useful for properties that do not have a good default natural value
+ // (e.g. file path arguments).
+ ALWAYS_INLINE std::optional<Value> GetValueOptional() const {
+ std::pair<Value, FlagOrigin> result = GetValueAndOrigin();
+ return std::get<1>(result) == FlagOrigin::kDefaultValue
+ ? std::nullopt
+ : std::make_optional(std::get<0>(result));
+ }
+
+ // Returns the value and the origin of that value for the given flag.
+ ALWAYS_INLINE std::pair<Value, FlagOrigin> GetValueAndOrigin() const {
DCHECK(initialized_);
if (from_server_setting_.has_value()) {
- return std::pair{from_server_setting_.value(), server_setting_name_};
+ return std::pair{from_server_setting_.value(), FlagOrigin::kServerSetting};
}
if (from_system_property_.has_value()) {
- return std::pair{from_system_property_.value(), system_property_name_};
+ return std::pair{from_system_property_.value(), FlagOrigin::kSystemProperty};
}
if (from_command_line_.has_value()) {
- return std::pair{from_command_line_.value(), command_line_argument_name_};
+ return std::pair{from_command_line_.value(), FlagOrigin::kCmdlineArg};
}
- return std::pair{default_, "default_value"};
+ return std::pair{default_, FlagOrigin::kDefaultValue};
}
void Dump(std::ostream& oss) const override;
@@ -199,7 +234,7 @@
//
// Example:
//
-// Flag<int> WriteMetricsToLog{"my-feature-test.flag", 42};
+// Flag<int> WriteMetricsToLog{"my-feature-test.flag", 42, FlagType::kDeviceConfig};
//
// This creates a boolean flag that can be read through gFlags.WriteMetricsToLog(). The default
// value is false. Note that the default value can be left unspecified, in which the value of the
@@ -221,7 +256,13 @@
struct Flags {
// Flag used to test the infra.
// TODO: can be removed once we add real flags.
- Flag<int> MyFeatureTestFlag{"my-feature-test.flag", /*default_value=*/ 42};
+ Flag<int> MyFeatureTestFlag{"my-feature-test.flag", 42, FlagType::kDeviceConfig};
+
+ // Metric infra flags.
+ Flag<bool> WriteMetricsToStatsd{ "metrics.write-to-statsd", false, FlagType::kDeviceConfig};
+ Flag<bool> WriteMetricsToLogcat{ "metrics.write-to-logcat", false, FlagType::kCmdlineOnly};
+ Flag<int> MetricsReportingPeriod{"metrics.reporting-period", 0, FlagType::kCmdlineOnly};
+ Flag<std::string> WriteMetricsToFile{"metrics.write-to-file", "", FlagType::kCmdlineOnly};
};
// This is the actual instance of all the flags.
diff --git a/libartbase/base/flags_test.cc b/libartbase/base/flags_test.cc
index 72f0431..b41df46 100644
--- a/libartbase/base/flags_test.cc
+++ b/libartbase/base/flags_test.cc
@@ -30,7 +30,7 @@
class TestFlag {
public:
// Takes control of the tmp_file pointer.
- explicit TestFlag(ScratchFile* tmp_file) {
+ TestFlag(ScratchFile* tmp_file, FlagType flag_type) {
tmp_file_.reset(tmp_file);
std::string tmp_name = tmp_file_->GetFilename();
@@ -43,7 +43,7 @@
cmd_line_name_ = flag_name_;
std::replace(cmd_line_name_.begin(), cmd_line_name_.end(), '.', '-');
- flag_.reset(new Flag<int>(flag_name_, /*default_value=*/ 42));
+ flag_.reset(new Flag<int>(flag_name_, /*default_value=*/ 42, flag_type));
}
void AssertCmdlineValue(bool has_value, int expected) {
@@ -104,7 +104,7 @@
//
// So we do it in SetUpRuntimeOptions.
virtual void SetUpRuntimeOptions(RuntimeOptions* options) {
- test_flag_.reset(new TestFlag(new ScratchFile()));
+ test_flag_.reset(new TestFlag(new ScratchFile(), FlagType::kDeviceConfig));
CommonRuntimeTest::SetUpRuntimeOptions(options);
}
@@ -116,7 +116,11 @@
std::unique_ptr<TestFlag> test_flag_;
};
-class FlagsTestsWithCmdLine : public FlagsTests {
+class FlagsTestsWithCmdLineBase : public FlagsTests {
+ public:
+ explicit FlagsTestsWithCmdLineBase(FlagType type) : flag_type_(type) {
+ }
+
protected:
virtual void TearDown() {
android::base::SetProperty(test_flag_->SystemProperty(), "");
@@ -125,10 +129,24 @@
}
virtual void SetUpRuntimeOptions(RuntimeOptions* options) {
- test_flag_.reset(new TestFlag(new ScratchFile()));
+ test_flag_.reset(new TestFlag(new ScratchFile(), flag_type_));
std::string option = "-X" + test_flag_->CmdLineName() + ":1";
options->emplace_back(option.c_str(), nullptr);
}
+
+ FlagType flag_type_;
+};
+
+class FlagsTestsWithCmdLine : public FlagsTestsWithCmdLineBase {
+ public:
+ FlagsTestsWithCmdLine() : FlagsTestsWithCmdLineBase(FlagType::kDeviceConfig) {
+ }
+};
+
+class FlagsTestsCmdLineOnly : public FlagsTestsWithCmdLineBase {
+ public:
+ FlagsTestsCmdLineOnly() : FlagsTestsWithCmdLineBase(FlagType::kCmdlineOnly) {
+ }
};
// Validate that when no flag is set, the default is taken and none of the other
@@ -202,4 +220,28 @@
ASSERT_EQ(test_flag_->Value(), 1);
}
+// Validate that cmdline only flags don't read system properties.
+TEST_F(FlagsTestsCmdLineOnly, CmdlineOnlyFlags) {
+ if (!android::base::SetProperty(test_flag_->SystemProperty(), "2")) {
+ LOG(ERROR) << "Release does not support property setting, skipping test: "
+ << test_flag_->SystemProperty();
+ return;
+ }
+
+ if (android::base::SetProperty(test_flag_->ServerSetting(), "3")) {
+ LOG(ERROR) << "Release does not support property setting, skipping test: "
+ << test_flag_->ServerSetting();
+ return;
+ }
+
+ FlagBase::ReloadAllFlags("test");
+
+ test_flag_->AssertCmdlineValue(true, 1);
+ test_flag_->AssertSysPropValue(false, 2);
+ test_flag_->AssertServerSettingValue(false, 3);
+ test_flag_->AssertDefaultValue(42);
+
+ ASSERT_EQ(test_flag_->Value(), 1);
+}
+
} // namespace art
diff --git a/runtime/metrics/reporter.cc b/runtime/metrics/reporter.cc
index 4cf1ba5..26b55b8 100644
--- a/runtime/metrics/reporter.cc
+++ b/runtime/metrics/reporter.cc
@@ -16,6 +16,7 @@
#include "reporter.h"
+#include "base/flags.h"
#include "runtime.h"
#include "runtime_options.h"
#include "statsd.h"
@@ -121,10 +122,7 @@
LOG_STREAM(DEBUG) << "Shutdown request received";
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";
@@ -178,14 +176,12 @@
}
}
-ReportingConfig ReportingConfig::FromRuntimeArguments(const RuntimeArgumentMap& args) {
- using M = RuntimeArgumentMap;
+ReportingConfig ReportingConfig::FromFlags() {
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.WriteMetricsToLogcat(),
+ .dump_to_file = gFlags.WriteMetricsToFile.GetValueOptional(),
+ .dump_to_statsd = gFlags.WriteMetricsToStatsd(),
+ .periodic_report_seconds = gFlags.MetricsReportingPeriod.GetValueOptional(),
};
}
diff --git a/runtime/metrics/reporter.h b/runtime/metrics/reporter.h
index 3ad55b0..3e8056b 100644
--- a/runtime/metrics/reporter.h
+++ b/runtime/metrics/reporter.h
@@ -28,7 +28,7 @@
// Defines the set of options for how metrics reporting happens.
struct ReportingConfig {
- static ReportingConfig FromRuntimeArguments(const RuntimeArgumentMap& args);
+ static ReportingConfig FromFlags();
// Causes metrics to be written to the log, which makes them show up in logcat.
bool dump_to_logcat{false};
@@ -39,11 +39,6 @@
// 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};
-
// If set, metrics will be reported every time this many seconds elapses.
std::optional<unsigned int> periodic_report_seconds;
};
diff --git a/runtime/parsed_options.cc b/runtime/parsed_options.cc
index cbee356..3acb975 100644
--- a/runtime/parsed_options.cc
+++ b/runtime/parsed_options.cc
@@ -415,25 +415,6 @@
.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 d54ef6b..f038969 100644
--- a/runtime/runtime.cc
+++ b/runtime/runtime.cc
@@ -1847,7 +1847,7 @@
// 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.
@@ -1947,8 +1947,8 @@
return true;
}
-void Runtime::InitMetrics(const RuntimeArgumentMap& runtime_options) {
- auto metrics_config = metrics::ReportingConfig::FromRuntimeArguments(runtime_options);
+void Runtime::InitMetrics() {
+ auto metrics_config = metrics::ReportingConfig::FromFlags();
metrics_reporter_ = metrics::MetricsReporter::Create(metrics_config, this);
}
diff --git a/runtime/runtime.h b/runtime/runtime.h
index f399849..1413b98 100644
--- a/runtime/runtime.h
+++ b/runtime/runtime.h
@@ -1032,7 +1032,7 @@
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 3717fbf..11387a2 100644
--- a/runtime/runtime_options.def
+++ b/runtime/runtime_options.def
@@ -192,11 +192,4 @@
// 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
diff --git a/test/2232-write-metrics-to-log/run b/test/2232-write-metrics-to-log/run
index b170317..07c1e60 100755
--- a/test/2232-write-metrics-to-log/run
+++ b/test/2232-write-metrics-to-log/run
@@ -15,4 +15,4 @@
# limitations under the License.
export ANDROID_LOG_TAGS="*:i"
-exec ${RUN} $@ --external-log-tags --runtime-option -Xwrite-metrics-to-log
+exec ${RUN} $@ --external-log-tags --runtime-option -Xmetrics-write-to-logcat:true