diff --git a/cmds/statsd/src/StatsLogProcessor.cpp b/cmds/statsd/src/StatsLogProcessor.cpp index 2690c7e43d79b..4779bc0b55afc 100644 --- a/cmds/statsd/src/StatsLogProcessor.cpp +++ b/cmds/statsd/src/StatsLogProcessor.cpp @@ -75,10 +75,8 @@ StatsLogProcessor::~StatsLogProcessor() { void StatsLogProcessor::onAnomalyAlarmFired( const uint64_t timestampNs, unordered_set, SpHash> 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 57928d07a568a..27e3854b91d62 100644 --- a/cmds/statsd/src/StatsLogProcessor.h +++ b/cmds/statsd/src/StatsLogProcessor.h @@ -44,6 +44,8 @@ public: size_t GetMetricsSize(const ConfigKey& key) const; void onDumpReport(const ConfigKey& key, vector* outData); + + /* Tells MetricsManager that the alarms in anomalySet have fired. Modifies anomalySet. */ void onAnomalyAlarmFired( const uint64_t timestampNs, unordered_set, SpHash> anomalySet); diff --git a/cmds/statsd/src/StatsService.cpp b/cmds/statsd/src/StatsService.cpp index 8b64f0db57283..31405b8eba33c 100644 --- a/cmds/statsd/src/StatsService.cpp +++ b/cmds/statsd/src/StatsService.cpp @@ -585,11 +585,10 @@ Status StatsService::informAnomalyAlarmFired() { } 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, SpHash> anomalySet = - mAnomalyMonitor->onAlarmFired(currentTimeNs); - mProcessor->onAnomalyAlarmFired(currentTimeNs, anomalySet); + mAnomalyMonitor->popSoonerThan(static_cast(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 da52a9d664944..2b2bcfc18fbfa 100644 --- a/cmds/statsd/src/anomaly/AnomalyMonitor.cpp +++ b/cmds/statsd/src/anomaly/AnomalyMonitor.cpp @@ -72,7 +72,8 @@ void AnomalyMonitor::remove(sp alarm) { 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 @@ int64_t AnomalyMonitor::secToMs(uint32_t timeSec) { return ((int64_t)timeSec) * 1000; } -unordered_set, SpHash> AnomalyMonitor::onAlarmFired( - uint64_t timestampNs) { - return popSoonerThan(static_cast(timestampNs)); -} - } // namespace statsd } // namespace os } // namespace android diff --git a/cmds/statsd/src/anomaly/AnomalyMonitor.h b/cmds/statsd/src/anomaly/AnomalyMonitor.h index 0bd5055631b60..e19c46929a198 100644 --- a/cmds/statsd/src/anomaly/AnomalyMonitor.h +++ b/cmds/statsd/src/anomaly/AnomalyMonitor.h @@ -23,7 +23,6 @@ #include #include -#include #include #include @@ -97,15 +96,6 @@ public: unordered_set, SpHash> 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 @@ public: return mRegisteredAlarmTimeSec; } - unordered_set, SpHash> 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 0904a04987e70..c2bf233c103b5 100644 --- a/cmds/statsd/src/anomaly/AnomalyTracker.cpp +++ b/cmds/statsd/src/anomaly/AnomalyTracker.cpp @@ -25,26 +25,23 @@ namespace android { 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 @@ AnomalyTracker::~AnomalyTracker() { 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 @@ void AnomalyTracker::flushPastBuckets(const int64_t& latestPastBucketNum) { // It is an update operation. if (latestPastBucketNum <= mMostRecentBucketNum && - latestPastBucketNum > mMostRecentBucketNum - mNumOfPastPackets) { + latestPastBucketNum > mMostRecentBucketNum - mNumOfPastBuckets) { subtractBucketFromSum(mPastBuckets[index(latestPastBucketNum)]); } } @@ -190,61 +186,79 @@ bool AnomalyTracker::detectAnomaly(const int64_t& currentBucketNum, const Hashab 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(timestamp / NS_PER_SEC) >= itr->second->timestampSec) { - declareAnomaly(timestamp); + static_cast(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 alarm = new AnomalyAlarm{static_cast(timestamp / NS_PER_SEC)}; + const uint64_t& timestampNs) { + uint32_t timestampSec = static_cast(timestampNs / NS_PER_SEC); + if (isInRefractoryPeriod(timestampNs)) { + VLOG("Skipping setting anomaly alarm since it'd fall in the refractory period"); + return; + } + + sp alarm = new AnomalyAlarm{timestampSec}; mAlarms.insert({dimensionKey, alarm}); if (mAnomalyMonitor != nullptr) { mAnomalyMonitor->add(alarm); @@ -255,9 +269,9 @@ void AnomalyTracker::stopAlarm(const HashableDimensionKey& dimensionKey) { 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 @@ void AnomalyTracker::stopAllAlarms() { } } +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, SpHash>& 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> 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 ce6c99518cda9..afa6fee99c6bc 100644 --- a/cmds/statsd/src/anomaly/AnomalyTracker.h +++ b/cmds/statsd/src/anomaly/AnomalyTracker.h @@ -32,10 +32,10 @@ namespace statsd { 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 @@ public: 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 @@ public: // 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 @@ public: 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, SpHash>& 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 @@ protected: // and remove any items with value 0. void subtractBucketFromSum(const shared_ptr& 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 4982d4b90b7df..99882d0337b19 100644 --- a/cmds/statsd/src/anomaly/indexed_priority_queue.h +++ b/cmds/statsd/src/anomaly/indexed_priority_queue.h @@ -37,7 +37,7 @@ struct SpHash { /** * 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 a, sp b)], returning * whether a should be closer to the top of the queue than b. */ template @@ -46,8 +46,11 @@ public: indexed_priority_queue(); /** Adds a into the priority queue. If already present or a==nullptr, does nothing. */ void push(sp a); - /** Removes a from the priority queue. If not present or a==nullptr, does nothing. */ - void remove(sp 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 a); /** Removes the top element, if there is one. */ void pop(); /** Removes all elements. */ @@ -97,17 +100,17 @@ void indexed_priority_queue::push(sp a) { } template -void indexed_priority_queue::remove(sp a) { - if (a == nullptr) return; - if (!contains(a)) return; +bool indexed_priority_queue::remove(sp 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 last_a = pq.back(); @@ -119,6 +122,8 @@ void indexed_priority_queue::remove(sp a) { // 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 ce60eb965d620..00642400b3e8f 100644 --- a/cmds/statsd/src/metrics/CountMetricProducer.cpp +++ b/cmds/statsd/src/metrics/CountMetricProducer.cpp @@ -62,8 +62,6 @@ const int FIELD_ID_START_BUCKET_NANOS = 1; 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& wizard, diff --git a/cmds/statsd/src/metrics/MetricsManager.cpp b/cmds/statsd/src/metrics/MetricsManager.cpp index c8669519119a1..e986c1ab2ddbe 100644 --- a/cmds/statsd/src/metrics/MetricsManager.cpp +++ b/cmds/statsd/src/metrics/MetricsManager.cpp @@ -168,9 +168,9 @@ void MetricsManager::onLogEvent(const LogEvent& event) { } void MetricsManager::onAnomalyAlarmFired(const uint64_t timestampNs, - sp anomaly) { + unordered_set, SpHash>& 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 517831d1b09d4..62b4c872b3184 100644 --- a/cmds/statsd/src/metrics/MetricsManager.h +++ b/cmds/statsd/src/metrics/MetricsManager.h @@ -46,7 +46,8 @@ public: // 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 anomaly); + void onAnomalyAlarmFired(const uint64_t timestampNs, + unordered_set, SpHash>& anomalySet); void setAnomalyMonitor(const sp& anomalyMonitor); diff --git a/cmds/statsd/src/metrics/duration_helper/OringDurationTracker.cpp b/cmds/statsd/src/metrics/duration_helper/OringDurationTracker.cpp index 22c33d60169e4..abdfbc06d7a71 100644 --- a/cmds/statsd/src/metrics/duration_helper/OringDurationTracker.cpp +++ b/cmds/statsd/src/metrics/duration_helper/OringDurationTracker.cpp @@ -293,12 +293,12 @@ int64_t OringDurationTracker::predictAnomalyTimestampNs(const AnomalyTracker& an 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 1b9efc51c7b77..da689b77f6ca1 100644 --- a/cmds/statsd/src/metrics/metrics_manager_util.cpp +++ b/cmds/statsd/src/metrics/metrics_manager_util.cpp @@ -444,9 +444,24 @@ bool initAlerts(const StatsdConfig& config, const unordered_map& me 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 = - 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 = 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 e0200f27316e0..65c2a05543b9b 100644 --- a/cmds/statsd/tests/anomaly/AnomalyTracker_test.cpp +++ b/cmds/statsd/tests/anomaly/AnomalyTracker_test.cpp @@ -51,7 +51,7 @@ TEST(AnomalyTrackerTest, TestConsecutiveBuckets) { 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 bucket0 = MockBucket({{"a", 1}, {"b", 2}, {"c", 1}}); int64_t eventTimestamp0 = 10; @@ -168,7 +168,7 @@ TEST(AnomalyTrackerTest, TestSparseBuckets) { 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 bucket9 = MockBucket({{"a", 1}, {"b", 2}, {"c", 1}}); std::shared_ptr bucket16 = MockBucket({{"b", 4}}); diff --git a/cmds/statsd/tests/metrics/CountMetricProducer_test.cpp b/cmds/statsd/tests/metrics/CountMetricProducer_test.cpp index df743644c3d54..2cbeaaa8601d0 100644 --- a/cmds/statsd/tests/metrics/CountMetricProducer_test.cpp +++ b/cmds/statsd/tests/metrics/CountMetricProducer_test.cpp @@ -196,7 +196,7 @@ TEST(CountMetricProducerTest, TestAnomalyDetection) { int64_t bucket2StartTimeNs = bucketStartTimeNs + bucketSizeNs; int64_t bucket3StartTimeNs = bucketStartTimeNs + 2 * bucketSizeNs; - sp anomalyTracker = new AnomalyTracker(alert, bucketSizeNs); + sp 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 b9e2b8a7752f8..85f50081ca1eb 100644 --- a/cmds/statsd/tests/metrics/GaugeMetricProducer_test.cpp +++ b/cmds/statsd/tests/metrics/GaugeMetricProducer_test.cpp @@ -148,7 +148,7 @@ TEST(GaugeMetricProducerTest, TestAnomalyDetection) { alert.set_metric_name("1"); alert.set_trigger_if_sum_gt(25); alert.set_number_of_buckets(2); - sp anomalyTracker = new AnomalyTracker(alert, bucketSizeNs); + sp anomalyTracker = new AnomalyTracker(alert); gaugeProducer.addAnomalyTracker(anomalyTracker); std::shared_ptr event1 = std::make_shared(1, bucketStartTimeNs + 1); diff --git a/cmds/statsd/tests/metrics/MaxDurationTracker_test.cpp b/cmds/statsd/tests/metrics/MaxDurationTracker_test.cpp index 0763c944c5561..5d47437c486c7 100644 --- a/cmds/statsd/tests/metrics/MaxDurationTracker_test.cpp +++ b/cmds/statsd/tests/metrics/MaxDurationTracker_test.cpp @@ -201,7 +201,7 @@ TEST(MaxDurationTrackerTest, TestAnomalyDetection) { uint64_t eventStartTimeNs = bucketStartTimeNs + NS_PER_SEC + 1; uint64_t bucketSizeNs = 30 * NS_PER_SEC; - sp anomalyTracker = new AnomalyTracker(alert, bucketSizeNs); + sp 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 63a0e2b3499fa..6913c81613455 100644 --- a/cmds/statsd/tests/metrics/OringDurationTracker_test.cpp +++ b/cmds/statsd/tests/metrics/OringDurationTracker_test.cpp @@ -260,7 +260,7 @@ TEST(OringDurationTrackerTest, TestPredictAnomalyTimestamp) { uint64_t eventStartTimeNs = bucketStartTimeNs + NS_PER_SEC + 1; uint64_t bucketSizeNs = 30 * NS_PER_SEC; - sp anomalyTracker = new AnomalyTracker(alert, bucketSizeNs); + sp anomalyTracker = new AnomalyTracker(alert); OringDurationTracker tracker(kConfigKey, "metric", "event", wizard, 1, true, bucketStartTimeNs, bucketSizeNs, {anomalyTracker}, buckets); @@ -320,7 +320,7 @@ TEST(OringDurationTrackerTest, TestAnomalyDetection) { uint64_t eventStartTimeNs = bucketStartTimeNs + NS_PER_SEC + 1; uint64_t bucketSizeNs = 30 * NS_PER_SEC; - sp anomalyTracker = new AnomalyTracker(alert, bucketSizeNs); + sp anomalyTracker = new AnomalyTracker(alert); OringDurationTracker tracker(kConfigKey, "metric", "event", wizard, 1, true /*nesting*/, bucketStartTimeNs, bucketSizeNs, {anomalyTracker}, buckets);