Snap for 8902501 from abcad7b838eec4a525da9adcd0884552fbf8ea21 to mainline-go-networking-release Change-Id: Ie7b77bb324516e6b2825d074365c5072d7eb57b5
diff --git a/service/Android.bp b/service/Android.bp index bad74ce..5e96b1e 100644 --- a/service/Android.bp +++ b/service/Android.bp
@@ -25,6 +25,7 @@ java_library { name: "service-statsd", + defaults: ["standalone-system-server-module-optimize-defaults"], srcs: [ ":service-statsd-sources" ], sdk_version: "system_server_current", libs: [
diff --git a/statsd/src/metrics/DurationMetricProducer.cpp b/statsd/src/metrics/DurationMetricProducer.cpp index 5b7a1c1..b32a170 100644 --- a/statsd/src/metrics/DurationMetricProducer.cpp +++ b/statsd/src/metrics/DurationMetricProducer.cpp
@@ -434,27 +434,28 @@ onSlicedConditionMayChangeInternalLocked(overallCondition, eventTime); } -void DurationMetricProducer::onActiveStateChangedLocked(const int64_t eventTimeNs) { - MetricProducer::onActiveStateChangedLocked(eventTimeNs); +void DurationMetricProducer::onActiveStateChangedLocked(const int64_t eventTimeNs, + const bool isActive) { + MetricProducer::onActiveStateChangedLocked(eventTimeNs, isActive); if (!mConditionSliced) { if (ConditionState::kTrue != mCondition) { return; } - if (mIsActive) { + if (isActive) { flushIfNeededLocked(eventTimeNs); } for (auto& whatIt : mCurrentSlicedDurationTrackerMap) { - whatIt.second->onConditionChanged(mIsActive, eventTimeNs); + whatIt.second->onConditionChanged(isActive, eventTimeNs); } - } else if (mIsActive) { + } else if (isActive) { flushIfNeededLocked(eventTimeNs); - onSlicedConditionMayChangeInternalLocked(mIsActive, eventTimeNs); - } else { // mConditionSliced == true && !mIsActive + onSlicedConditionMayChangeInternalLocked(isActive, eventTimeNs); + } else { // mConditionSliced == true && !isActive for (auto& whatIt : mCurrentSlicedDurationTrackerMap) { - whatIt.second->onConditionChanged(mIsActive, eventTimeNs); + whatIt.second->onConditionChanged(isActive, eventTimeNs); } } }
diff --git a/statsd/src/metrics/DurationMetricProducer.h b/statsd/src/metrics/DurationMetricProducer.h index 206882b..ba5fe95 100644 --- a/statsd/src/metrics/DurationMetricProducer.h +++ b/statsd/src/metrics/DurationMetricProducer.h
@@ -99,7 +99,7 @@ void onConditionChangedLocked(const bool conditionMet, const int64_t eventTime) override; // Internal interface to handle active state change. - void onActiveStateChangedLocked(const int64_t eventTimeNs) override; + void onActiveStateChangedLocked(const int64_t eventTimeNs, const bool isActive) override; // Internal interface to handle sliced condition change. void onSlicedConditionMayChangeLocked(bool overallCondition, const int64_t eventTime) override;
diff --git a/statsd/src/metrics/GaugeMetricProducer.cpp b/statsd/src/metrics/GaugeMetricProducer.cpp index 1627742..faa284c 100644 --- a/statsd/src/metrics/GaugeMetricProducer.cpp +++ b/statsd/src/metrics/GaugeMetricProducer.cpp
@@ -406,12 +406,15 @@ } } -void GaugeMetricProducer::onActiveStateChangedLocked(const int64_t eventTimeNs) { - MetricProducer::onActiveStateChangedLocked(eventTimeNs); - if (ConditionState::kTrue != mCondition || !mIsPulled) { +void GaugeMetricProducer::onActiveStateChangedLocked(const int64_t eventTimeNs, + const bool isActive) { + MetricProducer::onActiveStateChangedLocked(eventTimeNs, isActive); + + if (ConditionState::kTrue != mCondition) { return; } - if (mTriggerAtomId == -1 || (mIsActive && mSamplingType == GaugeMetric::RANDOM_ONE_SAMPLE)) { + + if (isActive && mIsPulled && mSamplingType == GaugeMetric::RANDOM_ONE_SAMPLE) { pullAndMatchEventsLocked(eventTimeNs); } } @@ -426,7 +429,9 @@ } flushIfNeededLocked(eventTimeNs); - if (conditionMet && mIsPulled && mTriggerAtomId == -1) { + if (conditionMet && mIsPulled && + (mSamplingType == GaugeMetric::RANDOM_ONE_SAMPLE || + mSamplingType == GaugeMetric::CONDITION_CHANGE_TO_TRUE)) { pullAndMatchEventsLocked(eventTimeNs); } // else: Push mode. No need to proactively pull the gauge data. } @@ -641,7 +646,7 @@ VLOG("Gauge gauge metric %lld, dump key value: %s", (long long)mMetricId, slice.first.toString().c_str()); } - } else { + } else if (mIsActive) { mCurrentSkippedBucket.bucketStartTimeNs = mCurrentBucketStartTimeNs; mCurrentSkippedBucket.bucketEndTimeNs = bucketEndTime; if (!maxDropEventsReached()) {
diff --git a/statsd/src/metrics/GaugeMetricProducer.h b/statsd/src/metrics/GaugeMetricProducer.h index 8bd4a8c..af99535 100644 --- a/statsd/src/metrics/GaugeMetricProducer.h +++ b/statsd/src/metrics/GaugeMetricProducer.h
@@ -120,7 +120,7 @@ void onConditionChangedLocked(const bool conditionMet, const int64_t eventTime) override; // Internal interface to handle active state change. - void onActiveStateChangedLocked(const int64_t eventTimeNs) override; + void onActiveStateChangedLocked(const int64_t eventTimeNs, const bool isActive) override; // Internal interface to handle sliced condition change. void onSlicedConditionMayChangeLocked(bool overallCondition, const int64_t eventTime) override;
diff --git a/statsd/src/metrics/MetricProducer.cpp b/statsd/src/metrics/MetricProducer.cpp index 17aa312..75c2476 100644 --- a/statsd/src/metrics/MetricProducer.cpp +++ b/statsd/src/metrics/MetricProducer.cpp
@@ -194,9 +194,13 @@ if (!mIsActive) { return; } - mIsActive = evaluateActiveStateLocked(elapsedTimestampNs); - if (!mIsActive) { - onActiveStateChangedLocked(elapsedTimestampNs); + const bool isActive = evaluateActiveStateLocked(elapsedTimestampNs); + if (!isActive) { // Metric went from active to not active. + onActiveStateChangedLocked(elapsedTimestampNs, false); + + // Set mIsActive to false after onActiveStateChangedLocked to ensure any pulls that occur + // through onActiveStateChangedLocked are processed. + mIsActive = false; } } @@ -215,10 +219,12 @@ } activation->start_ns = elapsedTimestampNs; activation->state = ActivationState::kActive; - bool oldActiveState = mIsActive; - mIsActive = true; - if (!oldActiveState) { // Metric went from not active to active. - onActiveStateChangedLocked(elapsedTimestampNs); + if (!mIsActive) { // Metric was previously inactive and now is active. + // Set mIsActive to true before onActiveStateChangedLocked to ensure any pulls that occur + // through onActiveStateChangedLocked are processed. + mIsActive = true; + + onActiveStateChangedLocked(elapsedTimestampNs, true); } }
diff --git a/statsd/src/metrics/MetricProducer.h b/statsd/src/metrics/MetricProducer.h index 95f8dd4..ab74951 100644 --- a/statsd/src/metrics/MetricProducer.h +++ b/statsd/src/metrics/MetricProducer.h
@@ -394,7 +394,7 @@ /** * Flushes all the data including the current partial bucket. */ - virtual void flushLocked(const int64_t& eventTimeNs) { + void flushLocked(const int64_t& eventTimeNs) { flushIfNeededLocked(eventTimeNs); flushCurrentBucketLocked(eventTimeNs, eventTimeNs); }; @@ -445,8 +445,8 @@ bool evaluateActiveStateLocked(int64_t elapsedTimestampNs); - virtual void onActiveStateChangedLocked(const int64_t eventTimeNs) { - if (!mIsActive) { + virtual void onActiveStateChangedLocked(const int64_t eventTimeNs, const bool isActive) { + if (!isActive) { flushLocked(eventTimeNs); } }
diff --git a/statsd/src/metrics/NumericValueMetricProducer.cpp b/statsd/src/metrics/NumericValueMetricProducer.cpp index 703b07b..4f8ae23 100644 --- a/statsd/src/metrics/NumericValueMetricProducer.cpp +++ b/statsd/src/metrics/NumericValueMetricProducer.cpp
@@ -128,10 +128,11 @@ protoOutput->end(valueToken); } -void NumericValueMetricProducer::onActiveStateChangedInternalLocked(const int64_t eventTimeNs) { +void NumericValueMetricProducer::onActiveStateChangedInternalLocked(const int64_t eventTimeNs, + const bool isActive) { // When active state changes from true to false for pulled metric, clear diff base but don't // reset other counters as we may accumulate more value in the bucket. - if (mUseDiff && !mIsActive) { + if (mUseDiff && !isActive) { resetBase(); } }
diff --git a/statsd/src/metrics/NumericValueMetricProducer.h b/statsd/src/metrics/NumericValueMetricProducer.h index 8eca3f8..1246815 100644 --- a/statsd/src/metrics/NumericValueMetricProducer.h +++ b/statsd/src/metrics/NumericValueMetricProducer.h
@@ -66,7 +66,8 @@ return config.value_metric(configIndex).links(); } - void onActiveStateChangedInternalLocked(const int64_t eventTimeNs) override; + void onActiveStateChangedInternalLocked(const int64_t eventTimeNs, + const bool isActive) override; // Only called when mIsActive and the event is NOT too late. void onConditionChangedInternalLocked(const ConditionState oldCondition,
diff --git a/statsd/src/metrics/ValueMetricProducer.cpp b/statsd/src/metrics/ValueMetricProducer.cpp index f829cb4..0500284 100644 --- a/statsd/src/metrics/ValueMetricProducer.cpp +++ b/statsd/src/metrics/ValueMetricProducer.cpp
@@ -447,19 +447,24 @@ template <typename AggregatedValue, typename DimExtras> void ValueMetricProducer<AggregatedValue, DimExtras>::skipCurrentBucket( const int64_t dropTimeNs, const BucketDropReason reason) { + if (!mIsActive) { + // Don't keep track of skipped buckets if metric is not active. + return; + } + if (!maxDropEventsReached()) { mCurrentSkippedBucket.dropEvents.push_back(buildDropEvent(dropTimeNs, reason)); } mCurrentBucketIsSkipped = true; } -// Handle active state change. Active state change is treated like a condition change: +// Handle active state change. Active state change is *mostly* treated like a condition change: // - drop bucket if active state change event arrives too late // - if condition is true, pull data on active state changes // - ConditionTimer tracks changes based on AND of condition and active state. template <typename AggregatedValue, typename DimExtras> void ValueMetricProducer<AggregatedValue, DimExtras>::onActiveStateChangedLocked( - const int64_t eventTimeNs) { + const int64_t eventTimeNs, const bool isActive) { const bool eventLate = isEventLateLocked(eventTimeNs); if (eventLate) { // Drop bucket because event arrived too late, ie. we are missing data for this bucket. @@ -467,10 +472,9 @@ invalidateCurrentBucket(eventTimeNs, BucketDropReason::EVENT_IN_WRONG_BUCKET); } - // Call parent method once we've verified the validity of current bucket. - MetricProducer::onActiveStateChangedLocked(eventTimeNs); - if (ConditionState::kTrue != mCondition) { + // Call parent method before early return. + MetricProducer::onActiveStateChangedLocked(eventTimeNs, isActive); return; } @@ -480,15 +484,17 @@ pullAndMatchEventsLocked(eventTimeNs); } - onActiveStateChangedInternalLocked(eventTimeNs); + onActiveStateChangedInternalLocked(eventTimeNs, isActive); } - flushIfNeededLocked(eventTimeNs); + // Once any pulls are processed, call through to parent method which might flush the current + // bucket. + MetricProducer::onActiveStateChangedLocked(eventTimeNs, isActive); // Let condition timer know of new active state. - mConditionTimer.onConditionChanged(mIsActive, eventTimeNs); + mConditionTimer.onConditionChanged(isActive, eventTimeNs); - updateCurrentSlicedBucketConditionTimers(mIsActive, eventTimeNs); + updateCurrentSlicedBucketConditionTimers(isActive, eventTimeNs); } template <typename AggregatedValue, typename DimExtras>
diff --git a/statsd/src/metrics/ValueMetricProducer.h b/statsd/src/metrics/ValueMetricProducer.h index 956580a..80d0bb6 100644 --- a/statsd/src/metrics/ValueMetricProducer.h +++ b/statsd/src/metrics/ValueMetricProducer.h
@@ -167,9 +167,10 @@ void clearPastBucketsLocked(const int64_t dumpTimeNs) override; // ValueMetricProducer internal interface to handle active state change. - void onActiveStateChangedLocked(const int64_t eventTimeNs) override; + void onActiveStateChangedLocked(const int64_t eventTimeNs, const bool isActive) override; - virtual void onActiveStateChangedInternalLocked(const int64_t eventTimeNs) { + virtual void onActiveStateChangedInternalLocked(const int64_t eventTimeNs, + const bool isActive) { } // ValueMetricProducer internal interface to handle condition change.
diff --git a/statsd/tests/e2e/GaugeMetric_e2e_pull_test.cpp b/statsd/tests/e2e/GaugeMetric_e2e_pull_test.cpp index 1f79f87..984b72e 100644 --- a/statsd/tests/e2e/GaugeMetric_e2e_pull_test.cpp +++ b/statsd/tests/e2e/GaugeMetric_e2e_pull_test.cpp
@@ -61,6 +61,7 @@ gaugeMetric->set_max_pull_delay_sec(INT_MAX); config.set_hash_strings_in_metric_report(false); gaugeMetric->set_split_bucket_for_app_upgrade(true); + gaugeMetric->set_min_bucket_size_nanos(1000); return config; } @@ -441,10 +442,10 @@ EXPECT_TRUE(processor->mMetricsManagers.begin()->second->isConfigValid()); processor->mPullerManager->ForceClearPullerCache(); - int startBucketNum = processor->mMetricsManagers.begin() - ->second->mAllMetricProducers[0] - ->getCurrentBucketNum(); - EXPECT_GT(startBucketNum, (int64_t)0); + const int startBucketNum = processor->mMetricsManagers.begin() + ->second->mAllMetricProducers[0] + ->getCurrentBucketNum(); + EXPECT_EQ(startBucketNum, 2); EXPECT_FALSE(processor->mMetricsManagers.begin()->second->mAllMetricProducers[0]->isActive()); // When creating the config, the gauge metric producer should register the alarm at the @@ -470,13 +471,28 @@ EXPECT_EQ(pulledAtomStats.atom_id(), ATOM_TAG); EXPECT_EQ(pulledAtomStats.total_pull(), 0); + // Check skipped bucket is not added when metric is not active. + int64_t dumpReportTimeNs = appUpgradeTimeNs + 1; // 10 mins + 3 ns. + vector<uint8_t> buffer; + processor->onDumpReport(cfgKey, dumpReportTimeNs, true /* include_current_partial_bucket */, + true /* erase_data */, ADB_DUMP, NO_TIME_CONSTRAINTS, &buffer); + ConfigMetricsReportList reports; + EXPECT_TRUE(buffer.size() > 0); + EXPECT_TRUE(reports.ParseFromArray(&buffer[0], buffer.size())); + ASSERT_EQ(1, reports.reports_size()); + ASSERT_EQ(1, reports.reports(0).metrics_size()); + StatsLogReport::GaugeMetricDataWrapper gaugeMetrics = + reports.reports(0).metrics(0).gauge_metrics(); + EXPECT_EQ(gaugeMetrics.skipped_size(), 0); + // Pulling alarm arrives on time and reset the sequential pulling alarm. // Event should not be kept. processor->informPullAlarmFired(nextPullTimeNs + 1); // 15 mins + 1 ns. EXPECT_EQ(baseTimeNs + startBucketNum * bucketSizeNs + 2 * bucketSizeNs, nextPullTimeNs); EXPECT_FALSE(processor->mMetricsManagers.begin()->second->mAllMetricProducers[0]->isActive()); - // Activate the metric. A pull occurs upon activation. + // Activate the metric. A pull occurs upon activation. The event is kept. 1 total + // 15 mins + 2 ms const int64_t activationNs = configAddedTimeNs + bucketSizeNs + (2 * 1000 * 1000); // 2 millis. auto batterySaverOnEvent = CreateBatterySaverOnEvent(activationNs); processor->OnLogEvent(batterySaverOnEvent.get()); // 15 mins + 2 ms. @@ -491,19 +507,27 @@ EXPECT_EQ(baseTimeNs + startBucketNum * bucketSizeNs + 4 * bucketSizeNs, nextPullTimeNs); // Create random event to deactivate metric. - auto deactivationEvent = CreateScreenBrightnessChangedEvent(activationNs + ttlNs + 1, 50); + // A pull should not occur here. 3 total. + // 25 mins + 2 ms + 1 ns. + const int64_t deactivationNs = activationNs + ttlNs + 1; + auto deactivationEvent = CreateScreenBrightnessChangedEvent(deactivationNs, 50); processor->OnLogEvent(deactivationEvent.get()); EXPECT_FALSE(processor->mMetricsManagers.begin()->second->mAllMetricProducers[0]->isActive()); // Event should not be kept. 3 total. + // 30 mins + 3 ns. processor->informPullAlarmFired(nextPullTimeNs + 3); EXPECT_EQ(baseTimeNs + startBucketNum * bucketSizeNs + 5 * bucketSizeNs, nextPullTimeNs); + // Event should not be kept. 3 total. + // 35 mins + 2 ns. processor->informPullAlarmFired(nextPullTimeNs + 2); + EXPECT_EQ(baseTimeNs + startBucketNum * bucketSizeNs + 6 * bucketSizeNs, nextPullTimeNs); - ConfigMetricsReportList reports; - vector<uint8_t> buffer; - processor->onDumpReport(cfgKey, configAddedTimeNs + 7 * bucketSizeNs + 10, false, true, + buffer.clear(); + // 40 mins + 10 ns. + processor->onDumpReport(cfgKey, configAddedTimeNs + 6 * bucketSizeNs + 10, + false /* include_current_partial_bucket */, true /* erase_data */, ADB_DUMP, FAST, &buffer); EXPECT_TRUE(buffer.size() > 0); EXPECT_TRUE(reports.ParseFromArray(&buffer[0], buffer.size())); @@ -513,7 +537,7 @@ backfillAggregatedAtoms(&reports); ASSERT_EQ(1, reports.reports_size()); ASSERT_EQ(1, reports.reports(0).metrics_size()); - StatsLogReport::GaugeMetricDataWrapper gaugeMetrics; + gaugeMetrics = StatsLogReport::GaugeMetricDataWrapper(); sortMetricDataByDimensionsValue(reports.reports(0).metrics(0).gauge_metrics(), &gaugeMetrics); ASSERT_GT((int)gaugeMetrics.data_size(), 0); @@ -552,10 +576,21 @@ ASSERT_EQ(0, bucketInfo.wall_clock_timestamp_nanos_size()); EXPECT_EQ(MillisToNano(NanoToMillis(baseTimeNs + 5 * bucketSizeNs)), bucketInfo.start_bucket_elapsed_nanos()); - EXPECT_EQ(MillisToNano(NanoToMillis(activationNs + ttlNs + 1)), - bucketInfo.end_bucket_elapsed_nanos()); + EXPECT_EQ(MillisToNano(NanoToMillis(deactivationNs)), bucketInfo.end_bucket_elapsed_nanos()); EXPECT_TRUE(bucketInfo.atom(0).subsystem_sleep_state().subsystem_name().empty()); EXPECT_GT(bucketInfo.atom(0).subsystem_sleep_state().time_millis(), 0); + + // Check skipped bucket is not added after deactivation. + dumpReportTimeNs = configAddedTimeNs + 8 * bucketSizeNs + 10; + buffer.clear(); + processor->onDumpReport(cfgKey, dumpReportTimeNs, true /* include_current_partial_bucket */, + true /* erase_data */, ADB_DUMP, NO_TIME_CONSTRAINTS, &buffer); + EXPECT_TRUE(buffer.size() > 0); + EXPECT_TRUE(reports.ParseFromArray(&buffer[0], buffer.size())); + ASSERT_EQ(1, reports.reports_size()); + ASSERT_EQ(1, reports.reports(0).metrics_size()); + gaugeMetrics = reports.reports(0).metrics(0).gauge_metrics(); + EXPECT_EQ(gaugeMetrics.skipped_size(), 0); } TEST_F(GaugeMetricE2ePulledTest, TestRandomSamplePulledEventsNoCondition) {
diff --git a/statsd/tests/e2e/ValueMetric_pull_e2e_test.cpp b/statsd/tests/e2e/ValueMetric_pull_e2e_test.cpp index 996c2fb..385a473 100644 --- a/statsd/tests/e2e/ValueMetric_pull_e2e_test.cpp +++ b/statsd/tests/e2e/ValueMetric_pull_e2e_test.cpp
@@ -61,6 +61,7 @@ valueMetric->set_skip_zero_diff_output(false); valueMetric->set_max_pull_delay_sec(INT_MAX); valueMetric->set_split_bucket_for_app_upgrade(true); + valueMetric->set_min_bucket_size_nanos(1000); return config; } @@ -433,10 +434,10 @@ EXPECT_TRUE(processor->mMetricsManagers.begin()->second->isConfigValid()); processor->mPullerManager->ForceClearPullerCache(); - int startBucketNum = processor->mMetricsManagers.begin() - ->second->mAllMetricProducers[0] - ->getCurrentBucketNum(); - EXPECT_GT(startBucketNum, (int64_t)0); + const int startBucketNum = processor->mMetricsManagers.begin() + ->second->mAllMetricProducers[0] + ->getCurrentBucketNum(); + EXPECT_EQ(startBucketNum, 2); EXPECT_FALSE(processor->mMetricsManagers.begin()->second->mAllMetricProducers[0]->isActive()); // When creating the config, the value metric producer should register the alarm at the @@ -448,61 +449,109 @@ processor->mPullerManager->mReceivers.begin()->second.front().nextPullTimeNs; EXPECT_EQ(baseTimeNs + startBucketNum * bucketSizeNs + bucketSizeNs, expectedPullTimeNs); - // Check no pull occurred on metric initialization when it's not active. + // Initialize metric. const int64_t metricInitTimeNs = configAddedTimeNs + 1; // 10 mins + 1 ns. processor->onStatsdInitCompleted(metricInitTimeNs); + + // Check no pull occurred since metric not active. StatsdStatsReport_PulledAtomStats pulledAtomStats = getPulledAtomStats(); EXPECT_EQ(pulledAtomStats.atom_id(), util::SUBSYSTEM_SLEEP_STATE); EXPECT_EQ(pulledAtomStats.total_pull(), 0); - // Check no pull occurred on app upgrade when metric is not active. - const int64_t appUpgradeTimeNs = metricInitTimeNs + 1; // 10 mins + 2 ns. - processor->notifyAppUpgrade(appUpgradeTimeNs, "appName", 1000 /* uid */, 2 /* version */); - pulledAtomStats = getPulledAtomStats(); - EXPECT_EQ(pulledAtomStats.atom_id(), util::SUBSYSTEM_SLEEP_STATE); - EXPECT_EQ(pulledAtomStats.total_pull(), 0); - - // Check no pull occurred on dump report when metric is not active. - int64_t dumpReportTimeNs = appUpgradeTimeNs + 1; // 10 mins + 3 ns. + // Check skip bucket is not added when metric is not active. + int64_t dumpReportTimeNs = metricInitTimeNs + 1; // 10 mins + 2 ns. vector<uint8_t> buffer; - processor->onDumpReport(cfgKey, dumpReportTimeNs, true, true, ADB_DUMP, NO_TIME_CONSTRAINTS, - &buffer); + processor->onDumpReport(cfgKey, dumpReportTimeNs, true /* include_current_partial_bucket */, + true /* erase_data */, ADB_DUMP, FAST, &buffer); + ConfigMetricsReportList reports; + EXPECT_TRUE(buffer.size() > 0); + EXPECT_TRUE(reports.ParseFromArray(&buffer[0], buffer.size())); + ASSERT_EQ(1, reports.reports_size()); + ASSERT_EQ(1, reports.reports(0).metrics_size()); + StatsLogReport::ValueMetricDataWrapper valueMetrics = + reports.reports(0).metrics(0).value_metrics(); + EXPECT_EQ(valueMetrics.skipped_size(), 0); + + // App upgrade. + const int64_t appUpgradeTimeNs = dumpReportTimeNs + 1; // 10 mins + 3 ns. + processor->notifyAppUpgrade(appUpgradeTimeNs, "appName", 1000 /* uid */, 2 /* version */); + + // Check no pull occurred since metric not active. pulledAtomStats = getPulledAtomStats(); EXPECT_EQ(pulledAtomStats.atom_id(), util::SUBSYSTEM_SLEEP_STATE); EXPECT_EQ(pulledAtomStats.total_pull(), 0); - // Pulling alarm arrives on time and reset the sequential pulling alarm. + // Check skip bucket is not added when metric is not active. + dumpReportTimeNs = appUpgradeTimeNs + 1; // 10 mins + 4 ns. + buffer.clear(); + processor->onDumpReport(cfgKey, dumpReportTimeNs, true /* include_current_partial_bucket */, + true /* erase_data */, ADB_DUMP, FAST, &buffer); + EXPECT_TRUE(buffer.size() > 0); + EXPECT_TRUE(reports.ParseFromArray(&buffer[0], buffer.size())); + ASSERT_EQ(1, reports.reports_size()); + ASSERT_EQ(1, reports.reports(0).metrics_size()); + valueMetrics = reports.reports(0).metrics(0).value_metrics(); + EXPECT_EQ(valueMetrics.skipped_size(), 0); + + // Dump report with a pull. The pull should not happen because metric is inactive. + dumpReportTimeNs = dumpReportTimeNs + 1; // 10 mins + 6 ns. + buffer.clear(); + processor->onDumpReport(cfgKey, dumpReportTimeNs, true /* include_current_partial_bucket */, + true /* erase_data */, ADB_DUMP, NO_TIME_CONSTRAINTS, &buffer); + pulledAtomStats = getPulledAtomStats(); + EXPECT_EQ(pulledAtomStats.atom_id(), util::SUBSYSTEM_SLEEP_STATE); + EXPECT_EQ(pulledAtomStats.total_pull(), 0); + + // Check skipped bucket is not added from the dump operation when metric is not active. + EXPECT_TRUE(buffer.size() > 0); + EXPECT_TRUE(reports.ParseFromArray(&buffer[0], buffer.size())); + ASSERT_EQ(1, reports.reports_size()); + ASSERT_EQ(1, reports.reports(0).metrics_size()); + valueMetrics = reports.reports(0).metrics(0).value_metrics(); + EXPECT_EQ(valueMetrics.skipped_size(), 0); + + // Pulling alarm arrives on time and reset the sequential pulling alarm. This bucket is skipped. processor->informPullAlarmFired(expectedPullTimeNs + 1); // 15 mins + 1 ns. EXPECT_EQ(baseTimeNs + startBucketNum * bucketSizeNs + 2 * bucketSizeNs, expectedPullTimeNs); EXPECT_FALSE(processor->mMetricsManagers.begin()->second->mAllMetricProducers[0]->isActive()); - // Activate the metric. A pull occurs here + // Activate the metric. A pull occurs here that sets the base. + // 15 mins + 2 ms const int64_t activationNs = configAddedTimeNs + bucketSizeNs + (2 * 1000 * 1000); // 2 millis. auto batterySaverOnEvent = CreateBatterySaverOnEvent(activationNs); processor->OnLogEvent(batterySaverOnEvent.get()); // 15 mins + 2 ms. EXPECT_TRUE(processor->mMetricsManagers.begin()->second->mAllMetricProducers[0]->isActive()); + // This bucket should be kept. 1 total processor->informPullAlarmFired(expectedPullTimeNs + 1); // 20 mins + 1 ns. EXPECT_EQ(baseTimeNs + startBucketNum * bucketSizeNs + 3 * bucketSizeNs, expectedPullTimeNs); + // 25 mins + 2 ns. + // This bucket should be kept. 2 total processor->informPullAlarmFired(expectedPullTimeNs + 2); // 25 mins + 2 ns. EXPECT_EQ(baseTimeNs + startBucketNum * bucketSizeNs + 4 * bucketSizeNs, expectedPullTimeNs); // Create random event to deactivate metric. - auto deactivationEvent = CreateScreenBrightnessChangedEvent(activationNs + ttlNs + 1, 50); + // A pull occurs here and a partial bucket is created. The bucket ending here is kept. 3 total. + // 25 mins + 2 ms + 1 ns. + const int64_t deactivationNs = activationNs + ttlNs + 1; + auto deactivationEvent = CreateScreenBrightnessChangedEvent(deactivationNs, 50); processor->OnLogEvent(deactivationEvent.get()); EXPECT_FALSE(processor->mMetricsManagers.begin()->second->mAllMetricProducers[0]->isActive()); + // 30 mins + 3 ns. This bucket is skipped. processor->informPullAlarmFired(expectedPullTimeNs + 3); EXPECT_EQ(baseTimeNs + startBucketNum * bucketSizeNs + 5 * bucketSizeNs, expectedPullTimeNs); + // 35 mins + 4 ns. This bucket is skipped processor->informPullAlarmFired(expectedPullTimeNs + 4); EXPECT_EQ(baseTimeNs + startBucketNum * bucketSizeNs + 6 * bucketSizeNs, expectedPullTimeNs); - ConfigMetricsReportList reports; + dumpReportTimeNs = configAddedTimeNs + 6 * bucketSizeNs + 10; buffer.clear(); - processor->onDumpReport(cfgKey, configAddedTimeNs + 7 * bucketSizeNs + 10, false, true, - ADB_DUMP, FAST, &buffer); + // 40 mins + 10 ns. + processor->onDumpReport(cfgKey, dumpReportTimeNs, false /* include_current_partial_bucket */, + true /* erase_data */, ADB_DUMP, FAST, &buffer); EXPECT_TRUE(buffer.size() > 0); EXPECT_TRUE(reports.ParseFromArray(&buffer[0], buffer.size())); backfillDimensionPath(&reports); @@ -510,7 +559,7 @@ backfillStartEndTimestamp(&reports); ASSERT_EQ(1, reports.reports_size()); ASSERT_EQ(1, reports.reports(0).metrics_size()); - StatsLogReport::ValueMetricDataWrapper valueMetrics; + valueMetrics = StatsLogReport::ValueMetricDataWrapper(); sortMetricDataByDimensionsValue(reports.reports(0).metrics(0).value_metrics(), &valueMetrics); ASSERT_GT((int)valueMetrics.data_size(), 0); @@ -520,8 +569,8 @@ EXPECT_EQ(1 /* subsystem name field */, data.dimensions_in_what().value_tuple().dimensions_value(0).field()); EXPECT_FALSE(data.dimensions_in_what().value_tuple().dimensions_value(0).value_str().empty()); - // We have 2 full buckets, the two surrounding the activation are dropped. - ASSERT_EQ(2, data.bucket_info_size()); + // We have 3 full buckets, the two surrounding the activation are dropped. + ASSERT_EQ(3, data.bucket_info_size()); auto bucketInfo = data.bucket_info(0); EXPECT_EQ(baseTimeNs + 3 * bucketSizeNs, bucketInfo.start_bucket_elapsed_nanos()); @@ -532,6 +581,25 @@ EXPECT_EQ(baseTimeNs + 4 * bucketSizeNs, bucketInfo.start_bucket_elapsed_nanos()); EXPECT_EQ(baseTimeNs + 5 * bucketSizeNs, bucketInfo.end_bucket_elapsed_nanos()); ASSERT_EQ(1, bucketInfo.values_size()); + + bucketInfo = data.bucket_info(2); + EXPECT_EQ(MillisToNano(NanoToMillis(baseTimeNs + 5 * bucketSizeNs)), + bucketInfo.start_bucket_elapsed_nanos()); + EXPECT_EQ(MillisToNano(NanoToMillis(deactivationNs)), bucketInfo.end_bucket_elapsed_nanos()); + ASSERT_EQ(1, bucketInfo.values_size()); + + // Check skipped bucket is not added after deactivation. + dumpReportTimeNs = configAddedTimeNs + 7 * bucketSizeNs + 10; + buffer.clear(); + // 45 mins + 10 ns. + processor->onDumpReport(cfgKey, dumpReportTimeNs, true /* include_current_partial_bucket */, + true /* erase_data */, ADB_DUMP, FAST, &buffer); + EXPECT_TRUE(buffer.size() > 0); + EXPECT_TRUE(reports.ParseFromArray(&buffer[0], buffer.size())); + ASSERT_EQ(1, reports.reports_size()); + ASSERT_EQ(1, reports.reports(0).metrics_size()); + valueMetrics = reports.reports(0).metrics(0).value_metrics(); + EXPECT_EQ(valueMetrics.skipped_size(), 0); } /**
diff --git a/tests/src/android/cts/statsd/metric/ValueMetricsTests.java b/tests/src/android/cts/statsd/metric/ValueMetricsTests.java index 0cf5bbb..adfc565 100644 --- a/tests/src/android/cts/statsd/metric/ValueMetricsTests.java +++ b/tests/src/android/cts/statsd/metric/ValueMetricsTests.java
@@ -319,11 +319,8 @@ LogUtil.CLog.d("Got the following value metric data: " + metricReport.toString()); assertThat(metricReport.getMetricId()).isEqualTo(MetricsUtils.VALUE_METRIC_ID); assertThat(metricReport.getValueMetrics().getDataList()).isEmpty(); - // Bucket is skipped because metric is not activated. - assertThat(metricReport.getValueMetrics().getSkippedList()).isNotEmpty(); - assertThat(metricReport.getValueMetrics().getSkipped(0).getDropEventList()).isNotEmpty(); - assertThat(metricReport.getValueMetrics().getSkipped(0).getDropEvent(0).getDropReason()) - .isEqualTo(BucketDropReason.NO_DATA); + // Skipped buckets are not added when metric is empty. + assertThat(metricReport.getValueMetrics().getSkippedList()).isEmpty(); } public void testValueMetricWithConditionAndActivation() throws Exception {