Set current state key earlier and do not reset hasCurrentState

- The current state key was updated to the new state key after the diff
calculations and base setting. There are a few cases where we exit the
current loop which results in the current state key not getting updated.
- When we reset the base, we should not reset the current state. All
instances where hasCurrentState was set to false have been removed.
- I added a unit test for a value metric that is sliced by state and has
a condition.

Bug: 157661456
Test: m statsd_test && adb sync data && adb shell
data/nativetest/statsd_test/statsd_test32

Change-Id: Ib75bd9d08e6785f9e03b4b5984dbdaf86a7e1749

cmds/statsd/src/metrics/MetricProducer.cpp

@@ -98,7 +98,7 @@ void MetricProducer::onMatchedLogEventLocked(const size_t matcherIndex, const Lo
 
     // Stores atom id to primary key pairs for each state atom that the metric is
     // sliced by.
-    std::map<int, HashableDimensionKey> statePrimaryKeys;
+    std::map<int32_t, HashableDimensionKey> statePrimaryKeys;
 
     // For states with primary fields, use MetricStateLinks to get the primary
     // field values from the log event. These values will form a primary key

cmds/statsd/src/metrics/ValueMetricProducer.cpp

@@ -189,11 +189,6 @@ void ValueMetricProducer::onStateChanged(int64_t eventTimeNs, int32_t atomId,
     VLOG("ValueMetric %lld onStateChanged time %lld, State %d, key %s, %d -> %d",
          (long long)mMetricId, (long long)eventTimeNs, atomId, primaryKey.toString().c_str(),
          oldState.mValue.int_value, newState.mValue.int_value);
-    // If condition is not true or metric is not active, we do not need to pull
-    // for this state change.
-    if (mCondition != ConditionState::kTrue || !mIsActive) {
-        return;
-    }
 
     // If old and new states are in the same StateGroup, then we do not need to
     // pull for this state change.
@@ -205,6 +200,12 @@ void ValueMetricProducer::onStateChanged(int64_t eventTimeNs, int32_t atomId,
         return;
     }
 
+    // If condition is not true or metric is not active, we do not need to pull
+    // for this state change.
+    if (mCondition != ConditionState::kTrue || !mIsActive) {
+        return;
+    }
+
     bool isEventLate = eventTimeNs < mCurrentBucketStartTimeNs;
     if (isEventLate) {
         VLOG("Skip event due to late arrival: %lld vs %lld", (long long)eventTimeNs,
@@ -412,7 +413,6 @@ void ValueMetricProducer::resetBase() {
     for (auto& slice : mCurrentBaseInfo) {
         for (auto& baseInfo : slice.second) {
             baseInfo.hasBase = false;
-            baseInfo.hasCurrentState = false;
         }
     }
     mHasGlobalBase = false;
@@ -625,7 +625,6 @@ void ValueMetricProducer::accumulateEvents(const std::vector<std::shared_ptr<Log
             auto it = mCurrentBaseInfo.find(whatKey);
             for (auto& baseInfo : it->second) {
                 baseInfo.hasBase = false;
-                baseInfo.hasCurrentState = false;
             }
         }
     }
@@ -820,6 +819,8 @@ void ValueMetricProducer::onMatchedLogEventInternalLocked(
         Interval& interval = intervals[i];
         interval.valueIndex = i;
         Value value;
+        baseInfo.hasCurrentState = true;
+        baseInfo.currentState = stateKey;
         if (!getDoubleOrLong(event, matcher, value)) {
             VLOG("Failed to get value %d from event %s", i, event.ToString().c_str());
             StatsdStats::getInstance().noteBadValueType(mMetricId);
@@ -907,7 +908,6 @@ void ValueMetricProducer::onMatchedLogEventInternalLocked(
             interval.hasValue = true;
         }
         interval.sampleSize += 1;
-        baseInfo.currentState = stateKey;
     }
 
     // Only trigger the tracker if all intervals are correct

cmds/statsd/src/metrics/ValueMetricProducer.h

@@ -313,6 +313,7 @@ private:
     FRIEND_TEST(ValueMetricProducerTest, TestSlicedState);
     FRIEND_TEST(ValueMetricProducerTest, TestSlicedStateWithMap);
     FRIEND_TEST(ValueMetricProducerTest, TestSlicedStateWithPrimaryField_WithDimensions);
+    FRIEND_TEST(ValueMetricProducerTest, TestSlicedStateWithCondition);
     FRIEND_TEST(ValueMetricProducerTest, TestTrimUnusedDimensionKey);
     FRIEND_TEST(ValueMetricProducerTest, TestUseZeroDefaultBase);
     FRIEND_TEST(ValueMetricProducerTest, TestUseZeroDefaultBaseWithPullFailures);

cmds/statsd/tests/metrics/ValueMetricProducer_test.cpp

@@ -167,6 +167,32 @@ public:
         return valueProducer;
     }
 
+    static sp<ValueMetricProducer> createValueProducerWithConditionAndState(
+            sp<MockStatsPullerManager>& pullerManager, ValueMetric& metric,
+            vector<int32_t> slicedStateAtoms,
+            unordered_map<int, unordered_map<int, int64_t>> stateGroupMap,
+            ConditionState conditionAfterFirstBucketPrepared) {
+        UidMap uidMap;
+        SimpleAtomMatcher atomMatcher;
+        atomMatcher.set_atom_id(tagId);
+        sp<EventMatcherWizard> eventMatcherWizard =
+                new EventMatcherWizard({new SimpleLogMatchingTracker(
+                        atomMatcherId, logEventMatcherIndex, atomMatcher, uidMap)});
+        sp<MockConditionWizard> wizard = new NaggyMock<MockConditionWizard>();
+        EXPECT_CALL(*pullerManager, RegisterReceiver(tagId, kConfigKey, _, _, _))
+                .WillOnce(Return());
+        EXPECT_CALL(*pullerManager, UnRegisterReceiver(tagId, kConfigKey, _))
+                .WillRepeatedly(Return());
+
+        sp<ValueMetricProducer> valueProducer = new ValueMetricProducer(
+                kConfigKey, metric, 0 /* condition tracker index */, {ConditionState::kUnknown},
+                wizard, logEventMatcherIndex, eventMatcherWizard, tagId, bucketStartTimeNs,
+                bucketStartTimeNs, pullerManager, {}, {}, slicedStateAtoms, stateGroupMap);
+        valueProducer->prepareFirstBucket();
+        valueProducer->mCondition = conditionAfterFirstBucketPrepared;
+        return valueProducer;
+    }
+
     static ValueMetric createMetric() {
         ValueMetric metric;
         metric.set_id(metricId);
@@ -188,6 +214,13 @@ public:
         metric.add_slice_by_state(StringToId(state));
         return metric;
     }
+
+    static ValueMetric createMetricWithConditionAndState(string state) {
+        ValueMetric metric = ValueMetricProducerTestHelper::createMetric();
+        metric.set_condition(StringToId("SCREEN_ON"));
+        metric.add_slice_by_state(StringToId(state));
+        return metric;
+    }
 };
 
 // Setup for parameterized tests.
@@ -3893,13 +3926,13 @@ TEST(ValueMetricProducerTest, TestSlicedState) {
                 return true;
             }));
 
+    StateManager::getInstance().clear();
     sp<ValueMetricProducer> valueProducer =
             ValueMetricProducerTestHelper::createValueProducerWithState(
                     pullerManager, metric, {util::SCREEN_STATE_CHANGED}, {});
     EXPECT_EQ(1, valueProducer->mSlicedStateAtoms.size());
 
     // Set up StateManager and check that StateTrackers are initialized.
-    StateManager::getInstance().clear();
     StateManager::getInstance().registerListener(SCREEN_STATE_ATOM_ID, valueProducer);
     EXPECT_EQ(1, StateManager::getInstance().getStateTrackersCount());
     EXPECT_EQ(1, StateManager::getInstance().getListenersCount(SCREEN_STATE_ATOM_ID));
@@ -4105,12 +4138,12 @@ TEST(ValueMetricProducerTest, TestSlicedStateWithMap) {
         }
     }
 
