Fix ValueMetric should only pull on real state changes

In ValueMetricProducer, we now map the new and old state values from
onStateChanged to their correct group ids (no mapping happens if the
metric has no state map). We then check if the group ids are the same
and return if they are. Having the same group id means that the state
values are in the same group and no pull is needed.

onStateChanged was updated to take state values stored in FieldValue
objects instead of as ints. This makes it easier to utilize the
mapStateValue function in MetricProducer.

Bug: b/156428844
Test: m statsd_test && adb sync data && adb shell
data/nativetest/statsd_test/statsd_test

Change-Id: Id8f110db593470b8923e7c4259d70cc5f5bc9147

12 files changed, 90 insertions, 67 deletions

diff --git a/cmds/statsd/src/metrics/CountMetricProducer.cpp b/cmds/statsd/src/metrics/CountMetricProducer.cpp
index 21ffff32f539..d865c2176c1e 100644
--- a/cmds/statsd/src/metrics/CountMetricProducer.cpp
+++ b/cmds/statsd/src/metrics/CountMetricProducer.cpp
@@ -122,11 +122,11 @@ CountMetricProducer::~CountMetricProducer() {
 }
 
 void CountMetricProducer::onStateChanged(const int64_t eventTimeNs, const int32_t atomId,
-                                         const HashableDimensionKey& primaryKey, int oldState,
-                                         int newState) {
+                                         const HashableDimensionKey& primaryKey,
+                                         const FieldValue& oldState, const FieldValue& newState) {
     VLOG("CountMetric %lld onStateChanged time %lld, State%d, key %s, %d -> %d",
          (long long)mMetricId, (long long)eventTimeNs, atomId, primaryKey.toString().c_str(),
-         oldState, newState);
+         oldState.mValue.int_value, newState.mValue.int_value);
 }
 
 void CountMetricProducer::dumpStatesLocked(FILE* out, bool verbose) const {
diff --git a/cmds/statsd/src/metrics/CountMetricProducer.h b/cmds/statsd/src/metrics/CountMetricProducer.h
index f9a8842efc3d..26b3d3cc6722 100644
--- a/cmds/statsd/src/metrics/CountMetricProducer.h
+++ b/cmds/statsd/src/metrics/CountMetricProducer.h
@@ -53,8 +53,8 @@ public:
     virtual ~CountMetricProducer();
 
     void onStateChanged(const int64_t eventTimeNs, const int32_t atomId,
-                        const HashableDimensionKey& primaryKey, int oldState,
-                        int newState) override;
+                        const HashableDimensionKey& primaryKey, const FieldValue& oldState,
+                        const FieldValue& newState) override;
 
 protected:
     void onMatchedLogEventInternalLocked(
diff --git a/cmds/statsd/src/metrics/DurationMetricProducer.cpp b/cmds/statsd/src/metrics/DurationMetricProducer.cpp
index 0de92f3d9f47..663365924829 100644
--- a/cmds/statsd/src/metrics/DurationMetricProducer.cpp
+++ b/cmds/statsd/src/metrics/DurationMetricProducer.cpp
@@ -161,13 +161,12 @@ sp<AnomalyTracker> DurationMetricProducer::addAnomalyTracker(
 
 void DurationMetricProducer::onStateChanged(const int64_t eventTimeNs, const int32_t atomId,
                                             const HashableDimensionKey& primaryKey,
-                                            const int32_t oldState, const int32_t newState) {
-    // Create a FieldValue object to hold the new state.
-    FieldValue value;
-    value.mValue.setInt(newState);
+                                            const FieldValue& oldState,
+                                            const FieldValue& newState) {
     // Check if this metric has a StateMap. If so, map the new state value to
     // the correct state group id.
-    mapStateValue(atomId, &value);
+    FieldValue newStateCopy = newState;
+    mapStateValue(atomId, &newStateCopy);
 
     flushIfNeededLocked(eventTimeNs);
 
@@ -185,7 +184,7 @@ void DurationMetricProducer::onStateChanged(const int64_t eventTimeNs, const int
         if (!containsLinkedStateValues(whatIt.first, primaryKey, mMetric2StateLinks, atomId)) {
             continue;
         }
-        whatIt.second->onStateChanged(eventTimeNs, atomId, value);
+        whatIt.second->onStateChanged(eventTimeNs, atomId, newStateCopy);
     }
 }
 
diff --git a/cmds/statsd/src/metrics/DurationMetricProducer.h b/cmds/statsd/src/metrics/DurationMetricProducer.h
index 6f84076ee6b5..53f0f28c3386 100644
--- a/cmds/statsd/src/metrics/DurationMetricProducer.h
+++ b/cmds/statsd/src/metrics/DurationMetricProducer.h
@@ -55,8 +55,8 @@ public:
                                          const sp<AlarmMonitor>& anomalyAlarmMonitor) override;
 
     void onStateChanged(const int64_t eventTimeNs, const int32_t atomId,
-                        const HashableDimensionKey& primaryKey, const int32_t oldState,
-                        const int32_t newState) override;
+                        const HashableDimensionKey& primaryKey, const FieldValue& oldState,
+                        const FieldValue& newState) override;
 
 protected:
     void onMatchedLogEventLocked(const size_t matcherIndex, const LogEvent& event) override;
