From ae63b0af946d921ca20aaf1916b12af593ec0e1d Mon Sep 17 00:00:00 2001 From: Chenjie Yu Date: Tue, 10 Apr 2018 14:59:31 -0700 Subject: [PATCH] Drop value if the bucket is totally tainted Bug: 77870358 Change-Id: Ia96970a3254de08f94b91ad53be2fdb9f4db7eb4 Fix: 77870358 Test: unit test --- .../src/metrics/ValueMetricProducer.cpp | 12 +++-- cmds/statsd/src/metrics/ValueMetricProducer.h | 3 ++ .../metrics/ValueMetricProducer_test.cpp | 48 ++++++------------- 3 files changed, 25 insertions(+), 38 deletions(-) diff --git a/cmds/statsd/src/metrics/ValueMetricProducer.cpp b/cmds/statsd/src/metrics/ValueMetricProducer.cpp index 51fac8cf54dc3..89efae35b6f75 100644 --- a/cmds/statsd/src/metrics/ValueMetricProducer.cpp +++ b/cmds/statsd/src/metrics/ValueMetricProducer.cpp @@ -334,6 +334,7 @@ void ValueMetricProducer::onMatchedLogEventInternalLocked( } else { interval.sum += value; } + interval.hasValue = true; interval.startUpdated = false; } else { VLOG("No start for matching end %ld", value); @@ -342,6 +343,7 @@ void ValueMetricProducer::onMatchedLogEventInternalLocked( } } else { // for pushed events interval.sum += value; + interval.hasValue = true; } long wholeBucketVal = interval.sum; @@ -393,10 +395,12 @@ void ValueMetricProducer::flushCurrentBucketLocked(const int64_t& eventTimeNs) { for (const auto& slice : mCurrentSlicedBucket) { tainted += slice.second.tainted; tainted += slice.second.startUpdated; - info.mValue = slice.second.sum; - // it will auto create new vector of ValuebucketInfo if the key is not found. - auto& bucketList = mPastBuckets[slice.first]; - bucketList.push_back(info); + if (slice.second.hasValue) { + info.mValue = slice.second.sum; + // it will auto create new vector of ValuebucketInfo if the key is not found. + auto& bucketList = mPastBuckets[slice.first]; + bucketList.push_back(info); + } } VLOG("%d tainted pairs in the bucket", tainted); diff --git a/cmds/statsd/src/metrics/ValueMetricProducer.h b/cmds/statsd/src/metrics/ValueMetricProducer.h index b5f642913c522..b19adbef6bad1 100644 --- a/cmds/statsd/src/metrics/ValueMetricProducer.h +++ b/cmds/statsd/src/metrics/ValueMetricProducer.h @@ -135,6 +135,9 @@ private: int tainted; // Running sum of known pairs in this bucket long sum; + // If this dimension has any non-tainted value. If not, don't report the + // dimension. + bool hasValue; } Interval; std::unordered_map mCurrentSlicedBucket; diff --git a/cmds/statsd/tests/metrics/ValueMetricProducer_test.cpp b/cmds/statsd/tests/metrics/ValueMetricProducer_test.cpp index a2bb7342037fc..7b52265bf55c8 100644 --- a/cmds/statsd/tests/metrics/ValueMetricProducer_test.cpp +++ b/cmds/statsd/tests/metrics/ValueMetricProducer_test.cpp @@ -90,8 +90,7 @@ TEST(ValueMetricProducerTest, TestNonDimensionalEvents) { EXPECT_EQ(0, curInterval.tainted); EXPECT_EQ(0, curInterval.sum); EXPECT_EQ(11, curInterval.start); - EXPECT_EQ(1UL, valueProducer.mPastBuckets.size()); - EXPECT_EQ(0, valueProducer.mPastBuckets.begin()->second.back().mValue); + EXPECT_EQ(0UL, valueProducer.mPastBuckets.size()); allData.clear(); event = make_shared(tagId, bucket3StartTimeNs + 1); @@ -108,7 +107,7 @@ TEST(ValueMetricProducerTest, TestNonDimensionalEvents) { EXPECT_EQ(0, curInterval.tainted); EXPECT_EQ(0, curInterval.sum); EXPECT_EQ(1UL, valueProducer.mPastBuckets.size()); - EXPECT_EQ(2UL, valueProducer.mPastBuckets.begin()->second.size()); + EXPECT_EQ(1UL, valueProducer.mPastBuckets.begin()->second.size()); EXPECT_EQ(12, valueProducer.mPastBuckets.begin()->second.back().mValue); allData.clear(); @@ -125,7 +124,7 @@ TEST(ValueMetricProducerTest, TestNonDimensionalEvents) { EXPECT_EQ(0, curInterval.tainted); EXPECT_EQ(0, curInterval.sum); EXPECT_EQ(1UL, valueProducer.mPastBuckets.size()); - EXPECT_EQ(3UL, valueProducer.mPastBuckets.begin()->second.size()); + EXPECT_EQ(2UL, valueProducer.mPastBuckets.begin()->second.size()); EXPECT_EQ(13, valueProducer.mPastBuckets.begin()->second.back().mValue); } @@ -464,15 +463,13 @@ TEST(ValueMetricProducerTest, TestBucketBoundaryNoCondition) { // has one slice EXPECT_EQ(1UL, valueProducer.mCurrentSlicedBucket.size()); ValueMetricProducer::Interval curInterval = valueProducer.mCurrentSlicedBucket.begin()->second; - valueProducer.setBucketSize(60 * NS_PER_SEC); // startUpdated:true tainted:0 sum:0 start:11 EXPECT_EQ(true, curInterval.startUpdated); EXPECT_EQ(0, curInterval.tainted); EXPECT_EQ(0, curInterval.sum); EXPECT_EQ(11, curInterval.start); - EXPECT_EQ(1UL, valueProducer.mPastBuckets.size()); - EXPECT_EQ(0, valueProducer.mPastBuckets.begin()->second.back().mValue); + EXPECT_EQ(0UL, valueProducer.mPastBuckets.size()); // pull 2 at correct time allData.clear(); @@ -490,7 +487,7 @@ TEST(ValueMetricProducerTest, TestBucketBoundaryNoCondition) { EXPECT_EQ(0, curInterval.tainted); EXPECT_EQ(0, curInterval.sum); EXPECT_EQ(1UL, valueProducer.mPastBuckets.size()); - EXPECT_EQ(2UL, valueProducer.mPastBuckets.begin()->second.size()); + EXPECT_EQ(1UL, valueProducer.mPastBuckets.begin()->second.size()); EXPECT_EQ(12, valueProducer.mPastBuckets.begin()->second.back().mValue); // pull 3 come late. @@ -512,11 +509,8 @@ TEST(ValueMetricProducerTest, TestBucketBoundaryNoCondition) { EXPECT_EQ(36, curInterval.start); EXPECT_EQ(0, curInterval.sum); EXPECT_EQ(1UL, valueProducer.mPastBuckets.size()); - EXPECT_EQ(4UL, valueProducer.mPastBuckets.begin()->second.size()); - EXPECT_EQ(0, valueProducer.mPastBuckets.begin()->second[0].mValue); - EXPECT_EQ(12, valueProducer.mPastBuckets.begin()->second[1].mValue); - EXPECT_EQ(0, valueProducer.mPastBuckets.begin()->second[2].mValue); - EXPECT_EQ(0, valueProducer.mPastBuckets.begin()->second[3].mValue); + EXPECT_EQ(1UL, valueProducer.mPastBuckets.begin()->second.size()); + EXPECT_EQ(12, valueProducer.mPastBuckets.begin()->second.back().mValue); } /* @@ -582,9 +576,7 @@ TEST(ValueMetricProducerTest, TestBucketBoundaryWithCondition) { EXPECT_EQ(false, curInterval.startUpdated); EXPECT_EQ(1, curInterval.tainted); EXPECT_EQ(0, curInterval.sum); - EXPECT_EQ(1UL, valueProducer.mPastBuckets.size()); - EXPECT_EQ(1UL, valueProducer.mPastBuckets.begin()->second.size()); - EXPECT_EQ(0, valueProducer.mPastBuckets.begin()->second[0].mValue); + EXPECT_EQ(0UL, valueProducer.mPastBuckets.size()); // Now the alarm is delivered. // since the condition turned to off before this pull finish, it has no effect @@ -601,9 +593,7 @@ TEST(ValueMetricProducerTest, TestBucketBoundaryWithCondition) { EXPECT_EQ(false, curInterval.startUpdated); EXPECT_EQ(1, curInterval.tainted); EXPECT_EQ(0, curInterval.sum); - EXPECT_EQ(1UL, valueProducer.mPastBuckets.size()); - EXPECT_EQ(1UL, valueProducer.mPastBuckets.begin()->second.size()); - EXPECT_EQ(0, valueProducer.mPastBuckets.begin()->second[0].mValue); + EXPECT_EQ(0UL, valueProducer.mPastBuckets.size()); } /* @@ -680,9 +670,7 @@ TEST(ValueMetricProducerTest, TestBucketBoundaryWithCondition2) { EXPECT_EQ(false, curInterval.startUpdated); EXPECT_EQ(1, curInterval.tainted); EXPECT_EQ(0, curInterval.sum); - EXPECT_EQ(1UL, valueProducer.mPastBuckets.size()); - EXPECT_EQ(1UL, valueProducer.mPastBuckets.begin()->second.size()); - EXPECT_EQ(0, valueProducer.mPastBuckets.begin()->second[0].mValue); + EXPECT_EQ(0UL, valueProducer.mPastBuckets.size()); // condition changed to true again, before the pull alarm is delivered valueProducer.onConditionChanged(true, bucket2StartTimeNs + 25); @@ -691,9 +679,7 @@ TEST(ValueMetricProducerTest, TestBucketBoundaryWithCondition2) { EXPECT_EQ(130, curInterval.start); EXPECT_EQ(1, curInterval.tainted); EXPECT_EQ(0, curInterval.sum); - EXPECT_EQ(1UL, valueProducer.mPastBuckets.size()); - EXPECT_EQ(1UL, valueProducer.mPastBuckets.begin()->second.size()); - EXPECT_EQ(0, valueProducer.mPastBuckets.begin()->second[0].mValue); + EXPECT_EQ(0UL, valueProducer.mPastBuckets.size()); // Now the alarm is delivered, but it is considered late, it has no effect vector> allData; @@ -710,9 +696,7 @@ TEST(ValueMetricProducerTest, TestBucketBoundaryWithCondition2) { EXPECT_EQ(130, curInterval.start); EXPECT_EQ(1, curInterval.tainted); EXPECT_EQ(0, curInterval.sum); - EXPECT_EQ(1UL, valueProducer.mPastBuckets.size()); - EXPECT_EQ(1UL, valueProducer.mPastBuckets.begin()->second.size()); - EXPECT_EQ(0, valueProducer.mPastBuckets.begin()->second[0].mValue); + EXPECT_EQ(0UL, valueProducer.mPastBuckets.size()); } /* @@ -779,9 +763,7 @@ TEST(ValueMetricProducerTest, TestBucketBoundaryWithCondition3) { EXPECT_EQ(false, curInterval.startUpdated); EXPECT_EQ(1, curInterval.tainted); EXPECT_EQ(0, curInterval.sum); - EXPECT_EQ(1UL, valueProducer.mPastBuckets.size()); - EXPECT_EQ(1UL, valueProducer.mPastBuckets.begin()->second.size()); - EXPECT_EQ(0, valueProducer.mPastBuckets.begin()->second[0].mValue); + EXPECT_EQ(0UL, valueProducer.mPastBuckets.size()); // Alarm is delivered in time, but the pull is very slow, and pullers are called in order, // so this one comes even later @@ -798,9 +780,7 @@ TEST(ValueMetricProducerTest, TestBucketBoundaryWithCondition3) { EXPECT_EQ(false, curInterval.startUpdated); EXPECT_EQ(1, curInterval.tainted); EXPECT_EQ(0, curInterval.sum); - EXPECT_EQ(1UL, valueProducer.mPastBuckets.size()); - EXPECT_EQ(1UL, valueProducer.mPastBuckets.begin()->second.size()); - EXPECT_EQ(0, valueProducer.mPastBuckets.begin()->second[0].mValue); + EXPECT_EQ(0UL, valueProducer.mPastBuckets.size()); } } // namespace statsd