+    StateManager::getInstance().clear();
     sp<ValueMetricProducer> valueProducer =
             ValueMetricProducerTestHelper::createValueProducerWithState(
                     pullerManager, metric, {util::SCREEN_STATE_CHANGED}, stateGroupMap);
 
     // Set up StateManager and check that StateTrackers are initialized.
-    StateManager::getInstance().clear();
     StateManager::getInstance().registerListener(SCREEN_STATE_ATOM_ID, valueProducer);
     EXPECT_EQ(1, StateManager::getInstance().getStateTrackersCount());
     EXPECT_EQ(1, StateManager::getInstance().getListenersCount(SCREEN_STATE_ATOM_ID));
@@ -4357,12 +4390,12 @@ TEST(ValueMetricProducerTest, TestSlicedStateWithPrimaryField_WithDimensions) {
                 return true;
             }));
 
+    StateManager::getInstance().clear();
     sp<ValueMetricProducer> valueProducer =
             ValueMetricProducerTestHelper::createValueProducerWithState(
                     pullerManager, metric, {UID_PROCESS_STATE_ATOM_ID}, {});
 
     // Set up StateManager and check that StateTrackers are initialized.
-    StateManager::getInstance().clear();
     StateManager::getInstance().registerListener(UID_PROCESS_STATE_ATOM_ID, valueProducer);
     EXPECT_EQ(1, StateManager::getInstance().getStateTrackersCount());
     EXPECT_EQ(1, StateManager::getInstance().getListenersCount(UID_PROCESS_STATE_ATOM_ID));
