From 888b7a8faf7d6c2aa4272bebeb875eac9a2ff21c Mon Sep 17 00:00:00 2001 From: Chris Wren Date: Fri, 17 Jun 2016 15:47:19 -0400 Subject: [PATCH] assume a generous prior in the rate estimator currently with one data point the estimator is biased high. If we don't have any data we want to be biased low to allow the first interaction to be an isolated burst. Also add metrics for enqueue rate to make it easier to debug this code in the future (currently we only have metrics after the rate limiter). Bug: 29379789 Change-Id: I7b1d379290a55f120d039fd29d1bc0abbc6d4932 --- .../NotificationManagerService.java | 2 ++ .../notification/NotificationUsageStats.java | 17 ++++++++++++++++ .../server/notification/RateEstimator.java | 13 ++++++------ .../notification/RateEstimatorTest.java | 20 +++++++++++++++++-- 4 files changed, 43 insertions(+), 9 deletions(-) diff --git a/services/core/java/com/android/server/notification/NotificationManagerService.java b/services/core/java/com/android/server/notification/NotificationManagerService.java index d4ee02e757549..662516e6b60ea 100644 --- a/services/core/java/com/android/server/notification/NotificationManagerService.java +++ b/services/core/java/com/android/server/notification/NotificationManagerService.java @@ -2521,6 +2521,8 @@ public class NotificationManagerService extends SystemService { return; } + mUsageStats.registerEnqueuedByApp(pkg); + // Limit the number of notifications that any given package except the android // package or a registered listener can enqueue. Prevents DOS attacks and deals with leaks. if (!isSystemNotification && !isNotificationFromListener) { diff --git a/services/core/java/com/android/server/notification/NotificationUsageStats.java b/services/core/java/com/android/server/notification/NotificationUsageStats.java index 7b573da482e3e..07142f07d31b0 100644 --- a/services/core/java/com/android/server/notification/NotificationUsageStats.java +++ b/services/core/java/com/android/server/notification/NotificationUsageStats.java @@ -113,6 +113,17 @@ public class NotificationUsageStats { } } + /** + * Called when a notification is tentatively enqueued by an app, before rate checking. + */ + public synchronized void registerEnqueuedByApp(String packageName) { + AggregatedStats[] aggregatedStatsArray = getAggregatedStatsLocked(packageName); + for (AggregatedStats stats : aggregatedStatsArray) { + stats.numEnqueuedByApp++; + } + releaseAggregatedStatsLocked(aggregatedStatsArray); + } + /** * Called when a notification has been posted. */ @@ -344,6 +355,7 @@ public class NotificationUsageStats { private AggregatedStats mPrevious; // ---- Updated as the respective events occur. + public int numEnqueuedByApp; public int numPostedByApp; public int numUpdatedByApp; public int numRemovedByApp; @@ -470,6 +482,7 @@ public class NotificationUsageStats { public void emit() { AggregatedStats previous = getPrevious(); + maybeCount("note_enqueued", (numEnqueuedByApp - previous.numEnqueuedByApp)); maybeCount("note_post", (numPostedByApp - previous.numPostedByApp)); maybeCount("note_update", (numUpdatedByApp - previous.numUpdatedByApp)); maybeCount("note_remove", (numRemovedByApp - previous.numRemovedByApp)); @@ -501,6 +514,7 @@ public class NotificationUsageStats { quietImportance.maybeCount(previous.quietImportance); finalImportance.maybeCount(previous.finalImportance); + previous.numEnqueuedByApp = numEnqueuedByApp; previous.numPostedByApp = numPostedByApp; previous.numUpdatedByApp = numUpdatedByApp; previous.numRemovedByApp = numRemovedByApp; @@ -568,6 +582,8 @@ public class NotificationUsageStats { output.append(indentPlusTwo); output.append("key='").append(key).append("',\n"); output.append(indentPlusTwo); + output.append("numEnqueuedByApp=").append(numEnqueuedByApp).append(",\n"); + output.append(indentPlusTwo); output.append("numPostedByApp=").append(numPostedByApp).append(",\n"); output.append(indentPlusTwo); output.append("numUpdatedByApp=").append(numUpdatedByApp).append(",\n"); @@ -631,6 +647,7 @@ public class NotificationUsageStats { JSONObject dump = new JSONObject(); dump.put("key", key); dump.put("duration", SystemClock.elapsedRealtime() - mCreated); + maybePut(dump, "numEnqueuedByApp", numEnqueuedByApp); maybePut(dump, "numPostedByApp", numPostedByApp); maybePut(dump, "numUpdatedByApp", numUpdatedByApp); maybePut(dump, "numRemovedByApp", numRemovedByApp); diff --git a/services/core/java/com/android/server/notification/RateEstimator.java b/services/core/java/com/android/server/notification/RateEstimator.java index c17db4acf3cd7..a2f93dce2bcaf 100644 --- a/services/core/java/com/android/server/notification/RateEstimator.java +++ b/services/core/java/com/android/server/notification/RateEstimator.java @@ -26,9 +26,12 @@ public class RateEstimator { private static final double RATE_ALPHA = 0.8; private static final double MINIMUM_DT = 0.0005; private Long mLastEventTime; - private Float mInterarrivalTime; + private double mInterarrivalTime; - public RateEstimator() {} + public RateEstimator() { + // assume something generous if we have no information + mInterarrivalTime = 1000.0; + } /** Update the estimate to account for an event that just happened. */ public float update(long now) { @@ -38,7 +41,7 @@ public class RateEstimator { rate = 0f; } else { // Calculate the new inter-arrival time based on last event time. - mInterarrivalTime = (float) getInterarrivalEstimate(now); + mInterarrivalTime = getInterarrivalEstimate(now); rate = (float) (1.0 / mInterarrivalTime); } mLastEventTime = now; @@ -57,10 +60,6 @@ public class RateEstimator { private double getInterarrivalEstimate(long now) { double dt = ((double) (now - mLastEventTime)) / 1000.0; dt = Math.max(dt, MINIMUM_DT); - if (mInterarrivalTime == null) { - // No last inter-arrival time, return the new value directly. - return dt; - } // a*iat_old + (1-a)*(t_now-t_last) return (RATE_ALPHA * mInterarrivalTime + (1.0 - RATE_ALPHA) * dt); } diff --git a/services/tests/servicestests/src/com/android/server/notification/RateEstimatorTest.java b/services/tests/servicestests/src/com/android/server/notification/RateEstimatorTest.java index b5698d5bd40bb..3278cf1c0b712 100644 --- a/services/tests/servicestests/src/com/android/server/notification/RateEstimatorTest.java +++ b/services/tests/servicestests/src/com/android/server/notification/RateEstimatorTest.java @@ -58,6 +58,15 @@ public class RateEstimatorTest extends AndroidTestCase { assertFalse(Float.isNaN(rate)); } + @SmallTest + public void testInstantaneousBurstIsEstimatedUnderTwoPercent() throws Exception { + assertUpdateTime(mTestStartTime); + long eventStart = mTestStartTime + 1000; // start event a long time after initialization + long nextEventTime = postEvents(eventStart, 0, 5); // five events at \inf + final float rate = mEstimator.getRate(nextEventTime); + assertLessThan("Rate", rate, 20f); + } + @SmallTest public void testCompactBurstIsEstimatedUnderTwoPercent() throws Exception { assertUpdateTime(mTestStartTime); @@ -110,12 +119,19 @@ public class RateEstimatorTest extends AndroidTestCase { assertLessThan("Rate", rate, 0.1f); } + @SmallTest + public void testGetRateWithOneUpdate() throws Exception { + assertUpdateTime(mTestStartTime); + final float rate = mEstimator.getRate(mTestStartTime+1); + assertLessThan("Rate", rate, 1f); + } + private void assertLessThan(String label, float a, float b) { - assertTrue(String.format("%s was %f, but should be less than %f", label, a, b), a < b); + assertTrue(String.format("%s was %f, but should be less than %f", label, a, b), a <= b); } private void assertGreaterThan(String label, float a, float b) { - assertTrue(String.format("%s was %f, but should be more than %f", label, a, b), a > b); + assertTrue(String.format("%s was %f, but should be more than %f", label, a, b), a >= b); } /** @returns the next event time. */