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
diff --git a/cmds/statsd/src/metrics/CountMetricProducer.cpp b/cmds/statsd/src/metrics/CountMetricProducer.cpp
index 21ffff3..d865c21 100644
--- a/cmds/statsd/src/metrics/CountMetricProducer.cpp
+++ b/cmds/statsd/src/metrics/CountMetricProducer.cpp
@@ -122,11 +122,11 @@
}
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 f9a8842..26b3d3c 100644
--- a/cmds/statsd/src/metrics/CountMetricProducer.h
+++ b/cmds/statsd/src/metrics/CountMetricProducer.h
@@ -53,8 +53,8 @@
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 0de92f3..6633659 100644
--- a/cmds/statsd/src/metrics/DurationMetricProducer.cpp
+++ b/cmds/statsd/src/metrics/DurationMetricProducer.cpp
@@ -161,13 +161,12 @@
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 @@
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 6f84076..53f0f28 100644
--- a/cmds/statsd/src/metrics/DurationMetricProducer.h
+++ b/cmds/statsd/src/metrics/DurationMetricProducer.h
@@ -55,8 +55,8 @@
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 28563ad..5fabb5f 100644
--- a/cmds/statsd/src/metrics/MetricProducer.h
+++ b/cmds/statsd/src/metrics/MetricProducer.h
@@ -182,8 +182,8 @@
};
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 bf636a4..f03ce45 100644
--- a/cmds/statsd/src/metrics/ValueMetricProducer.cpp
+++ b/cmds/statsd/src/metrics/ValueMetricProducer.cpp
@@ -182,15 +182,26 @@
}
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 c8dc8cc..751fef2 100644
--- a/cmds/statsd/src/metrics/ValueMetricProducer.h
+++ b/cmds/statsd/src/metrics/ValueMetricProducer.h
@@ -90,7 +90,7 @@
};
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 d1af196..6388001 100644
--- a/cmds/statsd/src/state/StateListener.h
+++ b/cmds/statsd/src/state/StateListener.h
@@ -45,8 +45,8 @@
* [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 b63713b..41e525c 100644
--- a/cmds/statsd/src/state/StateTracker.cpp
+++ b/cmds/statsd/src/state/StateTracker.cpp
@@ -35,31 +35,30 @@
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 @@
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 @@
// 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::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 @@
// 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 c5f6315..abd579e 100644
--- a/cmds/statsd/src/state/StateTracker.h
+++ b/cmds/statsd/src/state/StateTracker.h
@@ -72,19 +72,19 @@
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 b6e1075..474aa22 100644
--- a/cmds/statsd/tests/metrics/ValueMetricProducer_test.cpp
+++ b/cmds/statsd/tests/metrics/ValueMetricProducer_test.cpp
@@ -3898,14 +3898,12 @@
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 @@
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(true, it->second[0].hasValue);
- EXPECT_EQ(4, it->second[0].value.long_value);
+ 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);
+
+ // 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}
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 13e8f5c..530ac5e 100644
--- a/cmds/statsd/tests/state/StateTracker_test.cpp
+++ b/cmds/statsd/tests/state/StateTracker_test.cpp
@@ -50,8 +50,9 @@
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);
}
};