Fixes statsd returning too much data at once.
We observe a single ConfigMetricsReportList can be greater than the
safe size for the binder transaction buffer since we only check the
size of the current metrics in progress, but we also return the
previous reports stored on disk.
This change will attempt to send another ConfigMetricsReportList
as soon as possible if there's already a report on disk.
Also fixes a bug when trying to trigger data fetch before the client
has registered the corresponding dataFetchOperation.
Bug: 79201869
Test: Tested manually on marlin-eng
Change-Id: I2d3677162804a27e7a7a95d482d80c46bd994a67
diff --git a/cmds/statsd/benchmark/metric_util.cpp b/cmds/statsd/benchmark/metric_util.cpp
index e6272ed..50ed18d 100644
--- a/cmds/statsd/benchmark/metric_util.cpp
+++ b/cmds/statsd/benchmark/metric_util.cpp
@@ -366,7 +366,7 @@
sp<AlarmMonitor> periodicAlarmMonitor;
sp<StatsLogProcessor> processor = new StatsLogProcessor(
uidMap, anomalyAlarmMonitor, periodicAlarmMonitor, timeBaseSec * NS_PER_SEC,
- [](const ConfigKey&){});
+ [](const ConfigKey&){return true;});
processor->OnConfigUpdated(timeBaseSec * NS_PER_SEC, key, config);
return processor;
}
diff --git a/cmds/statsd/src/StatsLogProcessor.cpp b/cmds/statsd/src/StatsLogProcessor.cpp
index ed07acc..4d27948 100644
--- a/cmds/statsd/src/StatsLogProcessor.cpp
+++ b/cmds/statsd/src/StatsLogProcessor.cpp
@@ -75,7 +75,7 @@
const sp<AlarmMonitor>& anomalyAlarmMonitor,
const sp<AlarmMonitor>& periodicAlarmMonitor,
const int64_t timeBaseNs,
- const std::function<void(const ConfigKey&)>& sendBroadcast)
+ const std::function<bool(const ConfigKey&)>& sendBroadcast)
: mUidMap(uidMap),
mAnomalyAlarmMonitor(anomalyAlarmMonitor),
mPeriodicAlarmMonitor(periodicAlarmMonitor),
@@ -465,12 +465,21 @@
// We suspect that the byteSize() computation is expensive, so we set a rate limit.
size_t totalBytes = metricsManager.byteSize();
mLastByteSizeTimes[key] = timestampNs;
+ bool requestDump = false;
if (totalBytes >
StatsdStats::kMaxMetricsBytesPerConfig) { // Too late. We need to start clearing data.
metricsManager.dropData(timestampNs);
StatsdStats::getInstance().noteDataDropped(key);
VLOG("StatsD had to toss out metrics for %s", key.ToString().c_str());
- } else if (totalBytes > StatsdStats::kBytesPerConfigTriggerGetData) {
+ } else if ((totalBytes > StatsdStats::kBytesPerConfigTriggerGetData) ||
+ (mOnDiskDataConfigs.find(key) != mOnDiskDataConfigs.end())) {
+ // Request to send a broadcast if:
+ // 1. in memory data > threshold OR
+ // 2. config has old data report on disk.
+ requestDump = true;
+ }
+
+ if (requestDump) {
// Send broadcast so that receivers can pull data.
auto lastBroadcastTime = mLastBroadcastTimes.find(key);
if (lastBroadcastTime != mLastBroadcastTimes.end()) {
@@ -479,10 +488,12 @@
return;
}
}
- mLastBroadcastTimes[key] = timestampNs;
- VLOG("StatsD requesting broadcast for %s", key.ToString().c_str());
- mSendBroadcast(key);
- StatsdStats::getInstance().noteBroadcastSent(key);
+ if (mSendBroadcast(key)) {
+ mOnDiskDataConfigs.erase(key);
+ VLOG("StatsD triggered data fetch for %s", key.ToString().c_str());
+ mLastBroadcastTimes[key] = timestampNs;
+ StatsdStats::getInstance().noteBroadcastSent(key);
+ }
}
}
@@ -505,6 +516,8 @@
return;
}
proto.flush(fd.get());
+ // We were able to write the ConfigMetricsReport to disk, so we should trigger collection ASAP.
+ mOnDiskDataConfigs.insert(key);
}
void StatsLogProcessor::WriteDataToDiskLocked(const DumpReportReason dumpReportReason) {
@@ -533,6 +546,11 @@
}
}
+void StatsLogProcessor::noteOnDiskData(const ConfigKey& key) {
+ std::lock_guard<std::mutex> lock(mMetricsMutex);
+ mOnDiskDataConfigs.insert(key);
+}
+
} // namespace statsd
} // namespace os
} // namespace android
diff --git a/cmds/statsd/src/StatsLogProcessor.h b/cmds/statsd/src/StatsLogProcessor.h
index 8de0f41..d6fb8de 100644
--- a/cmds/statsd/src/StatsLogProcessor.h
+++ b/cmds/statsd/src/StatsLogProcessor.h
@@ -48,7 +48,7 @@
StatsLogProcessor(const sp<UidMap>& uidMap, const sp<AlarmMonitor>& anomalyAlarmMonitor,
const sp<AlarmMonitor>& subscriberTriggerAlarmMonitor,
const int64_t timeBaseNs,
- const std::function<void(const ConfigKey&)>& sendBroadcast);
+ const std::function<bool(const ConfigKey&)>& sendBroadcast);
virtual ~StatsLogProcessor();
void OnLogEvent(LogEvent* event, bool reconnectionStarts);
@@ -99,6 +99,9 @@
#endif
}
+ // Add a specific config key to the possible configs to dump ASAP.
+ void noteOnDiskData(const ConfigKey& key);
+
private:
// For testing only.
inline sp<AlarmMonitor> getAnomalyAlarmMonitor() const {
@@ -118,6 +121,9 @@
// Tracks when we last checked the bytes consumed for each config key.
std::unordered_map<ConfigKey, long> mLastByteSizeTimes;
+ // Tracks which config keys has metric reports on disk
+ std::set<ConfigKey> mOnDiskDataConfigs;
+
sp<UidMap> mUidMap; // Reference to the UidMap to lookup app name and version for each uid.
StatsPullerManager mStatsPullerManager;
@@ -159,7 +165,7 @@
// Function used to send a broadcast so that receiver for the config key can call getData
// to retrieve the stored data.
- std::function<void(const ConfigKey& key)> mSendBroadcast;
+ std::function<bool(const ConfigKey& key)> mSendBroadcast;
const int64_t mTimeBaseNs;
diff --git a/cmds/statsd/src/StatsService.cpp b/cmds/statsd/src/StatsService.cpp
index e823f68..451e682 100644
--- a/cmds/statsd/src/StatsService.cpp
+++ b/cmds/statsd/src/StatsService.cpp
@@ -158,11 +158,14 @@
auto receiver = mConfigManager->GetConfigReceiver(key);
if (sc == nullptr) {
VLOG("Could not find StatsCompanionService");
+ return false;
} else if (receiver == nullptr) {
VLOG("Statscompanion could not find a broadcast receiver for %s",
key.ToString().c_str());
+ return false;
} else {
sc->sendDataBroadcast(receiver, mProcessor->getLastReportTimeNs(key));
+ return true;
}
}
);
@@ -948,6 +951,11 @@
IPCThreadState* ipc = IPCThreadState::self();
ConfigKey configKey(ipc->getCallingUid(), key);
mConfigManager->SetConfigReceiver(configKey, intentSender);
+ if (StorageManager::hasConfigMetricsReport(configKey)) {
+ VLOG("StatsService::setDataFetchOperation marking configKey %s to dump reports on disk",
+ configKey.ToString().c_str());
+ mProcessor->noteOnDiskData(configKey);
+ }
return Status::ok();
}
diff --git a/cmds/statsd/src/metrics/MetricsManager.cpp b/cmds/statsd/src/metrics/MetricsManager.cpp
index bf0f720..d7d51d9 100644
--- a/cmds/statsd/src/metrics/MetricsManager.cpp
+++ b/cmds/statsd/src/metrics/MetricsManager.cpp
@@ -62,7 +62,7 @@
: mConfigKey(key), mUidMap(uidMap),
mTtlNs(config.has_ttl_in_seconds() ? config.ttl_in_seconds() * NS_PER_SEC : -1),
mTtlEndNs(-1),
- mLastReportTimeNs(timeBaseNs),
+ mLastReportTimeNs(currentTimeNs),
mLastReportWallClockNs(getWallClockNs()) {
// Init the ttl end timestamp.
refreshTtl(timeBaseNs);
diff --git a/cmds/statsd/src/metrics/MetricsManager.h b/cmds/statsd/src/metrics/MetricsManager.h
index 170d6a7..4f39df9e 100644
--- a/cmds/statsd/src/metrics/MetricsManager.h
+++ b/cmds/statsd/src/metrics/MetricsManager.h
@@ -79,7 +79,8 @@
}
};
- // Returns the elapsed realtime when this metric manager last reported metrics.
+ // Returns the elapsed realtime when this metric manager last reported metrics. If this config
+ // has not yet dumped any reports, this is the time the metricsmanager was initialized.
inline int64_t getLastReportTimeNs() const {
return mLastReportTimeNs;
};
diff --git a/cmds/statsd/src/storage/StorageManager.cpp b/cmds/statsd/src/storage/StorageManager.cpp
index ea8da14..1f81812 100644
--- a/cmds/statsd/src/storage/StorageManager.cpp
+++ b/cmds/statsd/src/storage/StorageManager.cpp
@@ -160,6 +160,34 @@
}
}
+bool StorageManager::hasConfigMetricsReport(const ConfigKey& key) {
+ unique_ptr<DIR, decltype(&closedir)> dir(opendir(STATS_DATA_DIR), closedir);
+ if (dir == NULL) {
+ VLOG("Path %s does not exist", STATS_DATA_DIR);
+ return false;
+ }
+
+ string suffix = StringPrintf("%d_%lld", key.GetUid(), (long long)key.GetId());
+
+ dirent* de;
+ while ((de = readdir(dir.get()))) {
+ char* name = de->d_name;
+ if (name[0] == '.') continue;
+
+ size_t nameLen = strlen(name);
+ size_t suffixLen = suffix.length();
+ if (suffixLen <= nameLen &&
+ strncmp(name + nameLen - suffixLen, suffix.c_str(), suffixLen) == 0) {
+ // Check again that the file name is parseable.
+ int64_t result[3];
+ parseFileName(name, result);
+ if (result[0] == -1) continue;
+ return true;
+ }
+ }
+ return false;
+}
+
void StorageManager::appendConfigMetricsReport(const ConfigKey& key, ProtoOutputStream* proto) {
unique_ptr<DIR, decltype(&closedir)> dir(opendir(STATS_DATA_DIR), closedir);
if (dir == NULL) {
diff --git a/cmds/statsd/src/storage/StorageManager.h b/cmds/statsd/src/storage/StorageManager.h
index 8953be9..4840f3c 100644
--- a/cmds/statsd/src/storage/StorageManager.h
+++ b/cmds/statsd/src/storage/StorageManager.h
@@ -63,6 +63,11 @@
const std::function<void(const ConfigKey&)>& sendBroadcast);
/**
+ * Returns true if there's at least one report on disk.
+ */
+ static bool hasConfigMetricsReport(const ConfigKey& key);
+
+ /**
* Appends ConfigMetricsReport found on disk to the specific proto and
* delete it.
*/
diff --git a/cmds/statsd/tests/StatsLogProcessor_test.cpp b/cmds/statsd/tests/StatsLogProcessor_test.cpp
index 9fdf7a3..de6e6e5 100644
--- a/cmds/statsd/tests/StatsLogProcessor_test.cpp
+++ b/cmds/statsd/tests/StatsLogProcessor_test.cpp
@@ -62,7 +62,7 @@
sp<AlarmMonitor> periodicAlarmMonitor;
// Construct the processor with a dummy sendBroadcast function that does nothing.
StatsLogProcessor p(m, anomalyAlarmMonitor, periodicAlarmMonitor, 0,
- [](const ConfigKey& key) {});
+ [](const ConfigKey& key) {return true;});
MockMetricsManager mockMetricsManager;
@@ -81,7 +81,7 @@
sp<AlarmMonitor> subscriberAlarmMonitor;
int broadcastCount = 0;
StatsLogProcessor p(m, anomalyAlarmMonitor, subscriberAlarmMonitor, 0,
- [&broadcastCount](const ConfigKey& key) { broadcastCount++; });
+ [&broadcastCount](const ConfigKey& key) { broadcastCount++; return true;});
MockMetricsManager mockMetricsManager;
@@ -107,7 +107,7 @@
sp<AlarmMonitor> subscriberAlarmMonitor;
int broadcastCount = 0;
StatsLogProcessor p(m, anomalyAlarmMonitor, subscriberAlarmMonitor, 0,
- [&broadcastCount](const ConfigKey& key) { broadcastCount++; });
+ [&broadcastCount](const ConfigKey& key) { broadcastCount++; return true;});
MockMetricsManager mockMetricsManager;
@@ -131,7 +131,7 @@
sp<AlarmMonitor> subscriberAlarmMonitor;
int broadcastCount = 0;
StatsLogProcessor p(m, anomalyAlarmMonitor, subscriberAlarmMonitor, 0,
- [&broadcastCount](const ConfigKey& key) { broadcastCount++; });
+ [&broadcastCount](const ConfigKey& key) { broadcastCount++; return true;});
ConfigKey key(3, 4);
StatsdConfig config;
config.add_allowed_log_source("AID_ROOT");
@@ -156,7 +156,7 @@
sp<AlarmMonitor> subscriberAlarmMonitor;
int broadcastCount = 0;
StatsLogProcessor p(m, anomalyAlarmMonitor, subscriberAlarmMonitor, 0,
- [&broadcastCount](const ConfigKey& key) { broadcastCount++; });
+ [&broadcastCount](const ConfigKey& key) { broadcastCount++; return true;});
ConfigKey key(3, 4);
StatsdConfig config;
auto annotation = config.add_annotation();
@@ -185,7 +185,7 @@
sp<AlarmMonitor> subscriberAlarmMonitor;
int broadcastCount = 0;
StatsLogProcessor p(m, anomalyAlarmMonitor, subscriberAlarmMonitor, 0,
- [&broadcastCount](const ConfigKey& key) { broadcastCount++; });
+ [&broadcastCount](const ConfigKey& key) { broadcastCount++; return true;});
LogEvent event1(0, 1 /*logd timestamp*/, 1001 /*elapsedRealtime*/);
event1.init();
diff --git a/cmds/statsd/tests/UidMap_test.cpp b/cmds/statsd/tests/UidMap_test.cpp
index dde50c2..e23131d 100644
--- a/cmds/statsd/tests/UidMap_test.cpp
+++ b/cmds/statsd/tests/UidMap_test.cpp
@@ -44,7 +44,7 @@
sp<AlarmMonitor> subscriberAlarmMonitor;
// Construct the processor with a dummy sendBroadcast function that does nothing.
StatsLogProcessor p(m, anomalyAlarmMonitor, subscriberAlarmMonitor, 0,
- [](const ConfigKey& key) {});
+ [](const ConfigKey& key) {return true;});
LogEvent addEvent(android::util::ISOLATED_UID_CHANGED, 1);
addEvent.write(100); // parent UID
addEvent.write(101); // isolated UID
diff --git a/cmds/statsd/tests/statsd_test_util.cpp b/cmds/statsd/tests/statsd_test_util.cpp
index 5903993..e0c98cb 100644
--- a/cmds/statsd/tests/statsd_test_util.cpp
+++ b/cmds/statsd/tests/statsd_test_util.cpp
@@ -459,7 +459,7 @@
new AlarmMonitor(1, [](const sp<IStatsCompanionService>&, int64_t){},
[](const sp<IStatsCompanionService>&){});
sp<StatsLogProcessor> processor = new StatsLogProcessor(
- uidMap, anomalyAlarmMonitor, periodicAlarmMonitor, timeBaseNs, [](const ConfigKey&){});
+ uidMap, anomalyAlarmMonitor, periodicAlarmMonitor, timeBaseNs, [](const ConfigKey&){return true;});
processor->OnConfigUpdated(currentTimeNs, key, config);
return processor;
}