From 1e0d8fcc6704b8ba004d354f7cb90f9ddfbbda9c Mon Sep 17 00:00:00 2001
From: Olivier Gaillard <gaillard@google.com>
Date: Mon, 11 Feb 2019 18:08:43 +0000
Subject: Marks the bucket as invalid when it reaches the guard rail limit.

It only impacts the BinderCallsStats atoms currently.

Bug: 120130099
Test: atest statsd_test
Change-Id: I8ba8d7cbe8798c1c503be8897a27a0729efe77d7
---
 cmds/statsd/src/metrics/ValueMetricProducer.cpp    | 17 +++++++-
 cmds/statsd/src/metrics/ValueMetricProducer.h      |  2 +
 .../tests/metrics/ValueMetricProducer_test.cpp     | 45 ++++++++++++++++++++++
 3 files changed, 63 insertions(+), 1 deletion(-)

diff --git a/cmds/statsd/src/metrics/ValueMetricProducer.cpp b/cmds/statsd/src/metrics/ValueMetricProducer.cpp
index ac6c27adceaa..851ae9962d09 100644
--- a/cmds/statsd/src/metrics/ValueMetricProducer.cpp
+++ b/cmds/statsd/src/metrics/ValueMetricProducer.cpp
@@ -450,6 +450,17 @@ void ValueMetricProducer::accumulateEvents(const std::vector<std::shared_ptr<Log
     }
     mMatchedMetricDimensionKeys.clear();
     mHasGlobalBase = true;
+
+    // If we reach the guardrail, we might have dropped some data which means the bucket is
+    // incomplete.
+    //
+    // The base also needs to be reset. If we do not have the full data, we might
+    // incorrectly compute the diff when mUseZeroDefaultBase is true since an existing key
+    // might be missing from mCurrentSlicedBucket.
+    if (hasReachedGuardRailLimit()) {
+        invalidateCurrentBucket();
+        mCurrentSlicedBucket.clear();
+    }
 }
 
 void ValueMetricProducer::dumpStatesLocked(FILE* out, bool verbose) const {
@@ -471,6 +482,10 @@ void ValueMetricProducer::dumpStatesLocked(FILE* out, bool verbose) const {
     }
 }
 
