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

@@ -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);
     }

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);

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);