From cfc311d2f098de441bb2eeb7d27ea158773e281b Mon Sep 17 00:00:00 2001 From: David Chen Date: Tue, 23 Jan 2018 17:55:54 -0800 Subject: Statsd always includes snapshot of uid map. Statsd will contain at least one snapshot of the uid map. The previous design was not very robust in case a snapshot was missing. Also fixes subtle bug with updating the isolated uid mapping since this should always be kept up to date even if there are no metrics being used (since metrics may be added later after the isolated uid was created). Test: Checked that unit-tests pass on marlin-eng. Change-Id: I99754ed9016d980564e409b0946a46b398fd12b7 --- cmds/statsd/src/StatsLogProcessor.cpp | 12 +++++++----- cmds/statsd/src/packages/UidMap.cpp | 25 ++++++++++++++----------- cmds/statsd/tests/UidMap_test.cpp | 14 ++++++-------- 3 files changed, 27 insertions(+), 24 deletions(-) diff --git a/cmds/statsd/src/StatsLogProcessor.cpp b/cmds/statsd/src/StatsLogProcessor.cpp index 7a7a2f617e6a..9be8f85d9dbb 100644 --- a/cmds/statsd/src/StatsLogProcessor.cpp +++ b/cmds/statsd/src/StatsLogProcessor.cpp @@ -127,15 +127,17 @@ void StatsLogProcessor::OnLogEvent(LogEvent* event) { StatsdStats::getInstance().noteAtomLogged( event->GetTagId(), event->GetTimestampNs() / NS_PER_SEC); - if (mMetricsManagers.empty()) { - return; - } - // Hard-coded logic to update the isolated uid's in the uid-map. // The field numbers need to be currently updated by hand with atoms.proto if (event->GetTagId() == android::util::ISOLATED_UID_CHANGED) { onIsolatedUidChangedEventLocked(*event); - } else { + } + + if (mMetricsManagers.empty()) { + return; + } + + if (event->GetTagId() != android::util::ISOLATED_UID_CHANGED) { // Map the isolated uid to host uid if necessary. mapIsolatedUidToHostUidIfNecessaryLocked(event); } diff --git a/cmds/statsd/src/packages/UidMap.cpp b/cmds/statsd/src/packages/UidMap.cpp index eefb7dc046ec..91279661b61f 100644 --- a/cmds/statsd/src/packages/UidMap.cpp +++ b/cmds/statsd/src/packages/UidMap.cpp @@ -281,20 +281,10 @@ int UidMap::getHostUidOrSelf(int uid) const { void UidMap::clearOutput() { mOutput.Clear(); - // Re-initialize the initial state for the outputs. This results in extra data being uploaded - // but helps ensure we can re-construct the UID->app name, versionCode mapping in server. - auto snapshot = mOutput.add_snapshots(); - for (auto it : mMap) { - auto t = snapshot->add_package_info(); - t->set_name(it.second.packageName); - t->set_version(it.second.versionCode); - t->set_uid(it.first); - } - // Also update the guardrail trackers. StatsdStats::getInstance().setUidMapChanges(0); StatsdStats::getInstance().setUidMapSnapshots(1); - mBytesUsed = snapshot->ByteSize(); + mBytesUsed = 0; StatsdStats::getInstance().setCurrentUidMapMemory(mBytesUsed); } @@ -348,6 +338,19 @@ UidMapping UidMap::getOutput(const int64_t& timestamp, const ConfigKey& key) { ++it_deltas; } } + + if (mOutput.snapshots_size() == 0) { + // Produce another snapshot. This results in extra data being uploaded but helps + // ensure we can re-construct the UID->app name, versionCode mapping in server. + auto snapshot = mOutput.add_snapshots(); + snapshot->set_timestamp_nanos(timestamp); + for (auto it : mMap) { + auto t = snapshot->add_package_info(); + t->set_name(it.second.packageName); + t->set_version(it.second.versionCode); + t->set_uid(it.first); + } + } } mBytesUsed = mOutput.ByteSize(); // Compute actual size after potential deletions. StatsdStats::getInstance().setCurrentUidMapMemory(mBytesUsed); diff --git a/cmds/statsd/tests/UidMap_test.cpp b/cmds/statsd/tests/UidMap_test.cpp index 5292f24f0bee..f26c10d33e67 100644 --- a/cmds/statsd/tests/UidMap_test.cpp +++ b/cmds/statsd/tests/UidMap_test.cpp @@ -178,16 +178,16 @@ TEST(UidMapTest, TestClearingOutput) { EXPECT_EQ(1, results.snapshots_size()); // It should be cleared now - EXPECT_EQ(0, m.mOutput.snapshots_size()); + EXPECT_EQ(1, m.mOutput.snapshots_size()); results = m.getOutput(3, config1); - EXPECT_EQ(0, results.snapshots_size()); + EXPECT_EQ(1, results.snapshots_size()); // Now add another configuration. m.OnConfigUpdated(config2); m.updateApp(5, String16(kApp1.c_str()), 1000, 40); EXPECT_EQ(1, m.mOutput.changes_size()); results = m.getOutput(6, config1); - EXPECT_EQ(0, results.snapshots_size()); + EXPECT_EQ(1, results.snapshots_size()); EXPECT_EQ(1, results.changes_size()); EXPECT_EQ(1, m.mOutput.changes_size()); @@ -197,15 +197,15 @@ TEST(UidMapTest, TestClearingOutput) { // We still can't remove anything. results = m.getOutput(8, config1); - EXPECT_EQ(0, results.snapshots_size()); + EXPECT_EQ(1, results.snapshots_size()); EXPECT_EQ(2, results.changes_size()); EXPECT_EQ(2, m.mOutput.changes_size()); results = m.getOutput(9, config2); - EXPECT_EQ(0, results.snapshots_size()); + EXPECT_EQ(1, results.snapshots_size()); EXPECT_EQ(2, results.changes_size()); // At this point both should be cleared. - EXPECT_EQ(0, m.mOutput.snapshots_size()); + EXPECT_EQ(1, m.mOutput.snapshots_size()); EXPECT_EQ(0, m.mOutput.changes_size()); } @@ -228,10 +228,8 @@ TEST(UidMapTest, TestMemoryComputed) { m.updateApp(3, String16(kApp1.c_str()), 1000, 40); EXPECT_TRUE(m.mBytesUsed > snapshot_bytes); - size_t bytesWithSnapshotChange = m.mBytesUsed; m.getOutput(2, config1); - EXPECT_TRUE(m.mBytesUsed < bytesWithSnapshotChange); size_t prevBytes = m.mBytesUsed; m.getOutput(4, config1); -- cgit v1.2.3-59-g8ed1b