From d9269e2ee70b01cad3dda14c786e0e4434192248 Mon Sep 17 00:00:00 2001 From: David Chen Date: Tue, 5 Dec 2017 13:43:51 -0800 Subject: [PATCH] Adds rate limit to checking byte size. Since there is a separate guardrail for memory used by uid map, we no longer add the memory from uid map with the memory per each config's metrics. We also prevent the byte size check from happening too frequently. In order to mock the MetricsManager, we refactor some of the existing methods. Test: Added unit-tests and verified they all pass on marlin. Change-Id: I15cf105f7d95f4016fdb0443b0a33eebe862cafb --- cmds/statsd/Android.mk | 1 + cmds/statsd/src/StatsLogProcessor.cpp | 37 +++--- cmds/statsd/src/StatsLogProcessor.h | 20 ++-- cmds/statsd/src/guardrail/StatsdStats.h | 11 ++ cmds/statsd/src/metrics/MetricsManager.h | 6 +- cmds/statsd/tests/StatsLogProcessor_test.cpp | 117 +++++++++++++++++++ 6 files changed, 162 insertions(+), 30 deletions(-) create mode 100644 cmds/statsd/tests/StatsLogProcessor_test.cpp diff --git a/cmds/statsd/Android.mk b/cmds/statsd/Android.mk index 87cde032ba795..337aeaaf37abd 100644 --- a/cmds/statsd/Android.mk +++ b/cmds/statsd/Android.mk @@ -161,6 +161,7 @@ LOCAL_SRC_FILES := \ tests/LogEntryMatcher_test.cpp \ tests/LogReader_test.cpp \ tests/MetricsManager_test.cpp \ + tests/StatsLogProcessor_test.cpp \ tests/UidMap_test.cpp \ tests/condition/CombinationConditionTracker_test.cpp \ tests/condition/SimpleConditionTracker_test.cpp \ diff --git a/cmds/statsd/src/StatsLogProcessor.cpp b/cmds/statsd/src/StatsLogProcessor.cpp index c29164781e479..2df0c90e9baf8 100644 --- a/cmds/statsd/src/StatsLogProcessor.cpp +++ b/cmds/statsd/src/StatsLogProcessor.cpp @@ -86,9 +86,7 @@ void StatsLogProcessor::OnLogEvent(const LogEvent& msg) { // pass the event to metrics managers. for (auto& pair : mMetricsManagers) { pair.second->onLogEvent(msg); - // TODO: THIS CHECK FAILS BECAUSE ONCE UIDMAP SIZE EXCEEDS LIMIT, DROPPING METRICS DATA - // DOESN'T HELP. FIX THIS. - // flushIfNecessary(msg.GetTimestampNs(), pair.first, pair.second); + flushIfNecessary(msg.GetTimestampNs(), pair.first, *(pair.second)); } // Hard-coded logic to update the isolated uid's in the uid-map. @@ -217,23 +215,32 @@ void StatsLogProcessor::OnConfigRemoved(const ConfigKey& key) { mLastBroadcastTimes.erase(key); } -void StatsLogProcessor::flushIfNecessary(uint64_t timestampNs, - const ConfigKey& key, - const unique_ptr& metricsManager) { +void StatsLogProcessor::flushIfNecessary(uint64_t timestampNs, const ConfigKey& key, + MetricsManager& metricsManager) { std::lock_guard lock(mBroadcastTimesMutex); - size_t totalBytes = metricsManager->byteSize() + mUidMap->getBytesUsed(); - // TODO: Find a way to test that the dropping and broadcasts are sent when memory is exceeded. - if (totalBytes > kMaxSerializedBytes) { // Too late. We need to start clearing data. + auto lastCheckTime = mLastByteSizeTimes.find(key); + if (lastCheckTime != mLastByteSizeTimes.end()) { + if (timestampNs - lastCheckTime->second < StatsdStats::kMinByteSizeCheckPeriodNs) { + return; + } + } + + // We suspect that the byteSize() computation is expensive, so we set a rate limit. + size_t totalBytes = metricsManager.byteSize(); + mLastByteSizeTimes[key] = timestampNs; + if (totalBytes > + StatsdStats::kMaxMetricsBytesPerConfig) { // Too late. We need to start clearing data. // We ignore the return value so we force each metric producer to clear its contents. - metricsManager->onDumpReport(); + metricsManager.onDumpReport(); StatsdStats::getInstance().noteDataDropped(key); VLOG("StatsD had to toss out metrics for %s", key.ToString().c_str()); - } else if (totalBytes > - .9 * kMaxSerializedBytes) { // Send broadcast so that receivers can pull data. - auto lastFlushNs = mLastBroadcastTimes.find(key); - if (lastFlushNs != mLastBroadcastTimes.end()) { - if (timestampNs - lastFlushNs->second < kMinBroadcastPeriod) { + } else if (totalBytes > .9 * StatsdStats::kMaxMetricsBytesPerConfig) { + // Send broadcast so that receivers can pull data. + auto lastBroadcastTime = mLastBroadcastTimes.find(key); + if (lastBroadcastTime != mLastBroadcastTimes.end()) { + if (timestampNs - lastBroadcastTime->second < StatsdStats::kMinBroadcastPeriodNs) { + VLOG("StatsD would've sent a broadcast but the rate limit stopped us."); return; } } diff --git a/cmds/statsd/src/StatsLogProcessor.h b/cmds/statsd/src/StatsLogProcessor.h index e9ac01536b423..a4df23a16c0f9 100644 --- a/cmds/statsd/src/StatsLogProcessor.h +++ b/cmds/statsd/src/StatsLogProcessor.h @@ -60,29 +60,25 @@ private: std::unordered_map mLastBroadcastTimes; + // Tracks when we last checked the bytes consumed for each config key. + std::unordered_map mLastByteSizeTimes; + sp mUidMap; // Reference to the UidMap to lookup app name and version for each uid. sp mAnomalyMonitor; - /* Max *serialized* size of the logs kept in memory before flushing through binder call. - Proto lite does not implement the SpaceUsed() function which gives the in memory byte size. - So we cap memory usage by limiting the serialized size. Note that protobuf's in memory size - is higher than its serialized size. - */ - static const size_t kMaxSerializedBytes = 16 * 1024; - /* Check if we should send a broadcast if approaching memory limits and if we're over, we * actually delete the data. */ - void flushIfNecessary(uint64_t timestampNs, - const ConfigKey& key, - const unique_ptr& metricsManager); + void flushIfNecessary(uint64_t timestampNs, const ConfigKey& key, + MetricsManager& metricsManager); // Function used to send a broadcast so that receiver for the config key can call getData // to retrieve the stored data. std::function mSendBroadcast; - /* Minimum period between two broadcasts in nanoseconds. Currently set to 60 seconds. */ - static const unsigned long long kMinBroadcastPeriod = 60 * NS_PER_SEC; + FRIEND_TEST(StatsLogProcessorTest, TestRateLimitByteSize); + FRIEND_TEST(StatsLogProcessorTest, TestRateLimitBroadcast); + FRIEND_TEST(StatsLogProcessorTest, TestDropWhenByteSizeTooLarge); }; } // namespace statsd diff --git a/cmds/statsd/src/guardrail/StatsdStats.h b/cmds/statsd/src/guardrail/StatsdStats.h index d6f6566cbc33d..08a1a6623e83b 100644 --- a/cmds/statsd/src/guardrail/StatsdStats.h +++ b/cmds/statsd/src/guardrail/StatsdStats.h @@ -19,6 +19,7 @@ #include "frameworks/base/cmds/statsd/src/stats_log.pb.h" #include +#include #include #include #include @@ -45,10 +46,20 @@ public: const static int kMaxTimestampCount = 20; + // Max memory allowed for storing metrics per configuration. When this limit is approached, + // statsd will send a broadcast so that the client can fetch the data and clear this memory. + static const size_t kMaxMetricsBytesPerConfig = 128 * 1024; + // Cap the UID map's memory usage to this. This should be fairly high since the UID information // is critical for understanding the metrics. const static size_t kMaxBytesUsedUidMap = 50 * 1024; + /* Minimum period between two broadcasts in nanoseconds. */ + static const unsigned long long kMinBroadcastPeriodNs = 60 * NS_PER_SEC; + + /* Min period between two checks of byte size per config key in nanoseconds. */ + static const unsigned long long kMinByteSizeCheckPeriodNs = 10 * NS_PER_SEC; + /** * Report a new config has been received and report the static stats about the config. * diff --git a/cmds/statsd/src/metrics/MetricsManager.h b/cmds/statsd/src/metrics/MetricsManager.h index 4e6a0ce5439b2..86c47333d7e6f 100644 --- a/cmds/statsd/src/metrics/MetricsManager.h +++ b/cmds/statsd/src/metrics/MetricsManager.h @@ -36,7 +36,7 @@ class MetricsManager { public: MetricsManager(const ConfigKey& configKey, const StatsdConfig& config); - ~MetricsManager(); + virtual ~MetricsManager(); // Return whether the configuration is valid. bool isConfigValid() const; @@ -52,11 +52,11 @@ public: void setAnomalyMonitor(const sp& anomalyMonitor); // Config source owner can call onDumpReport() to get all the metrics collected. - std::vector>> onDumpReport(); + virtual std::vector>> onDumpReport(); // Computes the total byte size of all metrics managed by a single config source. // Does not change the state. - size_t byteSize(); + virtual size_t byteSize(); private: const ConfigKey mConfigKey; diff --git a/cmds/statsd/tests/StatsLogProcessor_test.cpp b/cmds/statsd/tests/StatsLogProcessor_test.cpp new file mode 100644 index 0000000000000..ff04d955ecd0f --- /dev/null +++ b/cmds/statsd/tests/StatsLogProcessor_test.cpp @@ -0,0 +1,117 @@ +// Copyright (C) 2017 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 "StatsLogProcessor.h" +#include "config/ConfigKey.h" +#include "frameworks/base/cmds/statsd/src/statsd_config.pb.h" +#include "guardrail/StatsdStats.h" +#include "logd/LogEvent.h" +#include "packages/UidMap.h" +#include "statslog.h" + +#include +#include + +#include + +using namespace android; +using namespace testing; + +namespace android { +namespace os { +namespace statsd { + +#ifdef __ANDROID__ + +/** + * Mock MetricsManager (ByteSize() is called). + */ +class MockMetricsManager : public MetricsManager { +public: + MockMetricsManager() : MetricsManager(ConfigKey(1, "key"), StatsdConfig()) { + } + + MOCK_METHOD0(byteSize, size_t()); + MOCK_METHOD0(onDumpReport, std::vector>>()); +}; + +TEST(StatsLogProcessorTest, TestRateLimitByteSize) { + sp m = new UidMap(); + sp anomalyMonitor; + // Construct the processor with a dummy sendBroadcast function that does nothing. + StatsLogProcessor p(m, anomalyMonitor, [](const ConfigKey& key) {}); + + MockMetricsManager mockMetricsManager; + + ConfigKey key(100, "key"); + // Expect only the first flush to trigger a check for byte size since the last two are + // rate-limited. + EXPECT_CALL(mockMetricsManager, byteSize()).Times(1); + p.flushIfNecessary(99, key, mockMetricsManager); + p.flushIfNecessary(100, key, mockMetricsManager); + p.flushIfNecessary(101, key, mockMetricsManager); +} + +TEST(StatsLogProcessorTest, TestRateLimitBroadcast) { + sp m = new UidMap(); + sp anomalyMonitor; + int broadcastCount = 0; + StatsLogProcessor p(m, anomalyMonitor, + [&broadcastCount](const ConfigKey& key) { broadcastCount++; }); + + MockMetricsManager mockMetricsManager; + + ConfigKey key(100, "key"); + EXPECT_CALL(mockMetricsManager, byteSize()) + .Times(2) + .WillRepeatedly(Return(int(StatsdStats::kMaxMetricsBytesPerConfig * .95))); + + // Expect only one broadcast despite always returning a size that should trigger broadcast. + p.flushIfNecessary(1, key, mockMetricsManager); + EXPECT_EQ(1, broadcastCount); + + // This next call to flush should not trigger a broadcast. + p.mLastByteSizeTimes.clear(); // Force another check for byte size. + p.flushIfNecessary(2, key, mockMetricsManager); + EXPECT_EQ(1, broadcastCount); +} + +TEST(StatsLogProcessorTest, TestDropWhenByteSizeTooLarge) { + sp m = new UidMap(); + sp anomalyMonitor; + int broadcastCount = 0; + StatsLogProcessor p(m, anomalyMonitor, + [&broadcastCount](const ConfigKey& key) { broadcastCount++; }); + + MockMetricsManager mockMetricsManager; + + ConfigKey key(100, "key"); + EXPECT_CALL(mockMetricsManager, byteSize()) + .Times(1) + .WillRepeatedly(Return(int(StatsdStats::kMaxMetricsBytesPerConfig * 1.2))); + + EXPECT_CALL(mockMetricsManager, onDumpReport()).Times(1); + + // Expect to call the onDumpReport and skip the broadcast. + p.flushIfNecessary(1, key, mockMetricsManager); + EXPECT_EQ(0, broadcastCount); +} + +#else +GTEST_LOG_(INFO) << "This test does nothing.\n"; +#endif + +} // namespace statsd +} // namespace os +} // namespace android \ No newline at end of file