diff --git a/cmds/statsd/src/metrics/MetricProducer.h b/cmds/statsd/src/metrics/MetricProducer.h
index 28563ad4b0f5..5fabb5fb6ffc 100644
--- a/cmds/statsd/src/metrics/MetricProducer.h
+++ b/cmds/statsd/src/metrics/MetricProducer.h
@@ -182,8 +182,8 @@ public:
     };
 
     void onStateChanged(const int64_t eventTimeNs, const int32_t atomId,
-                        const HashableDimensionKey& primaryKey, const int32_t oldState,
-                        const int32_t newState){};
+                        const HashableDimensionKey& primaryKey, const FieldValue& oldState,
+                        const FieldValue& newState){};
 
     // Output the metrics data to [protoOutput]. All metrics reports end with the same timestamp.
     // This method clears all the past buckets.
diff --git a/cmds/statsd/src/metrics/ValueMetricProducer.cpp b/cmds/statsd/src/metrics/ValueMetricProducer.cpp
index bf636a4f048d..f03ce4550bc4 100644
--- a/cmds/statsd/src/metrics/ValueMetricProducer.cpp
+++ b/cmds/statsd/src/metrics/ValueMetricProducer.cpp
@@ -182,15 +182,26 @@ ValueMetricProducer::~ValueMetricProducer() {
 }
 
 void ValueMetricProducer::onStateChanged(int64_t eventTimeNs, int32_t atomId,
-                                         const HashableDimensionKey& primaryKey, int oldState,
-                                         int newState) {
+                                         const HashableDimensionKey& primaryKey,
+                                         const FieldValue& oldState, const FieldValue& newState) {
     VLOG("ValueMetric %lld onStateChanged time %lld, State %d, key %s, %d -> %d",
          (long long)mMetricId, (long long)eventTimeNs, atomId, primaryKey.toString().c_str(),
-         oldState, newState);
+         oldState.mValue.int_value, newState.mValue.int_value);
     // If condition is not true, we do not need to pull for this state change.
     if (mCondition != ConditionState::kTrue) {
         return;
     }
+
+    // If old and new states are in the same StateGroup, then we do not need to
+    // pull for this state change.
+    FieldValue oldStateCopy = oldState;
+    FieldValue newStateCopy = newState;
+    mapStateValue(atomId, &oldStateCopy);
+    mapStateValue(atomId, &newStateCopy);
+    if (oldStateCopy == newStateCopy) {
+        return;
+    }
+
     bool isEventLate = eventTimeNs < mCurrentBucketStartTimeNs;
     if (isEventLate) {
         VLOG("Skip event due to late arrival: %lld vs %lld", (long long)eventTimeNs,
diff --git a/cmds/statsd/src/metrics/ValueMetricProducer.h b/cmds/statsd/src/metrics/ValueMetricProducer.h
index c8dc8cc290c4..751fef2bf2b1 100644
--- a/cmds/statsd/src/metrics/ValueMetricProducer.h
+++ b/cmds/statsd/src/metrics/ValueMetricProducer.h
@@ -90,7 +90,7 @@ public:
     };
 
     void onStateChanged(int64_t eventTimeNs, int32_t atomId, const HashableDimensionKey& primaryKey,
-                        int oldState, int newState) override;
+                        const FieldValue& oldState, const FieldValue& newState) override;
 
 protected:
     void onMatchedLogEventInternalLocked(
diff --git a/cmds/statsd/src/state/StateListener.h b/cmds/statsd/src/state/StateListener.h
index d1af1968ac38..63880017ca18 100644
--- a/cmds/statsd/src/state/StateListener.h
+++ b/cmds/statsd/src/state/StateListener.h
@@ -45,8 +45,8 @@ public:
      * [newState]: Current state value after state change
      */
     virtual void onStateChanged(const int64_t eventTimeNs, const int32_t atomId,
-                                const HashableDimensionKey& primaryKey, int oldState,
-                                int newState) = 0;
+                                const HashableDimensionKey& primaryKey, const FieldValue& oldState,
+                                const FieldValue& newState) = 0;
 };
 
 }  // namespace statsd
