Unit tests for ValueMetricProducer
StatsPullerManager is refactored so that we can mock it.
It may need more refactor pass to make is safer for longer runs.
Test: unit test
Change-Id: Ief0c99710e4d06e1454678f8b749c9599467d114
diff --git a/cmds/statsd/src/StatsService.h b/cmds/statsd/src/StatsService.h
index 1d7e5a61..fa92f65 100644
--- a/cmds/statsd/src/StatsService.h
+++ b/cmds/statsd/src/StatsService.h
@@ -157,7 +157,7 @@
/**
* Fetches external metrics.
*/
- StatsPullerManager& mStatsPullerManager = StatsPullerManager::GetInstance();
+ StatsPullerManager mStatsPullerManager;
/**
* Tracks the configurations that have been passed to statsd.
diff --git a/cmds/statsd/src/external/StatsPullerManager.h b/cmds/statsd/src/external/StatsPullerManager.h
index 67580d6..2e803c9 100644
--- a/cmds/statsd/src/external/StatsPullerManager.h
+++ b/cmds/statsd/src/external/StatsPullerManager.h
@@ -16,71 +16,39 @@
#pragma once
-#include <android/os/IStatsCompanionService.h>
-#include <binder/IServiceManager.h>
-#include <utils/RefBase.h>
-#include <utils/String16.h>
-#include <utils/String8.h>
-#include <utils/threads.h>
-#include <string>
-#include <unordered_map>
-#include <vector>
-#include "PullDataReceiver.h"
-#include "StatsPuller.h"
-#include "logd/LogEvent.h"
+#include "StatsPullerManagerImpl.h"
namespace android {
namespace os {
namespace statsd {
-class StatsPullerManager : public virtual RefBase {
-public:
- static StatsPullerManager& GetInstance();
+class StatsPullerManager{
+ public:
+ virtual ~StatsPullerManager() {}
- void RegisterReceiver(int tagId, sp<PullDataReceiver> receiver, long intervalMs);
+ virtual void RegisterReceiver(int tagId, wp<PullDataReceiver> receiver, long intervalMs) {
+ mPullerManager.RegisterReceiver(tagId, receiver, intervalMs);
+ };
- void UnRegisterReceiver(int tagId, sp<PullDataReceiver> receiver);
+ virtual void UnRegisterReceiver(int tagId, wp<PullDataReceiver> receiver) {
+ mPullerManager.UnRegisterReceiver(tagId, receiver);
+ };
- // Verify if we know how to pull for this matcher
- bool PullerForMatcherExists(int tagId);
+ // Verify if we know how to pull for this matcher
+ bool PullerForMatcherExists(int tagId) {
+ return mPullerManager.PullerForMatcherExists(tagId);
+ }
- void OnAlarmFired();
+ void OnAlarmFired() {
+ mPullerManager.OnAlarmFired();
+ }
- bool Pull(const int pullCode, vector<std::shared_ptr<LogEvent>>* data);
+ virtual bool Pull(const int tagId, vector<std::shared_ptr<LogEvent>>* data) {
+ return mPullerManager.Pull(tagId, data);
+ }
-private:
- StatsPullerManager();
-
- // use this to update alarm
- sp<IStatsCompanionService> mStatsCompanionService = nullptr;
-
- sp<IStatsCompanionService> get_stats_companion_service();
-
- // mapping from simple matcher tagId to puller
- std::map<int, std::shared_ptr<StatsPuller>> mPullers;
-
- typedef struct {
- // pull_interval_sec : last_pull_time_sec
- std::pair<uint64_t, uint64_t> timeInfo;
- sp<PullDataReceiver> receiver;
- } ReceiverInfo;
-
- // mapping from simple matcher tagId to receivers
- std::map<int, std::vector<ReceiverInfo>> mReceivers;
-
- Mutex mReceiversLock;
-
- long mCurrentPullingInterval;
-
- // for pulled metrics, it is important for the buckets to be aligned to multiple of smallest
- // bucket size. All pulled metrics start pulling based on this time, so that they can be
- // correctly attributed to the correct buckets. Pulled data attach a timestamp which is the
- // request time.
- const long mPullStartTimeMs;
-
- long get_pull_start_time_ms();
-
- LogEvent parse_pulled_data(String16 data);
+ private:
+ StatsPullerManagerImpl& mPullerManager = StatsPullerManagerImpl::GetInstance();
};
} // namespace statsd
diff --git a/cmds/statsd/src/external/StatsPullerManager.cpp b/cmds/statsd/src/external/StatsPullerManagerImpl.cpp
similarity index 82%
rename from cmds/statsd/src/external/StatsPullerManager.cpp
rename to cmds/statsd/src/external/StatsPullerManagerImpl.cpp
index 5a05b45..07d0b3e 100644
--- a/cmds/statsd/src/external/StatsPullerManager.cpp
+++ b/cmds/statsd/src/external/StatsPullerManagerImpl.cpp
@@ -25,7 +25,7 @@
#include "CpuTimePerUidPuller.h"
#include "ResourcePowerManagerPuller.h"
#include "StatsCompanionServicePuller.h"
-#include "StatsPullerManager.h"
+#include "StatsPullerManagerImpl.h"
#include "StatsService.h"
#include "logd/LogEvent.h"
#include "statslog.h"
@@ -37,12 +37,13 @@
using std::shared_ptr;
using std::string;
using std::vector;
+using std::list;
namespace android {
namespace os {
namespace statsd {
-StatsPullerManager::StatsPullerManager()
+StatsPullerManagerImpl::StatsPullerManagerImpl()
: mCurrentPullingInterval(LONG_MAX), mPullStartTimeMs(get_pull_start_time_ms()) {
shared_ptr<StatsPuller> statsCompanionServicePuller = make_shared<StatsCompanionServicePuller>();
shared_ptr<StatsPuller> resourcePowerManagerPuller = make_shared<ResourcePowerManagerPuller>();
@@ -71,7 +72,7 @@
mStatsCompanionService = StatsService::getStatsCompanionService();
}
-bool StatsPullerManager::Pull(int tagId, vector<shared_ptr<LogEvent>>* data) {
+bool StatsPullerManagerImpl::Pull(int tagId, vector<shared_ptr<LogEvent>>* data) {
if (DEBUG) ALOGD("Initiating pulling %d", tagId);
if (mPullers.find(tagId) != mPullers.end()) {
@@ -82,26 +83,26 @@
}
}
-StatsPullerManager& StatsPullerManager::GetInstance() {
- static StatsPullerManager instance;
+StatsPullerManagerImpl& StatsPullerManagerImpl::GetInstance() {
+ static StatsPullerManagerImpl instance;
return instance;
}
-bool StatsPullerManager::PullerForMatcherExists(int tagId) {
+bool StatsPullerManagerImpl::PullerForMatcherExists(int tagId) {
return mPullers.find(tagId) != mPullers.end();
}
-long StatsPullerManager::get_pull_start_time_ms() {
+long StatsPullerManagerImpl::get_pull_start_time_ms() {
// TODO: limit and align pull intervals to 10min boundaries if this turns out to be a problem
return time(nullptr) * 1000;
}
-void StatsPullerManager::RegisterReceiver(int tagId, sp<PullDataReceiver> receiver,
- long intervalMs) {
+void StatsPullerManagerImpl::RegisterReceiver(int tagId, wp<PullDataReceiver> receiver,
+ long intervalMs) {
AutoMutex _l(mReceiversLock);
- vector<ReceiverInfo>& receivers = mReceivers[tagId];
+ auto& receivers = mReceivers[tagId];
for (auto it = receivers.begin(); it != receivers.end(); it++) {
- if (it->receiver.get() == receiver.get()) {
+ if (it->receiver == receiver) {
VLOG("Receiver already registered of %d", (int)receivers.size());
return;
}
@@ -124,7 +125,7 @@
VLOG("Puller for tagId %d registered of %d", tagId, (int)receivers.size());
}
-void StatsPullerManager::UnRegisterReceiver(int tagId, sp<PullDataReceiver> receiver) {
+void StatsPullerManagerImpl::UnRegisterReceiver(int tagId, wp<PullDataReceiver> receiver) {
AutoMutex _l(mReceiversLock);
if (mReceivers.find(tagId) == mReceivers.end()) {
VLOG("Unknown pull code or no receivers: %d", tagId);
@@ -132,7 +133,7 @@
}
auto& receivers = mReceivers.find(tagId)->second;
for (auto it = receivers.begin(); it != receivers.end(); it++) {
- if (receiver.get() == it->receiver.get()) {
+ if (receiver == it->receiver) {
receivers.erase(it);
VLOG("Puller for tagId %d unregistered of %d", tagId, (int)receivers.size());
return;
@@ -140,7 +141,7 @@
}
}
-void StatsPullerManager::OnAlarmFired() {
+void StatsPullerManagerImpl::OnAlarmFired() {
AutoMutex _l(mReceiversLock);
uint64_t currentTimeMs = time(nullptr) * 1000;
@@ -165,8 +166,13 @@
vector<shared_ptr<LogEvent>> data;
if (Pull(pullInfo.first, &data)) {
for (const auto& receiverInfo : pullInfo.second) {
- receiverInfo->receiver->onDataPulled(data);
- receiverInfo->timeInfo.second = currentTimeMs;
+ sp<PullDataReceiver> receiverPtr = receiverInfo->receiver.promote();
+ if (receiverPtr != nullptr) {
+ receiverPtr->onDataPulled(data);
+ receiverInfo->timeInfo.second = currentTimeMs;
+ } else {
+ VLOG("receiver already gone.");
+ }
}
}
}
diff --git a/cmds/statsd/src/external/StatsPullerManagerImpl.h b/cmds/statsd/src/external/StatsPullerManagerImpl.h
new file mode 100644
index 0000000..0b9f21e
--- /dev/null
+++ b/cmds/statsd/src/external/StatsPullerManagerImpl.h
@@ -0,0 +1,87 @@
+/*
+ * Copyright (C) 2017 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#pragma once
+
+#include <android/os/IStatsCompanionService.h>
+#include <binder/IServiceManager.h>
+#include <utils/RefBase.h>
+#include <utils/threads.h>
+#include <string>
+#include <unordered_map>
+#include <vector>
+#include <list>
+#include "PullDataReceiver.h"
+#include "StatsPuller.h"
+#include "logd/LogEvent.h"
+
+namespace android {
+namespace os {
+namespace statsd {
+
+class StatsPullerManagerImpl : public virtual RefBase {
+public:
+ static StatsPullerManagerImpl& GetInstance();
+
+ void RegisterReceiver(int tagId, wp<PullDataReceiver> receiver, long intervalMs);
+
+ void UnRegisterReceiver(int tagId, wp<PullDataReceiver> receiver);
+
+ // Verify if we know how to pull for this matcher
+ bool PullerForMatcherExists(int tagId);
+
+ void OnAlarmFired();
+
+ bool Pull(const int tagId, vector<std::shared_ptr<LogEvent>>* data);
+
+private:
+ StatsPullerManagerImpl();
+
+ // use this to update alarm
+ sp<IStatsCompanionService> mStatsCompanionService = nullptr;
+
+ sp<IStatsCompanionService> get_stats_companion_service();
+
+ // mapping from simple matcher tagId to puller
+ std::map<int, std::shared_ptr<StatsPuller>> mPullers;
+
+ typedef struct {
+ // pull_interval_sec : last_pull_time_sec
+ std::pair<uint64_t, uint64_t> timeInfo;
+ wp<PullDataReceiver> receiver;
+ } ReceiverInfo;
+
+ // mapping from simple matcher tagId to receivers
+ std::map<int, std::list<ReceiverInfo>> mReceivers;
+
+ Mutex mReceiversLock;
+
+ long mCurrentPullingInterval;
+
+ // for pulled metrics, it is important for the buckets to be aligned to multiple of smallest
+ // bucket size. All pulled metrics start pulling based on this time, so that they can be
+ // correctly attributed to the correct buckets. Pulled data attach a timestamp which is the
+ // request time.
+ const long mPullStartTimeMs;
+
+ long get_pull_start_time_ms();
+
+ LogEvent parse_pulled_data(String16 data);
+};
+
+} // namespace statsd
+} // namespace os
+} // namespace android
diff --git a/cmds/statsd/src/metrics/GaugeMetricProducer.h b/cmds/statsd/src/metrics/GaugeMetricProducer.h
index d80672d..f9e4deb 100644
--- a/cmds/statsd/src/metrics/GaugeMetricProducer.h
+++ b/cmds/statsd/src/metrics/GaugeMetricProducer.h
@@ -81,7 +81,7 @@
static const uint64_t kDefaultGaugemBucketSizeNs = 1000 * 1000 * 1000;
const GaugeMetric mMetric;
- StatsPullerManager& mStatsPullerManager = StatsPullerManager::GetInstance();
+ StatsPullerManager mStatsPullerManager;
// tagId for pulled data. -1 if this is not pulled
const int mPullTagId;
diff --git a/cmds/statsd/src/metrics/ValueMetricProducer.cpp b/cmds/statsd/src/metrics/ValueMetricProducer.cpp
index 5bd10fa..5cffec1 100644
--- a/cmds/statsd/src/metrics/ValueMetricProducer.cpp
+++ b/cmds/statsd/src/metrics/ValueMetricProducer.cpp
@@ -31,6 +31,7 @@
using android::util::FIELD_TYPE_MESSAGE;
using android::util::ProtoOutputStream;
using std::list;
+using std::make_pair;
using std::make_shared;
using std::map;
using std::shared_ptr;
@@ -62,13 +63,23 @@
const int FIELD_ID_END_BUCKET_NANOS = 2;
const int FIELD_ID_VALUE = 3;
+static const uint64_t kDefaultBucketSizeMillis = 60 * 60 * 1000L;
+
// ValueMetric has a minimum bucket size of 10min so that we don't pull too frequently
ValueMetricProducer::ValueMetricProducer(const ValueMetric& metric, const int conditionIndex,
const sp<ConditionWizard>& wizard, const int pullTagId,
- const uint64_t startTimeNs)
- : MetricProducer(startTimeNs, conditionIndex, wizard), mMetric(metric), mPullTagId(pullTagId) {
+ const uint64_t startTimeNs,
+ shared_ptr<StatsPullerManager> statsPullerManager)
+ : MetricProducer(startTimeNs, conditionIndex, wizard),
+ mMetric(metric),
+ mStatsPullerManager(statsPullerManager),
+ mPullTagId(pullTagId) {
// TODO: valuemetric for pushed events may need unlimited bucket length
- mBucketSizeNs = mMetric.bucket().bucket_size_millis() * 1000 * 1000;
+ if (metric.has_bucket() && metric.bucket().has_bucket_size_millis()) {
+ mBucketSizeNs = mMetric.bucket().bucket_size_millis() * 1000 * 1000;
+ } else {
+ mBucketSizeNs = kDefaultBucketSizeMillis * 1000 * 1000;
+ }
mDimension.insert(mDimension.begin(), metric.dimension().begin(), metric.dimension().end());
@@ -79,8 +90,9 @@
}
if (!metric.has_condition() && mPullTagId != -1) {
- mStatsPullerManager.RegisterReceiver(mPullTagId, this,
- metric.bucket().bucket_size_millis());
+ VLOG("Setting up periodic pulling for %d", mPullTagId);
+ mStatsPullerManager->RegisterReceiver(mPullTagId, this,
+ metric.bucket().bucket_size_millis());
}
startNewProtoOutputStream(mStartTimeNs);
@@ -89,8 +101,19 @@
(long long)mBucketSizeNs, (long long)mStartTimeNs);
}
+// for testing
+ValueMetricProducer::ValueMetricProducer(const ValueMetric& metric, const int conditionIndex,
+ const sp<ConditionWizard>& wizard, const int pullTagId,
+ const uint64_t startTimeNs)
+ : ValueMetricProducer(metric, conditionIndex, wizard, pullTagId, startTimeNs,
+ make_shared<StatsPullerManager>()) {
+}
+
ValueMetricProducer::~ValueMetricProducer() {
VLOG("~ValueMetricProducer() called");
+ if (mPullTagId != -1) {
+ mStatsPullerManager->UnRegisterReceiver(mPullTagId, this);
+ }
}
void ValueMetricProducer::startNewProtoOutputStream(long long startTime) {
@@ -177,14 +200,14 @@
if (mPullTagId != -1) {
if (mCondition == true) {
- mStatsPullerManager.RegisterReceiver(mPullTagId, this,
- mMetric.bucket().bucket_size_millis());
- } else if (mCondition == ConditionState::kFalse) {
- mStatsPullerManager.UnRegisterReceiver(mPullTagId, this);
+ mStatsPullerManager->RegisterReceiver(mPullTagId, this,
+ mMetric.bucket().bucket_size_millis());
+ } else if (mCondition == false) {
+ mStatsPullerManager->UnRegisterReceiver(mPullTagId, this);
}
vector<shared_ptr<LogEvent>> allData;
- if (mStatsPullerManager.Pull(mPullTagId, &allData)) {
+ if (mStatsPullerManager->Pull(mPullTagId, &allData)) {
if (allData.size() == 0) {
return;
}
@@ -199,11 +222,15 @@
void ValueMetricProducer::onDataPulled(const std::vector<std::shared_ptr<LogEvent>>& allData) {
AutoMutex _l(mLock);
- if (mCondition == ConditionState::kTrue || !mMetric.has_condition()) {
+ if (mCondition == true || !mMetric.has_condition()) {
if (allData.size() == 0) {
return;
}
uint64_t eventTime = allData.at(0)->GetTimestampNs();
+ // alarm is not accurate and might drift.
+ if (eventTime > mCurrentBucketStartTimeNs + mBucketSizeNs * 3 / 2) {
+ flush_if_needed(eventTime);
+ }
for (const auto& data : allData) {
onMatchedLogEvent(0, *data, true);
}
@@ -226,24 +253,36 @@
long value = get_value(event);
- if (scheduledPull) {
- if (interval.raw.size() > 0) {
- interval.raw.back().second = value;
- } else {
- interval.raw.push_back(std::make_pair(value, value));
- }
- mNextSlicedBucket[eventKey].raw[0].first = value;
- } else {
- if (mCondition == ConditionState::kTrue) {
- interval.raw.push_back(std::make_pair(value, 0));
- } else {
- if (interval.raw.size() != 0) {
+ if (mPullTagId != -1) {
+ if (scheduledPull) {
+ // scheduled pull always sets beginning of current bucket and end
+ // of next bucket
+ if (interval.raw.size() > 0) {
interval.raw.back().second = value;
+ } else {
+ interval.raw.push_back(make_pair(value, value));
+ }
+ Interval& nextInterval = mNextSlicedBucket[eventKey];
+ if (nextInterval.raw.size() == 0) {
+ nextInterval.raw.push_back(make_pair(value, 0));
+ } else {
+ nextInterval.raw.front().first = value;
+ }
+ } else {
+ if (mCondition == true) {
+ interval.raw.push_back(make_pair(value, 0));
+ } else {
+ if (interval.raw.size() != 0) {
+ interval.raw.back().second = value;
+ } else {
+ interval.tainted = true;
+ VLOG("Data on condition true missing!");
+ }
}
}
- }
- if (mPullTagId == -1) {
+ } else {
flush_if_needed(eventTimeNs);
+ interval.raw.push_back(make_pair(value, 0));
}
}
@@ -253,7 +292,7 @@
if (err == NO_ERROR) {
return val;
} else {
- VLOG("Can't find value in message.");
+ VLOG("Can't find value in message. %s", event.ToString().c_str());
return 0;
}
}
@@ -271,13 +310,21 @@
info.mBucketStartNs = mCurrentBucketStartTimeNs;
info.mBucketEndNs = mCurrentBucketStartTimeNs + mBucketSizeNs;
+ int tainted = 0;
for (const auto& slice : mCurrentSlicedBucket) {
long value = 0;
- for (const auto& pair : slice.second.raw) {
- value += pair.second - pair.first;
+ if (mPullTagId != -1) {
+ for (const auto& pair : slice.second.raw) {
+ value += (pair.second - pair.first);
+ }
+ } else {
+ for (const auto& pair : slice.second.raw) {
+ value += pair.first;
+ }
}
+ tainted += slice.second.tainted;
info.mValue = value;
- VLOG(" %s, %ld", slice.first.c_str(), value);
+ VLOG(" %s, %ld, %d", slice.first.c_str(), value, tainted);
// it will auto create new vector of ValuebucketInfo if the key is not found.
auto& bucketList = mPastBuckets[slice.first];
bucketList.push_back(info);
diff --git a/cmds/statsd/src/metrics/ValueMetricProducer.h b/cmds/statsd/src/metrics/ValueMetricProducer.h
index ef9868b..c6c87f5 100644
--- a/cmds/statsd/src/metrics/ValueMetricProducer.h
+++ b/cmds/statsd/src/metrics/ValueMetricProducer.h
@@ -16,6 +16,7 @@
#pragma once
+#include <gtest/gtest_prod.h>
#include <utils/threads.h>
#include <list>
#include "../condition/ConditionTracker.h"
@@ -71,7 +72,13 @@
private:
const ValueMetric mMetric;
- StatsPullerManager& mStatsPullerManager = StatsPullerManager::GetInstance();
+ std::shared_ptr<StatsPullerManager> mStatsPullerManager;
+
+ // for testing
+ ValueMetricProducer(const ValueMetric& valueMetric, const int conditionIndex,
+ const sp<ConditionWizard>& wizard, const int pullTagId,
+ const uint64_t startTimeNs,
+ std::shared_ptr<StatsPullerManager> statsPullerManager);
Mutex mLock;
@@ -81,6 +88,7 @@
// internal state of a bucket.
typedef struct {
std::vector<std::pair<long, long>> raw;
+ bool tainted;
} Interval;
std::unordered_map<HashableDimensionKey, Interval> mCurrentSlicedBucket;
@@ -97,6 +105,10 @@
void flush_if_needed(const uint64_t eventTimeNs);
size_t mByteSize;
+
+ FRIEND_TEST(ValueMetricProducerTest, TestNonDimensionalEvents);
+ FRIEND_TEST(ValueMetricProducerTest, TestEventsWithNonSlicedCondition);
+ FRIEND_TEST(ValueMetricProducerTest, TestPushedEventsWithoutCondition);
};
} // namespace statsd
diff --git a/cmds/statsd/src/metrics/metrics_manager_util.cpp b/cmds/statsd/src/metrics/metrics_manager_util.cpp
index ca9cdfb..226e4d1 100644
--- a/cmds/statsd/src/metrics/metrics_manager_util.cpp
+++ b/cmds/statsd/src/metrics/metrics_manager_util.cpp
@@ -195,7 +195,7 @@
const int allMetricsCount = config.count_metric_size() + config.duration_metric_size() +
config.event_metric_size() + config.value_metric_size();
allMetricProducers.reserve(allMetricsCount);
- StatsPullerManager& statsPullerManager = StatsPullerManager::GetInstance();
+ StatsPullerManager statsPullerManager;
uint64_t startTimeNs = time(nullptr) * NS_PER_SEC;
// Build MetricProducers for each metric defined in config.
diff --git a/cmds/statsd/src/metrics/metrics_manager_util.h b/cmds/statsd/src/metrics/metrics_manager_util.h
index e089d065..edf3af0 100644
--- a/cmds/statsd/src/metrics/metrics_manager_util.h
+++ b/cmds/statsd/src/metrics/metrics_manager_util.h
@@ -21,7 +21,7 @@
#include <vector>
#include "../condition/ConditionTracker.h"
-#include "../external/StatsPullerManager.h"
+#include "../external/StatsPullerManagerImpl.h"
#include "../matchers/LogMatchingTracker.h"
namespace android {