Merge "Statsd anomaly detection - fixes"
diff --git a/cmds/statsd/src/StatsLogProcessor.cpp b/cmds/statsd/src/StatsLogProcessor.cpp
index 2690c7e..4779bc0 100644
--- a/cmds/statsd/src/StatsLogProcessor.cpp
+++ b/cmds/statsd/src/StatsLogProcessor.cpp
@@ -75,10 +75,8 @@
void StatsLogProcessor::onAnomalyAlarmFired(
const uint64_t timestampNs,
unordered_set<sp<const AnomalyAlarm>, SpHash<AnomalyAlarm>> anomalySet) {
- for (const auto& anomaly : anomalySet) {
- for (const auto& itr : mMetricsManagers) {
- itr.second->onAnomalyAlarmFired(timestampNs, anomaly);
- }
+ for (const auto& itr : mMetricsManagers) {
+ itr.second->onAnomalyAlarmFired(timestampNs, anomalySet);
}
}
diff --git a/cmds/statsd/src/StatsLogProcessor.h b/cmds/statsd/src/StatsLogProcessor.h
index 57928d0..27e3854 100644
--- a/cmds/statsd/src/StatsLogProcessor.h
+++ b/cmds/statsd/src/StatsLogProcessor.h
@@ -44,6 +44,8 @@
size_t GetMetricsSize(const ConfigKey& key) const;
void onDumpReport(const ConfigKey& key, vector<uint8_t>* outData);
+
+ /* Tells MetricsManager that the alarms in anomalySet have fired. Modifies anomalySet. */
void onAnomalyAlarmFired(
const uint64_t timestampNs,
unordered_set<sp<const AnomalyAlarm>, SpHash<AnomalyAlarm>> anomalySet);
diff --git a/cmds/statsd/src/StatsService.cpp b/cmds/statsd/src/StatsService.cpp
index 8b64f0d..31405b8 100644
--- a/cmds/statsd/src/StatsService.cpp
+++ b/cmds/statsd/src/StatsService.cpp
@@ -585,11 +585,10 @@
}
if (DEBUG) ALOGD("StatsService::informAnomalyAlarmFired succeeded");
- // TODO: check through all counters/timers and see if an anomaly has indeed occurred.
- uint64_t currentTimeNs = time(nullptr) * NS_PER_SEC;
+ uint64_t currentTimeSec = time(nullptr);
std::unordered_set<sp<const AnomalyAlarm>, SpHash<AnomalyAlarm>> anomalySet =
- mAnomalyMonitor->onAlarmFired(currentTimeNs);
- mProcessor->onAnomalyAlarmFired(currentTimeNs, anomalySet);
+ mAnomalyMonitor->popSoonerThan(static_cast<uint32_t>(currentTimeSec));
+ mProcessor->onAnomalyAlarmFired(currentTimeSec * NS_PER_SEC, anomalySet);
return Status::ok();
}
diff --git a/cmds/statsd/src/anomaly/AnomalyMonitor.cpp b/cmds/statsd/src/anomaly/AnomalyMonitor.cpp
index da52a9d..2b2bcfc 100644
--- a/cmds/statsd/src/anomaly/AnomalyMonitor.cpp
+++ b/cmds/statsd/src/anomaly/AnomalyMonitor.cpp
@@ -72,7 +72,8 @@
return;
}
if (DEBUG) ALOGD("Removing alarm with time %u", alarm->timestampSec);
- mPq.remove(alarm);
+ bool wasPresent = mPq.remove(alarm);
+ if (!wasPresent) return;
if (mPq.empty()) {
if (DEBUG) ALOGD("Queue is empty. Cancel any alarm.");
mRegisteredAlarmTimeSec = 0;
@@ -129,11 +130,6 @@
return ((int64_t)timeSec) * 1000;
}
-unordered_set<sp<const AnomalyAlarm>, SpHash<AnomalyAlarm>> AnomalyMonitor::onAlarmFired(
- uint64_t timestampNs) {
- return popSoonerThan(static_cast<uint32_t>(timestampNs));
-}
-
} // namespace statsd
} // namespace os
} // namespace android
diff --git a/cmds/statsd/src/anomaly/AnomalyMonitor.h b/cmds/statsd/src/anomaly/AnomalyMonitor.h
index 0bd5055..e19c469 100644
--- a/cmds/statsd/src/anomaly/AnomalyMonitor.h
+++ b/cmds/statsd/src/anomaly/AnomalyMonitor.h
@@ -23,7 +23,6 @@
#include <queue>
#include <set>
-#include <unordered_map>
#include <unordered_set>
#include <vector>
@@ -97,15 +96,6 @@
unordered_set<sp<const AnomalyAlarm>, SpHash<AnomalyAlarm>> popSoonerThan(
uint32_t timestampSec);
- // TODO: Function that uses popSoonerThan to get all alarms that have fired, and then
- // iterates over all DurationAnomalyTracker, looking for those alarms. When they're found,
- // have them declareAnomaly on those alarms. This means that DurationAnomalyTracker
- // must be thread-safe (since this is being called on a different thread). There is no
- // worry about missing the alarms (due to them being cancelled after this function being called)
- // because DurationAnomalyTracker guarantees that it checks for anaomlies when it cancels
- // alarms anyway.
- // void declareAnomalies(uint32_t timestampSec);
-
/**
* Returns the projected alarm timestamp that is registered with
* StatsCompanionService. This may not be equal to the soonest alarm,
@@ -115,8 +105,6 @@
return mRegisteredAlarmTimeSec;
}
- unordered_set<sp<const AnomalyAlarm>, SpHash<AnomalyAlarm>> onAlarmFired(uint64_t timestampNs);
-
private:
std::mutex mLock;
diff --git a/cmds/statsd/src/anomaly/AnomalyTracker.cpp b/cmds/statsd/src/anomaly/AnomalyTracker.cpp
index 0904a04..c2bf233 100644
--- a/cmds/statsd/src/anomaly/AnomalyTracker.cpp
+++ b/cmds/statsd/src/anomaly/AnomalyTracker.cpp
@@ -25,26 +25,23 @@
namespace os {
namespace statsd {
-AnomalyTracker::AnomalyTracker(const Alert& alert, const int64_t& bucketSizeNs)
+// TODO: Separate DurationAnomalyTracker as a separate subclass and let each MetricProducer
+// decide and let which one it wants.
+// TODO: Get rid of bucketNumbers, and return to the original circular array method.
+AnomalyTracker::AnomalyTracker(const Alert& alert)
: mAlert(alert),
- mBucketSizeNs(bucketSizeNs),
- mNumOfPastPackets(mAlert.number_of_buckets() - 1) {
+ mNumOfPastBuckets(mAlert.number_of_buckets() - 1) {
VLOG("AnomalyTracker() called");
if (mAlert.number_of_buckets() <= 0) {
- ALOGE("Cannot create DiscreteAnomalyTracker with %lld buckets",
+ ALOGE("Cannot create AnomalyTracker with %lld buckets",
(long long)mAlert.number_of_buckets());
return;
}
- if (mBucketSizeNs <= 0) {
- ALOGE("Cannot create DiscreteAnomalyTracker with bucket size %lld ",
- (long long)mBucketSizeNs);
- return;
- }
if (!mAlert.has_trigger_if_sum_gt()) {
- ALOGE("Cannot create DiscreteAnomalyTracker without threshold");
+ ALOGE("Cannot create AnomalyTracker without threshold");
return;
}
- reset(); // initialization
+ resetStorage(); // initialization
}
AnomalyTracker::~AnomalyTracker() {
@@ -52,37 +49,36 @@
stopAllAlarms();
}
-void AnomalyTracker::reset() {
- VLOG("reset() called.");
- stopAllAlarms();
+void AnomalyTracker::resetStorage() {
+ VLOG("resetStorage() called.");
mPastBuckets.clear();
// Excludes the current bucket.
- mPastBuckets.resize(mNumOfPastPackets);
+ mPastBuckets.resize(mNumOfPastBuckets);
mSumOverPastBuckets.clear();
- mMostRecentBucketNum = -1;
- mLastAlarmTimestampNs = -1;
+
+ if (!mAlarms.empty()) VLOG("AnomalyTracker.resetStorage() called but mAlarms is NOT empty!");
}
size_t AnomalyTracker::index(int64_t bucketNum) const {
- return bucketNum % mNumOfPastPackets;
+ return bucketNum % mNumOfPastBuckets;
}
void AnomalyTracker::flushPastBuckets(const int64_t& latestPastBucketNum) {
VLOG("addPastBucket() called.");
- if (latestPastBucketNum <= mMostRecentBucketNum - mNumOfPastPackets) {
+ if (latestPastBucketNum <= mMostRecentBucketNum - mNumOfPastBuckets) {
ALOGE("Cannot add a past bucket %lld units in past", (long long)latestPastBucketNum);
return;
}
// The past packets are ancient. Empty out old mPastBuckets[i] values and reset
// mSumOverPastBuckets.
- if (latestPastBucketNum - mMostRecentBucketNum >= mNumOfPastPackets) {
+ if (latestPastBucketNum - mMostRecentBucketNum >= mNumOfPastBuckets) {
mPastBuckets.clear();
- mPastBuckets.resize(mNumOfPastPackets);
+ mPastBuckets.resize(mNumOfPastBuckets);
mSumOverPastBuckets.clear();
} else {
- for (int64_t i = std::max(0LL, (long long)(mMostRecentBucketNum - mNumOfPastPackets + 1));
- i <= latestPastBucketNum - mNumOfPastPackets; i++) {
+ for (int64_t i = std::max(0LL, (long long)(mMostRecentBucketNum - mNumOfPastBuckets + 1));
+ i <= latestPastBucketNum - mNumOfPastBuckets; i++) {
const int idx = index(i);
subtractBucketFromSum(mPastBuckets[idx]);
mPastBuckets[idx] = nullptr; // release (but not clear) the old bucket.
@@ -91,7 +87,7 @@
// It is an update operation.
if (latestPastBucketNum <= mMostRecentBucketNum &&
- latestPastBucketNum > mMostRecentBucketNum - mNumOfPastPackets) {
+ latestPastBucketNum > mMostRecentBucketNum - mNumOfPastBuckets) {
subtractBucketFromSum(mPastBuckets[index(latestPastBucketNum)]);
}
}
@@ -190,61 +186,79 @@
if (currentBucketNum > mMostRecentBucketNum + 1) {
addPastBucket(key, 0, currentBucketNum - 1);
}
- return getSumOverPastBuckets(key) + currentBucketValue > mAlert.trigger_if_sum_gt();
+ return mAlert.has_trigger_if_sum_gt()
+ && getSumOverPastBuckets(key) + currentBucketValue > mAlert.trigger_if_sum_gt();
}
-void AnomalyTracker::declareAnomaly(const uint64_t& timestamp) {
- if (mLastAlarmTimestampNs >= 0 &&
- timestamp - mLastAlarmTimestampNs <= mAlert.refractory_period_secs() * NS_PER_SEC) {
- VLOG("Skipping anomaly check since within refractory period");
+void AnomalyTracker::declareAnomaly(const uint64_t& timestampNs) {
+ // TODO: This should also take in the const HashableDimensionKey& key, to pass
+ // more details to incidentd and to make mRefractoryPeriodEndsSec key-specific.
+ // TODO: Why receive timestamp? RefractoryPeriod should always be based on real time right now.
+ if (isInRefractoryPeriod(timestampNs)) {
+ VLOG("Skipping anomaly declaration since within refractory period");
return;
}
// TODO(guardrail): Consider guarding against too short refractory periods.
- mLastAlarmTimestampNs = timestamp;
+ mLastAlarmTimestampNs = timestampNs;
+
+
+ // TODO: If we had access to the bucket_size_millis, consider calling resetStorage()
+ // if (mAlert.refractory_period_secs() > mNumOfPastBuckets * bucketSizeNs) { resetStorage(); }
if (mAlert.has_incidentd_details()) {
- // TODO: Can construct a name based on the criteria (and/or relay the criteria).
- ALOGW("An anomaly (nameless) has occurred! Informing incidentd.");
- // TODO: Send incidentd_details.name and incidentd_details.incidentd_sections to incidentd
+ if (mAlert.has_name()) {
+ ALOGW("An anomaly (%s) has occurred! Informing incidentd.",
+ mAlert.name().c_str());
+ } else {
+ // TODO: Can construct a name based on the criteria (and/or relay the criteria).
+ ALOGW("An anomaly (nameless) has occurred! Informing incidentd.");
+ }
+ // TODO: informIncidentd();
} else {
ALOGW("An anomaly has occurred! (But informing incidentd not requested.)");
}
}
void AnomalyTracker::declareAnomalyIfAlarmExpired(const HashableDimensionKey& dimensionKey,
- const uint64_t& timestamp) {
+ const uint64_t& timestampNs) {
auto itr = mAlarms.find(dimensionKey);
if (itr == mAlarms.end()) {
return;
}
if (itr->second != nullptr &&
- static_cast<uint32_t>(timestamp / NS_PER_SEC) >= itr->second->timestampSec) {
- declareAnomaly(timestamp);
+ static_cast<uint32_t>(timestampNs / NS_PER_SEC) >= itr->second->timestampSec) {
+ declareAnomaly(timestampNs);
stopAlarm(dimensionKey);
}
}
-void AnomalyTracker::detectAndDeclareAnomaly(const uint64_t& timestamp,
+void AnomalyTracker::detectAndDeclareAnomaly(const uint64_t& timestampNs,
const int64_t& currBucketNum,
const HashableDimensionKey& key,
const int64_t& currentBucketValue) {
if (detectAnomaly(currBucketNum, key, currentBucketValue)) {
- declareAnomaly(timestamp);
+ declareAnomaly(timestampNs);
}
}
-void AnomalyTracker::detectAndDeclareAnomaly(const uint64_t& timestamp,
+void AnomalyTracker::detectAndDeclareAnomaly(const uint64_t& timestampNs,
const int64_t& currBucketNum,
const DimToValMap& currentBucket) {
if (detectAnomaly(currBucketNum, currentBucket)) {
- declareAnomaly(timestamp);
+ declareAnomaly(timestampNs);
}
}
void AnomalyTracker::startAlarm(const HashableDimensionKey& dimensionKey,
- const uint64_t& timestamp) {
- sp<const AnomalyAlarm> alarm = new AnomalyAlarm{static_cast<uint32_t>(timestamp / NS_PER_SEC)};
+ const uint64_t& timestampNs) {
+ uint32_t timestampSec = static_cast<uint32_t>(timestampNs / NS_PER_SEC);
+ if (isInRefractoryPeriod(timestampNs)) {
+ VLOG("Skipping setting anomaly alarm since it'd fall in the refractory period");
+ return;
+ }
+
+ sp<const AnomalyAlarm> alarm = new AnomalyAlarm{timestampSec};
mAlarms.insert({dimensionKey, alarm});
if (mAnomalyMonitor != nullptr) {
mAnomalyMonitor->add(alarm);
@@ -255,9 +269,9 @@
auto itr = mAlarms.find(dimensionKey);
if (itr != mAlarms.end()) {
mAlarms.erase(dimensionKey);
- }
- if (mAnomalyMonitor != nullptr) {
- mAnomalyMonitor->remove(itr->second);
+ if (mAnomalyMonitor != nullptr) {
+ mAnomalyMonitor->remove(itr->second);
+ }
}
}
@@ -271,6 +285,35 @@
}
}
+bool AnomalyTracker::isInRefractoryPeriod(const uint64_t& timestampNs) {
+ return mLastAlarmTimestampNs >= 0 &&
+ timestampNs - mLastAlarmTimestampNs <= mAlert.refractory_period_secs() * NS_PER_SEC;
+}
+
+void AnomalyTracker::informAlarmsFired(const uint64_t& timestampNs,
+ unordered_set<sp<const AnomalyAlarm>, SpHash<AnomalyAlarm>>& firedAlarms) {
+
+ if (firedAlarms.empty() || mAlarms.empty()) return;
+ // Find the intersection of firedAlarms and mAlarms.
+ // The for loop is inefficient, since it loops over all keys, but that's okay since it is very
+ // seldomly called. The alternative would be having AnomalyAlarms store information about the
+ // AnomalyTracker and key, but that's a lot of data overhead to speed up something that is
+ // rarely ever called.
+ unordered_map<HashableDimensionKey, sp<const AnomalyAlarm>> matchedAlarms;
+ for (const auto& kv : mAlarms) {
+ if (firedAlarms.count(kv.second) > 0) {
+ matchedAlarms.insert({kv.first, kv.second});
+ }
+ }
+
+ // Now declare each of these alarms to have fired.
+ for (const auto& kv : matchedAlarms) {
+ declareAnomaly(timestampNs /* TODO: , kv.first */);
+ mAlarms.erase(kv.first);
+ firedAlarms.erase(kv.second); // No one else can also own it, so we're done with it.
+ }
+}
+
} // namespace statsd
} // namespace os
} // namespace android
diff --git a/cmds/statsd/src/anomaly/AnomalyTracker.h b/cmds/statsd/src/anomaly/AnomalyTracker.h
index ce6c995..afa6fee 100644
--- a/cmds/statsd/src/anomaly/AnomalyTracker.h
+++ b/cmds/statsd/src/anomaly/AnomalyTracker.h
@@ -32,10 +32,10 @@
using std::unordered_map;
using std::shared_ptr;
-// This anomaly track assmues that all values are non-negative.
+// Does NOT allow negative values.
class AnomalyTracker : public virtual RefBase {
public:
- AnomalyTracker(const Alert& alert, const int64_t& bucketSizeNs);
+ AnomalyTracker(const Alert& alert);
virtual ~AnomalyTracker();
@@ -51,12 +51,12 @@
const int64_t& currentBucketValue);
// Informs incidentd about the detected alert.
- void declareAnomaly(const uint64_t& timestamp);
+ void declareAnomaly(const uint64_t& timestampNs);
// Detects the alert and informs the incidentd when applicable.
- void detectAndDeclareAnomaly(const uint64_t& timestamp, const int64_t& currBucketNum,
+ void detectAndDeclareAnomaly(const uint64_t& timestampNs, const int64_t& currBucketNum,
const DimToValMap& currentBucket);
- void detectAndDeclareAnomaly(const uint64_t& timestamp, const int64_t& currBucketNum,
+ void detectAndDeclareAnomaly(const uint64_t& timestampNs, const int64_t& currBucketNum,
const HashableDimensionKey& key,
const int64_t& currentBucketValue);
@@ -75,7 +75,7 @@
// Declares the anomaly when the alarm expired given the current timestamp.
void declareAnomalyIfAlarmExpired(const HashableDimensionKey& dimensionKey,
- const uint64_t& timestamp);
+ const uint64_t& timestampNs);
// Helper function to return the sum value of past buckets at given dimension.
int64_t getSumOverPastBuckets(const HashableDimensionKey& key) const;
@@ -93,20 +93,26 @@
return mLastAlarmTimestampNs;
}
- inline int getNumOfPastPackets() const {
- return mNumOfPastPackets;
+ inline int getNumOfPastBuckets() const {
+ return mNumOfPastBuckets;
}
+ // Declares an anomaly for each alarm in firedAlarms that belongs to this AnomalyTracker,
+ // and removes it from firedAlarms. Does NOT remove the alarm from the AnomalyMonitor.
+ // TODO: This will actually be called from a different thread, so make it thread-safe!
+ // TODO: Consider having AnomalyMonitor have a reference to each relevant MetricProducer
+ // instead of calling it from a chain starting at StatsLogProcessor.
+ void informAlarmsFired(const uint64_t& timestampNs,
+ unordered_set<sp<const AnomalyAlarm>, SpHash<AnomalyAlarm>>& firedAlarms);
+
protected:
void flushPastBuckets(const int64_t& currBucketNum);
// statsd_config.proto Alert message that defines this tracker.
const Alert mAlert;
- // Bucket duration in ns.
- int64_t mBucketSizeNs = 0;
-
- // The number of past packets to track in the anomaly detection.
- int mNumOfPastPackets = 0;
+ // Number of past buckets. One less than the total number of buckets needed
+ // for the anomaly detection (since the current bucket is not in the past).
+ int mNumOfPastBuckets;
// The alarms owned by this tracker. The alarm monitor also shares the alarm pointers when they
// are still active.
@@ -134,11 +140,13 @@
// and remove any items with value 0.
void subtractBucketFromSum(const shared_ptr<DimToValMap>& bucket);
+ bool isInRefractoryPeriod(const uint64_t& timestampNs);
+
// Calculates the corresponding bucket index within the circular array.
size_t index(int64_t bucketNum) const;
- // Resets all data. For use when all the data gets stale.
- void reset();
+ // Resets all bucket data. For use when all the data gets stale.
+ void resetStorage();
FRIEND_TEST(AnomalyTrackerTest, TestConsecutiveBuckets);
FRIEND_TEST(AnomalyTrackerTest, TestSparseBuckets);
diff --git a/cmds/statsd/src/anomaly/indexed_priority_queue.h b/cmds/statsd/src/anomaly/indexed_priority_queue.h
index 4982d4b..99882d0 100644
--- a/cmds/statsd/src/anomaly/indexed_priority_queue.h
+++ b/cmds/statsd/src/anomaly/indexed_priority_queue.h
@@ -37,7 +37,7 @@
/**
* Min priority queue for generic type AA.
* Unlike a regular priority queue, this class is also capable of removing interior elements.
- * @tparam Comparator must implement [bool operator()(sp< AA> a, sp< AA> b)], returning
+ * @tparam Comparator must implement [bool operator()(sp<const AA> a, sp<const AA> b)], returning
* whether a should be closer to the top of the queue than b.
*/
template <class AA, class Comparator>
@@ -46,8 +46,11 @@
indexed_priority_queue();
/** Adds a into the priority queue. If already present or a==nullptr, does nothing. */
void push(sp<const AA> a);
- /** Removes a from the priority queue. If not present or a==nullptr, does nothing. */
- void remove(sp<const AA> a);
+ /*
+ * Removes a from the priority queue. If not present or a==nullptr, does nothing.
+ * Returns true if a had been present (and is now removed), else false.
+ */
+ bool remove(sp<const AA> a);
/** Removes the top element, if there is one. */
void pop();
/** Removes all elements. */
@@ -97,17 +100,17 @@
}
template <class AA, class Comparator>
-void indexed_priority_queue<AA, Comparator>::remove(sp<const AA> a) {
- if (a == nullptr) return;
- if (!contains(a)) return;
+bool indexed_priority_queue<AA, Comparator>::remove(sp<const AA> a) {
+ if (a == nullptr) return false;
+ if (!contains(a)) return false;
size_t idx = indices[a];
if (idx >= pq.size()) {
- return;
+ return false;
}
if (idx == size()) { // if a is the last element, i.e. at index idx == size() == (pq.size()-1)
pq.pop_back();
indices.erase(a);
- return;
+ return true;
}
// move last element (guaranteed not to be at idx) to idx, then delete a
sp<const AA> last_a = pq.back();
@@ -119,6 +122,8 @@
// get the heap back in order (since the element at idx is not in order)
sift_up(idx);
sift_down(idx);
+
+ return true;
}
// The same as, but slightly more efficient than, remove(top()).
diff --git a/cmds/statsd/src/metrics/CountMetricProducer.cpp b/cmds/statsd/src/metrics/CountMetricProducer.cpp
index ce60eb9..0064240 100644
--- a/cmds/statsd/src/metrics/CountMetricProducer.cpp
+++ b/cmds/statsd/src/metrics/CountMetricProducer.cpp
@@ -62,8 +62,6 @@
const int FIELD_ID_END_BUCKET_NANOS = 2;
const int FIELD_ID_COUNT = 3;
-// TODO: add back AnomalyTracker.
-
CountMetricProducer::CountMetricProducer(const ConfigKey& key, const CountMetric& metric,
const int conditionIndex,
const sp<ConditionWizard>& wizard,
diff --git a/cmds/statsd/src/metrics/MetricsManager.cpp b/cmds/statsd/src/metrics/MetricsManager.cpp
index c866951..e986c1a 100644
--- a/cmds/statsd/src/metrics/MetricsManager.cpp
+++ b/cmds/statsd/src/metrics/MetricsManager.cpp
@@ -168,9 +168,9 @@
}
void MetricsManager::onAnomalyAlarmFired(const uint64_t timestampNs,
- sp<const AnomalyAlarm> anomaly) {
+ unordered_set<sp<const AnomalyAlarm>, SpHash<AnomalyAlarm>>& anomalySet) {
for (const auto& itr : mAllAnomalyTrackers) {
- itr->declareAnomaly(timestampNs);
+ itr->informAlarmsFired(timestampNs, anomalySet);
}
}
diff --git a/cmds/statsd/src/metrics/MetricsManager.h b/cmds/statsd/src/metrics/MetricsManager.h
index 517831d..62b4c87 100644
--- a/cmds/statsd/src/metrics/MetricsManager.h
+++ b/cmds/statsd/src/metrics/MetricsManager.h
@@ -46,7 +46,8 @@
// Called when everything should wrap up. We are about to finish (e.g., new config comes).
void finish();
- void onAnomalyAlarmFired(const uint64_t timestampNs, sp<const AnomalyAlarm> anomaly);
+ void onAnomalyAlarmFired(const uint64_t timestampNs,
+ unordered_set<sp<const AnomalyAlarm>, SpHash<AnomalyAlarm>>& anomalySet);
void setAnomalyMonitor(const sp<AnomalyMonitor>& anomalyMonitor);
diff --git a/cmds/statsd/src/metrics/duration_helper/OringDurationTracker.cpp b/cmds/statsd/src/metrics/duration_helper/OringDurationTracker.cpp
index 22c33d6..abdfbc0 100644
--- a/cmds/statsd/src/metrics/duration_helper/OringDurationTracker.cpp
+++ b/cmds/statsd/src/metrics/duration_helper/OringDurationTracker.cpp
@@ -293,12 +293,12 @@
pastNs += currRemainingBucketSizeNs;
// Now deal with the past buckets, starting with the oldest.
- for (int futBucketIdx = 0; futBucketIdx < anomalyTracker.getNumOfPastPackets();
+ for (int futBucketIdx = 0; futBucketIdx < anomalyTracker.getNumOfPastBuckets();
futBucketIdx++) {
// We now overwrite the oldest bucket with the previous 'current', and start a new
// 'current'.
pastNs -= anomalyTracker.getPastBucketValue(
- mEventKey, mCurrentBucketNum - anomalyTracker.getNumOfPastPackets() + futBucketIdx);
+ mEventKey, mCurrentBucketNum - anomalyTracker.getNumOfPastBuckets() + futBucketIdx);
leftNs = thresholdNs - pastNs;
if (leftNs <= mBucketSizeNs) { // Predict anomaly will occur in this bucket.
return eventTimestampNs + currRemainingBucketSizeNs + (futBucketIdx * mBucketSizeNs) +
diff --git a/cmds/statsd/src/metrics/metrics_manager_util.cpp b/cmds/statsd/src/metrics/metrics_manager_util.cpp
index fe3082b..9e5163f 100644
--- a/cmds/statsd/src/metrics/metrics_manager_util.cpp
+++ b/cmds/statsd/src/metrics/metrics_manager_util.cpp
@@ -462,9 +462,24 @@
alert.metric_name().c_str());
return false;
}
+ if (alert.trigger_if_sum_gt() < 0 || alert.number_of_buckets() <= 0) {
+ ALOGW("invalid alert: threshold=%lld num_buckets= %d",
+ alert.trigger_if_sum_gt(), alert.number_of_buckets());
+ return false;
+ }
const int metricIndex = itr->second;
- sp<AnomalyTracker> anomalyTracker =
- new AnomalyTracker(alert, allMetricProducers[metricIndex]->getBuckeSizeInNs());
+ if (alert.trigger_if_sum_gt() >
+ (int64_t) alert.number_of_buckets()
+ * allMetricProducers[metricIndex]->getBuckeSizeInNs()) {
+ ALOGW("invalid alert: threshold (%lld) > possible recordable value (%d x %lld)",
+ alert.trigger_if_sum_gt(), alert.number_of_buckets(),
+ (long long) allMetricProducers[metricIndex]->getBuckeSizeInNs());
+ return false;
+ }
+
+ // TODO: Give each MetricProducer a method called createAnomalyTracker(alert), which
+ // creates either an AnomalyTracker or a DurationAnomalyTracker and returns it.
+ sp<AnomalyTracker> anomalyTracker = new AnomalyTracker(alert);
allMetricProducers[metricIndex]->addAnomalyTracker(anomalyTracker);
allAnomalyTrackers.push_back(anomalyTracker);
}
diff --git a/cmds/statsd/tests/anomaly/AnomalyTracker_test.cpp b/cmds/statsd/tests/anomaly/AnomalyTracker_test.cpp
index e0200f27..65c2a05 100644
--- a/cmds/statsd/tests/anomaly/AnomalyTracker_test.cpp
+++ b/cmds/statsd/tests/anomaly/AnomalyTracker_test.cpp
@@ -51,7 +51,7 @@
alert.set_refractory_period_secs(2 * bucketSizeNs / NS_PER_SEC);
alert.set_trigger_if_sum_gt(2);
- AnomalyTracker anomalyTracker(alert, bucketSizeNs);
+ AnomalyTracker anomalyTracker(alert);
std::shared_ptr<DimToValMap> bucket0 = MockBucket({{"a", 1}, {"b", 2}, {"c", 1}});
int64_t eventTimestamp0 = 10;
@@ -168,7 +168,7 @@
alert.set_refractory_period_secs(2 * bucketSizeNs / NS_PER_SEC);
alert.set_trigger_if_sum_gt(2);
- AnomalyTracker anomalyTracker(alert, bucketSizeNs);
+ AnomalyTracker anomalyTracker(alert);
std::shared_ptr<DimToValMap> bucket9 = MockBucket({{"a", 1}, {"b", 2}, {"c", 1}});
std::shared_ptr<DimToValMap> bucket16 = MockBucket({{"b", 4}});
diff --git a/cmds/statsd/tests/metrics/CountMetricProducer_test.cpp b/cmds/statsd/tests/metrics/CountMetricProducer_test.cpp
index df74364..2cbeaaa 100644
--- a/cmds/statsd/tests/metrics/CountMetricProducer_test.cpp
+++ b/cmds/statsd/tests/metrics/CountMetricProducer_test.cpp
@@ -196,7 +196,7 @@
int64_t bucket2StartTimeNs = bucketStartTimeNs + bucketSizeNs;
int64_t bucket3StartTimeNs = bucketStartTimeNs + 2 * bucketSizeNs;
- sp<AnomalyTracker> anomalyTracker = new AnomalyTracker(alert, bucketSizeNs);
+ sp<AnomalyTracker> anomalyTracker = new AnomalyTracker(alert);
CountMetric metric;
metric.set_name("1");
diff --git a/cmds/statsd/tests/metrics/GaugeMetricProducer_test.cpp b/cmds/statsd/tests/metrics/GaugeMetricProducer_test.cpp
index b9e2b8a..85f5008 100644
--- a/cmds/statsd/tests/metrics/GaugeMetricProducer_test.cpp
+++ b/cmds/statsd/tests/metrics/GaugeMetricProducer_test.cpp
@@ -148,7 +148,7 @@
alert.set_metric_name("1");
alert.set_trigger_if_sum_gt(25);
alert.set_number_of_buckets(2);
- sp<AnomalyTracker> anomalyTracker = new AnomalyTracker(alert, bucketSizeNs);
+ sp<AnomalyTracker> anomalyTracker = new AnomalyTracker(alert);
gaugeProducer.addAnomalyTracker(anomalyTracker);
std::shared_ptr<LogEvent> event1 = std::make_shared<LogEvent>(1, bucketStartTimeNs + 1);
diff --git a/cmds/statsd/tests/metrics/MaxDurationTracker_test.cpp b/cmds/statsd/tests/metrics/MaxDurationTracker_test.cpp
index 0763c94..5d47437 100644
--- a/cmds/statsd/tests/metrics/MaxDurationTracker_test.cpp
+++ b/cmds/statsd/tests/metrics/MaxDurationTracker_test.cpp
@@ -201,7 +201,7 @@
uint64_t eventStartTimeNs = bucketStartTimeNs + NS_PER_SEC + 1;
uint64_t bucketSizeNs = 30 * NS_PER_SEC;
- sp<AnomalyTracker> anomalyTracker = new AnomalyTracker(alert, bucketSizeNs);
+ sp<AnomalyTracker> anomalyTracker = new AnomalyTracker(alert);
MaxDurationTracker tracker(kConfigKey, "metric", "event", wizard, -1, true, bucketStartTimeNs,
bucketSizeNs, {anomalyTracker}, buckets);
diff --git a/cmds/statsd/tests/metrics/OringDurationTracker_test.cpp b/cmds/statsd/tests/metrics/OringDurationTracker_test.cpp
index 63a0e2b..6913c81 100644
--- a/cmds/statsd/tests/metrics/OringDurationTracker_test.cpp
+++ b/cmds/statsd/tests/metrics/OringDurationTracker_test.cpp
@@ -260,7 +260,7 @@
uint64_t eventStartTimeNs = bucketStartTimeNs + NS_PER_SEC + 1;
uint64_t bucketSizeNs = 30 * NS_PER_SEC;
- sp<AnomalyTracker> anomalyTracker = new AnomalyTracker(alert, bucketSizeNs);
+ sp<AnomalyTracker> anomalyTracker = new AnomalyTracker(alert);
OringDurationTracker tracker(kConfigKey, "metric", "event", wizard, 1, true, bucketStartTimeNs,
bucketSizeNs, {anomalyTracker}, buckets);
@@ -320,7 +320,7 @@
uint64_t eventStartTimeNs = bucketStartTimeNs + NS_PER_SEC + 1;
uint64_t bucketSizeNs = 30 * NS_PER_SEC;
- sp<AnomalyTracker> anomalyTracker = new AnomalyTracker(alert, bucketSizeNs);
+ sp<AnomalyTracker> anomalyTracker = new AnomalyTracker(alert);
OringDurationTracker tracker(kConfigKey, "metric", "event", wizard, 1, true /*nesting*/,
bucketStartTimeNs, bucketSizeNs, {anomalyTracker}, buckets);