Tracks unknown condition states.
When statsd starts or is killed, the conditions are not initialized.
Tracks how many buckets are skipped because the condition was unknown.
This change does not change the behavior of statsd. In the future, we
might want to invalid buckets with a condition that has been partially
unknown.
Bug: 124100912
Test: atest statsd_test
Change-Id: Ie3d2eb5e30c21dda3203ad30f9b486961ff570cf
diff --git a/bin/src/guardrail/StatsdStats.cpp b/bin/src/guardrail/StatsdStats.cpp
index c4034ff..06a5bd9 100644
--- a/bin/src/guardrail/StatsdStats.cpp
+++ b/bin/src/guardrail/StatsdStats.cpp
@@ -448,6 +448,11 @@
getAtomMetricStats(metricId).bucketDropped++;
}
+void StatsdStats::noteBucketUnknownCondition(int64_t metricId) {
+ lock_guard<std::mutex> lock(mLock);
+ getAtomMetricStats(metricId).bucketUnknownCondition++;
+}
+
void StatsdStats::noteConditionChangeInNextBucket(int64_t metricId) {
lock_guard<std::mutex> lock(mLock);
getAtomMetricStats(metricId).conditionChangeInNextBucket++;
diff --git a/bin/src/guardrail/StatsdStats.h b/bin/src/guardrail/StatsdStats.h
index ea3f3b3..1de46b8 100644
--- a/bin/src/guardrail/StatsdStats.h
+++ b/bin/src/guardrail/StatsdStats.h
@@ -381,6 +381,11 @@
void noteBucketBoundaryDelayNs(int64_t metricId, int64_t timeDelayNs);
/**
+ * Number of buckets with unknown condition.
+ */
+ void noteBucketUnknownCondition(int64_t metricId);
+
+ /**
* Reset the historical stats. Including all stats in icebox, and the tracked stats about
* metrics, matchers, and atoms. The active configs will be kept and StatsdStats will continue
* to collect stats after reset() has been called.
@@ -428,6 +433,7 @@
long bucketDropped = 0;
int64_t minBucketBoundaryDelayNs = 0;
int64_t maxBucketBoundaryDelayNs = 0;
+ long bucketUnknownCondition = 0;
} AtomMetricStats;
private:
diff --git a/bin/src/metrics/CountMetricProducer.cpp b/bin/src/metrics/CountMetricProducer.cpp
index 350745b..5707de5 100644
--- a/bin/src/metrics/CountMetricProducer.cpp
+++ b/bin/src/metrics/CountMetricProducer.cpp
@@ -250,7 +250,7 @@
void CountMetricProducer::onConditionChangedLocked(const bool conditionMet,
const int64_t eventTime) {
VLOG("Metric %lld onConditionChanged", (long long)mMetricId);
- mCondition = conditionMet;
+ mCondition = conditionMet ? ConditionState::kTrue : ConditionState::kFalse;
}
bool CountMetricProducer::hitGuardRailLocked(const MetricDimensionKey& newKey) {
diff --git a/bin/src/metrics/DurationMetricProducer.cpp b/bin/src/metrics/DurationMetricProducer.cpp
index 6c1c47b..da6b97c 100644
--- a/bin/src/metrics/DurationMetricProducer.cpp
+++ b/bin/src/metrics/DurationMetricProducer.cpp
@@ -433,7 +433,7 @@
void DurationMetricProducer::onConditionChangedLocked(const bool conditionMet,
const int64_t eventTime) {
VLOG("Metric %lld onConditionChanged", (long long)mMetricId);
- mCondition = conditionMet;
+ mCondition = conditionMet ? ConditionState::kTrue : ConditionState::kFalse;
flushIfNeededLocked(eventTime);
for (auto& whatIt : mCurrentSlicedDurationTrackerMap) {
for (auto& pair : whatIt.second) {
@@ -767,12 +767,13 @@
!mSameConditionDimensionsInTracker,
!mHasLinksToAllConditionDimensionsInTracker,
&dimensionKeysInCondition);
- condition = (conditionState == ConditionState::kTrue);
+ condition = conditionState == ConditionState::kTrue;
if (mDimensionsInCondition.empty() && condition) {
dimensionKeysInCondition.insert(DEFAULT_DIMENSION_KEY);
}
} else {
- condition = mCondition;
+ // TODO: The unknown condition state is not handled here, we should fix it.
+ condition = mCondition == ConditionState::kTrue;
if (condition) {
dimensionKeysInCondition.insert(DEFAULT_DIMENSION_KEY);
}
diff --git a/bin/src/metrics/DurationMetricProducer.h b/bin/src/metrics/DurationMetricProducer.h
index 1b830a3..ec561b5 100644
--- a/bin/src/metrics/DurationMetricProducer.h
+++ b/bin/src/metrics/DurationMetricProducer.h
@@ -137,6 +137,7 @@
FRIEND_TEST(DurationMetricTrackerTest, TestNoCondition);
FRIEND_TEST(DurationMetricTrackerTest, TestNonSlicedCondition);
+ FRIEND_TEST(DurationMetricTrackerTest, TestNonSlicedConditionUnknownState);
FRIEND_TEST(DurationMetricTrackerTest, TestSumDurationWithUpgrade);
FRIEND_TEST(DurationMetricTrackerTest, TestSumDurationWithUpgradeInFollowingBucket);
FRIEND_TEST(DurationMetricTrackerTest, TestMaxDurationWithUpgrade);
diff --git a/bin/src/metrics/EventMetricProducer.cpp b/bin/src/metrics/EventMetricProducer.cpp
index 7e695a6..3b4af65 100644
--- a/bin/src/metrics/EventMetricProducer.cpp
+++ b/bin/src/metrics/EventMetricProducer.cpp
@@ -132,7 +132,7 @@
void EventMetricProducer::onConditionChangedLocked(const bool conditionMet,
const int64_t eventTime) {
VLOG("Metric %lld onConditionChanged", (long long)mMetricId);
- mCondition = conditionMet;
+ mCondition = conditionMet ? ConditionState::kTrue : ConditionState::kFalse;
}
void EventMetricProducer::onMatchedLogEventInternalLocked(
diff --git a/bin/src/metrics/GaugeMetricProducer.cpp b/bin/src/metrics/GaugeMetricProducer.cpp
index d56a355..837d532 100644
--- a/bin/src/metrics/GaugeMetricProducer.cpp
+++ b/bin/src/metrics/GaugeMetricProducer.cpp
@@ -320,15 +320,15 @@
// When the metric wants to do random sampling and there is already one gauge atom for the
// current bucket, do not do it again.
case GaugeMetric::RANDOM_ONE_SAMPLE: {
- triggerPuller = mCondition && mCurrentSlicedBucket->empty();
+ triggerPuller = mCondition == ConditionState::kTrue && mCurrentSlicedBucket->empty();
break;
}
case GaugeMetric::CONDITION_CHANGE_TO_TRUE: {
- triggerPuller = mCondition;
+ triggerPuller = mCondition == ConditionState::kTrue;
break;
}
case GaugeMetric::FIRST_N_SAMPLES: {
- triggerPuller = mCondition;
+ triggerPuller = mCondition == ConditionState::kTrue;
break;
}
default:
@@ -364,7 +364,7 @@
const int64_t eventTimeNs) {
VLOG("GaugeMetric %lld onConditionChanged", (long long)mMetricId);
flushIfNeededLocked(eventTimeNs);
- mCondition = conditionMet;
+ mCondition = conditionMet ? ConditionState::kTrue : ConditionState::kFalse;
if (mIsPulled && mTriggerAtomId == -1) {
pullAndMatchEventsLocked(eventTimeNs);
} // else: Push mode. No need to proactively pull the gauge data.
@@ -377,7 +377,7 @@
flushIfNeededLocked(eventTimeNs);
// If the condition is sliced, mCondition is true if any of the dimensions is true. And we will
// pull for every dimension.
- mCondition = overallCondition;
+ mCondition = overallCondition ? ConditionState::kTrue : ConditionState::kFalse;
if (mIsPulled && mTriggerAtomId == -1) {
pullAndMatchEventsLocked(eventTimeNs);
} // else: Push mode. No need to proactively pull the gauge data.
diff --git a/bin/src/metrics/MetricProducer.cpp b/bin/src/metrics/MetricProducer.cpp
index 495138e..423d050 100644
--- a/bin/src/metrics/MetricProducer.cpp
+++ b/bin/src/metrics/MetricProducer.cpp
@@ -45,7 +45,8 @@
&dimensionKeysInCondition);
condition = (conditionState == ConditionState::kTrue);
} else {
- condition = mCondition;
+ // TODO: The unknown condition state is not handled here, we should fix it.
+ condition = mCondition == ConditionState::kTrue;
}
if (mDimensionsInCondition.empty() && condition) {
diff --git a/bin/src/metrics/MetricProducer.h b/bin/src/metrics/MetricProducer.h
index 849cb76..8ab3b06 100644
--- a/bin/src/metrics/MetricProducer.h
+++ b/bin/src/metrics/MetricProducer.h
@@ -59,7 +59,7 @@
mTimeBaseNs(timeBaseNs),
mCurrentBucketStartTimeNs(timeBaseNs),
mCurrentBucketNum(0),
- mCondition(conditionIndex >= 0 ? false : true),
+ mCondition(conditionIndex >= 0 ? ConditionState::kUnknown : ConditionState::kTrue),
mConditionSliced(false),
mWizard(wizard),
mConditionTrackerIndex(conditionIndex),
@@ -315,7 +315,7 @@
int64_t mBucketSizeNs;
- bool mCondition;
+ ConditionState mCondition;
bool mConditionSliced;
diff --git a/bin/src/metrics/ValueMetricProducer.cpp b/bin/src/metrics/ValueMetricProducer.cpp
index 851ae99..bc7c872 100644
--- a/bin/src/metrics/ValueMetricProducer.cpp
+++ b/bin/src/metrics/ValueMetricProducer.cpp
@@ -154,7 +154,7 @@
// Adjust start for partial bucket
mCurrentBucketStartTimeNs = startTimeNs;
// Kicks off the puller immediately if condition is true and diff based.
- if (mIsPulled && mCondition && mUseDiff) {
+ if (mIsPulled && mCondition == ConditionState::kTrue && mUseDiff) {
pullAndMatchEventsLocked(startTimeNs);
}
VLOG("value metric %lld created. bucket size %lld start_time: %lld", (long long)metric.id(),
@@ -341,17 +341,21 @@
flushIfNeededLocked(eventTimeNs);
// Pull on condition changes.
- if (mIsPulled && (mCondition != condition)) {
+ bool conditionChanged = mCondition != condition;
+ bool unknownToFalse = mCondition == ConditionState::kUnknown
+ && condition == ConditionState::kFalse;
+ // We do not need to pull when we go from unknown to false.
+ if (mIsPulled && conditionChanged && !unknownToFalse) {
pullAndMatchEventsLocked(eventTimeNs);
}
// when condition change from true to false, clear diff base but don't
// reset other counters as we may accumulate more value in the bucket.
- if (mUseDiff && mCondition && !condition) {
+ if (mUseDiff && mCondition == ConditionState::kTrue && condition == ConditionState::kFalse) {
resetBase();
}
- mCondition = condition;
+ mCondition = condition ? ConditionState::kTrue : ConditionState::kFalse;
}
void ValueMetricProducer::pullAndMatchEventsLocked(const int64_t timestampNs) {
@@ -372,7 +376,7 @@
void ValueMetricProducer::onDataPulled(const std::vector<std::shared_ptr<LogEvent>>& allData,
bool pullSuccess, int64_t originalPullTimeNs) {
std::lock_guard<std::mutex> lock(mMutex);
- if (mCondition) {
+ if (mCondition == ConditionState::kTrue) {
if (!pullSuccess) {
// If the pull failed, we won't be able to compute a diff.
invalidateCurrentBucket();
@@ -725,6 +729,10 @@
}
void ValueMetricProducer::flushCurrentBucketLocked(const int64_t& eventTimeNs) {
+ if (mCondition == ConditionState::kUnknown) {
+ StatsdStats::getInstance().noteBucketUnknownCondition(mMetricId);
+ }
+
VLOG("finalizing bucket for %ld, dumping %d slices", (long)mCurrentBucketStartTimeNs,
(int)mCurrentSlicedBucket.size());
int64_t fullBucketEndTimeNs = getCurrentBucketEndTimeNs();
diff --git a/bin/src/stats_log.proto b/bin/src/stats_log.proto
index 6a07a3f..7a3ba7f 100644
--- a/bin/src/stats_log.proto
+++ b/bin/src/stats_log.proto
@@ -421,6 +421,7 @@
optional int64 bucket_dropped = 8;
optional int64 min_bucket_boundary_delay_ns = 9;
optional int64 max_bucket_boundary_delay_ns = 10;
+ optional int64 bucket_unknown_condition = 11;
}
repeated AtomMetricStats atom_metric_stats = 17;
diff --git a/bin/src/stats_log_util.cpp b/bin/src/stats_log_util.cpp
index aa8cfc5..ed6eb4b 100644
--- a/bin/src/stats_log_util.cpp
+++ b/bin/src/stats_log_util.cpp
@@ -82,6 +82,7 @@
const int FIELD_ID_BUCKET_DROPPED = 8;
const int FIELD_ID_MIN_BUCKET_BOUNDARY_DELAY_NS = 9;
const int FIELD_ID_MAX_BUCKET_BOUNDARY_DELAY_NS = 10;
+const int FIELD_ID_BUCKET_UNKNOWN_CONDITION = 11;
namespace {
@@ -506,6 +507,8 @@
(long long)pair.second.minBucketBoundaryDelayNs);
protoOutput->write(FIELD_TYPE_INT64 | FIELD_ID_MAX_BUCKET_BOUNDARY_DELAY_NS,
(long long)pair.second.maxBucketBoundaryDelayNs);
+ protoOutput->write(FIELD_TYPE_INT64 | FIELD_ID_BUCKET_UNKNOWN_CONDITION,
+ (long long)pair.second.bucketUnknownCondition);
protoOutput->end(token);
}
diff --git a/bin/tests/metrics/DurationMetricProducer_test.cpp b/bin/tests/metrics/DurationMetricProducer_test.cpp
index b540964..b294cad 100644
--- a/bin/tests/metrics/DurationMetricProducer_test.cpp
+++ b/bin/tests/metrics/DurationMetricProducer_test.cpp
@@ -117,6 +117,7 @@
DurationMetricProducer durationProducer(
kConfigKey, metric, 0 /* condition index */, 1 /* start index */, 2 /* stop index */,
3 /* stop_all index */, false /*nesting*/, wizard, dimensions, bucketStartTimeNs, bucketStartTimeNs);
+ durationProducer.mCondition = ConditionState::kFalse;
EXPECT_FALSE(durationProducer.mCondition);
EXPECT_FALSE(durationProducer.isConditionSliced());
@@ -140,6 +141,51 @@
EXPECT_EQ(1LL, buckets2[0].mDuration);
}
+TEST(DurationMetricTrackerTest, TestNonSlicedConditionUnknownState) {
+ sp<MockConditionWizard> wizard = new NaggyMock<MockConditionWizard>();
+ int64_t bucketStartTimeNs = 10000000000;
+ int64_t bucketSizeNs = TimeUnitToBucketSizeInMillis(ONE_MINUTE) * 1000000LL;
+
+ DurationMetric metric;
+ metric.set_id(1);
+ metric.set_bucket(ONE_MINUTE);
+ metric.set_aggregation_type(DurationMetric_AggregationType_SUM);
+
+ int tagId = 1;
+ LogEvent event1(tagId, bucketStartTimeNs + 1);
+ event1.init();
+ LogEvent event2(tagId, bucketStartTimeNs + 2);
+ event2.init();
+ LogEvent event3(tagId, bucketStartTimeNs + bucketSizeNs + 1);
+ event3.init();
+ LogEvent event4(tagId, bucketStartTimeNs + bucketSizeNs + 3);
+ event4.init();
+
+ FieldMatcher dimensions;
+ DurationMetricProducer durationProducer(
+ kConfigKey, metric, 0 /* condition index */, 1 /* start index */, 2 /* stop index */,
+ 3 /* stop_all index */, false /*nesting*/, wizard, dimensions, bucketStartTimeNs, bucketStartTimeNs);
+
+ EXPECT_EQ(ConditionState::kUnknown, durationProducer.mCondition);
+ EXPECT_FALSE(durationProducer.isConditionSliced());
+
+ durationProducer.onMatchedLogEvent(1 /* start index*/, event1);
+ durationProducer.onMatchedLogEvent(2 /* stop index*/, event2);
+ durationProducer.flushIfNeededLocked(bucketStartTimeNs + bucketSizeNs + 1);
+ EXPECT_EQ(0UL, durationProducer.mPastBuckets.size());
+
+ durationProducer.onMatchedLogEvent(1 /* start index*/, event3);
+ durationProducer.onConditionChanged(true /* condition */, bucketStartTimeNs + bucketSizeNs + 2);
+ durationProducer.onMatchedLogEvent(2 /* stop index*/, event4);
+ durationProducer.flushIfNeededLocked(bucketStartTimeNs + 2 * bucketSizeNs + 1);
+ EXPECT_EQ(1UL, durationProducer.mPastBuckets.size());
+ const auto& buckets2 = durationProducer.mPastBuckets[DEFAULT_METRIC_DIMENSION_KEY];
+ EXPECT_EQ(1UL, buckets2.size());
+ EXPECT_EQ(bucketStartTimeNs + bucketSizeNs, buckets2[0].mBucketStartNs);
+ EXPECT_EQ(bucketStartTimeNs + 2 * bucketSizeNs, buckets2[0].mBucketEndNs);
+ EXPECT_EQ(1LL, buckets2[0].mDuration);
+}
+
TEST(DurationMetricTrackerTest, TestSumDurationWithUpgrade) {
/**
* The duration starts from the first bucket, through the two partial buckets (10-70sec),
diff --git a/bin/tests/metrics/ValueMetricProducer_test.cpp b/bin/tests/metrics/ValueMetricProducer_test.cpp
index 572b199..7e7ffed 100644
--- a/bin/tests/metrics/ValueMetricProducer_test.cpp
+++ b/bin/tests/metrics/ValueMetricProducer_test.cpp
@@ -2176,7 +2176,7 @@
eventMatcherWizard, tagId, bucketStartTimeNs,
bucketStartTimeNs, pullerManager);
- valueProducer.mCondition = true;
+ valueProducer.mCondition = ConditionState::kTrue;
vector<shared_ptr<LogEvent>> allData;
valueProducer.onDataPulled(allData, /** succeed */ false, bucketStartTimeNs);
@@ -2226,7 +2226,7 @@
eventMatcherWizard, tagId, bucketStartTimeNs,
bucketStartTimeNs, pullerManager);
- valueProducer.mCondition = false;
+ valueProducer.mCondition = ConditionState::kFalse;
// Max delay is set to 0 so pull will exceed max delay.
valueProducer.onConditionChanged(true, bucketStartTimeNs + 1);
@@ -2257,7 +2257,7 @@
eventMatcherWizard, tagId, bucket2StartTimeNs,
bucket2StartTimeNs, pullerManager);
- valueProducer.mCondition = false;
+ valueProducer.mCondition = ConditionState::kFalse;
// Event should be skipped since it is from previous bucket.
// Pull should not be called.
@@ -2299,7 +2299,7 @@
eventMatcherWizard, tagId, bucketStartTimeNs,
bucketStartTimeNs, pullerManager);
- valueProducer.mCondition = false;
+ valueProducer.mCondition = ConditionState::kFalse;
valueProducer.mHasGlobalBase = false;
valueProducer.onConditionChanged(true, bucketStartTimeNs + 1);
@@ -2351,7 +2351,7 @@
eventMatcherWizard, tagId, bucketStartTimeNs,
bucketStartTimeNs, pullerManager);
- valueProducer.mCondition = true;
+ valueProducer.mCondition = ConditionState::kTrue;
// Bucket start.
vector<shared_ptr<LogEvent>> allData;
@@ -2429,7 +2429,7 @@
eventMatcherWizard, tagId, bucketStartTimeNs,
bucketStartTimeNs, pullerManager);
- valueProducer.mCondition = false;
+ valueProducer.mCondition = ConditionState::kFalse;
valueProducer.onConditionChanged(true, bucket2StartTimeNs + 2);
EXPECT_EQ(true, valueProducer.mCurrentBucketIsInvalid);
EXPECT_EQ(0UL, valueProducer.mCurrentSlicedBucket.size());
@@ -2481,7 +2481,7 @@
eventMatcherWizard, tagId, bucketStartTimeNs,
bucketStartTimeNs, pullerManager);
- valueProducer.mCondition = true;
+ valueProducer.mCondition = ConditionState::kTrue;
// Bucket start.
vector<shared_ptr<LogEvent>> allData;
@@ -2564,7 +2564,7 @@
eventMatcherWizard, tagId, bucketStartTimeNs,
bucketStartTimeNs, pullerManager);
- valueProducer.mCondition = true;
+ valueProducer.mCondition = ConditionState::kTrue;
// Bucket start.
vector<shared_ptr<LogEvent>> allData;
