Refactor determine metric update status
Refactor and create a generic determineMetricUpdateStatus() function
that works for all metrics. Will simplify the process of determining
if a metric needs an update for all future metric types.
Test: atest statsd_test
Bug: 168863134
Bug: 176249205
Bug: 176248686
Merged-In: I751e2c7c8bbbd819f216821cbf4f0ec315c4563b
Change-Id: If89e1625cf7536406fbd168aca63193d5e9a908f
diff --git a/bin/src/metrics/parsing_utils/config_update_utils.cpp b/bin/src/metrics/parsing_utils/config_update_utils.cpp
index 5dbf16d..2ae5763 100644
--- a/bin/src/metrics/parsing_utils/config_update_utils.cpp
+++ b/bin/src/metrics/parsing_utils/config_update_utils.cpp
@@ -24,6 +24,8 @@
#include "matchers/EventMatcherWizard.h"
#include "metrics_manager_util.h"
+using google::protobuf::MessageLite;
+
namespace android {
namespace os {
namespace statsd {
@@ -419,16 +421,19 @@
return false;
}
-bool determineEventMetricUpdateStatus(const StatsdConfig& config, const EventMetric& metric,
- const unordered_map<int64_t, int>& oldMetricProducerMap,
- const vector<sp<MetricProducer>>& oldMetricProducers,
- const unordered_map<int64_t, int>& metricToActivationMap,
- const set<int64_t>& replacedMatchers,
- const set<int64_t>& replacedConditions,
- UpdateStatus& updateStatus) {
- int64_t id = metric.id();
+bool determineMetricUpdateStatus(
+ const StatsdConfig& config, const MessageLite& metric, const int64_t metricId,
+ const MetricType metricType, const set<int64_t>& matcherDependencies,
+ const set<int64_t>& conditionDependencies,
+ const ::google::protobuf::RepeatedField<int64_t>& stateDependencies,
+ const ::google::protobuf::RepeatedPtrField<MetricConditionLink>& conditionLinks,
+ const unordered_map<int64_t, int>& oldMetricProducerMap,
+ const vector<sp<MetricProducer>>& oldMetricProducers,
+ const unordered_map<int64_t, int>& metricToActivationMap,
+ const set<int64_t>& replacedMatchers, const set<int64_t>& replacedConditions,
+ const set<int64_t>& replacedStates, UpdateStatus& updateStatus) {
// Check if new metric
- const auto& oldMetricProducerIt = oldMetricProducerMap.find(id);
+ const auto& oldMetricProducerIt = oldMetricProducerMap.find(metricId);
if (oldMetricProducerIt == oldMetricProducerMap.end()) {
updateStatus = UPDATE_NEW;
return true;
@@ -436,44 +441,85 @@
// This is an existing metric, check if it has changed.
uint64_t metricHash;
- if (!getMetricProtoHash(config, metric, id, metricToActivationMap, metricHash)) {
+ if (!getMetricProtoHash(config, metric, metricId, metricToActivationMap, metricHash)) {
return false;
}
const sp<MetricProducer> oldMetricProducer = oldMetricProducers[oldMetricProducerIt->second];
- if (oldMetricProducer->getMetricType() != METRIC_TYPE_EVENT ||
+ if (oldMetricProducer->getMetricType() != metricType ||
oldMetricProducer->getProtoHash() != metricHash) {
updateStatus = UPDATE_REPLACE;
return true;
}
- // Metric type and definition are the same. Need to check dependencies to see if they changed.
- if (replacedMatchers.find(metric.what()) != replacedMatchers.end()) {
+ // Take intersections of the matchers/predicates/states that the metric
+ // depends on with those that have been replaced. If a metric depends on any
+ // replaced component, it too must be replaced.
+ set<int64_t> intersection;
+ set_intersection(matcherDependencies.begin(), matcherDependencies.end(),
+ replacedMatchers.begin(), replacedMatchers.end(),
+ inserter(intersection, intersection.begin()));
+ if (intersection.size() > 0) {
+ updateStatus = UPDATE_REPLACE;
+ return true;
+ }
+ set_intersection(conditionDependencies.begin(), conditionDependencies.end(),
+ replacedConditions.begin(), replacedConditions.end(),
+ inserter(intersection, intersection.begin()));
+ if (intersection.size() > 0) {
+ updateStatus = UPDATE_REPLACE;
+ return true;
+ }
+ set_intersection(stateDependencies.begin(), stateDependencies.end(), replacedStates.begin(),
+ replacedStates.end(), inserter(intersection, intersection.begin()));
+ if (intersection.size() > 0) {
updateStatus = UPDATE_REPLACE;
return true;
}
- if (metric.has_condition()) {
- if (replacedConditions.find(metric.condition()) != replacedConditions.end()) {
- updateStatus = UPDATE_REPLACE;
- return true;
- }
- }
-
- if (metricActivationDepsChange(config, metricToActivationMap, id, replacedMatchers)) {
- updateStatus = UPDATE_REPLACE;
- return true;
- }
-
- for (const auto& metricConditionLink : metric.links()) {
+ for (const auto& metricConditionLink : conditionLinks) {
if (replacedConditions.find(metricConditionLink.condition()) != replacedConditions.end()) {
updateStatus = UPDATE_REPLACE;
return true;
}
}
+
+ if (metricActivationDepsChange(config, metricToActivationMap, metricId, replacedMatchers)) {
+ updateStatus = UPDATE_REPLACE;
+ return true;
+ }
+
updateStatus = UPDATE_PRESERVE;
return true;
}
+bool determineAllMetricUpdateStatuses(const StatsdConfig& config,
+ const unordered_map<int64_t, int>& oldMetricProducerMap,
+ const vector<sp<MetricProducer>>& oldMetricProducers,
+ const unordered_map<int64_t, int>& metricToActivationMap,
+ const set<int64_t>& replacedMatchers,
+ const set<int64_t>& replacedConditions,
+ const set<int64_t>& replacedStates,
+ vector<UpdateStatus>& metricsToUpdate) {
+ int metricIndex = 0;
+ for (int i = 0; i < config.event_metric_size(); i++, metricIndex++) {
+ const EventMetric& metric = config.event_metric(i);
+ set<int64_t> conditionDependencies;
+ if (metric.has_condition()) {
+ conditionDependencies.insert(metric.condition());
+ }
+ if (!determineMetricUpdateStatus(
+ config, metric, metric.id(), METRIC_TYPE_EVENT, {metric.what()},
+ conditionDependencies, ::google::protobuf::RepeatedField<int64_t>(),
+ metric.links(), oldMetricProducerMap, oldMetricProducers, metricToActivationMap,
+ replacedMatchers, replacedConditions, replacedStates,
+ metricsToUpdate[metricIndex])) {
+ return false;
+ }
+ }
+ // TODO: determine update status for count, gauge, value, duration metrics.
+ return true;
+}
+
bool updateMetrics(const ConfigKey& key, const StatsdConfig& config, const int64_t timeBaseNs,
const int64_t currentTimeNs, const sp<StatsPullerManager>& pullerManager,
const unordered_map<int64_t, int>& oldAtomMatchingTrackerMap,
@@ -518,22 +564,16 @@
}
vector<UpdateStatus> metricsToUpdate(allMetricsCount, UPDATE_UNKNOWN);
+ if (!determineAllMetricUpdateStatuses(config, oldMetricProducerMap, oldMetricProducers,
+ metricToActivationMap, replacedMatchers,
+ replacedConditions, replacedStates, metricsToUpdate)) {
+ return false;
+ }
+
+ // Now, perform the update. Must iterate the metric types in the same order
int metricIndex = 0;
for (int i = 0; i < config.event_metric_size(); i++, metricIndex++) {
newMetricProducerMap[config.event_metric(i).id()] = metricIndex;
- if (!determineEventMetricUpdateStatus(config, config.event_metric(i), oldMetricProducerMap,
- oldMetricProducers, metricToActivationMap,
- replacedMatchers, replacedConditions,
- metricsToUpdate[metricIndex])) {
- return false;
- }
- }
-
- // TODO: determine update status for count, gauge, value, duration metrics.
-
- // Now, perform the update. Must iterate the metric types in the same order
- metricIndex = 0;
- for (int i = 0; i < config.event_metric_size(); i++, metricIndex++) {
const EventMetric& metric = config.event_metric(i);
switch (metricsToUpdate[metricIndex]) {
case UPDATE_PRESERVE: {
diff --git a/bin/src/metrics/parsing_utils/config_update_utils.h b/bin/src/metrics/parsing_utils/config_update_utils.h
index 1cd0ce5..34d7e9c 100644
--- a/bin/src/metrics/parsing_utils/config_update_utils.h
+++ b/bin/src/metrics/parsing_utils/config_update_utils.h
@@ -125,23 +125,25 @@
std::vector<ConditionState>& conditionCache,
std::set<int64_t>& replacedConditions);
-// Function to determine if an event metric needs to be updated. Populates updateStatus.
+// Function to determine the update status (preserve/replace/new) of all metrics in the config.
// [config]: the input StatsdConfig
-// [metric]: the current metric to be updated
// [oldMetricProducerMap]: metric id to index mapping in the existing MetricsManager
// [oldMetricProducers]: stores the existing MetricProducers
-// [metricToActivationMap]: map from metric id to metric activation index.
-// [replacedMatchers]: set of replaced matcher ids. conditions using these matchers must be replaced
+// [metricToActivationMap]: map from metric id to metric activation index
+// [replacedMatchers]: set of replaced matcher ids. metrics using these matchers must be replaced
+// [replacedConditions]: set of replaced conditions. metrics using these conditions must be replaced
+// [replacedStates]: set of replaced state ids. metrics using these states must be replaced
// output:
-// [updateStatus]: update status for the metric. Will be changed from UPDATE_UNKNOWN after this call
+// [metricsToUpdate]: update status of each metric. Will be changed from UPDATE_UNKNOWN
// Returns whether the function was successful or not.
-bool determineEventMetricUpdateStatus(const StatsdConfig& config, const EventMetric& metric,
+bool determineAllMetricUpdateStatuses(const StatsdConfig& config,
const unordered_map<int64_t, int>& oldMetricProducerMap,
const vector<sp<MetricProducer>>& oldMetricProducers,
const unordered_map<int64_t, int>& metricToActivationMap,
const set<int64_t>& replacedMatchers,
const set<int64_t>& replacedConditions,
- UpdateStatus& updateStatus);
+ const set<int64_t>& replacedStates,
+ vector<UpdateStatus>& metricsToUpdate);
// Update MetricProducers.
// input:
diff --git a/bin/tests/metrics/parsing_utils/config_update_utils_test.cpp b/bin/tests/metrics/parsing_utils/config_update_utils_test.cpp
index 5d0f6c4..b2b0879 100644
--- a/bin/tests/metrics/parsing_utils/config_update_utils_test.cpp
+++ b/bin/tests/metrics/parsing_utils/config_update_utils_test.cpp
@@ -920,14 +920,13 @@
// Create an initial config.
EXPECT_TRUE(initConfig(config));
- set<int64_t> replacedMatchers;
- set<int64_t> replacedConditions;
unordered_map<int64_t, int> metricToActivationMap;
- UpdateStatus status = UPDATE_UNKNOWN;
- EXPECT_TRUE(determineEventMetricUpdateStatus(config, *metric, oldMetricProducerMap,
- oldMetricProducers, metricToActivationMap,
- replacedMatchers, replacedConditions, status));
- EXPECT_EQ(status, UPDATE_PRESERVE);
+ vector<UpdateStatus> metricsToUpdate(1, UPDATE_UNKNOWN);
+ EXPECT_TRUE(determineAllMetricUpdateStatuses(config, oldMetricProducerMap, oldMetricProducers,
+ metricToActivationMap,
+ /*replacedMatchers*/ {}, /*replacedConditions=*/{},
+ /*replacedStates=*/{}, metricsToUpdate));
+ EXPECT_EQ(metricsToUpdate[0], UPDATE_PRESERVE);
}
TEST_F(ConfigUpdateTest, TestEventMetricActivationAdded) {
@@ -957,14 +956,13 @@
eventActivation->set_atom_matcher_id(startMatcher.id());
eventActivation->set_ttl_seconds(5);
- set<int64_t> replacedMatchers;
- set<int64_t> replacedConditions;
unordered_map<int64_t, int> metricToActivationMap = {{12345, 0}};
- UpdateStatus status = UPDATE_UNKNOWN;
- EXPECT_TRUE(determineEventMetricUpdateStatus(config, *metric, oldMetricProducerMap,
- oldMetricProducers, metricToActivationMap,
- replacedMatchers, replacedConditions, status));
- EXPECT_EQ(status, UPDATE_REPLACE);
+ vector<UpdateStatus> metricsToUpdate(1, UPDATE_UNKNOWN);
+ EXPECT_TRUE(determineAllMetricUpdateStatuses(config, oldMetricProducerMap, oldMetricProducers,
+ metricToActivationMap,
+ /*replacedMatchers*/ {}, /*replacedConditions=*/{},
+ /*replacedStates=*/{}, metricsToUpdate));
+ EXPECT_EQ(metricsToUpdate[0], UPDATE_REPLACE);
}
TEST_F(ConfigUpdateTest, TestEventMetricWhatChanged) {
@@ -987,14 +985,13 @@
// Create an initial config.
EXPECT_TRUE(initConfig(config));
- set<int64_t> replacedMatchers = {whatMatcher.id()};
- set<int64_t> replacedConditions;
unordered_map<int64_t, int> metricToActivationMap;
- UpdateStatus status = UPDATE_UNKNOWN;
- EXPECT_TRUE(determineEventMetricUpdateStatus(config, *metric, oldMetricProducerMap,
- oldMetricProducers, metricToActivationMap,
- replacedMatchers, replacedConditions, status));
- EXPECT_EQ(status, UPDATE_REPLACE);
+ vector<UpdateStatus> metricsToUpdate(1, UPDATE_UNKNOWN);
+ EXPECT_TRUE(determineAllMetricUpdateStatuses(
+ config, oldMetricProducerMap, oldMetricProducers, metricToActivationMap,
+ /*replacedMatchers*/ {whatMatcher.id()}, /*replacedConditions=*/{},
+ /*replacedStates=*/{}, metricsToUpdate));
+ EXPECT_EQ(metricsToUpdate[0], UPDATE_REPLACE);
}
TEST_F(ConfigUpdateTest, TestEventMetricConditionChanged) {
@@ -1017,14 +1014,13 @@
// Create an initial config.
EXPECT_TRUE(initConfig(config));
- set<int64_t> replacedMatchers;
- set<int64_t> replacedConditions = {predicate.id()};
unordered_map<int64_t, int> metricToActivationMap;
- UpdateStatus status = UPDATE_UNKNOWN;
- EXPECT_TRUE(determineEventMetricUpdateStatus(config, *metric, oldMetricProducerMap,
- oldMetricProducers, metricToActivationMap,
- replacedMatchers, replacedConditions, status));
- EXPECT_EQ(status, UPDATE_REPLACE);
+ vector<UpdateStatus> metricsToUpdate(1, UPDATE_UNKNOWN);
+ EXPECT_TRUE(determineAllMetricUpdateStatuses(
+ config, oldMetricProducerMap, oldMetricProducers, metricToActivationMap,
+ /*replacedMatchers*/ {}, /*replacedConditions=*/{predicate.id()},
+ /*replacedStates=*/{}, metricsToUpdate));
+ EXPECT_EQ(metricsToUpdate[0], UPDATE_REPLACE);
}
TEST_F(ConfigUpdateTest, TestMetricConditionLinkDepsChanged) {
@@ -1054,14 +1050,13 @@
// Create an initial config.
EXPECT_TRUE(initConfig(config));
- set<int64_t> replacedMatchers;
- set<int64_t> replacedConditions = {linkPredicate.id()};
unordered_map<int64_t, int> metricToActivationMap;
- UpdateStatus status = UPDATE_UNKNOWN;
- EXPECT_TRUE(determineEventMetricUpdateStatus(config, *metric, oldMetricProducerMap,
- oldMetricProducers, metricToActivationMap,
- replacedMatchers, replacedConditions, status));
- EXPECT_EQ(status, UPDATE_REPLACE);
+ vector<UpdateStatus> metricsToUpdate(1, UPDATE_UNKNOWN);
+ EXPECT_TRUE(determineAllMetricUpdateStatuses(
+ config, oldMetricProducerMap, oldMetricProducers, metricToActivationMap,
+ /*replacedMatchers*/ {}, /*replacedConditions=*/{linkPredicate.id()},
+ /*replacedStates=*/{}, metricsToUpdate));
+ EXPECT_EQ(metricsToUpdate[0], UPDATE_REPLACE);
}
TEST_F(ConfigUpdateTest, TestEventMetricActivationDepsChange) {
@@ -1090,14 +1085,13 @@
// Create an initial config.
EXPECT_TRUE(initConfig(config));
- set<int64_t> replacedMatchers = {startMatcher.id()}; // The activation matcher is replaced.
- set<int64_t> replacedConditions;
unordered_map<int64_t, int> metricToActivationMap = {{12345, 0}};
- UpdateStatus status = UPDATE_UNKNOWN;
- EXPECT_TRUE(determineEventMetricUpdateStatus(config, *metric, oldMetricProducerMap,
- oldMetricProducers, metricToActivationMap,
- replacedMatchers, replacedConditions, status));
- EXPECT_EQ(status, UPDATE_REPLACE);
+ vector<UpdateStatus> metricsToUpdate(1, UPDATE_UNKNOWN);
+ EXPECT_TRUE(determineAllMetricUpdateStatuses(
+ config, oldMetricProducerMap, oldMetricProducers, metricToActivationMap,
+ /*replacedMatchers*/ {startMatcher.id()}, /*replacedConditions=*/{},
+ /*replacedStates=*/{}, metricsToUpdate));
+ EXPECT_EQ(metricsToUpdate[0], UPDATE_REPLACE);
}
TEST_F(ConfigUpdateTest, TestUpdateEventMetrics) {
