Merge "Reject the config if condition config has errors. And add log tag."

2 files changed, 67 insertions, 15 deletions

diff --git a/cmds/statsd/src/metrics/metrics_manager_util.cpp b/cmds/statsd/src/metrics/metrics_manager_util.cpp
index 1b9efc51c7b7..fe3082b4b038 100644
--- a/cmds/statsd/src/metrics/metrics_manager_util.cpp
+++ b/cmds/statsd/src/metrics/metrics_manager_util.cpp
@@ -14,6 +14,9 @@
  * limitations under the License.
  */
 
+#define DEBUG true  // STOPSHIP if true
+#include "Log.h"
+
 #include "../condition/CombinationConditionTracker.h"
 #include "../condition/SimpleConditionTracker.h"
 #include "../external/StatsPullerManager.h"
@@ -221,9 +224,12 @@ bool initMetrics(const ConfigKey& key, const StatsdConfig& config,
 
         int conditionIndex = -1;
         if (metric.has_condition()) {
-            handleMetricWithConditions(metric.condition(), metricIndex, conditionTrackerMap,
-                                       metric.links(), allConditionTrackers, conditionIndex,
-                                       conditionToMetricMap);
+            bool good = handleMetricWithConditions(
+                    metric.condition(), metricIndex, conditionTrackerMap, metric.links(),
+                    allConditionTrackers, conditionIndex, conditionToMetricMap);
+            if (!good) {
+                return false;
+            }
         } else {
             if (metric.links_size() > 0) {
                 ALOGW("metrics has a EventConditionLink but doesn't have a condition");
@@ -289,9 +295,12 @@ bool initMetrics(const ConfigKey& key, const StatsdConfig& config,
         int conditionIndex = -1;
 
         if (metric.has_condition()) {
-            handleMetricWithConditions(metric.condition(), metricIndex, conditionTrackerMap,
-                                       metric.links(), allConditionTrackers, conditionIndex,
-                                       conditionToMetricMap);
+            bool good = handleMetricWithConditions(
+                    metric.condition(), metricIndex, conditionTrackerMap, metric.links(),
+                    allConditionTrackers, conditionIndex, conditionToMetricMap);
+            if (!good) {
+                return false;
+            }
         } else {
             if (metric.links_size() > 0) {
                 ALOGW("metrics has a EventConditionLink but doesn't have a condition");
@@ -323,9 +332,12 @@ bool initMetrics(const ConfigKey& key, const StatsdConfig& config,
 
         int conditionIndex = -1;
         if (metric.has_condition()) {
-            handleMetricWithConditions(metric.condition(), metricIndex, conditionTrackerMap,
-                                       metric.links(), allConditionTrackers, conditionIndex,
-                                       conditionToMetricMap);
+            bool good = handleMetricWithConditions(
+                    metric.condition(), metricIndex, conditionTrackerMap, metric.links(),
+                    allConditionTrackers, conditionIndex, conditionToMetricMap);
+            if (!good) {
+                return false;
+            }
         } else {
             if (metric.links_size() > 0) {
                 ALOGW("metrics has a EventConditionLink but doesn't have a condition");
@@ -370,9 +382,12 @@ bool initMetrics(const ConfigKey& key, const StatsdConfig& config,
 
         int conditionIndex = -1;
         if (metric.has_condition()) {
-            handleMetricWithConditions(metric.condition(), metricIndex, conditionTrackerMap,
-                                       metric.links(), allConditionTrackers, conditionIndex,
-                                       conditionToMetricMap);
+            bool good = handleMetricWithConditions(
+                    metric.condition(), metricIndex, conditionTrackerMap, metric.links(),
+                    allConditionTrackers, conditionIndex, conditionToMetricMap);
+            if (!good) {
+                return false;
+            }
         } else {
             if (metric.links_size() > 0) {
                 ALOGW("metrics has a EventConditionLink but doesn't have a condition");
@@ -416,9 +431,12 @@ bool initMetrics(const ConfigKey& key, const StatsdConfig& config,
 
         int conditionIndex = -1;
         if (metric.has_condition()) {
-            handleMetricWithConditions(metric.condition(), metricIndex, conditionTrackerMap,
-                                       metric.links(), allConditionTrackers, conditionIndex,
-                                       conditionToMetricMap);
+            bool good = handleMetricWithConditions(
+                    metric.condition(), metricIndex, conditionTrackerMap, metric.links(),
+                    allConditionTrackers, conditionIndex, conditionToMetricMap);
+            if (!good) {
+                return false;
+            }
         } else {
             if (metric.links_size() > 0) {
                 ALOGW("metrics has a EventConditionLink but doesn't have a condition");
diff --git a/cmds/statsd/tests/MetricsManager_test.cpp b/cmds/statsd/tests/MetricsManager_test.cpp
index 2adec945b85a..31d395fea3d4 100644
--- a/cmds/statsd/tests/MetricsManager_test.cpp
+++ b/cmds/statsd/tests/MetricsManager_test.cpp
@@ -165,6 +165,25 @@ StatsdConfig buildMissingMatchers() {
     return config;
 }
 
+StatsdConfig buildMissingCondition() {
+    StatsdConfig config;
+    config.set_name("12345");
+
+    CountMetric* metric = config.add_count_metric();
+    metric->set_name("3");
+    metric->set_what("SCREEN_EVENT");
+    metric->mutable_bucket()->set_bucket_size_millis(30 * 1000L);
+    metric->set_condition("SOME_CONDITION");
+
+    LogEntryMatcher* eventMatcher = config.add_log_entry_matcher();
+    eventMatcher->set_name("SCREEN_EVENT");
+
+    SimpleLogEntryMatcher* simpleLogEntryMatcher = eventMatcher->mutable_simple_log_entry_matcher();
+    simpleLogEntryMatcher->set_tag(2);
+
+    return config;
+}
+
 StatsdConfig buildDimensionMetricsWithMultiTags() {
     StatsdConfig config;
     config.set_name("12345");
@@ -310,6 +329,21 @@ TEST(MetricsManagerTest, TestMissingMatchers) {
                                   conditionToMetricMap, trackerToMetricMap, trackerToConditionMap));
 }
 
+TEST(MetricsManagerTest, TestMissingCondition) {
+    StatsdConfig config = buildMissingCondition();
+    set<int> allTagIds;
+    vector<sp<LogMatchingTracker>> allLogEntryMatchers;
+    vector<sp<ConditionTracker>> allConditionTrackers;
+    vector<sp<MetricProducer>> allMetricProducers;
+    std::vector<sp<AnomalyTracker>> allAnomalyTrackers;
+    unordered_map<int, std::vector<int>> conditionToMetricMap;
+    unordered_map<int, std::vector<int>> trackerToMetricMap;
+    unordered_map<int, std::vector<int>> trackerToConditionMap;
+    EXPECT_FALSE(initStatsdConfig(config, allTagIds, allLogEntryMatchers, allConditionTrackers,
+                                  allMetricProducers, allAnomalyTrackers, conditionToMetricMap,
+                                  trackerToMetricMap, trackerToConditionMap));
+}
+
 TEST(MetricsManagerTest, TestCircleConditionDependency) {
     StatsdConfig config = buildCircleConditions();
     set<int> allTagIds;