Clean up TODOs in statsd
+ Created bugs for those TODOs that are still relevant.
+ Remove obsolete TODOs.
Test: no code change.
Change-Id: I41c2a89a882f087817ee6cbc3f095e1d80e1928e
diff --git a/cmds/statsd/src/FieldValue.h b/cmds/statsd/src/FieldValue.h
index 02c49b9..b1d6ab3 100644
--- a/cmds/statsd/src/FieldValue.h
+++ b/cmds/statsd/src/FieldValue.h
@@ -212,7 +212,7 @@
* the result is equal to the Matcher Field. That's a bit wise AND operation + check if 2 ints are
* equal. Nothing can beat the performance of this matching algorithm.
*
- * TODO: ADD EXAMPLE HERE.
+ * TODO(b/110561213): ADD EXAMPLE HERE.
*/
struct Matcher {
Matcher(const Field& matcher, int32_t mask) : mMatcher(matcher), mMask(mask){};
diff --git a/cmds/statsd/src/HashableDimensionKey.cpp b/cmds/statsd/src/HashableDimensionKey.cpp
index 7103034..af8b3af 100644
--- a/cmds/statsd/src/HashableDimensionKey.cpp
+++ b/cmds/statsd/src/HashableDimensionKey.cpp
@@ -65,8 +65,6 @@
for (const auto& value : values) {
for (size_t i = 0; i < matcherFields.size(); ++i) {
const auto& matcher = matcherFields[i];
- // TODO: potential optimization here to break early because all fields are naturally
- // sorted.
if (value.mField.matches(matcher)) {
output->addValue(value);
output->mutableValue(num_matches)->mField.setTag(value.mField.getTag());
@@ -196,4 +194,4 @@
} // namespace statsd
} // namespace os
-} // namespace android
\ No newline at end of file
+} // namespace android
diff --git a/cmds/statsd/src/StatsService.cpp b/cmds/statsd/src/StatsService.cpp
index ae44ee9..1119eb3 100644
--- a/cmds/statsd/src/StatsService.cpp
+++ b/cmds/statsd/src/StatsService.cpp
@@ -440,7 +440,6 @@
if (argCount == 2) {
// Automatically pick the UID
uid = IPCThreadState::self()->getCallingUid();
- // TODO: What if this isn't a binder call? Should we fail?
name.assign(args[1].c_str(), args[1].size());
good = true;
} else if (argCount == 3) {
@@ -493,7 +492,6 @@
if (argCount == 3) {
// Automatically pick the UID
uid = IPCThreadState::self()->getCallingUid();
- // TODO: What if this isn't a binder call? Should we fail?
name.assign(args[2].c_str(), args[2].size());
good = true;
} else if (argCount == 4) {
@@ -578,7 +576,6 @@
if (argCount == 2) {
// Automatically pick the UID
uid = IPCThreadState::self()->getCallingUid();
- // TODO: What if this isn't a binder call? Should we fail?
name.assign(args[1].c_str(), args[1].size());
good = true;
} else if (argCount == 3) {
@@ -604,7 +601,6 @@
vector<uint8_t> data;
mProcessor->onDumpReport(ConfigKey(uid, StrToInt64(name)), getElapsedRealtimeNs(),
false /* include_current_bucket*/, ADB_DUMP, &data);
- // TODO: print the returned StatsLogReport to file instead of printing to logcat.
if (proto) {
for (size_t i = 0; i < data.size(); i ++) {
fprintf(out, "%c", data[i]);
diff --git a/cmds/statsd/src/StatsService.h b/cmds/statsd/src/StatsService.h
index 0a11f7c..ed90050 100644
--- a/cmds/statsd/src/StatsService.h
+++ b/cmds/statsd/src/StatsService.h
@@ -49,7 +49,6 @@
virtual ~StatsService();
/** The anomaly alarm registered with AlarmManager won't be updated by less than this. */
- // TODO: Consider making this configurable. And choose a good number.
const uint32_t MIN_DIFF_TO_UPDATE_REGISTERED_ALARM_SECS = 5;
virtual status_t onTransact(uint32_t code, const Parcel& data, Parcel* reply, uint32_t flags);
diff --git a/cmds/statsd/src/anomaly/AlarmMonitor.cpp b/cmds/statsd/src/anomaly/AlarmMonitor.cpp
index 78f0c2b..bc36dad 100644
--- a/cmds/statsd/src/anomaly/AlarmMonitor.cpp
+++ b/cmds/statsd/src/anomaly/AlarmMonitor.cpp
@@ -60,7 +60,7 @@
ALOGW("Asked to add a 0-time alarm.");
return;
}
- // TODO: Ensure that refractory period is respected.
+ // TODO(b/110563466): Ensure that refractory period is respected.
VLOG("Adding alarm with time %u", alarm->timestampSec);
mPq.push(alarm);
if (mRegisteredAlarmTimeSec < 1 ||
diff --git a/cmds/statsd/src/anomaly/AnomalyTracker.cpp b/cmds/statsd/src/anomaly/AnomalyTracker.cpp
index f32efee..ee111cd 100644
--- a/cmds/statsd/src/anomaly/AnomalyTracker.cpp
+++ b/cmds/statsd/src/anomaly/AnomalyTracker.cpp
@@ -208,7 +208,8 @@
}
void AnomalyTracker::declareAnomaly(const int64_t& timestampNs, const MetricDimensionKey& key) {
- // TODO: Why receive timestamp? RefractoryPeriod should always be based on real time right now.
+ // TODO(b/110563466): Why receive timestamp? RefractoryPeriod should always be based on
+ // real time right now.
if (isInRefractoryPeriod(timestampNs, key)) {
VLOG("Skipping anomaly declaration since within refractory period");
return;
@@ -216,7 +217,8 @@
if (mAlert.has_refractory_period_secs()) {
mRefractoryPeriodEndsSec[key] = ((timestampNs + NS_PER_SEC - 1) / NS_PER_SEC) // round up
+ mAlert.refractory_period_secs();
- // TODO: If we had access to the bucket_size_millis, consider calling resetStorage()
+ // TODO(b/110563466): If we had access to the bucket_size_millis, consider
+ // calling resetStorage()
// if (mAlert.refractory_period_secs() > mNumOfPastBuckets * bucketSizeNs) {resetStorage();}
}
@@ -230,7 +232,7 @@
StatsdStats::getInstance().noteAnomalyDeclared(mConfigKey, mAlert.id());
- // TODO: This should also take in the const MetricDimensionKey& key?
+ // TODO(b/110564268): This should also take in the const MetricDimensionKey& key?
android::util::stats_write(android::util::ANOMALY_DETECTED, mConfigKey.GetUid(),
mConfigKey.GetId(), mAlert.id());
}
diff --git a/cmds/statsd/src/external/ResourceHealthManagerPuller.cpp b/cmds/statsd/src/external/ResourceHealthManagerPuller.cpp
index 3741202..ae97d7a 100644
--- a/cmds/statsd/src/external/ResourceHealthManagerPuller.cpp
+++ b/cmds/statsd/src/external/ResourceHealthManagerPuller.cpp
@@ -54,7 +54,7 @@
ResourceHealthManagerPuller::ResourceHealthManagerPuller(int tagId) : StatsPuller(tagId) {
}
-// TODO: add other health atoms (eg. Temperature).
+// TODO(b/110565992): add other health atoms (eg. Temperature).
bool ResourceHealthManagerPuller::PullInternal(vector<shared_ptr<LogEvent>>* data) {
if (!getHealthHal()) {
ALOGE("Health Hal not loaded");
diff --git a/cmds/statsd/src/guardrail/StatsdStats.cpp b/cmds/statsd/src/guardrail/StatsdStats.cpp
index 764366f..038cb95 100644
--- a/cmds/statsd/src/guardrail/StatsdStats.cpp
+++ b/cmds/statsd/src/guardrail/StatsdStats.cpp
@@ -103,7 +103,6 @@
{android::util::CPU_TIME_PER_UID_FREQ, {6000, 10000}},
};
-// TODO: add stats for pulled atoms.
StatsdStats::StatsdStats() {
mPushedAtomStats.resize(android::util::kMaxPushedAtomId + 1);
mStartTimeSec = getWallClockSec();
diff --git a/cmds/statsd/src/guardrail/StatsdStats.h b/cmds/statsd/src/guardrail/StatsdStats.h
index 74541d3..9fb2cd8 100644
--- a/cmds/statsd/src/guardrail/StatsdStats.h
+++ b/cmds/statsd/src/guardrail/StatsdStats.h
@@ -86,7 +86,6 @@
static StatsdStats& getInstance();
~StatsdStats(){};
- // TODO: set different limit if the device is low ram.
const static int kDimensionKeySizeSoftLimit = 500;
const static int kDimensionKeySizeHardLimit = 800;
diff --git a/cmds/statsd/src/logd/LogEvent.cpp b/cmds/statsd/src/logd/LogEvent.cpp
index 4e4f146..04d34f3 100644
--- a/cmds/statsd/src/logd/LogEvent.cpp
+++ b/cmds/statsd/src/logd/LogEvent.cpp
@@ -273,7 +273,7 @@
}
int64_t LogEvent::GetLong(size_t key, status_t* err) const {
- // TODO: encapsulate the magical operations all in Field struct as a static function.
+ // TODO(b/110561208): encapsulate the magical operations in Field struct as static functions
int field = getSimpleField(key);
for (const auto& value : mValues) {
if (value.mField.getField() == field) {
diff --git a/cmds/statsd/src/main.cpp b/cmds/statsd/src/main.cpp
index 6aa68da..2f15d0f 100644
--- a/cmds/statsd/src/main.cpp
+++ b/cmds/statsd/src/main.cpp
@@ -82,7 +82,6 @@
if (err != NO_ERROR) {
return err;
}
- // TODO: Do we need to tweak thread priority?
err = pthread_attr_setdetachstate(&attr, PTHREAD_CREATE_DETACHED);
if (err != NO_ERROR) {
pthread_attr_destroy(&attr);
diff --git a/cmds/statsd/src/matchers/LogMatchingTracker.h b/cmds/statsd/src/matchers/LogMatchingTracker.h
index 4f30a04..88ab4e6 100644
--- a/cmds/statsd/src/matchers/LogMatchingTracker.h
+++ b/cmds/statsd/src/matchers/LogMatchingTracker.h
@@ -86,8 +86,6 @@
// The collection of the event tag ids that this LogMatchingTracker cares. So we can quickly
// return kNotMatched when we receive an event with an id not in the list. This is especially
// useful when we have a complex CombinationLogMatcherTracker.
- // TODO: Consider use an array instead of stl set. In reality, the number of the tag ids a
- // LogMatchingTracker cares is only a few.
std::set<int> mAtomIds;
};
diff --git a/cmds/statsd/src/metrics/CountMetricProducer.cpp b/cmds/statsd/src/metrics/CountMetricProducer.cpp
index 43f53e0..a894782 100644
--- a/cmds/statsd/src/metrics/CountMetricProducer.cpp
+++ b/cmds/statsd/src/metrics/CountMetricProducer.cpp
@@ -68,7 +68,6 @@
const sp<ConditionWizard>& wizard,
const int64_t startTimeNs)
: MetricProducer(metric.id(), key, startTimeNs, conditionIndex, wizard) {
- // TODO: evaluate initial conditions. and set mConditionMet.
if (metric.has_bucket()) {
mBucketSizeNs =
TimeUnitToBucketSizeInMillisGuardrailed(key.GetUid(), metric.bucket()) * 1000000;
diff --git a/cmds/statsd/src/metrics/CountMetricProducer.h b/cmds/statsd/src/metrics/CountMetricProducer.h
index 139c083..520d5de 100644
--- a/cmds/statsd/src/metrics/CountMetricProducer.h
+++ b/cmds/statsd/src/metrics/CountMetricProducer.h
@@ -40,7 +40,6 @@
class CountMetricProducer : public MetricProducer {
public:
- // TODO: Pass in the start time from MetricsManager, it should be consistent for all metrics.
CountMetricProducer(const ConfigKey& key, const CountMetric& countMetric,
const int conditionIndex, const sp<ConditionWizard>& wizard,
const int64_t startTimeNs);
@@ -80,7 +79,6 @@
void flushCurrentBucketLocked(const int64_t& eventTimeNs) override;
- // TODO: Add a lock to mPastBuckets.
std::unordered_map<MetricDimensionKey, std::vector<CountBucket>> mPastBuckets;
// The current bucket (may be a partial bucket).
diff --git a/cmds/statsd/src/metrics/DurationMetricProducer.cpp b/cmds/statsd/src/metrics/DurationMetricProducer.cpp
index 62237bc..a19eb0b 100644
--- a/cmds/statsd/src/metrics/DurationMetricProducer.cpp
+++ b/cmds/statsd/src/metrics/DurationMetricProducer.cpp
@@ -76,9 +76,6 @@
mStopAllIndex(stopAllIndex),
mNested(nesting),
mContainANYPositionInInternalDimensions(false) {
- // TODO: The following boiler plate code appears in all MetricProducers, but we can't abstract
- // them in the base class, because the proto generated CountMetric, and DurationMetric are
- // not related. Maybe we should add a template in the future??
if (metric.has_bucket()) {
mBucketSizeNs =
TimeUnitToBucketSizeInMillisGuardrailed(key.GetUid(), metric.bucket()) * 1000000;
@@ -434,8 +431,6 @@
VLOG("Metric %lld onConditionChanged", (long long)mMetricId);
mCondition = conditionMet;
flushIfNeededLocked(eventTime);
- // TODO: need to populate the condition change time from the event which triggers the condition
- // change, instead of using current time.
for (auto& whatIt : mCurrentSlicedDurationTrackerMap) {
for (auto& pair : whatIt.second) {
pair.second->onConditionChanged(conditionMet, eventTime);
diff --git a/cmds/statsd/src/metrics/DurationMetricProducer.h b/cmds/statsd/src/metrics/DurationMetricProducer.h
index 88e455a..c496b12 100644
--- a/cmds/statsd/src/metrics/DurationMetricProducer.h
+++ b/cmds/statsd/src/metrics/DurationMetricProducer.h
@@ -115,7 +115,6 @@
ConditionState mUnSlicedPartCondition;
// Save the past buckets and we can clear when the StatsLogReport is dumped.
- // TODO: Add a lock to mPastBuckets.
std::unordered_map<MetricDimensionKey, std::vector<DurationBucket>> mPastBuckets;
// The duration trackers in the current bucket.
diff --git a/cmds/statsd/src/metrics/EventMetricProducer.h b/cmds/statsd/src/metrics/EventMetricProducer.h
index 62d1105..7f7aa37 100644
--- a/cmds/statsd/src/metrics/EventMetricProducer.h
+++ b/cmds/statsd/src/metrics/EventMetricProducer.h
@@ -33,7 +33,6 @@
class EventMetricProducer : public MetricProducer {
public:
- // TODO: Pass in the start time from MetricsManager, it should be consistent for all metrics.
EventMetricProducer(const ConfigKey& key, const EventMetric& eventMetric,
const int conditionIndex, const sp<ConditionWizard>& wizard,
const int64_t startTimeNs);
diff --git a/cmds/statsd/src/metrics/GaugeMetricProducer.cpp b/cmds/statsd/src/metrics/GaugeMetricProducer.cpp
index d75bb10..a779410 100644
--- a/cmds/statsd/src/metrics/GaugeMetricProducer.cpp
+++ b/cmds/statsd/src/metrics/GaugeMetricProducer.cpp
@@ -101,7 +101,6 @@
translateFieldMatcher(metric.gauge_fields_filter().fields(), &mFieldMatchers);
}
- // TODO: use UidMap if uid->pkg_name is required
if (metric.has_dimensions_in_what()) {
translateFieldMatcher(metric.dimensions_in_what(), &mDimensionsInWhat);
mContainANYPositionInDimensionsInWhat = HasPositionANY(metric.dimensions_in_what());
@@ -299,7 +298,6 @@
protoOutput->end(protoToken);
mPastBuckets.clear();
- // TODO: Clear mDimensionKeyMap once the report is dumped.
}
void GaugeMetricProducer::pullLocked(const int64_t timestampNs) {
diff --git a/cmds/statsd/src/metrics/GaugeMetricProducer.h b/cmds/statsd/src/metrics/GaugeMetricProducer.h
index 62ec27eb..89b1f41 100644
--- a/cmds/statsd/src/metrics/GaugeMetricProducer.h
+++ b/cmds/statsd/src/metrics/GaugeMetricProducer.h
@@ -122,7 +122,6 @@
const int mPullTagId;
// Save the past buckets and we can clear when the StatsLogReport is dumped.
- // TODO: Add a lock to mPastBuckets.
std::unordered_map<MetricDimensionKey, std::vector<GaugeBucket>> mPastBuckets;
// The current partial bucket.
diff --git a/cmds/statsd/src/metrics/MetricsManager.cpp b/cmds/statsd/src/metrics/MetricsManager.cpp
index 213f9ab..0e5ef4d 100644
--- a/cmds/statsd/src/metrics/MetricsManager.cpp
+++ b/cmds/statsd/src/metrics/MetricsManager.cpp
@@ -238,7 +238,6 @@
if (event.GetTagId() == android::util::APP_BREADCRUMB_REPORTED) {
// Check that app breadcrumb reported fields are valid.
- // TODO: Find a way to make these checks easier to maintain.
status_t err = NO_ERROR;
// Uid is 3rd from last field and must match the caller's uid,
diff --git a/cmds/statsd/src/metrics/ValueMetricProducer.cpp b/cmds/statsd/src/metrics/ValueMetricProducer.cpp
index ef8b6cc..f5e953a 100644
--- a/cmds/statsd/src/metrics/ValueMetricProducer.cpp
+++ b/cmds/statsd/src/metrics/ValueMetricProducer.cpp
@@ -89,7 +89,6 @@
? StatsdStats::kAtomDimensionKeySizeLimitMap.at(pullTagId).second
: StatsdStats::kDimensionKeySizeHardLimit),
mUseAbsoluteValueOnReset(metric.use_absolute_value_on_reset()) {
- // TODO: valuemetric for pushed events may need unlimited bucket length
int64_t bucketSizeMills = 0;
if (metric.has_bucket()) {
bucketSizeMills = TimeUnitToBucketSizeInMillisGuardrailed(key.GetUid(), metric.bucket());
diff --git a/cmds/statsd/src/metrics/ValueMetricProducer.h b/cmds/statsd/src/metrics/ValueMetricProducer.h
index 75f3113..943d6dc 100644
--- a/cmds/statsd/src/metrics/ValueMetricProducer.h
+++ b/cmds/statsd/src/metrics/ValueMetricProducer.h
@@ -144,7 +144,6 @@
std::unordered_map<MetricDimensionKey, int64_t> mCurrentFullBucket;
// Save the past buckets and we can clear when the StatsLogReport is dumped.
- // TODO: Add a lock to mPastBuckets.
std::unordered_map<MetricDimensionKey, std::vector<ValueBucket>> mPastBuckets;
// Pairs of (elapsed start, elapsed end) denoting buckets that were skipped.
diff --git a/cmds/statsd/src/metrics/duration_helper/DurationTracker.h b/cmds/statsd/src/metrics/duration_helper/DurationTracker.h
index 149b318..ccb1d43 100644
--- a/cmds/statsd/src/metrics/duration_helper/DurationTracker.h
+++ b/cmds/statsd/src/metrics/duration_helper/DurationTracker.h
@@ -44,7 +44,6 @@
int64_t lastStartTime;
// existing duration in current bucket.
int64_t lastDuration;
- // TODO: Optimize the way we track sliced condition in duration metrics.
// cache the HashableDimensionKeys we need to query the condition for this duration event.
ConditionKey conditionKeys;
diff --git a/cmds/statsd/src/metrics/duration_helper/OringDurationTracker.cpp b/cmds/statsd/src/metrics/duration_helper/OringDurationTracker.cpp
index b833dfc..956383a 100644
--- a/cmds/statsd/src/metrics/duration_helper/OringDurationTracker.cpp
+++ b/cmds/statsd/src/metrics/duration_helper/OringDurationTracker.cpp
@@ -326,7 +326,6 @@
int64_t OringDurationTracker::predictAnomalyTimestampNs(
const DurationAnomalyTracker& anomalyTracker, const int64_t eventTimestampNs) const {
- // TODO: Unit-test this and see if it can be done more efficiently (e.g. use int32).
// The anomaly threshold.
const int64_t thresholdNs = anomalyTracker.getAnomalyThreshold();
diff --git a/cmds/statsd/src/metrics/metrics_manager_util.cpp b/cmds/statsd/src/metrics/metrics_manager_util.cpp
index deb9893..e03edb3 100644
--- a/cmds/statsd/src/metrics/metrics_manager_util.cpp
+++ b/cmds/statsd/src/metrics/metrics_manager_util.cpp
@@ -103,7 +103,6 @@
}
allConditionTrackers[condition_it->second]->setSliced(true);
allConditionTrackers[it->second]->setSliced(true);
- // TODO: We need to verify the link is valid.
}
conditionIndex = condition_it->second;
@@ -169,7 +168,6 @@
bool isStateTracker(const SimplePredicate& simplePredicate, vector<Matcher>* primaryKeys) {
// 1. must not have "stop". must have "dimension"
if (!simplePredicate.has_stop() && simplePredicate.has_dimensions()) {
- // TODO: need to check the start atom matcher too.
auto it = android::util::AtomsInfo::kStateAtomsFieldOptions.find(
simplePredicate.dimensions().field());
// 2. must be based on a state atom.
diff --git a/cmds/statsd/src/packages/UidMap.h b/cmds/statsd/src/packages/UidMap.h
index 514eec7..91f2030 100644
--- a/cmds/statsd/src/packages/UidMap.h
+++ b/cmds/statsd/src/packages/UidMap.h
@@ -146,7 +146,6 @@
void getListenerListCopyLocked(std::vector<wp<PackageInfoListener>>* output);
- // TODO: Use shared_mutex for improved read-locking if a library can be found in Android.
mutable mutex mMutex;
mutable mutex mIsolatedMutex;
diff --git a/cmds/statsd/src/storage/StorageManager.cpp b/cmds/statsd/src/storage/StorageManager.cpp
index 1f81812..3ebc8a4 100644
--- a/cmds/statsd/src/storage/StorageManager.cpp
+++ b/cmds/statsd/src/storage/StorageManager.cpp
@@ -57,7 +57,7 @@
}
// When index ends before hitting 3, file name is corrupted. We
// intentionally put -1 at index 0 to indicate the error to caller.
- // TODO: consider removing files with unexpected name format.
+ // TODO(b/110563137): consider removing files with unexpected name format.
if (index < 3) {
result[0] = -1;
}