Prevent incorrect alert triggering

Alerts were being incorrectly triggered in pulled value metrics because
a diffed value is always assumed to be over one bucket interval, which
may not be true when multiple buckets are skipped.

Bug: 159390273
Test: atest statsd_test
Change-Id: Iada3df54ebb30353572a14e0db86bed49c5515b0

cmds/statsd/src/metrics/ValueMetricProducer.cpp

@@ -733,6 +733,11 @@ bool getDoubleOrLong(const LogEvent& event, const Matcher& matcher, Value& ret)
     return false;
 }
 
+bool ValueMetricProducer::multipleBucketsSkipped(const int64_t numBucketsForward) {
+    // Skip buckets if this is a pulled metric or a pushed metric that is diffed.
+    return numBucketsForward > 1 && (mIsPulled || mUseDiff);
+}
+
 void ValueMetricProducer::onMatchedLogEventInternalLocked(
         const size_t matcherIndex, const MetricDimensionKey& eventKey,
         const ConditionKey& conditionKey, bool condition, const LogEvent& event,
@@ -910,8 +915,9 @@ void ValueMetricProducer::onMatchedLogEventInternalLocked(
         interval.sampleSize += 1;
     }
 
-    // Only trigger the tracker if all intervals are correct
+    // Only trigger the tracker if all intervals are correct and we have not skipped the bucket due
-    if (useAnomalyDetection) {
+    // to MULTIPLE_BUCKETS_SKIPPED.
+    if (useAnomalyDetection && !multipleBucketsSkipped(calcBucketsForwardCount(eventTimeNs))) {
         // TODO: propgate proper values down stream when anomaly support doubles
         long wholeBucketVal = intervals[0].value.long_value;
         auto prev = mCurrentFullBucket.find(eventKey);
@@ -961,9 +967,7 @@ void ValueMetricProducer::flushCurrentBucketLocked(const int64_t& eventTimeNs,
     int64_t bucketEndTime = fullBucketEndTimeNs;
     int64_t numBucketsForward = calcBucketsForwardCount(eventTimeNs);
 
-    // Skip buckets if this is a pulled metric or a pushed metric that is diffed.
+    if (multipleBucketsSkipped(numBucketsForward)) {
-    if (numBucketsForward > 1 && (mIsPulled || mUseDiff)) {
-
         VLOG("Skipping forward %lld buckets", (long long)numBucketsForward);
         StatsdStats::getInstance().noteSkippedForwardBuckets(mMetricId);
         // Something went wrong. Maybe the device was sleeping for a long time. It is better

cmds/statsd/src/metrics/ValueMetricProducer.h

@@ -219,6 +219,8 @@ private:
 
     void pullAndMatchEventsLocked(const int64_t timestampNs);
 
+    bool multipleBucketsSkipped(const int64_t numBucketsForward);
+
     void accumulateEvents(const std::vector<std::shared_ptr<LogEvent>>& allData,
                           int64_t originalPullTimeNs, int64_t eventElapsedTimeNs);
 

cmds/statsd/tests/metrics/ValueMetricProducer_test.cpp

@@ -1081,6 +1081,49 @@ TEST(ValueMetricProducerTest, TestAnomalyDetection) {
               std::ceil(1.0 * event6.GetElapsedTimestampNs() / NS_PER_SEC + refPeriodSec));
 }
 
+TEST(ValueMetricProducerTest, TestAnomalyDetectionMultipleBucketsSkipped) {
+    sp<AlarmMonitor> alarmMonitor;
+    Alert alert;
+    alert.set_id(101);
+    alert.set_metric_id(metricId);
+    alert.set_trigger_if_sum_gt(100);
+    alert.set_num_buckets(1);
+    const int32_t refPeriodSec = 3;
+    alert.set_refractory_period_secs(refPeriodSec);
+
+    ValueMetric metric = ValueMetricProducerTestHelper::createMetricWithCondition();
+
+    sp<MockStatsPullerManager> pullerManager = new StrictMock<MockStatsPullerManager>();
+    EXPECT_CALL(*pullerManager, Pull(tagId, kConfigKey, _, _))
+            .WillOnce(Invoke([](int tagId, const ConfigKey&, const int64_t eventTimeNs,
+                                vector<std::shared_ptr<LogEvent>>* data) {
+                EXPECT_EQ(eventTimeNs, bucketStartTimeNs + 1);  // Condition change to true time.
+                data->clear();
+                data->push_back(CreateRepeatedValueLogEvent(tagId, bucketStartTimeNs + 1, 0));
+                return true;
+            }))
+            .WillOnce(Invoke([](int tagId, const ConfigKey&, const int64_t eventTimeNs,
+                                vector<std::shared_ptr<LogEvent>>* data) {
+                EXPECT_EQ(eventTimeNs,
+                          bucket3StartTimeNs + 100);  // Condition changed to false time.
+                data->clear();
+                data->push_back(CreateRepeatedValueLogEvent(tagId, bucket3StartTimeNs + 100, 120));
+                return true;
+            }));
+    sp<ValueMetricProducer> valueProducer =
+            ValueMetricProducerTestHelper::createValueProducerWithCondition(pullerManager, metric,
+                                                                            ConditionState::kFalse);
+    sp<AnomalyTracker> anomalyTracker = valueProducer->addAnomalyTracker(alert, alarmMonitor);
+
+    valueProducer->onConditionChanged(true, bucketStartTimeNs + 1);
+
+    // multiple buckets should be skipped here.
+    valueProducer->onConditionChanged(false, bucket3StartTimeNs + 100);
+
+    // No alert is fired when multiple buckets are skipped.
+    EXPECT_EQ(anomalyTracker->getRefractoryPeriodEndsSec(DEFAULT_METRIC_DIMENSION_KEY), 0U);
+}
+
 // Test value metric no condition, the pull on bucket boundary come in time and too late
 TEST(ValueMetricProducerTest, TestBucketBoundaryNoCondition) {
     ValueMetric metric = ValueMetricProducerTestHelper::createMetric();