@@ -4722,6 +4755,172 @@ TEST(ValueMetricProducerTest, TestSlicedStateWithPrimaryField_WithDimensions) {
     EXPECT_EQ(5, report.value_metrics().data(4).bucket_info(1).values(0).value_long());
 }
 
+TEST(ValueMetricProducerTest, TestSlicedStateWithCondition) {
+    // Set up ValueMetricProducer.
+    ValueMetric metric = ValueMetricProducerTestHelper::createMetricWithConditionAndState(
+            "BATTERY_SAVER_MODE_STATE");
+    sp<MockStatsPullerManager> pullerManager = new StrictMock<MockStatsPullerManager>();
+    EXPECT_CALL(*pullerManager, Pull(tagId, kConfigKey, _, _, _))
+            // Condition changed to true.
+            .WillOnce(Invoke([](int tagId, const ConfigKey&, const int64_t eventTimeNs,
+                                vector<std::shared_ptr<LogEvent>>* data, bool) {
+                EXPECT_EQ(eventTimeNs, bucketStartTimeNs + 20 * NS_PER_SEC);
+                data->clear();
+                data->push_back(
+                        CreateRepeatedValueLogEvent(tagId, bucketStartTimeNs + 20 * NS_PER_SEC, 3));
+                return true;
+            }))
+            // Battery saver mode state changed to OFF.
+            .WillOnce(Invoke([](int tagId, const ConfigKey&, const int64_t eventTimeNs,
+                                vector<std::shared_ptr<LogEvent>>* data, bool) {
+                EXPECT_EQ(eventTimeNs, bucketStartTimeNs + 30 * NS_PER_SEC);
+                data->clear();
+                data->push_back(
+                        CreateRepeatedValueLogEvent(tagId, bucketStartTimeNs + 30 * NS_PER_SEC, 5));
+                return true;
+            }))
+            // Condition changed to false.
+            .WillOnce(Invoke([](int tagId, const ConfigKey&, const int64_t eventTimeNs,
+                                vector<std::shared_ptr<LogEvent>>* data, bool) {
+                EXPECT_EQ(eventTimeNs, bucket2StartTimeNs + 10 * NS_PER_SEC);
+                data->clear();
+                data->push_back(CreateRepeatedValueLogEvent(
+                        tagId, bucket2StartTimeNs + 10 * NS_PER_SEC, 15));
+                return true;
+            }));
+
+    StateManager::getInstance().clear();
+    sp<ValueMetricProducer> valueProducer =
+            ValueMetricProducerTestHelper::createValueProducerWithConditionAndState(
+                    pullerManager, metric, {util::BATTERY_SAVER_MODE_STATE_CHANGED}, {},
+                    ConditionState::kFalse);
+    EXPECT_EQ(1, valueProducer->mSlicedStateAtoms.size());
+
+    // Set up StateManager and check that StateTrackers are initialized.
+    StateManager::getInstance().registerListener(util::BATTERY_SAVER_MODE_STATE_CHANGED,
+                                                 valueProducer);
+    EXPECT_EQ(1, StateManager::getInstance().getStateTrackersCount());
+    EXPECT_EQ(1, StateManager::getInstance().getListenersCount(
+                         util::BATTERY_SAVER_MODE_STATE_CHANGED));
+
+    // Bucket status after battery saver mode ON event.
+    // Condition is false so we do nothing.
+    unique_ptr<LogEvent> batterySaverOnEvent =
+            CreateBatterySaverOnEvent(/*timestamp=*/bucketStartTimeNs + 10 * NS_PER_SEC);
+    StateManager::getInstance().onLogEvent(*batterySaverOnEvent);
+    EXPECT_EQ(0UL, valueProducer->mCurrentSlicedBucket.size());
+    EXPECT_EQ(0UL, valueProducer->mCurrentBaseInfo.size());
+
+    // Bucket status after condition change to true.
+    valueProducer->onConditionChanged(true, bucketStartTimeNs + 20 * NS_PER_SEC);
+    // Base for dimension key {}
+    ASSERT_EQ(1UL, valueProducer->mCurrentBaseInfo.size());
+    std::unordered_map<HashableDimensionKey, std::vector<ValueMetricProducer::BaseInfo>>::iterator
+            itBase = valueProducer->mCurrentBaseInfo.find(DEFAULT_DIMENSION_KEY);
+    EXPECT_TRUE(itBase->second[0].hasBase);
+    EXPECT_EQ(3, itBase->second[0].base.long_value);
+    EXPECT_TRUE(itBase->second[0].hasCurrentState);
+    ASSERT_EQ(1, itBase->second[0].currentState.getValues().size());
+    EXPECT_EQ(BatterySaverModeStateChanged::ON,
+              itBase->second[0].currentState.getValues()[0].mValue.int_value);
+    // Value for key {{}, -1}
+    ASSERT_EQ(1UL, valueProducer->mCurrentSlicedBucket.size());
+    std::unordered_map<MetricDimensionKey, std::vector<ValueMetricProducer::Interval>>::iterator
+            it = valueProducer->mCurrentSlicedBucket.begin();
+    EXPECT_EQ(0, it->first.getDimensionKeyInWhat().getValues().size());
+    ASSERT_EQ(1, it->first.getStateValuesKey().getValues().size());
+    EXPECT_EQ(-1 /*StateTracker::kUnknown*/,
+              it->first.getStateValuesKey().getValues()[0].mValue.int_value);
+    EXPECT_FALSE(it->second[0].hasValue);
+
+    // Bucket status after battery saver mode OFF event.
+    unique_ptr<LogEvent> batterySaverOffEvent =
+            CreateBatterySaverOffEvent(/*timestamp=*/bucketStartTimeNs + 30 * NS_PER_SEC);
+    StateManager::getInstance().onLogEvent(*batterySaverOffEvent);
+    // Base for dimension key {}
+    ASSERT_EQ(1UL, valueProducer->mCurrentBaseInfo.size());
+    itBase = valueProducer->mCurrentBaseInfo.find(DEFAULT_DIMENSION_KEY);
+    EXPECT_TRUE(itBase->second[0].hasBase);
+    EXPECT_EQ(5, itBase->second[0].base.long_value);
+    EXPECT_TRUE(itBase->second[0].hasCurrentState);
+    ASSERT_EQ(1, itBase->second[0].currentState.getValues().size());
+    EXPECT_EQ(BatterySaverModeStateChanged::OFF,
+              itBase->second[0].currentState.getValues()[0].mValue.int_value);
+    // Value for key {{}, ON}
+    ASSERT_EQ(2UL, valueProducer->mCurrentSlicedBucket.size());
+    it = valueProducer->mCurrentSlicedBucket.begin();
+    EXPECT_EQ(0, it->first.getDimensionKeyInWhat().getValues().size());
+    ASSERT_EQ(1, it->first.getStateValuesKey().getValues().size());
+    EXPECT_EQ(BatterySaverModeStateChanged::ON,
+              it->first.getStateValuesKey().getValues()[0].mValue.int_value);
+    EXPECT_TRUE(it->second[0].hasValue);
+    EXPECT_EQ(2, it->second[0].value.long_value);
+
+    // Pull at end of first bucket.
+    vector<shared_ptr<LogEvent>> allData;
+    allData.clear();
+    allData.push_back(CreateRepeatedValueLogEvent(tagId, bucket2StartTimeNs, 11));
+    valueProducer->onDataPulled(allData, /** succeed */ true, bucket2StartTimeNs);
+
+    EXPECT_EQ(2UL, valueProducer->mPastBuckets.size());
+    EXPECT_EQ(3UL, valueProducer->mCurrentSlicedBucket.size());
+    // Base for dimension key {}
+    ASSERT_EQ(1UL, valueProducer->mCurrentBaseInfo.size());
+    itBase = valueProducer->mCurrentBaseInfo.find(DEFAULT_DIMENSION_KEY);
+    EXPECT_TRUE(itBase->second[0].hasBase);
+    EXPECT_EQ(11, itBase->second[0].base.long_value);
+    EXPECT_TRUE(itBase->second[0].hasCurrentState);
+    ASSERT_EQ(1, itBase->second[0].currentState.getValues().size());
+    EXPECT_EQ(BatterySaverModeStateChanged::OFF,
+              itBase->second[0].currentState.getValues()[0].mValue.int_value);
+
+    // Bucket 2 status after condition change to false.
+    valueProducer->onConditionChanged(false, bucket2StartTimeNs + 10 * NS_PER_SEC);
+    // Base for dimension key {}
+    ASSERT_EQ(1UL, valueProducer->mCurrentBaseInfo.size());
+    itBase = valueProducer->mCurrentBaseInfo.find(DEFAULT_DIMENSION_KEY);
+    EXPECT_FALSE(itBase->second[0].hasBase);
+    EXPECT_TRUE(itBase->second[0].hasCurrentState);
+    ASSERT_EQ(1, itBase->second[0].currentState.getValues().size());
+    EXPECT_EQ(BatterySaverModeStateChanged::OFF,
+              itBase->second[0].currentState.getValues()[0].mValue.int_value);
+    // Value for key {{}, OFF}
+    ASSERT_EQ(3UL, valueProducer->mCurrentSlicedBucket.size());
+    it = valueProducer->mCurrentSlicedBucket.begin();
+    EXPECT_EQ(0, it->first.getDimensionKeyInWhat().getValues().size());
+    ASSERT_EQ(1, it->first.getStateValuesKey().getValues().size());
+    EXPECT_EQ(BatterySaverModeStateChanged::OFF,
+              it->first.getStateValuesKey().getValues()[0].mValue.int_value);
+    EXPECT_TRUE(it->second[0].hasValue);
+    EXPECT_EQ(4, it->second[0].value.long_value);
+
+    // Start dump report and check output.
+    ProtoOutputStream output;
+    std::set<string> strSet;
+    valueProducer->onDumpReport(bucket2StartTimeNs + 50 * NS_PER_SEC,
+                                true /* include recent buckets */, true, NO_TIME_CONSTRAINTS,
+                                &strSet, &output);
+
+    StatsLogReport report = outputStreamToProto(&output);
+    EXPECT_TRUE(report.has_value_metrics());
+    ASSERT_EQ(2, report.value_metrics().data_size());
+
+    ValueMetricData data = report.value_metrics().data(0);
+    EXPECT_EQ(util::BATTERY_SAVER_MODE_STATE_CHANGED, data.slice_by_state(0).atom_id());
+    EXPECT_TRUE(data.slice_by_state(0).has_value());
+    EXPECT_EQ(BatterySaverModeStateChanged::ON, data.slice_by_state(0).value());
+    ASSERT_EQ(1, data.bucket_info_size());
+    EXPECT_EQ(2, data.bucket_info(0).values(0).value_long());
+
+    data = report.value_metrics().data(1);
+    EXPECT_EQ(util::BATTERY_SAVER_MODE_STATE_CHANGED, data.slice_by_state(0).atom_id());
+    EXPECT_TRUE(data.slice_by_state(0).has_value());
+    EXPECT_EQ(BatterySaverModeStateChanged::OFF, data.slice_by_state(0).value());
+    ASSERT_EQ(2, data.bucket_info_size());
+    EXPECT_EQ(6, data.bucket_info(0).values(0).value_long());
+    EXPECT_EQ(4, data.bucket_info(1).values(0).value_long());
+}
+
 /*
  * Test bucket splits when condition is unknown.
  */

