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
diff --git a/bin/src/metrics/ValueMetricProducer.cpp b/bin/src/metrics/ValueMetricProducer.cpp
index 5987a72..9b684f1 100644
--- a/bin/src/metrics/ValueMetricProducer.cpp
+++ b/bin/src/metrics/ValueMetricProducer.cpp
@@ -733,6 +733,11 @@
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 @@
interval.sampleSize += 1;
}
- // Only trigger the tracker if all intervals are correct
- if (useAnomalyDetection) {
+ // Only trigger the tracker if all intervals are correct and we have not skipped the bucket due
+ // 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 @@
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 (numBucketsForward > 1 && (mIsPulled || mUseDiff)) {
-
+ if (multipleBucketsSkipped(numBucketsForward)) {
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
diff --git a/bin/src/metrics/ValueMetricProducer.h b/bin/src/metrics/ValueMetricProducer.h
index b359af7..e72002e 100644
--- a/bin/src/metrics/ValueMetricProducer.h
+++ b/bin/src/metrics/ValueMetricProducer.h
@@ -219,6 +219,8 @@
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);
diff --git a/bin/tests/metrics/ValueMetricProducer_test.cpp b/bin/tests/metrics/ValueMetricProducer_test.cpp
index 97757af..9889250 100644
--- a/bin/tests/metrics/ValueMetricProducer_test.cpp
+++ b/bin/tests/metrics/ValueMetricProducer_test.cpp
@@ -1081,6 +1081,49 @@
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();