+bool ValueMetricProducer::hasReachedGuardRailLimit() const {
+    return mCurrentSlicedBucket.size() >= mDimensionHardLimit;
+}
+
 bool ValueMetricProducer::hitGuardRailLocked(const MetricDimensionKey& newKey) {
     // ===========GuardRail==============
     // 1. Report the tuple count if the tuple count > soft limit
@@ -481,7 +496,7 @@ bool ValueMetricProducer::hitGuardRailLocked(const MetricDimensionKey& newKey) {
         size_t newTupleCount = mCurrentSlicedBucket.size() + 1;
         StatsdStats::getInstance().noteMetricDimensionSize(mConfigKey, mMetricId, newTupleCount);
         // 2. Don't add more tuples, we are above the allowed threshold. Drop the data.
-        if (newTupleCount > mDimensionHardLimit) {
+        if (hasReachedGuardRailLimit()) {
             ALOGE("ValueMetric %lld dropping data for dimension key %s", (long long)mMetricId,
                   newKey.toString().c_str());
             StatsdStats::getInstance().noteHardDimensionLimitReached(mMetricId);
diff --git a/cmds/statsd/src/metrics/ValueMetricProducer.h b/cmds/statsd/src/metrics/ValueMetricProducer.h
index d1c2315b28be..f26ad85acf05 100644
--- a/cmds/statsd/src/metrics/ValueMetricProducer.h
+++ b/cmds/statsd/src/metrics/ValueMetricProducer.h
@@ -158,6 +158,7 @@ private:
 
     // Util function to check whether the specified dimension hits the guardrail.
     bool hitGuardRailLocked(const MetricDimensionKey& newKey);
+    bool hasReachedGuardRailLimit() const;
 
     bool hitFullBucketGuardRailLocked(const MetricDimensionKey& newKey);
 
@@ -244,6 +245,7 @@ private:
     FRIEND_TEST(ValueMetricProducerTest, TestResetBaseOnPullFailAfterConditionChange_EndOfBucket);
     FRIEND_TEST(ValueMetricProducerTest, TestResetBaseOnPullTooLate);
     FRIEND_TEST(ValueMetricProducerTest, TestInvalidBucketWhenOneConditionFailed);
+    FRIEND_TEST(ValueMetricProducerTest, TestInvalidBucketWhenGuardRailHit);
     FRIEND_TEST(ValueMetricProducerTest, TestInvalidBucketWhenInitialPullFailed);
     FRIEND_TEST(ValueMetricProducerTest, TestInvalidBucketWhenLastPullFailed);
     FRIEND_TEST(ValueMetricProducerTest, TestResetBaseOnPullDelayExceeded);
diff --git a/cmds/statsd/tests/metrics/ValueMetricProducer_test.cpp b/cmds/statsd/tests/metrics/ValueMetricProducer_test.cpp
index ae3cdbcb5eb4..572b1991f426 100644
--- a/cmds/statsd/tests/metrics/ValueMetricProducer_test.cpp
+++ b/cmds/statsd/tests/metrics/ValueMetricProducer_test.cpp
@@ -2390,6 +2390,51 @@ TEST(ValueMetricProducerTest, TestInvalidBucketWhenOneConditionFailed) {
     EXPECT_EQ(true, valueProducer.mHasGlobalBase);
 }
 
+TEST(ValueMetricProducerTest, TestInvalidBucketWhenGuardRailHit) {
+    ValueMetric metric;
+    metric.set_id(metricId);
+    metric.set_bucket(ONE_MINUTE);
+    metric.mutable_value_field()->set_field(tagId);
+    metric.mutable_value_field()->add_child()->set_field(2);
+    metric.mutable_dimensions_in_what()->set_field(tagId);
+    metric.mutable_dimensions_in_what()->add_child()->set_field(1);
+    metric.set_condition(StringToId("SCREEN_ON"));
+    metric.set_max_pull_delay_sec(INT_MAX);
+
+    UidMap uidMap;
+    SimpleAtomMatcher atomMatcher;
+    atomMatcher.set_atom_id(tagId);
+    sp<EventMatcherWizard> eventMatcherWizard =
+            new EventMatcherWizard({new SimpleLogMatchingTracker(
+                    atomMatcherId, logEventMatcherIndex, atomMatcher, uidMap)});
+    sp<MockConditionWizard> wizard = new NaggyMock<MockConditionWizard>();
+    sp<MockStatsPullerManager> pullerManager = new StrictMock<MockStatsPullerManager>();
+    EXPECT_CALL(*pullerManager, RegisterReceiver(tagId, _, _, _)).WillOnce(Return());
+    EXPECT_CALL(*pullerManager, UnRegisterReceiver(tagId, _)).WillRepeatedly(Return());
+
+    EXPECT_CALL(*pullerManager, Pull(tagId, _))
+            // First onConditionChanged
+            .WillOnce(Invoke([](int tagId, vector<std::shared_ptr<LogEvent>>* data) {
+                for (int i = 0; i < 2000; i++) {
+                    shared_ptr<LogEvent> event = make_shared<LogEvent>(tagId, bucket2StartTimeNs + 1);
+                    event->write(i);
+                    event->write(i);
+                    event->init();
+                    data->push_back(event);
+                }
+                return true;
+            }));
+
+    ValueMetricProducer valueProducer(kConfigKey, metric, 1, wizard, logEventMatcherIndex,
+                                      eventMatcherWizard, tagId, bucketStartTimeNs,
+                                      bucketStartTimeNs, pullerManager);
+
+    valueProducer.mCondition = false;
+    valueProducer.onConditionChanged(true, bucket2StartTimeNs + 2);
+    EXPECT_EQ(true, valueProducer.mCurrentBucketIsInvalid);
+    EXPECT_EQ(0UL, valueProducer.mCurrentSlicedBucket.size());
+}
+
 TEST(ValueMetricProducerTest, TestInvalidBucketWhenInitialPullFailed) {
     ValueMetric metric;
     metric.set_id(metricId);
-- 
cgit v1.2.3-59-g8ed1b