From 090ffd79c33519d46c42f867e5ea9f6c9ca03fe2 Mon Sep 17 00:00:00 2001 From: Tej Singh Date: Mon, 17 Aug 2020 20:27:17 -0700 Subject: [PATCH] Update indices of preserved matchers This cl updates mIndex and mChildren of preserved atom matchers across config updates, since the index of the sp<> in mAllAtomMatchers can change during a config update. Test: atest statsd_test Bug: 164282429 Change-Id: I7eb252439876d7330b9bba81e046fe3e89df6e5d --- .../statsd/src/matchers/AtomMatchingTracker.h | 11 ++++++- .../CombinationAtomMatchingTracker.cpp | 17 +++++++++++ .../matchers/CombinationAtomMatchingTracker.h | 7 ++++- .../matchers/SimpleAtomMatchingTracker.cpp | 8 +++++ .../src/matchers/SimpleAtomMatchingTracker.h | 5 +++- .../parsing_utils/config_update_utils.cpp | 9 ++++-- .../config_update_utils_test.cpp | 30 ++++++++++++++++++- 7 files changed, 81 insertions(+), 6 deletions(-) diff --git a/cmds/statsd/src/matchers/AtomMatchingTracker.h b/cmds/statsd/src/matchers/AtomMatchingTracker.h index 5194f99707666..c1384972464cd 100644 --- a/cmds/statsd/src/matchers/AtomMatchingTracker.h +++ b/cmds/statsd/src/matchers/AtomMatchingTracker.h @@ -52,6 +52,15 @@ public: const std::unordered_map& matcherMap, std::vector& stack) = 0; + // Update appropriate state on config updates. Primarily, all indices need to be updated. + // This matcher and all of its children are guaranteed to be preserved across the update. + // matcher: the AtomMatcher proto from the config. + // index: the index of this matcher in mAllAtomMatchingTrackers. + // atomMatchingTrackerMap: map from matcher id to index in mAllAtomMatchingTrackers + virtual bool onConfigUpdated( + const AtomMatcher& matcher, const int index, + const std::unordered_map& atomMatchingTrackerMap) = 0; + // Called when a log event comes. // event: the log event. // allAtomMatchingTrackers: the list of all AtomMatchingTrackers. This is needed because the log @@ -83,7 +92,7 @@ protected: const int64_t mId; // Index of this AtomMatchingTracker in MetricsManager's container. - const int mIndex; + int mIndex; // Whether this AtomMatchingTracker has been properly initialized. bool mInitialized; diff --git a/cmds/statsd/src/matchers/CombinationAtomMatchingTracker.cpp b/cmds/statsd/src/matchers/CombinationAtomMatchingTracker.cpp index 19637a449c44f..45685ce5bfee2 100644 --- a/cmds/statsd/src/matchers/CombinationAtomMatchingTracker.cpp +++ b/cmds/statsd/src/matchers/CombinationAtomMatchingTracker.cpp @@ -93,6 +93,23 @@ bool CombinationAtomMatchingTracker::init( return true; } +bool CombinationAtomMatchingTracker::onConfigUpdated( + const AtomMatcher& matcher, const int index, + const unordered_map& atomMatchingTrackerMap) { + mIndex = index; + mChildren.clear(); + AtomMatcher_Combination combinationMatcher = matcher.combination(); + for (const int64_t child : combinationMatcher.matcher()) { + const auto& pair = atomMatchingTrackerMap.find(child); + if (pair == atomMatchingTrackerMap.end()) { + ALOGW("Matcher %lld not found in the config", (long long)child); + return false; + } + mChildren.push_back(pair->second); + } + return true; +} + void CombinationAtomMatchingTracker::onLogEvent( const LogEvent& event, const vector>& allAtomMatchingTrackers, vector& matcherResults) { diff --git a/cmds/statsd/src/matchers/CombinationAtomMatchingTracker.h b/cmds/statsd/src/matchers/CombinationAtomMatchingTracker.h index 06a4932804f56..3160448b6c76b 100644 --- a/cmds/statsd/src/matchers/CombinationAtomMatchingTracker.h +++ b/cmds/statsd/src/matchers/CombinationAtomMatchingTracker.h @@ -27,7 +27,7 @@ namespace os { namespace statsd { // Represents a AtomMatcher_Combination in the StatsdConfig. -class CombinationAtomMatchingTracker : public virtual AtomMatchingTracker { +class CombinationAtomMatchingTracker : public AtomMatchingTracker { public: CombinationAtomMatchingTracker(const int64_t& id, const int index, const uint64_t protoHash); @@ -35,6 +35,9 @@ public: const std::vector>& allAtomMatchingTrackers, const std::unordered_map& matcherMap, std::vector& stack); + bool onConfigUpdated(const AtomMatcher& matcher, const int index, + const std::unordered_map& atomMatchingTrackerMap) override; + ~CombinationAtomMatchingTracker(); void onLogEvent(const LogEvent& event, @@ -45,6 +48,8 @@ private: LogicalOperation mLogicalOperation; std::vector mChildren; + + FRIEND_TEST(ConfigUpdateTest, TestUpdateMatchers); }; } // namespace statsd diff --git a/cmds/statsd/src/matchers/SimpleAtomMatchingTracker.cpp b/cmds/statsd/src/matchers/SimpleAtomMatchingTracker.cpp index 86b148dd86570..423da5bd3cf8a 100644 --- a/cmds/statsd/src/matchers/SimpleAtomMatchingTracker.cpp +++ b/cmds/statsd/src/matchers/SimpleAtomMatchingTracker.cpp @@ -50,6 +50,14 @@ bool SimpleAtomMatchingTracker::init(const vector& allAtomMatchers, return mInitialized; } +bool SimpleAtomMatchingTracker::onConfigUpdated( + const AtomMatcher& matcher, const int index, + const unordered_map& atomMatchingTrackerMap) { + mIndex = index; + // Do not need to update mMatcher since the matcher must be identical across the update. + return mInitialized; +} + void SimpleAtomMatchingTracker::onLogEvent( const LogEvent& event, const vector>& allAtomMatchingTrackers, vector& matcherResults) { diff --git a/cmds/statsd/src/matchers/SimpleAtomMatchingTracker.h b/cmds/statsd/src/matchers/SimpleAtomMatchingTracker.h index 49cbe09f8482d..b67e6c20e8f11 100644 --- a/cmds/statsd/src/matchers/SimpleAtomMatchingTracker.h +++ b/cmds/statsd/src/matchers/SimpleAtomMatchingTracker.h @@ -28,7 +28,7 @@ namespace android { namespace os { namespace statsd { -class SimpleAtomMatchingTracker : public virtual AtomMatchingTracker { +class SimpleAtomMatchingTracker : public AtomMatchingTracker { public: SimpleAtomMatchingTracker(const int64_t& id, const int index, const uint64_t protoHash, const SimpleAtomMatcher& matcher, const sp& uidMap); @@ -40,6 +40,9 @@ public: const std::unordered_map& matcherMap, std::vector& stack) override; + bool onConfigUpdated(const AtomMatcher& matcher, const int index, + const std::unordered_map& atomMatchingTrackerMap) override; + void onLogEvent(const LogEvent& event, const std::vector>& allAtomMatchingTrackers, std::vector& matcherResults) override; 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 a9ae5a4e854d6..0983dc0b2c832 100644 --- a/cmds/statsd/src/metrics/parsing_utils/config_update_utils.cpp +++ b/cmds/statsd/src/metrics/parsing_utils/config_update_utils.cpp @@ -148,8 +148,13 @@ bool updateAtomTrackers(const StatsdConfig& config, const sp& uidMap, (long long)id); return false; } - const int oldIndex = oldAtomMatchingTrackerIt->second; - newAtomMatchingTrackers.push_back(oldAtomMatchingTrackers[oldIndex]); + const sp& tracker = + oldAtomMatchingTrackers[oldAtomMatchingTrackerIt->second]; + if (!tracker->onConfigUpdated(matcherProtos[i], i, newAtomMatchingTrackerMap)) { + ALOGW("Config update failed for matcher %lld", (long long)id); + return false; + } + newAtomMatchingTrackers.push_back(tracker); break; } case UPDATE_REPLACE: { 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 f6d30618ee159..8c698eb15d8d3 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 @@ -23,6 +23,7 @@ #include #include "frameworks/base/cmds/statsd/src/statsd_config.pb.h" +#include "src/matchers/CombinationAtomMatchingTracker.h" #include "src/metrics/parsing_utils/metrics_manager_util.h" #include "tests/statsd_test_util.h" @@ -370,13 +371,40 @@ TEST_F(ConfigUpdateTest, TestUpdateMatchers) { EXPECT_NE(oldAtomMatchingTrackers[oldAtomMatchingTrackerMap.at(combination2Id)], newAtomMatchingTrackers[newAtomMatchingTrackerMap.at(combination2Id)]); - // Validation, make sure the matchers have the proper ids. Could do more checks here. + // Validation, make sure the matchers have the proper ids/indices. Could do more checks here. EXPECT_EQ(newAtomMatchingTrackers[0]->getId(), combination3Id); + EXPECT_EQ(newAtomMatchingTrackers[0]->mIndex, 0); EXPECT_EQ(newAtomMatchingTrackers[1]->getId(), simple2Id); + EXPECT_EQ(newAtomMatchingTrackers[1]->mIndex, 1); EXPECT_EQ(newAtomMatchingTrackers[2]->getId(), combination2Id); + EXPECT_EQ(newAtomMatchingTrackers[2]->mIndex, 2); EXPECT_EQ(newAtomMatchingTrackers[3]->getId(), simple1Id); + EXPECT_EQ(newAtomMatchingTrackers[3]->mIndex, 3); EXPECT_EQ(newAtomMatchingTrackers[4]->getId(), simple4Id); + EXPECT_EQ(newAtomMatchingTrackers[4]->mIndex, 4); EXPECT_EQ(newAtomMatchingTrackers[5]->getId(), combination1Id); + EXPECT_EQ(newAtomMatchingTrackers[5]->mIndex, 5); + + // Verify child indices of Combination Matchers are correct. + CombinationAtomMatchingTracker* combinationTracker1 = + static_cast(newAtomMatchingTrackers[5].get()); + vector* childMatchers = &combinationTracker1->mChildren; + EXPECT_EQ(childMatchers->size(), 1); + EXPECT_NE(std::find(childMatchers->begin(), childMatchers->end(), 3), childMatchers->end()); + + CombinationAtomMatchingTracker* combinationTracker2 = + static_cast(newAtomMatchingTrackers[2].get()); + childMatchers = &combinationTracker2->mChildren; + EXPECT_EQ(childMatchers->size(), 2); + EXPECT_NE(std::find(childMatchers->begin(), childMatchers->end(), 1), childMatchers->end()); + EXPECT_NE(std::find(childMatchers->begin(), childMatchers->end(), 3), childMatchers->end()); + + CombinationAtomMatchingTracker* combinationTracker3 = + static_cast(newAtomMatchingTrackers[0].get()); + childMatchers = &combinationTracker3->mChildren; + EXPECT_EQ(childMatchers->size(), 2); + EXPECT_NE(std::find(childMatchers->begin(), childMatchers->end(), 1), childMatchers->end()); + EXPECT_NE(std::find(childMatchers->begin(), childMatchers->end(), 4), childMatchers->end()); } } // namespace statsd