Merge "Enforce duration metric dimension rules"

6 files changed, 82 insertions, 15 deletions

diff --git a/cmds/statsd/src/metrics/DurationMetricProducer.cpp b/cmds/statsd/src/metrics/DurationMetricProducer.cpp
index bbaa20f232a9..001ad36134e9 100644
--- a/cmds/statsd/src/metrics/DurationMetricProducer.cpp
+++ b/cmds/statsd/src/metrics/DurationMetricProducer.cpp
@@ -107,6 +107,12 @@ DurationMetricProducer::DurationMetricProducer(
         ALOGE("Position ANY in dimension_in_what not supported.");
     }
 
+    // Dimensions in what must be subset of internal dimensions
+    if (!subsetDimensions(mDimensionsInWhat, mInternalDimensions)) {
+        ALOGE("Dimensions in what must be a subset of the internal dimensions");
+        mValid = false;
+    }
+
     mSliceByPositionALL = HasPositionALL(metric.dimensions_in_what());
 
     if (metric.links().size() > 0) {
@@ -115,6 +121,10 @@ DurationMetricProducer::DurationMetricProducer(
             mc.conditionId = link.condition();
             translateFieldMatcher(link.fields_in_what(), &mc.metricFields);
             translateFieldMatcher(link.fields_in_condition(), &mc.conditionFields);
+            if (!subsetDimensions(mc.metricFields, mInternalDimensions)) {
+                ALOGE(("Condition links must be a subset of the internal dimensions"));
+                mValid = false;
+            }
             mMetric2ConditionLinks.push_back(mc);
         }
         mConditionSliced = true;
@@ -126,6 +136,10 @@ DurationMetricProducer::DurationMetricProducer(
         ms.stateAtomId = stateLink.state_atom_id();
         translateFieldMatcher(stateLink.fields_in_what(), &ms.metricFields);
         translateFieldMatcher(stateLink.fields_in_state(), &ms.stateFields);
+        if (!subsetDimensions(ms.metricFields, mInternalDimensions)) {
+            ALOGE(("State links must be a subset of the dimensions in what  internal dimensions"));
+            mValid = false;
+        }
         mMetric2StateLinks.push_back(ms);
     }
 
diff --git a/cmds/statsd/src/metrics/MetricProducer.cpp b/cmds/statsd/src/metrics/MetricProducer.cpp
index 5b321a0e3d60..c68e61e78f2e 100644
--- a/cmds/statsd/src/metrics/MetricProducer.cpp
+++ b/cmds/statsd/src/metrics/MetricProducer.cpp
@@ -56,6 +56,7 @@ MetricProducer::MetricProducer(
     : mMetricId(metricId),
       mProtoHash(protoHash),
       mConfigKey(key),
+      mValid(true),
       mTimeBaseNs(timeBaseNs),
       mCurrentBucketStartTimeNs(timeBaseNs),
       mCurrentBucketNum(0),
diff --git a/cmds/statsd/src/metrics/MetricProducer.h b/cmds/statsd/src/metrics/MetricProducer.h
index 0dc8edae8056..7a1c008bd134 100644
--- a/cmds/statsd/src/metrics/MetricProducer.h
+++ b/cmds/statsd/src/metrics/MetricProducer.h
@@ -336,6 +336,10 @@ public:
         return mSlicedStateAtoms;
     }
 
+    inline bool isValid() const {
+        return mValid;
+    }
+
     /* Adds an AnomalyTracker and returns it. */
     virtual sp<AnomalyTracker> addAnomalyTracker(const Alert &alert,
                                                  const sp<AlarmMonitor>& anomalyAlarmMonitor) {
@@ -468,6 +472,8 @@ protected:
 
     const ConfigKey mConfigKey;
 
+    bool mValid;
+
     // The time when this metric producer was first created. The end time for the current bucket
     // can be computed from this based on mCurrentBucketNum.
     int64_t mTimeBaseNs;
diff --git a/cmds/statsd/src/metrics/parsing_utils/metrics_manager_util.cpp b/cmds/statsd/src/metrics/parsing_utils/metrics_manager_util.cpp
index ff7938cc7243..bfa409cb8a64 100644
--- a/cmds/statsd/src/metrics/parsing_utils/metrics_manager_util.cpp
+++ b/cmds/statsd/src/metrics/parsing_utils/metrics_manager_util.cpp
@@ -519,6 +519,7 @@ optional<sp<MetricProducer>> createDurationMetricProducerAndUpdateMetadata(
     translateFieldMatcher(metric.dimensions_in_what(), &dimensionsInWhat);
     for (const auto& stateLink : metric.state_link()) {
         if (!handleMetricWithStateLink(stateLink.fields_in_what(), dimensionsInWhat)) {
+            ALOGW("DurationMetric's MetricStateLinks must be a subset of dimensions in what");
             return nullopt;
         }
     }
@@ -537,11 +538,15 @@ optional<sp<MetricProducer>> createDurationMetricProducerAndUpdateMetadata(
         return nullopt;
     }
 
-    return {new DurationMetricProducer(key, metric, conditionIndex, initialConditionCache,
-                                       whatIndex, startIndex, stopIndex, stopAllIndex, nesting,
-                                       wizard, metricHash, internalDimensions, timeBaseNs,
-                                       currentTimeNs, eventActivationMap, eventDeactivationMap,
-                                       slicedStateAtoms, stateGroupMap)};
+    sp<MetricProducer> producer = new DurationMetricProducer(
+            key, metric, conditionIndex, initialConditionCache, whatIndex, startIndex, stopIndex,
+            stopAllIndex, nesting, wizard, metricHash, internalDimensions, timeBaseNs,
+            currentTimeNs, eventActivationMap, eventDeactivationMap, slicedStateAtoms,
+            stateGroupMap);
+    if (!producer->isValid()) {
+        return nullopt;
+    }
+    return {producer};
 }
 
 optional<sp<MetricProducer>> createEventMetricProducerAndUpdateMetadata(
@@ -679,6 +684,7 @@ optional<sp<MetricProducer>> createValueMetricProducerAndUpdateMetadata(
     translateFieldMatcher(metric.dimensions_in_what(), &dimensionsInWhat);
     for (const auto& stateLink : metric.state_link()) {
         if (!handleMetricWithStateLink(stateLink.fields_in_what(), dimensionsInWhat)) {
+            ALOGW("ValueMetric's MetricStateLinks must be a subset of the dimensions in what");
             return nullopt;
         }
     }
diff --git a/cmds/statsd/tests/e2e/DurationMetric_e2e_test.cpp b/cmds/statsd/tests/e2e/DurationMetric_e2e_test.cpp
index 4efb038e538d..2473c1ca1101 100644
--- a/cmds/statsd/tests/e2e/DurationMetric_e2e_test.cpp
+++ b/cmds/statsd/tests/e2e/DurationMetric_e2e_test.cpp
@@ -486,11 +486,11 @@ TEST(DurationMetricE2eTest, TestWithSlicedCondition) {
     // Links between wakelock state atom and condition of app is in background.
     auto links = durationMetric->add_links();
     links->set_condition(isInBackgroundPredicate.id());
-    auto dimensionWhat = links->mutable_fields_in_what();
-    dimensionWhat->set_field(util::WAKELOCK_STATE_CHANGED);
-    dimensionWhat->add_child()->set_field(1);  // uid field.
-    *links->mutable_fields_in_condition() = CreateAttributionUidDimensions(
-            util::ACTIVITY_FOREGROUND_STATE_CHANGED, {Position::FIRST});
+    *links->mutable_fields_in_what() =
+            CreateAttributionUidDimensions(util::WAKELOCK_STATE_CHANGED, {Position::FIRST});
+    auto dimensionCondition = links->mutable_fields_in_condition();
+    dimensionCondition->set_field(util::ACTIVITY_FOREGROUND_STATE_CHANGED);
+    dimensionCondition->add_child()->set_field(1);  // uid field.
 
     ConfigKey cfgKey;
     uint64_t bucketStartTimeNs = 10000000000;
@@ -591,11 +591,11 @@ TEST(DurationMetricE2eTest, TestWithActivationAndSlicedCondition) {
     // Links between wakelock state atom and condition of app is in background.
     auto links = durationMetric->add_links();
     links->set_condition(isInBackgroundPredicate.id());
-    auto dimensionWhat = links->mutable_fields_in_what();
-    dimensionWhat->set_field(util::WAKELOCK_STATE_CHANGED);
-    dimensionWhat->add_child()->set_field(1);  // uid field.
-    *links->mutable_fields_in_condition() = CreateAttributionUidDimensions(
-            util::ACTIVITY_FOREGROUND_STATE_CHANGED, {Position::FIRST});
+    *links->mutable_fields_in_what() =
+            CreateAttributionUidDimensions(util::WAKELOCK_STATE_CHANGED, {Position::FIRST});
+    auto dimensionCondition = links->mutable_fields_in_condition();
+    dimensionCondition->set_field(util::ACTIVITY_FOREGROUND_STATE_CHANGED);
+    dimensionCondition->add_child()->set_field(1);  // uid field.
 
     auto metric_activation1 = config.add_metric_activation();
     metric_activation1->set_metric_id(durationMetric->id());
@@ -1228,6 +1228,9 @@ TEST(DurationMetricE2eTest, TestWithSlicedStatePrimaryFieldsSubset) {
     *config.add_atom_matcher() = CreateReleaseWakelockAtomMatcher();
 
     auto holdingWakelockPredicate = CreateHoldingWakelockPredicate();
+    *(holdingWakelockPredicate.mutable_simple_predicate()->mutable_dimensions()) =
+            CreateAttributionUidAndOtherDimensions(util::WAKELOCK_STATE_CHANGED, {Position::FIRST},
+                                                   {3 /* tag */});
     *config.add_predicate() = holdingWakelockPredicate;
 
     auto uidProcessState = CreateUidProcessState();
diff --git a/cmds/statsd/tests/metrics/parsing_utils/metrics_manager_util_test.cpp b/cmds/statsd/tests/metrics/parsing_utils/metrics_manager_util_test.cpp
index 88df2ff0caa8..9ab7e47dce43 100644
--- a/cmds/statsd/tests/metrics/parsing_utils/metrics_manager_util_test.cpp
+++ b/cmds/statsd/tests/metrics/parsing_utils/metrics_manager_util_test.cpp
@@ -882,6 +882,43 @@ TEST(MetricsManagerTest, TestCreateAnomalyTrackerDurationTooLong) {
     EXPECT_EQ(createAnomalyTracker(alert, anomalyAlarmMonitor, {{1, 0}}, metricProducers), nullopt);
 }
 
+TEST(MetricsManagerTest, TestCreateDurationProducerDimensionsInWhatInvalid) {
+    StatsdConfig config;
+    config.add_allowed_log_source("AID_ROOT");
+    *config.add_atom_matcher() = CreateAcquireWakelockAtomMatcher();
+    *config.add_atom_matcher() = CreateReleaseWakelockAtomMatcher();
+    *config.add_atom_matcher() = CreateMoveToBackgroundAtomMatcher();
+    *config.add_atom_matcher() = CreateMoveToForegroundAtomMatcher();
+
+    Predicate holdingWakelockPredicate = CreateHoldingWakelockPredicate();
+    // The predicate is dimensioning by first attribution node by uid.
+    FieldMatcher dimensions =
+            CreateAttributionUidDimensions(util::WAKELOCK_STATE_CHANGED, {Position::FIRST});
+    *holdingWakelockPredicate.mutable_simple_predicate()->mutable_dimensions() = dimensions;
+    *config.add_predicate() = holdingWakelockPredicate;
+
+    DurationMetric* durationMetric = config.add_duration_metric();
+    durationMetric->set_id(StringToId("WakelockDuration"));
+    durationMetric->set_what(holdingWakelockPredicate.id());
+    durationMetric->set_aggregation_type(DurationMetric::SUM);
+    // The metric is dimensioning by first attribution node by uid AND tag.
+    // Invalid since the predicate only dimensions by uid.
+    *durationMetric->mutable_dimensions_in_what() = CreateAttributionUidAndOtherDimensions(
+            util::WAKELOCK_STATE_CHANGED, {Position::FIRST}, {3 /* tag */});
+    durationMetric->set_bucket(FIVE_MINUTES);
+
+    ConfigKey key(123, 987);
+    uint64_t timeNs = 456;
+    sp<StatsPullerManager> pullerManager = new StatsPullerManager();
+    sp<AlarmMonitor> anomalyAlarmMonitor;
+    sp<AlarmMonitor> periodicAlarmMonitor;
+    sp<UidMap> uidMap;
+    sp<MetricsManager> metricsManager =
+            new MetricsManager(key, config, timeNs, timeNs, uidMap, pullerManager,
+                               anomalyAlarmMonitor, periodicAlarmMonitor);
+    EXPECT_FALSE(metricsManager->isConfigValid());
+}
+
 }  // namespace statsd
 }  // namespace os
 }  // namespace android