From 9e6dbbdadf8de3bcc58a6c26784219217cd35b53 Mon Sep 17 00:00:00 2001
From: David Chen <dwchen@google.com>
Date: Mon, 7 May 2018 17:52:29 -0700
Subject: [PATCH] Fix statsd returning uidmap with empty reports.

We notice devices uploading a bunch of bytes for the uidmap even if
the device is running an empty config, so there are no actual metrics
to report. This hardcodes some logic to skip the inclusion of the
uidmap if there are exactly 0 metrics.

Bug: 79381210
Test: Tested unit-tests on marlin-eng
Change-Id: I96348235341a7faf15ff57d4d1eccac635a3a999
---
 cmds/statsd/src/StatsLogProcessor.cpp        | 11 +++--
 cmds/statsd/src/metrics/MetricsManager.h     |  4 ++
 cmds/statsd/tests/StatsLogProcessor_test.cpp | 43 +++++++++++++++++++-
 3 files changed, 52 insertions(+), 6 deletions(-)

diff --git a/cmds/statsd/src/StatsLogProcessor.cpp b/cmds/statsd/src/StatsLogProcessor.cpp
index 4d279481dc765..daafe9c0390df 100644
--- a/cmds/statsd/src/StatsLogProcessor.cpp
+++ b/cmds/statsd/src/StatsLogProcessor.cpp
@@ -382,10 +382,13 @@ void StatsLogProcessor::onConfigMetricsReportLocked(const ConfigKey& key,
     it->second->onDumpReport(dumpTimeStampNs, include_current_partial_bucket,
                              &str_set, proto);
 
-    // Fill in UidMap.
-    uint64_t uidMapToken = proto->start(FIELD_TYPE_MESSAGE | FIELD_ID_UID_MAP);
-    mUidMap->appendUidMap(dumpTimeStampNs, key, &str_set, proto);
-    proto->end(uidMapToken);
+    // Fill in UidMap if there is at least one metric to report.
+    // This skips the uid map if it's an empty config.
+    if (it->second->getNumMetrics() > 0) {
+        uint64_t uidMapToken = proto->start(FIELD_TYPE_MESSAGE | FIELD_ID_UID_MAP);
+        mUidMap->appendUidMap(dumpTimeStampNs, key, &str_set, proto);
+        proto->end(uidMapToken);
+    }
 
     // Fill in the timestamps.
     proto->write(FIELD_TYPE_INT64 | FIELD_ID_LAST_REPORT_ELAPSED_NANOS,
diff --git a/cmds/statsd/src/metrics/MetricsManager.h b/cmds/statsd/src/metrics/MetricsManager.h
index 4f39df9e7ad2c..456da982bd9bc 100644
--- a/cmds/statsd/src/metrics/MetricsManager.h
+++ b/cmds/statsd/src/metrics/MetricsManager.h
@@ -89,6 +89,10 @@ public:
         return mLastReportWallClockNs;
     };
 
+    inline size_t getNumMetrics() const {
+        return mAllMetricProducers.size();
+    }
+
     virtual void dropData(const int64_t dropTimeNs);
 
     virtual void onDumpReport(const int64_t dumpTimeNs,
diff --git a/cmds/statsd/tests/StatsLogProcessor_test.cpp b/cmds/statsd/tests/StatsLogProcessor_test.cpp
index de6e6e542c604..3395aa639aabf 100644
--- a/cmds/statsd/tests/StatsLogProcessor_test.cpp
+++ b/cmds/statsd/tests/StatsLogProcessor_test.cpp
@@ -24,6 +24,8 @@
 #include <gmock/gmock.h>
 #include <gtest/gtest.h>
 
+#include "tests/statsd_test_util.h"
+
 #include <stdio.h>
 
 using namespace android;
@@ -123,6 +125,21 @@ TEST(StatsLogProcessorTest, TestDropWhenByteSizeTooLarge) {
     EXPECT_EQ(0, broadcastCount);
 }
 
+StatsdConfig MakeConfig(bool includeMetric) {
+    StatsdConfig config;
+    config.add_allowed_log_source("AID_ROOT");  // LogEvent defaults to UID of root.
+
+    if (includeMetric) {
+        auto appCrashMatcher = CreateProcessCrashAtomMatcher();
+        *config.add_atom_matcher() = appCrashMatcher;
+        auto countMetric = config.add_count_metric();
+        countMetric->set_id(StringToId("AppCrashes"));
+        countMetric->set_what(appCrashMatcher.id());
+        countMetric->set_bucket(FIVE_MINUTES);
+    }
+    return config;
+}
+
 TEST(StatsLogProcessorTest, TestUidMapHasSnapshot) {
     // Setup simple config key corresponding to empty config.
     sp<UidMap> m = new UidMap();
@@ -133,8 +150,7 @@ TEST(StatsLogProcessorTest, TestUidMapHasSnapshot) {
     StatsLogProcessor p(m, anomalyAlarmMonitor, subscriberAlarmMonitor, 0,
                         [&broadcastCount](const ConfigKey& key) { broadcastCount++; return true;});
     ConfigKey key(3, 4);
-    StatsdConfig config;
-    config.add_allowed_log_source("AID_ROOT");
+    StatsdConfig config = MakeConfig(true);
     p.OnConfigUpdated(0, key, config);
 
     // Expect to get no metrics, but snapshot specified above in uidmap.
@@ -149,6 +165,29 @@ TEST(StatsLogProcessorTest, TestUidMapHasSnapshot) {
     EXPECT_EQ(2, uidmap.snapshots(0).package_info_size());
 }
 
+TEST(StatsLogProcessorTest, TestEmptyConfigHasNoUidMap) {
+    // Setup simple config key corresponding to empty config.
+    sp<UidMap> m = new UidMap();
+    m->updateMap(1, {1, 2}, {1, 2}, {String16("p1"), String16("p2")});
+    sp<AlarmMonitor> anomalyAlarmMonitor;
+    sp<AlarmMonitor> subscriberAlarmMonitor;
+    int broadcastCount = 0;
+    StatsLogProcessor p(m, anomalyAlarmMonitor, subscriberAlarmMonitor, 0,
+                        [&broadcastCount](const ConfigKey& key) { broadcastCount++; return true;});
+    ConfigKey key(3, 4);
+    StatsdConfig config = MakeConfig(false);
+    p.OnConfigUpdated(0, key, config);
+
+    // Expect to get no metrics, but snapshot specified above in uidmap.
+    vector<uint8_t> bytes;
+    p.onDumpReport(key, 1, false, true, ADB_DUMP, &bytes);
+
+    ConfigMetricsReportList output;
+    output.ParseFromArray(bytes.data(), bytes.size());
+    EXPECT_TRUE(output.reports_size() > 0);
+    EXPECT_FALSE(output.reports(0).has_uid_map());
+}
+
 TEST(StatsLogProcessorTest, TestReportIncludesSubConfig) {
     // Setup simple config key corresponding to empty config.
     sp<UidMap> m = new UidMap();