cmds/statsd/tests/statsd_test_util.cpp

@@ -663,6 +663,8 @@ std::unique_ptr<LogEvent> CreateBatterySaverOnEvent(uint64_t timestampNs) {
     AStatsEvent_setAtomId(statsEvent, util::BATTERY_SAVER_MODE_STATE_CHANGED);
     AStatsEvent_overwriteTimestamp(statsEvent, timestampNs);
     AStatsEvent_writeInt32(statsEvent, BatterySaverModeStateChanged::ON);
+    AStatsEvent_addBoolAnnotation(statsEvent, util::ANNOTATION_ID_EXCLUSIVE_STATE, true);
+    AStatsEvent_addBoolAnnotation(statsEvent, util::ANNOTATION_ID_STATE_NESTED, false);
 
     std::unique_ptr<LogEvent> logEvent = std::make_unique<LogEvent>(/*uid=*/0, /*pid=*/0);
     parseStatsEventToLogEvent(statsEvent, logEvent.get());
@@ -674,6 +676,8 @@ std::unique_ptr<LogEvent> CreateBatterySaverOffEvent(uint64_t timestampNs) {
     AStatsEvent_setAtomId(statsEvent, util::BATTERY_SAVER_MODE_STATE_CHANGED);
     AStatsEvent_overwriteTimestamp(statsEvent, timestampNs);
     AStatsEvent_writeInt32(statsEvent, BatterySaverModeStateChanged::OFF);
+    AStatsEvent_addBoolAnnotation(statsEvent, util::ANNOTATION_ID_EXCLUSIVE_STATE, true);
+    AStatsEvent_addBoolAnnotation(statsEvent, util::ANNOTATION_ID_STATE_NESTED, false);
 
     std::unique_ptr<LogEvent> logEvent = std::make_unique<LogEvent>(/*uid=*/0, /*pid=*/0);
     parseStatsEventToLogEvent(statsEvent, logEvent.get());