Merge "Fix a problem with ValueMetric when used with conditions and no diffs." into qt-dev

3 files changed, 156 insertions, 4 deletions

diff --git a/cmds/statsd/src/metrics/ValueMetricProducer.cpp b/cmds/statsd/src/metrics/ValueMetricProducer.cpp
index 90a4e8b90051..c44ea8aa8ed0 100644
--- a/cmds/statsd/src/metrics/ValueMetricProducer.cpp
+++ b/cmds/statsd/src/metrics/ValueMetricProducer.cpp
@@ -633,10 +633,17 @@ void ValueMetricProducer::onMatchedLogEventInternalLocked(const size_t matcherIn
         flushIfNeededLocked(eventTimeNs);
     }
 
-    // For pulled data, we already check condition when we decide to pull or
-    // in onDataPulled. So take all of them.
-    // For pushed data, just check condition.
-    if (!(mIsPulled || condition)) {
+    // We should not accumulate the data for pushed metrics when the condition is false.
+    bool shouldSkipForPushMetric = !mIsPulled && !condition;
+    // For pulled metrics, there are two cases:
+    // - to compute diffs, we need to process all the state changes
+    // - for non-diffs metrics, we should ignore the data if the condition wasn't true. If we have a
+    // state change from
+    //     + True -> True: we should process the data, it might be a bucket boundary
+    //     + True -> False: we als need to process the data.
+    bool shouldSkipForPulledMetric = mIsPulled && !mUseDiff
+            && mCondition != ConditionState::kTrue;
+    if (shouldSkipForPushMetric || shouldSkipForPulledMetric) {
         VLOG("ValueMetric skip event because condition is false");
         return;
     }
diff --git a/cmds/statsd/src/metrics/ValueMetricProducer.h b/cmds/statsd/src/metrics/ValueMetricProducer.h
index 0f5633732db9..8c1999518ea6 100644
--- a/cmds/statsd/src/metrics/ValueMetricProducer.h
+++ b/cmds/statsd/src/metrics/ValueMetricProducer.h
@@ -259,6 +259,11 @@ private:
     FRIEND_TEST(ValueMetricProducerTest, TestLateOnDataPulledWithoutDiff);
     FRIEND_TEST(ValueMetricProducerTest, TestPartialBucketCreated);
     FRIEND_TEST(ValueMetricProducerTest, TestPartialResetOnBucketBoundaries);
+    FRIEND_TEST(ValueMetricProducerTest, TestPulledData_noDiff_bucketBoundaryFalse);
+    FRIEND_TEST(ValueMetricProducerTest, TestPulledData_noDiff_bucketBoundaryTrue);
+    FRIEND_TEST(ValueMetricProducerTest, TestPulledData_noDiff_withFailure);
+    FRIEND_TEST(ValueMetricProducerTest, TestPulledData_noDiff_withMultipleConditionChanges);
+    FRIEND_TEST(ValueMetricProducerTest, TestPulledData_noDiff_withoutCondition);
     FRIEND_TEST(ValueMetricProducerTest, TestPulledEventsNoCondition);
     FRIEND_TEST(ValueMetricProducerTest, TestPulledEventsTakeAbsoluteValueOnReset);
     FRIEND_TEST(ValueMetricProducerTest, TestPulledEventsTakeZeroOnReset);
diff --git a/cmds/statsd/tests/metrics/ValueMetricProducer_test.cpp b/cmds/statsd/tests/metrics/ValueMetricProducer_test.cpp
index c12a59003bab..43a3c7b0b2ea 100644
--- a/cmds/statsd/tests/metrics/ValueMetricProducer_test.cpp
+++ b/cmds/statsd/tests/metrics/ValueMetricProducer_test.cpp
@@ -2964,6 +2964,146 @@ TEST(ValueMetricProducerTest, TestPullNeededNoTimeConstraints) {
     EXPECT_EQ(10, report.value_metrics().data(0).bucket_info(0).condition_true_nanos());
 }
 
+TEST(ValueMetricProducerTest, TestPulledData_noDiff_withoutCondition) {
+    ValueMetric metric = ValueMetricProducerTestHelper::createMetric();
+    metric.set_use_diff(false);
+
+    sp<MockStatsPullerManager> pullerManager = new StrictMock<MockStatsPullerManager>();
+    sp<ValueMetricProducer> valueProducer =
+            ValueMetricProducerTestHelper::createValueProducerNoConditions(pullerManager, metric);
+
+    vector<shared_ptr<LogEvent>> allData;
+    allData.push_back(ValueMetricProducerTestHelper::createEvent(bucket2StartTimeNs + 30, 10));
+    valueProducer->onDataPulled(allData, /** succeed */ true, bucket2StartTimeNs + 30);
+
+    // Bucket should have been completed.
+    assertPastBucketValuesSingleKey(valueProducer->mPastBuckets, {10}, {bucketSizeNs});
+}
+
+TEST(ValueMetricProducerTest, TestPulledData_noDiff_withMultipleConditionChanges) {
+    ValueMetric metric = ValueMetricProducerTestHelper::createMetricWithCondition();
+    metric.set_use_diff(false);
+
+    sp<MockStatsPullerManager> pullerManager = new StrictMock<MockStatsPullerManager>();
+    EXPECT_CALL(*pullerManager, Pull(tagId, _))
+            // condition becomes true
+            .WillOnce(Invoke([](int tagId, vector<std::shared_ptr<LogEvent>>* data) {
+                data->clear();
+                data->push_back(ValueMetricProducerTestHelper::createEvent(
+                        bucketStartTimeNs + 30, 10));
+                return true;
+            }))
+            // condition becomes false
+            .WillOnce(Invoke([](int tagId, vector<std::shared_ptr<LogEvent>>* data) {
+                data->clear();
+                data->push_back(ValueMetricProducerTestHelper::createEvent(
+                        bucketStartTimeNs + 50, 20));
+                return true;
+            }));
+    sp<ValueMetricProducer> valueProducer =
+            ValueMetricProducerTestHelper::createValueProducerWithCondition(pullerManager, metric);
+    valueProducer->mCondition = ConditionState::kFalse;
+
+    valueProducer->onConditionChanged(true, bucketStartTimeNs + 8);
+    valueProducer->onConditionChanged(false, bucketStartTimeNs + 50);
+    // has one slice
+    EXPECT_EQ(1UL, valueProducer->mCurrentSlicedBucket.size());
+    ValueMetricProducer::Interval curInterval =
+            valueProducer->mCurrentSlicedBucket.begin()->second[0];
+    EXPECT_EQ(false, curInterval.hasBase);
+    EXPECT_EQ(true, curInterval.hasValue);
+    EXPECT_EQ(20, curInterval.value.long_value);
+
+
+    // Now the alarm is delivered. Condition is off though.
+    vector<shared_ptr<LogEvent>> allData;
+    allData.push_back(ValueMetricProducerTestHelper::createEvent(bucket2StartTimeNs + 30, 110));
+    valueProducer->onDataPulled(allData, /** succeed */ true, bucket2StartTimeNs);
+
+    assertPastBucketValuesSingleKey(valueProducer->mPastBuckets, {20}, {50 - 8});
+    curInterval = valueProducer->mCurrentSlicedBucket.begin()->second[0];
+    EXPECT_EQ(false, curInterval.hasBase);
+    EXPECT_EQ(false, curInterval.hasValue);
+}
+
+TEST(ValueMetricProducerTest, TestPulledData_noDiff_bucketBoundaryTrue) {
+    ValueMetric metric = ValueMetricProducerTestHelper::createMetricWithCondition();
+    metric.set_use_diff(false);
+
+    sp<MockStatsPullerManager> pullerManager = new StrictMock<MockStatsPullerManager>();
+    EXPECT_CALL(*pullerManager, Pull(tagId, _))
+            // condition becomes true
+            .WillOnce(Invoke([](int tagId, vector<std::shared_ptr<LogEvent>>* data) {
+                data->clear();
+                data->push_back(ValueMetricProducerTestHelper::createEvent(
+                        bucketStartTimeNs + 30, 10));
+                return true;
+            }));
+    sp<ValueMetricProducer> valueProducer =
+            ValueMetricProducerTestHelper::createValueProducerWithCondition(pullerManager, metric);
+    valueProducer->mCondition = ConditionState::kFalse;
+
+    valueProducer->onConditionChanged(true, bucketStartTimeNs + 8);
+
+    // Now the alarm is delivered. Condition is off though.
+    vector<shared_ptr<LogEvent>> allData;
+    allData.push_back(ValueMetricProducerTestHelper::createEvent(bucket2StartTimeNs + 30, 30));
+    valueProducer->onDataPulled(allData, /** succeed */ true, bucket2StartTimeNs);
+
+    assertPastBucketValuesSingleKey(valueProducer->mPastBuckets, {30}, {bucketSizeNs - 8});
+    ValueMetricProducer::Interval curInterval =
+            valueProducer->mCurrentSlicedBucket.begin()->second[0];
+    EXPECT_EQ(false, curInterval.hasBase);
+    EXPECT_EQ(false, curInterval.hasValue);
+}
+
+TEST(ValueMetricProducerTest, TestPulledData_noDiff_bucketBoundaryFalse) {
+    ValueMetric metric = ValueMetricProducerTestHelper::createMetricWithCondition();
+    metric.set_use_diff(false);
+
+    sp<MockStatsPullerManager> pullerManager = new StrictMock<MockStatsPullerManager>();
+    sp<ValueMetricProducer> valueProducer =
+            ValueMetricProducerTestHelper::createValueProducerWithCondition(pullerManager, metric);
+    valueProducer->mCondition = ConditionState::kFalse;
+
+    // Now the alarm is delivered. Condition is off though.
+    vector<shared_ptr<LogEvent>> allData;
+    allData.push_back(ValueMetricProducerTestHelper::createEvent(bucket2StartTimeNs + 30, 30));
+    valueProducer->onDataPulled(allData, /** succeed */ true, bucket2StartTimeNs);
+
+    // Condition was always false.
+    assertPastBucketValuesSingleKey(valueProducer->mPastBuckets, {}, {});
+}
+
+TEST(ValueMetricProducerTest, TestPulledData_noDiff_withFailure) {
+    ValueMetric metric = ValueMetricProducerTestHelper::createMetricWithCondition();
+    metric.set_use_diff(false);
+
+    sp<MockStatsPullerManager> pullerManager = new StrictMock<MockStatsPullerManager>();
+    EXPECT_CALL(*pullerManager, Pull(tagId, _))
+            // condition becomes true
+            .WillOnce(Invoke([](int tagId, vector<std::shared_ptr<LogEvent>>* data) {
+                data->clear();
+                data->push_back(ValueMetricProducerTestHelper::createEvent(
+                        bucketStartTimeNs + 30, 10));
+                return true;
+            }))
+            .WillOnce(Return(false));
+    sp<ValueMetricProducer> valueProducer =
+            ValueMetricProducerTestHelper::createValueProducerWithCondition(pullerManager, metric);
+    valueProducer->mCondition = ConditionState::kFalse;
+
+    valueProducer->onConditionChanged(true, bucketStartTimeNs + 8);
+    valueProducer->onConditionChanged(false, bucketStartTimeNs + 50);
+
+    // Now the alarm is delivered. Condition is off though.
+    vector<shared_ptr<LogEvent>> allData;
+    allData.push_back(ValueMetricProducerTestHelper::createEvent(bucket2StartTimeNs + 30, 30));
+    valueProducer->onDataPulled(allData, /** succeed */ true, bucket2StartTimeNs);
+
+    // No buckets, we had a failure.
+    assertPastBucketValuesSingleKey(valueProducer->mPastBuckets, {}, {});
+}
 
 }  // namespace statsd
 }  // namespace os