From 5cd0e659310d7308df6fc501c6b379787bdfbeee Mon Sep 17 00:00:00 2001 From: Jeff DeCew Date: Wed, 15 Jun 2022 19:10:06 +0000 Subject: [PATCH] Reduce log spam when RankingMap is missing rankings for non-cancelled entries. Fixes: 236140753 Test: dumpsysui NotifCollection Test: atest NotifCollectionLoggerTest Change-Id: I7308af63ec67aa9b1b3444bfe90a3953468024ad --- .../collection/NotifCollection.java | 23 ++++- .../notifcollection/NotifCollectionLogger.kt | 55 +++++++++-- .../NotifCollectionLoggerTest.kt | 98 +++++++++++++++++++ 3 files changed, 168 insertions(+), 8 deletions(-) create mode 100644 packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/collection/notifcollection/NotifCollectionLoggerTest.kt diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/NotifCollection.java b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/NotifCollection.java index add177ddd76ca..38830c2e33003 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/NotifCollection.java +++ b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/NotifCollection.java @@ -89,6 +89,7 @@ import com.android.systemui.statusbar.notification.collection.notifcollection.In import com.android.systemui.statusbar.notification.collection.notifcollection.InternalNotifUpdater; import com.android.systemui.statusbar.notification.collection.notifcollection.NotifCollectionListener; import com.android.systemui.statusbar.notification.collection.notifcollection.NotifCollectionLogger; +import com.android.systemui.statusbar.notification.collection.notifcollection.NotifCollectionLoggerKt; import com.android.systemui.statusbar.notification.collection.notifcollection.NotifDismissInterceptor; import com.android.systemui.statusbar.notification.collection.notifcollection.NotifEvent; import com.android.systemui.statusbar.notification.collection.notifcollection.NotifLifetimeExtender; @@ -110,6 +111,7 @@ import java.util.List; import java.util.Map; import java.util.Objects; import java.util.Queue; +import java.util.Set; import java.util.concurrent.TimeUnit; import javax.inject.Inject; @@ -157,6 +159,8 @@ public class NotifCollection implements Dumpable { private final List mLifetimeExtenders = new ArrayList<>(); private final List mDismissInterceptors = new ArrayList<>(); + private Set mNotificationsWithoutRankings = Collections.emptySet(); + private Queue mEventQueue = new ArrayDeque<>(); private boolean mAttached = false; @@ -560,6 +564,7 @@ public class NotifCollection implements Dumpable { } private void applyRanking(@NonNull RankingMap rankingMap) { + ArrayMap currentEntriesWithoutRankings = null; for (NotificationEntry entry : mNotificationSet.values()) { if (!isCanceled(entry)) { @@ -581,10 +586,21 @@ public class NotifCollection implements Dumpable { } } } else { - mLogger.logRankingMissing(entry, rankingMap); + if (currentEntriesWithoutRankings == null) { + currentEntriesWithoutRankings = new ArrayMap<>(); + } + currentEntriesWithoutRankings.put(entry.getKey(), entry); } } } + NotifCollectionLoggerKt.maybeLogInconsistentRankings( + mLogger, + mNotificationsWithoutRankings, + currentEntriesWithoutRankings, + rankingMap + ); + mNotificationsWithoutRankings = currentEntriesWithoutRankings == null + ? Collections.emptySet() : currentEntriesWithoutRankings.keySet(); mEventQueue.add(new RankingAppliedEvent()); } @@ -836,6 +852,11 @@ public class NotifCollection implements Dumpable { entries, true, "\t\t")); + + pw.println("\n\tmNotificationsWithoutRankings: " + mNotificationsWithoutRankings.size()); + for (String key : mNotificationsWithoutRankings) { + pw.println("\t * : " + key); + } } private final BatchableNotificationHandler mNotifHandler = new BatchableNotificationHandler() { diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/notifcollection/NotifCollectionLogger.kt b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/notifcollection/NotifCollectionLogger.kt index 46e01c33bae33..ebcac6b6afb3b 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/notifcollection/NotifCollectionLogger.kt +++ b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/notifcollection/NotifCollectionLogger.kt @@ -196,16 +196,32 @@ class NotifCollectionLogger @Inject constructor( }) } - fun logRankingMissing(entry: NotificationEntry, rankingMap: RankingMap) { + fun logMissingRankings( + newlyInconsistentEntries: List, + totalInconsistent: Int, + rankingMap: RankingMap + ) { buffer.log(TAG, WARNING, { - str1 = entry.logKey + int1 = totalInconsistent + int2 = newlyInconsistentEntries.size + str1 = newlyInconsistentEntries.joinToString { it.logKey ?: "null" } }, { - "Ranking update is missing ranking for $str1" + "Ranking update is missing ranking for $int1 entries ($int2 new): $str1" + }) + buffer.log(TAG, DEBUG, { + str1 = rankingMap.orderedKeys.map { logKey(it) ?: "null" }.toString() + }, { + "Ranking map contents: $str1" + }) + } + + fun logRecoveredRankings(newlyConsistentKeys: List) { + buffer.log(TAG, INFO, { + int1 = newlyConsistentKeys.size + str1 = newlyConsistentKeys.joinToString { logKey(it) ?: "null" } + }, { + "Ranking update now contains rankings for $int1 previously inconsistent entries: $str1" }) - buffer.log(TAG, DEBUG, {}, { "Ranking map contents:" }) - for (entry in rankingMap.orderedKeys) { - buffer.log(TAG, DEBUG, { str1 = logKey(entry) }, { " $str1" }) - } } fun logRemoteExceptionOnNotificationClear(entry: NotificationEntry, e: RemoteException) { @@ -346,4 +362,29 @@ class NotifCollectionLogger @Inject constructor( } } +fun maybeLogInconsistentRankings( + logger: NotifCollectionLogger, + oldKeysWithoutRankings: Set, + newEntriesWithoutRankings: Map?, + rankingMap: RankingMap +) { + if (oldKeysWithoutRankings.isEmpty() && newEntriesWithoutRankings == null) return + val newlyConsistent: List = oldKeysWithoutRankings + .mapNotNull { key -> + key.takeIf { key !in (newEntriesWithoutRankings ?: emptyMap()) } + .takeIf { key in rankingMap.orderedKeys } + }.sorted() + if (newlyConsistent.isNotEmpty()) { + logger.logRecoveredRankings(newlyConsistent) + } + val newlyInconsistent: List = newEntriesWithoutRankings + ?.mapNotNull { (key, entry) -> + entry.takeIf { key !in oldKeysWithoutRankings } + }?.sortedBy { it.key } ?: emptyList() + if (newlyInconsistent.isNotEmpty()) { + val totalInconsistent: Int = newEntriesWithoutRankings?.size ?: 0 + logger.logMissingRankings(newlyInconsistent, totalInconsistent, rankingMap) + } +} + private const val TAG = "NotifCollection" diff --git a/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/collection/notifcollection/NotifCollectionLoggerTest.kt b/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/collection/notifcollection/NotifCollectionLoggerTest.kt new file mode 100644 index 0000000000000..6c07174bce659 --- /dev/null +++ b/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/collection/notifcollection/NotifCollectionLoggerTest.kt @@ -0,0 +1,98 @@ +/* + * Copyright (C) 2022 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. + */ +package com.android.systemui.statusbar.notification.collection.notifcollection + +import android.service.notification.NotificationListenerService.RankingMap +import android.testing.AndroidTestingRunner +import android.testing.TestableLooper.RunWithLooper +import androidx.test.filters.SmallTest +import com.android.systemui.SysuiTestCase +import com.android.systemui.statusbar.notification.collection.NotificationEntry +import com.android.systemui.statusbar.notification.collection.NotificationEntryBuilder +import com.android.systemui.util.mockito.eq +import com.android.systemui.util.mockito.mock +import org.junit.Test +import org.junit.runner.RunWith +import org.mockito.Mockito.verify +import org.mockito.Mockito.verifyNoMoreInteractions + +@SmallTest +@RunWith(AndroidTestingRunner::class) +@RunWithLooper +class NotifCollectionLoggerTest : SysuiTestCase() { + private val logger: NotifCollectionLogger = mock() + private val entry1: NotificationEntry = NotificationEntryBuilder().setId(1).build() + private val entry2: NotificationEntry = NotificationEntryBuilder().setId(2).build() + + private fun mapOfEntries(vararg entries: NotificationEntry): Map = + entries.associateBy { it.key } + + private fun rankingMapOf(vararg entries: NotificationEntry): RankingMap = + RankingMap(entries.map { it.ranking }.toTypedArray()) + + @Test + fun testMaybeLogInconsistentRankings_logsNewlyInconsistentRanking() { + val rankingMap = rankingMapOf(entry1) + maybeLogInconsistentRankings( + logger = logger, + oldKeysWithoutRankings = emptySet(), + newEntriesWithoutRankings = mapOfEntries(entry2), + rankingMap = rankingMap + ) + verify(logger).logMissingRankings( + newlyInconsistentEntries = eq(listOf(entry2)), + totalInconsistent = eq(1), + rankingMap = eq(rankingMap), + ) + verifyNoMoreInteractions(logger) + } + + @Test + fun testMaybeLogInconsistentRankings_doesNotLogAlreadyInconsistentRanking() { + maybeLogInconsistentRankings( + logger = logger, + oldKeysWithoutRankings = setOf(entry2.key), + newEntriesWithoutRankings = mapOfEntries(entry2), + rankingMap = rankingMapOf(entry1) + ) + verifyNoMoreInteractions(logger) + } + + @Test + fun testMaybeLogInconsistentRankings_logsWhenRankingIsAdded() { + maybeLogInconsistentRankings( + logger = logger, + oldKeysWithoutRankings = setOf(entry2.key), + newEntriesWithoutRankings = mapOfEntries(), + rankingMap = rankingMapOf(entry1, entry2) + ) + verify(logger).logRecoveredRankings( + newlyConsistentKeys = eq(listOf(entry2.key)), + ) + verifyNoMoreInteractions(logger) + } + + @Test + fun testMaybeLogInconsistentRankings_doesNotLogsWhenEntryIsRemoved() { + maybeLogInconsistentRankings( + logger = logger, + oldKeysWithoutRankings = setOf(entry2.key), + newEntriesWithoutRankings = mapOfEntries(), + rankingMap = rankingMapOf(entry1) + ) + verifyNoMoreInteractions(logger) + } +}