Fix some bugs found in statsd
+ in log matcher, condition tracker and duration metric
Test: added unit test
Change-Id: Id633e856ba5453842487321d7ddc0c64100e4bb8
diff --git a/cmds/statsd/src/condition/SimpleConditionTracker.cpp b/cmds/statsd/src/condition/SimpleConditionTracker.cpp
index a694dbf..60060fe 100644
--- a/cmds/statsd/src/condition/SimpleConditionTracker.cpp
+++ b/cmds/statsd/src/condition/SimpleConditionTracker.cpp
@@ -206,7 +206,7 @@
vector<bool>& conditionChangedCache) {
if (conditionCache[mIndex] != ConditionState::kNotEvaluated) {
// it has been evaluated.
- VLOG("Yes, already evaluated, %s %d", mName.c_str(), mNonSlicedConditionState);
+ VLOG("Yes, already evaluated, %s %d", mName.c_str(), conditionCache[mIndex]);
return;
}
@@ -230,8 +230,23 @@
}
if (matchedState < 0) {
+ // The event doesn't match this condition. So we just report existing condition values.
conditionChangedCache[mIndex] = false;
- conditionCache[mIndex] = mNonSlicedConditionState;
+ if (mSliced) {
+ // if the condition result is sliced. metrics won't directly get value from the
+ // cache, so just set any value other than kNotEvaluated.
+ conditionCache[mIndex] = ConditionState::kUnknown;
+ } else if (mSlicedConditionState.find(DEFAULT_DIMENSION_KEY) ==
+ mSlicedConditionState.end()) {
+ // condition not sliced, but we haven't seen the matched start or stop yet. so return
+ // initial value.
+ conditionCache[mIndex] = mInitialValue;
+ } else {
+ // return the cached condition.
+ conditionCache[mIndex] = mSlicedConditionState[DEFAULT_DIMENSION_KEY] > 0
+ ? ConditionState::kTrue
+ : ConditionState::kFalse;
+ }
return;
}
diff --git a/cmds/statsd/src/matchers/matcher_util.cpp b/cmds/statsd/src/matchers/matcher_util.cpp
index cccc9b3..1c699e8 100644
--- a/cmds/statsd/src/matchers/matcher_util.cpp
+++ b/cmds/statsd/src/matchers/matcher_util.cpp
@@ -112,6 +112,7 @@
if (err == NO_ERROR && val != NULL) {
if (!(cur.eq_string() == val)) {
allMatched = false;
+ break;
}
}
} else if (matcherCase == KeyValueMatcher::ValueMatcherCase::kEqInt ||
@@ -126,26 +127,30 @@
if (matcherCase == KeyValueMatcher::ValueMatcherCase::kEqInt) {
if (!(val == cur.eq_int())) {
allMatched = false;
+ break;
}
} else if (matcherCase == KeyValueMatcher::ValueMatcherCase::kLtInt) {
if (!(val < cur.lt_int())) {
allMatched = false;
+ break;
}
} else if (matcherCase == KeyValueMatcher::ValueMatcherCase::kGtInt) {
if (!(val > cur.gt_int())) {
allMatched = false;
+ break;
}
} else if (matcherCase == KeyValueMatcher::ValueMatcherCase::kLteInt) {
if (!(val <= cur.lte_int())) {
allMatched = false;
+ break;
}
} else if (matcherCase == KeyValueMatcher::ValueMatcherCase::kGteInt) {
if (!(val >= cur.gte_int())) {
allMatched = false;
+ break;
}
}
}
- break;
} else if (matcherCase == KeyValueMatcher::ValueMatcherCase::kEqBool) {
// Boolean fields
status_t err = NO_ERROR;
@@ -153,6 +158,7 @@
if (err == NO_ERROR) {
if (!(cur.eq_bool() == val)) {
allMatched = false;
+ break;
}
}
} else if (matcherCase == KeyValueMatcher::ValueMatcherCase::kLtFloat ||
@@ -164,10 +170,12 @@
if (matcherCase == KeyValueMatcher::ValueMatcherCase::kLtFloat) {
if (!(cur.lt_float() <= val)) {
allMatched = false;
+ break;
}
} else if (matcherCase == KeyValueMatcher::ValueMatcherCase::kGtFloat) {
if (!(cur.gt_float() >= val)) {
allMatched = false;
+ break;
}
}
}
diff --git a/cmds/statsd/src/metrics/DurationMetricProducer.cpp b/cmds/statsd/src/metrics/DurationMetricProducer.cpp
index aaf3ec2..de6f365 100644
--- a/cmds/statsd/src/metrics/DurationMetricProducer.cpp
+++ b/cmds/statsd/src/metrics/DurationMetricProducer.cpp
@@ -233,10 +233,12 @@
}
VLOG("flushing...........");
- for (auto it = mCurrentSlicedDuration.begin(); it != mCurrentSlicedDuration.end(); ++it) {
+ for (auto it = mCurrentSlicedDuration.begin(); it != mCurrentSlicedDuration.end();) {
if (it->second->flushIfNeeded(eventTime)) {
VLOG("erase bucket for key %s", it->first.c_str());
- mCurrentSlicedDuration.erase(it);
+ it = mCurrentSlicedDuration.erase(it);
+ } else {
+ ++it;
}
}
diff --git a/cmds/statsd/tests/LogEntryMatcher_test.cpp b/cmds/statsd/tests/LogEntryMatcher_test.cpp
index d0898b0..bb4930a 100644
--- a/cmds/statsd/tests/LogEntryMatcher_test.cpp
+++ b/cmds/statsd/tests/LogEntryMatcher_test.cpp
@@ -109,6 +109,39 @@
EXPECT_TRUE(matchesSimple(*simpleMatcher, event));
}
+TEST(LogEntryMatcherTest, TestMultiFieldsMatcher) {
+ // Set up the matcher
+ LogEntryMatcher matcher;
+ auto simpleMatcher = matcher.mutable_simple_log_entry_matcher();
+ simpleMatcher->set_tag(TAG_ID);
+ auto keyValue1 = simpleMatcher->add_key_value_matcher();
+ keyValue1->mutable_key_matcher()->set_key(FIELD_ID_1);
+ auto keyValue2 = simpleMatcher->add_key_value_matcher();
+ keyValue2->mutable_key_matcher()->set_key(FIELD_ID_2);
+
+ // Set up the event
+ LogEvent event(TAG_ID, 0);
+ auto list = event.GetAndroidLogEventList();
+ *list << 2;
+ *list << 3;
+
+ // Convert to a LogEvent
+ event.init();
+
+ // Test
+ keyValue1->set_eq_int(2);
+ keyValue2->set_eq_int(3);
+ EXPECT_TRUE(matchesSimple(*simpleMatcher, event));
+
+ keyValue1->set_eq_int(2);
+ keyValue2->set_eq_int(4);
+ EXPECT_FALSE(matchesSimple(*simpleMatcher, event));
+
+ keyValue1->set_eq_int(4);
+ keyValue2->set_eq_int(3);
+ EXPECT_FALSE(matchesSimple(*simpleMatcher, event));
+}
+
TEST(LogEntryMatcherTest, TestIntComparisonMatcher) {
// Set up the matcher
LogEntryMatcher matcher;
diff --git a/cmds/statsd/tests/condition/SimpleConditionTracker_test.cpp b/cmds/statsd/tests/condition/SimpleConditionTracker_test.cpp
index 05aad29..855e666 100644
--- a/cmds/statsd/tests/condition/SimpleConditionTracker_test.cpp
+++ b/cmds/statsd/tests/condition/SimpleConditionTracker_test.cpp
@@ -108,6 +108,18 @@
EXPECT_EQ(ConditionState::kTrue, conditionCache[0]);
EXPECT_TRUE(changedCache[0]);
+ // match nothing.
+ matcherState.clear();
+ matcherState.push_back(MatchingState::kNotMatched);
+ matcherState.push_back(MatchingState::kNotMatched);
+ conditionCache[0] = ConditionState::kNotEvaluated;
+ changedCache[0] = false;
+
+ conditionTracker.evaluateCondition(event, matcherState, allConditions, conditionCache,
+ changedCache);
+ EXPECT_EQ(ConditionState::kTrue, conditionCache[0]);
+ EXPECT_FALSE(changedCache[0]);
+
// the case for match stop.
matcherState.clear();
matcherState.push_back(MatchingState::kNotMatched);