diff --git a/cmds/statsd/src/state/StateTracker.cpp b/cmds/statsd/src/state/StateTracker.cpp
index b63713b64c5d..41e525c343ba 100644
--- a/cmds/statsd/src/state/StateTracker.cpp
+++ b/cmds/statsd/src/state/StateTracker.cpp
@@ -35,31 +35,30 @@ void StateTracker::onLogEvent(const LogEvent& event) {
     HashableDimensionKey primaryKey;
     filterPrimaryKey(event.getValues(), &primaryKey);
 
-    FieldValue stateValue;
-    if (!getStateFieldValueFromLogEvent(event, &stateValue)) {
+    FieldValue newState;
+    if (!getStateFieldValueFromLogEvent(event, &newState)) {
         ALOGE("StateTracker error extracting state from log event. Missing exclusive state field.");
         clearStateForPrimaryKey(eventTimeNs, primaryKey);
         return;
     }
 
-    mField.setField(stateValue.mField.getField());
+    mField.setField(newState.mField.getField());
 
-    if (stateValue.mValue.getType() != INT) {
+    if (newState.mValue.getType() != INT) {
         ALOGE("StateTracker error extracting state from log event. Type: %d",
-              stateValue.mValue.getType());
+              newState.mValue.getType());
         clearStateForPrimaryKey(eventTimeNs, primaryKey);
         return;
     }
 
-    const int32_t resetState = event.getResetState();
-    if (resetState != -1) {
+    if (int resetState = event.getResetState(); resetState != -1) {
         VLOG("StateTracker new reset state: %d", resetState);
-        handleReset(eventTimeNs, resetState);
+        const FieldValue resetStateFieldValue(mField, Value(resetState));
+        handleReset(eventTimeNs, resetStateFieldValue);
         return;
     }
 
-    const int32_t newState = stateValue.mValue.int_value;
-    const bool nested = stateValue.mAnnotations.isNested();
+    const bool nested = newState.mAnnotations.isNested();
     StateValueInfo* stateValueInfo = &mStateMap[primaryKey];
     updateStateForPrimaryKey(eventTimeNs, primaryKey, newState, nested, stateValueInfo);
 }
@@ -85,7 +84,7 @@ bool StateTracker::getStateValue(const HashableDimensionKey& queryKey, FieldValu
     return false;
 }
 
-void StateTracker::handleReset(const int64_t eventTimeNs, const int32_t newState) {
+void StateTracker::handleReset(const int64_t eventTimeNs, const FieldValue& newState) {
     VLOG("StateTracker handle reset");
     for (auto& [primaryKey, stateValueInfo] : mStateMap) {
         updateStateForPrimaryKey(eventTimeNs, primaryKey, newState,
@@ -102,8 +101,9 @@ void StateTracker::clearStateForPrimaryKey(const int64_t eventTimeNs,
 
     // If there is no entry for the primaryKey in mStateMap, then the state is already
     // kStateUnknown.
+    const FieldValue state(mField, Value(kStateUnknown));
     if (it != mStateMap.end()) {
-        updateStateForPrimaryKey(eventTimeNs, primaryKey, kStateUnknown,
+        updateStateForPrimaryKey(eventTimeNs, primaryKey, state,
                                  false /* nested; treat this state change as not nested */,
                                  &it->second);
     }
@@ -111,22 +111,26 @@ void StateTracker::clearStateForPrimaryKey(const int64_t eventTimeNs,
 
 void StateTracker::updateStateForPrimaryKey(const int64_t eventTimeNs,
                                             const HashableDimensionKey& primaryKey,
-                                            const int32_t newState, const bool nested,
+                                            const FieldValue& newState, const bool nested,
                                             StateValueInfo* stateValueInfo) {
-    const int32_t oldState = stateValueInfo->state;
+    FieldValue oldState;
+    oldState.mField = mField;
+    oldState.mValue.setInt(stateValueInfo->state);
+    const int32_t oldStateValue = stateValueInfo->state;
+    const int32_t newStateValue = newState.mValue.int_value;
 
-    if (kStateUnknown == newState) {
+    if (kStateUnknown == newStateValue) {
         mStateMap.erase(primaryKey);
     }
 
     // Update state map for non-nested counting case.
     // Every state event triggers a state overwrite.
     if (!nested) {
-        stateValueInfo->state = newState;
+        stateValueInfo->state = newStateValue;
         stateValueInfo->count = 1;
 
         // Notify listeners if state has changed.
-        if (oldState != newState) {
+        if (oldStateValue != newStateValue) {
             notifyListeners(eventTimeNs, primaryKey, oldState, newState);
         }
         return;
@@ -142,26 +146,26 @@ void StateTracker::updateStateForPrimaryKey(const int64_t eventTimeNs,
     // In atoms.proto, a state atom with nested counting enabled
     // must only have 2 states. There is no enforcemnt here of this requirement.
     // The atom must be logged correctly.
-    if (kStateUnknown == newState) {
-        if (kStateUnknown != oldState) {
+    if (kStateUnknown == newStateValue) {
+        if (kStateUnknown != oldStateValue) {
             notifyListeners(eventTimeNs, primaryKey, oldState, newState);
         }
-    } else if (oldState == kStateUnknown) {
-        stateValueInfo->state = newState;
+    } else if (oldStateValue == kStateUnknown) {
+        stateValueInfo->state = newStateValue;
         stateValueInfo->count = 1;
         notifyListeners(eventTimeNs, primaryKey, oldState, newState);
-    } else if (oldState == newState) {
+    } else if (oldStateValue == newStateValue) {
         stateValueInfo->count++;
     } else if (--stateValueInfo->count == 0) {
-        stateValueInfo->state = newState;
+        stateValueInfo->state = newStateValue;
         stateValueInfo->count = 1;
         notifyListeners(eventTimeNs, primaryKey, oldState, newState);
     }
 }
 
 void StateTracker::notifyListeners(const int64_t eventTimeNs,
-                                   const HashableDimensionKey& primaryKey, const int32_t oldState,
-                                   const int32_t newState) {
+                                   const HashableDimensionKey& primaryKey,
+                                   const FieldValue& oldState, const FieldValue& newState) {
     for (auto l : mListeners) {
         auto sl = l.promote();
         if (sl != nullptr) {
diff --git a/cmds/statsd/src/state/StateTracker.h b/cmds/statsd/src/state/StateTracker.h
index c5f6315fc992..abd579e7e302 100644
--- a/cmds/statsd/src/state/StateTracker.h
+++ b/cmds/statsd/src/state/StateTracker.h
@@ -72,19 +72,19 @@ private:
     std::set<wp<StateListener>> mListeners;
 
     // Reset all state values in map to the given state.
-    void handleReset(const int64_t eventTimeNs, const int32_t newState);
+    void handleReset(const int64_t eventTimeNs, const FieldValue& newState);
 
     // Clears the state value mapped to the given primary key by setting it to kStateUnknown.
     void clearStateForPrimaryKey(const int64_t eventTimeNs, const HashableDimensionKey& primaryKey);
 
     // Update the StateMap based on the received state value.
     void updateStateForPrimaryKey(const int64_t eventTimeNs, const HashableDimensionKey& primaryKey,
-                                  const int32_t newState, const bool nested,
+                                  const FieldValue& newState, const bool nested,
                                   StateValueInfo* stateValueInfo);
 
     // Notify registered state listeners of state change.
     void notifyListeners(const int64_t eventTimeNs, const HashableDimensionKey& primaryKey,
-                         const int32_t oldState, const int32_t newState);
+                         const FieldValue& oldState, const FieldValue& newState);
 };
 
 bool getStateFieldValueFromLogEvent(const LogEvent& event, FieldValue* output);
diff --git a/cmds/statsd/tests/metrics/ValueMetricProducer_test.cpp b/cmds/statsd/tests/metrics/ValueMetricProducer_test.cpp
index b6e1075bcb72..474aa2234837 100644
--- a/cmds/statsd/tests/metrics/ValueMetricProducer_test.cpp
+++ b/cmds/statsd/tests/metrics/ValueMetricProducer_test.cpp
@@ -3898,14 +3898,12 @@ TEST(ValueMetricProducerTest, TestSlicedStateWithMap) {
                 data->push_back(CreateRepeatedValueLogEvent(tagId, bucketStartTimeNs + 5, 5));
                 return true;
             }))
-            // Screen state change to VR.
-            .WillOnce(Invoke([](int tagId, const ConfigKey&, const int64_t eventTimeNs,
-                                vector<std::shared_ptr<LogEvent>>* data, bool) {
-                EXPECT_EQ(eventTimeNs, bucketStartTimeNs + 10);
-                data->clear();
-                data->push_back(CreateRepeatedValueLogEvent(tagId, bucketStartTimeNs + 10, 9));
-                return true;
-            }))
+            // Screen state change to VR has no pull because it is in the same
+            // state group as ON.
+
+            // Screen state change to ON has no pull because it is in the same
+            // state group as VR.
+
             // Screen state change to OFF.
             .WillOnce(Invoke([](int tagId, const ConfigKey&, const int64_t eventTimeNs,
                                 vector<std::shared_ptr<LogEvent>>* data, bool) {
@@ -3969,23 +3967,33 @@ TEST(ValueMetricProducerTest, TestSlicedStateWithMap) {
     EXPECT_EQ(true, it->second[0].hasValue);
     EXPECT_EQ(2, it->second[0].value.long_value);
 
-    // Bucket status after screen state change ON->VR (also ON).
+    // Bucket status after screen state change ON->VR.
+    // Both ON and VR are in the same state group, so the base should not change.
     screenEvent = CreateScreenStateChangedEvent(bucketStartTimeNs + 10,
                                                 android::view::DisplayStateEnum::DISPLAY_STATE_VR);
     StateManager::getInstance().onLogEvent(*screenEvent);
-    ASSERT_EQ(2UL, valueProducer->mCurrentSlicedBucket.size());
+    ASSERT_EQ(1UL, valueProducer->mCurrentSlicedBucket.size());
     // Base for dimension key {}
     it = valueProducer->mCurrentSlicedBucket.begin();
     itBase = valueProducer->mCurrentBaseInfo.find(it->first.getDimensionKeyInWhat());
     EXPECT_EQ(true, itBase->second[0].hasBase);
-    EXPECT_EQ(9, itBase->second[0].base.long_value);
-    // Value for dimension, state key {{}, ON GROUP}
-    EXPECT_EQ(screenOnGroup.group_id(),
-              it->first.getStateValuesKey().getValues()[0].mValue.long_value);
+    EXPECT_EQ(5, itBase->second[0].base.long_value);
+    // Value for dimension, state key {{}, kStateUnknown}
     EXPECT_EQ(true, it->second[0].hasValue);
-    EXPECT_EQ(4, it->second[0].value.long_value);
+    EXPECT_EQ(2, it->second[0].value.long_value);
+
+    // Bucket status after screen state change VR->ON.
+    // Both ON and VR are in the same state group, so the base should not change.
+    screenEvent = CreateScreenStateChangedEvent(bucketStartTimeNs + 12,
+                                                android::view::DisplayStateEnum::DISPLAY_STATE_ON);
+    StateManager::getInstance().onLogEvent(*screenEvent);
+    EXPECT_EQ(1UL, valueProducer->mCurrentSlicedBucket.size());
+    // Base for dimension key {}
+    it = valueProducer->mCurrentSlicedBucket.begin();
+    itBase = valueProducer->mCurrentBaseInfo.find(it->first.getDimensionKeyInWhat());
+    EXPECT_EQ(true, itBase->second[0].hasBase);
+    EXPECT_EQ(5, itBase->second[0].base.long_value);
     // Value for dimension, state key {{}, kStateUnknown}
-    it++;
     EXPECT_EQ(true, it->second[0].hasValue);
     EXPECT_EQ(2, it->second[0].value.long_value);
 
diff --git a/cmds/statsd/tests/state/StateTracker_test.cpp b/cmds/statsd/tests/state/StateTracker_test.cpp
index 13e8f5c343f2..530ac5e01f3e 100644
--- a/cmds/statsd/tests/state/StateTracker_test.cpp
+++ b/cmds/statsd/tests/state/StateTracker_test.cpp
@@ -50,8 +50,9 @@ public:
     std::vector<Update> updates;
 
     void onStateChanged(const int64_t eventTimeNs, const int32_t atomId,
-                        const HashableDimensionKey& primaryKey, int oldState, int newState) {
-        updates.emplace_back(primaryKey, newState);
+                        const HashableDimensionKey& primaryKey, const FieldValue& oldState,
+                        const FieldValue& newState) {
+        updates.emplace_back(primaryKey, newState.mValue.int_value);
     }
 };