From 8b46bc11b43926989d129cf17dbad3648ee79045 Mon Sep 17 00:00:00 2001 From: Tej Singh Date: Thu, 29 Oct 2020 22:54:41 -0700 Subject: [PATCH] Update Alarms/Subscriptions This is the same as initializing because alarms have no state to carry over. We can just replace them all without losing anything. Test: atest statsd_test Bug: 162323547 Change-Id: Ia2fb33e9ac79476babce57e2e0fb1ca49091e260 --- cmds/statsd/src/anomaly/AlarmTracker.h | 1 + cmds/statsd/src/metrics/MetricsManager.cpp | 9 +- .../parsing_utils/config_update_utils.cpp | 7 ++ .../parsing_utils/config_update_utils.h | 2 + .../parsing_utils/metrics_manager_util.cpp | 21 +--- .../parsing_utils/metrics_manager_util.h | 6 ++ .../config_update_utils_test.cpp | 101 +++++++++++++++++- 7 files changed, 124 insertions(+), 23 deletions(-) diff --git a/cmds/statsd/src/anomaly/AlarmTracker.h b/cmds/statsd/src/anomaly/AlarmTracker.h index 2da4a18682ae8..406086da557bf 100644 --- a/cmds/statsd/src/anomaly/AlarmTracker.h +++ b/cmds/statsd/src/anomaly/AlarmTracker.h @@ -73,6 +73,7 @@ protected: FRIEND_TEST(AlarmTrackerTest, TestTriggerTimestamp); FRIEND_TEST(AlarmE2eTest, TestMultipleAlarms); + FRIEND_TEST(ConfigUpdateTest, TestUpdateAlarms); }; } // namespace statsd diff --git a/cmds/statsd/src/metrics/MetricsManager.cpp b/cmds/statsd/src/metrics/MetricsManager.cpp index b1d4397037097..d80f9dbb42562 100644 --- a/cmds/statsd/src/metrics/MetricsManager.cpp +++ b/cmds/statsd/src/metrics/MetricsManager.cpp @@ -211,6 +211,7 @@ bool MetricsManager::updateConfig(const StatsdConfig& config, const int64_t time unordered_map newMetricProducerMap; vector> newAnomalyTrackers; unordered_map newAlertTrackerMap; + vector> newPeriodicAlarmTrackers; mTagIds.clear(); mConditionToMetricMap.clear(); mTrackerToMetricMap.clear(); @@ -226,9 +227,10 @@ bool MetricsManager::updateConfig(const StatsdConfig& config, const int64_t time mAllAnomalyTrackers, mAlertTrackerMap, mStateProtoHashes, mTagIds, newAtomMatchingTrackers, newAtomMatchingTrackerMap, newConditionTrackers, newConditionTrackerMap, newMetricProducers, newMetricProducerMap, newAnomalyTrackers, - newAlertTrackerMap, mConditionToMetricMap, mTrackerToMetricMap, mTrackerToConditionMap, - mActivationAtomTrackerToMetricMap, mDeactivationAtomTrackerToMetricMap, - mMetricIndexesWithActivation, newStateProtoHashes, mNoReportMetricIds); + newAlertTrackerMap, newPeriodicAlarmTrackers, mConditionToMetricMap, + mTrackerToMetricMap, mTrackerToConditionMap, mActivationAtomTrackerToMetricMap, + mDeactivationAtomTrackerToMetricMap, mMetricIndexesWithActivation, newStateProtoHashes, + mNoReportMetricIds); mAllAtomMatchingTrackers = newAtomMatchingTrackers; mAtomMatchingTrackerMap = newAtomMatchingTrackerMap; mAllConditionTrackers = newConditionTrackers; @@ -238,6 +240,7 @@ bool MetricsManager::updateConfig(const StatsdConfig& config, const int64_t time mStateProtoHashes = newStateProtoHashes; mAllAnomalyTrackers = newAnomalyTrackers; mAlertTrackerMap = newAlertTrackerMap; + mAllPeriodicAlarmTrackers = newPeriodicAlarmTrackers; return mConfigValid; } diff --git a/cmds/statsd/src/metrics/parsing_utils/config_update_utils.cpp b/cmds/statsd/src/metrics/parsing_utils/config_update_utils.cpp index 0c4dc4476f480..637236145bf5a 100644 --- a/cmds/statsd/src/metrics/parsing_utils/config_update_utils.cpp +++ b/cmds/statsd/src/metrics/parsing_utils/config_update_utils.cpp @@ -1017,6 +1017,7 @@ bool updateStatsdConfig(const ConfigKey& key, const StatsdConfig& config, const unordered_map& newMetricProducerMap, vector>& newAnomalyTrackers, unordered_map& newAlertTrackerMap, + vector>& newPeriodicAlarmTrackers, unordered_map>& conditionToMetricMap, unordered_map>& trackerToMetricMap, unordered_map>& trackerToConditionMap, @@ -1081,6 +1082,12 @@ bool updateStatsdConfig(const ConfigKey& key, const StatsdConfig& config, const return false; } + // Alarms do not have any state, so we can reuse the initialization logic. + if (!initAlarms(config, key, periodicAlarmMonitor, timeBaseNs, currentTimeNs, + newPeriodicAlarmTrackers)) { + ALOGE("initAlarms failed"); + return false; + } return true; } diff --git a/cmds/statsd/src/metrics/parsing_utils/config_update_utils.h b/cmds/statsd/src/metrics/parsing_utils/config_update_utils.h index b714f58f6c067..178a9d220b4dd 100644 --- a/cmds/statsd/src/metrics/parsing_utils/config_update_utils.h +++ b/cmds/statsd/src/metrics/parsing_utils/config_update_utils.h @@ -19,6 +19,7 @@ #include #include "anomaly/AlarmMonitor.h" +#include "anomaly/AlarmTracker.h" #include "condition/ConditionTracker.h" #include "external/StatsPullerManager.h" #include "matchers/AtomMatchingTracker.h" @@ -253,6 +254,7 @@ bool updateStatsdConfig(const ConfigKey& key, const StatsdConfig& config, const std::unordered_map& newMetricProducerMap, std::vector>& newAlertTrackers, std::unordered_map& newAlertTrackerMap, + std::vector>& newPeriodicAlarmTrackers, std::unordered_map>& conditionToMetricMap, std::unordered_map>& trackerToMetricMap, std::unordered_map>& trackerToConditionMap, diff --git a/cmds/statsd/src/metrics/parsing_utils/metrics_manager_util.cpp b/cmds/statsd/src/metrics/parsing_utils/metrics_manager_util.cpp index f15c8b0f33462..4474df4346cf3 100644 --- a/cmds/statsd/src/metrics/parsing_utils/metrics_manager_util.cpp +++ b/cmds/statsd/src/metrics/parsing_utils/metrics_manager_util.cpp @@ -1143,24 +1143,9 @@ bool initAlarms(const StatsdConfig& config, const ConfigKey& key, allAlarmTrackers.push_back( new AlarmTracker(startMillis, currentTimeMillis, alarm, key, periodicAlarmMonitor)); } - for (int i = 0; i < config.subscription_size(); ++i) { - const Subscription& subscription = config.subscription(i); - if (subscription.rule_type() != Subscription::ALARM) { - continue; - } - if (subscription.subscriber_information_case() == - Subscription::SubscriberInformationCase::SUBSCRIBER_INFORMATION_NOT_SET) { - ALOGW("subscription \"%lld\" has no subscriber info.\"", (long long)subscription.id()); - return false; - } - const auto& itr = alarmTrackerMap.find(subscription.rule_id()); - if (itr == alarmTrackerMap.end()) { - ALOGW("subscription \"%lld\" has unknown rule id: \"%lld\"", - (long long)subscription.id(), (long long)subscription.rule_id()); - return false; - } - const int trackerIndex = itr->second; - allAlarmTrackers[trackerIndex]->addSubscription(subscription); + if (!initSubscribersForSubscriptionType(config, Subscription::ALARM, alarmTrackerMap, + allAlarmTrackers)) { + return false; } return true; } diff --git a/cmds/statsd/src/metrics/parsing_utils/metrics_manager_util.h b/cmds/statsd/src/metrics/parsing_utils/metrics_manager_util.h index 781a4ef16149a..84e1e4e043398 100644 --- a/cmds/statsd/src/metrics/parsing_utils/metrics_manager_util.h +++ b/cmds/statsd/src/metrics/parsing_utils/metrics_manager_util.h @@ -306,6 +306,12 @@ bool initMetrics( std::unordered_map>& deactivationAtomTrackerToMetricMap, std::vector& metricsWithActivation); +// Initialize alarms +// Is called both on initialize new configs and config updates since alarms do not have any state. +bool initAlarms(const StatsdConfig& config, const ConfigKey& key, + const sp& periodicAlarmMonitor, const int64_t timeBaseNs, + const int64_t currentTimeNs, std::vector>& allAlarmTrackers); + // Initialize MetricsManager from StatsdConfig. // Parameters are the members of MetricsManager. See MetricsManager for declaration. bool initStatsdConfig(const ConfigKey& key, const StatsdConfig& config, const sp& uidMap, diff --git a/cmds/statsd/tests/metrics/parsing_utils/config_update_utils_test.cpp b/cmds/statsd/tests/metrics/parsing_utils/config_update_utils_test.cpp index 8a1b74ba0ff0b..66bab4ea70daf 100644 --- a/cmds/statsd/tests/metrics/parsing_utils/config_update_utils_test.cpp +++ b/cmds/statsd/tests/metrics/parsing_utils/config_update_utils_test.cpp @@ -52,11 +52,15 @@ namespace statsd { namespace { ConfigKey key(123, 456); -const int64_t timeBaseNs = 1000; +const int64_t timeBaseNs = 1000 * NS_PER_SEC; + sp uidMap = new UidMap(); sp pullerManager = new StatsPullerManager(); sp anomalyAlarmMonitor; -sp periodicAlarmMonitor; +sp periodicAlarmMonitor = new AlarmMonitor( + /*minDiffToUpdateRegisteredAlarmTimeSec=*/0, + [](const shared_ptr&, int64_t) {}, + [](const shared_ptr&) {}); set allTagIds; vector> oldAtomMatchingTrackers; unordered_map oldAtomMatchingTrackerMap; @@ -206,6 +210,14 @@ Subscription createSubscription(string name, Subscription_RuleType type, int64_t subscription.mutable_broadcast_subscriber_details(); return subscription; } + +Alarm createAlarm(string name, int64_t offsetMillis, int64_t periodMillis) { + Alarm alarm; + alarm.set_id(StringToId(name)); + alarm.set_offset_millis(offsetMillis); + alarm.set_period_millis(periodMillis); + return alarm; +} } // anonymous namespace TEST_F(ConfigUpdateTest, TestSimpleMatcherPreserve) { @@ -3475,6 +3487,91 @@ TEST_F(ConfigUpdateTest, TestUpdateAlerts) { EXPECT_THAT(newAnomalyTrackers[alert4Index]->mSubscriptions, IsEmpty()); } +TEST_F(ConfigUpdateTest, TestUpdateAlarms) { + StatsdConfig config; + // Add alarms. + Alarm alarm1 = createAlarm("Alarm1", /*offset*/ 1 * MS_PER_SEC, /*period*/ 50 * MS_PER_SEC); + int64_t alarm1Id = alarm1.id(); + *config.add_alarm() = alarm1; + + Alarm alarm2 = createAlarm("Alarm2", /*offset*/ 1 * MS_PER_SEC, /*period*/ 2000 * MS_PER_SEC); + int64_t alarm2Id = alarm2.id(); + *config.add_alarm() = alarm2; + + Alarm alarm3 = createAlarm("Alarm3", /*offset*/ 10 * MS_PER_SEC, /*period*/ 5000 * MS_PER_SEC); + int64_t alarm3Id = alarm3.id(); + *config.add_alarm() = alarm3; + + // Add Subscriptions. + Subscription subscription1 = createSubscription("S1", Subscription::ALARM, alarm1Id); + *config.add_subscription() = subscription1; + Subscription subscription2 = createSubscription("S2", Subscription::ALARM, alarm1Id); + *config.add_subscription() = subscription2; + Subscription subscription3 = createSubscription("S3", Subscription::ALARM, alarm2Id); + *config.add_subscription() = subscription3; + + EXPECT_TRUE(initConfig(config)); + + ASSERT_EQ(oldAlarmTrackers.size(), 3); + // Config is created at statsd start time, so just add the offsets. + EXPECT_EQ(oldAlarmTrackers[0]->getAlarmTimestampSec(), timeBaseNs / NS_PER_SEC + 1); + EXPECT_EQ(oldAlarmTrackers[1]->getAlarmTimestampSec(), timeBaseNs / NS_PER_SEC + 1); + EXPECT_EQ(oldAlarmTrackers[2]->getAlarmTimestampSec(), timeBaseNs / NS_PER_SEC + 10); + + // Change alarm2/alarm3. + config.mutable_alarm(1)->set_offset_millis(5 * MS_PER_SEC); + config.mutable_alarm(2)->set_period_millis(10000 * MS_PER_SEC); + + // Move subscription2 to be on alarm2 and make a new subscription. + config.mutable_subscription(1)->set_rule_id(alarm2Id); + Subscription subscription4 = createSubscription("S4", Subscription::ALARM, alarm1Id); + *config.add_subscription() = subscription4; + + // Update time is 2 seconds after the base time. + int64_t currentTimeNs = timeBaseNs + 2 * NS_PER_SEC; + vector> newAlarmTrackers; + EXPECT_TRUE(initAlarms(config, key, periodicAlarmMonitor, timeBaseNs, currentTimeNs, + newAlarmTrackers)); + + ASSERT_EQ(newAlarmTrackers.size(), 3); + // Config is updated 2 seconds after statsd start + // The offset has passed for alarm1, but not for alarms 2/3. + EXPECT_EQ(newAlarmTrackers[0]->getAlarmTimestampSec(), timeBaseNs / NS_PER_SEC + 1 + 50); + EXPECT_EQ(newAlarmTrackers[1]->getAlarmTimestampSec(), timeBaseNs / NS_PER_SEC + 5); + EXPECT_EQ(newAlarmTrackers[2]->getAlarmTimestampSec(), timeBaseNs / NS_PER_SEC + 10); + + // Verify alarms have the correct subscriptions. Use subscription id as proxy for equivalency. + vector alarm1Subscriptions; + for (const Subscription& subscription : newAlarmTrackers[0]->mSubscriptions) { + alarm1Subscriptions.push_back(subscription.id()); + } + EXPECT_THAT(alarm1Subscriptions, UnorderedElementsAre(subscription1.id(), subscription4.id())); + vector alarm2Subscriptions; + for (const Subscription& subscription : newAlarmTrackers[1]->mSubscriptions) { + alarm2Subscriptions.push_back(subscription.id()); + } + EXPECT_THAT(alarm2Subscriptions, UnorderedElementsAre(subscription2.id(), subscription3.id())); + EXPECT_THAT(newAlarmTrackers[2]->mSubscriptions, IsEmpty()); + + // Verify the alarm monitor is updated accordingly once the old alarms are removed. + // Alarm2 fires the earliest. + oldAlarmTrackers.clear(); + EXPECT_EQ(periodicAlarmMonitor->getRegisteredAlarmTimeSec(), timeBaseNs / NS_PER_SEC + 5); + + // Do another update 60 seconds after config creation time, after the offsets of each alarm. + currentTimeNs = timeBaseNs + 60 * NS_PER_SEC; + newAlarmTrackers.clear(); + EXPECT_TRUE(initAlarms(config, key, periodicAlarmMonitor, timeBaseNs, currentTimeNs, + newAlarmTrackers)); + + ASSERT_EQ(newAlarmTrackers.size(), 3); + // Config is updated one minute after statsd start. + // Two periods have passed for alarm 1, one has passed for alarms2/3. + EXPECT_EQ(newAlarmTrackers[0]->getAlarmTimestampSec(), timeBaseNs / NS_PER_SEC + 1 + 2 * 50); + EXPECT_EQ(newAlarmTrackers[1]->getAlarmTimestampSec(), timeBaseNs / NS_PER_SEC + 5 + 2000); + EXPECT_EQ(newAlarmTrackers[2]->getAlarmTimestampSec(), timeBaseNs / NS_PER_SEC + 10 + 10000); +} + } // namespace statsd } // namespace os } // namespace android