From b209bdeff03c3bf5a898cbec7b220acb1d4e6cc1 Mon Sep 17 00:00:00 2001 From: Caitlin Shkuratov Date: Wed, 1 Feb 2023 18:53:31 +0000 Subject: [PATCH] [SB Refactor] Add a table logger for the parent mobile classes. This logs almost every flow in the parent repo and interactor. Once we've sufficiently dogfooded and tested the new pipeline, we can remove some of these logs. Bug: 267217719 Bug: 238425913 Test: manual: Dumped MobileSummaryLog Change-Id: I1f8b2b4ed7316b20eb994170578742f3dbc5dae4 --- .../pipeline/dagger/MobileSummaryLog.kt | 31 ++++++++++++++ .../dagger/StatusBarPipelineModule.kt | 7 ++++ .../data/model/MobileConnectivityModel.kt | 24 ++++++++++- .../prod/MobileConnectionsRepositoryImpl.kt | 42 +++++++++++++++++-- .../interactor/MobileIconsInteractor.kt | 34 ++++++++++++++- .../shared/ConnectivityPipelineLogger.kt | 9 ---- .../MobileRepositorySwitcherTest.kt | 3 ++ .../prod/MobileConnectionsRepositoryTest.kt | 3 ++ .../interactor/MobileIconsInteractorTest.kt | 1 + 9 files changed, 139 insertions(+), 15 deletions(-) create mode 100644 packages/SystemUI/src/com/android/systemui/statusbar/pipeline/dagger/MobileSummaryLog.kt diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/dagger/MobileSummaryLog.kt b/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/dagger/MobileSummaryLog.kt new file mode 100644 index 0000000000000..2ac9ab3c25100 --- /dev/null +++ b/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/dagger/MobileSummaryLog.kt @@ -0,0 +1,31 @@ +/* + * Copyright (C) 2023 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.pipeline.dagger + +import javax.inject.Qualifier + +/** + * Logs for mobile data that's **the same across all connections**. + * + * This buffer should only be used for the mobile parent classes like [MobileConnectionsRepository] + * and [MobileIconsInteractor]. It should *not* be used for classes that represent an individual + * connection, like [MobileConnectionRepository] or [MobileIconInteractor]. + */ +@Qualifier +@MustBeDocumented +@Retention(AnnotationRetention.RUNTIME) +annotation class MobileSummaryLog diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/dagger/StatusBarPipelineModule.kt b/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/dagger/StatusBarPipelineModule.kt index 5f3b0dcb1654d..60de1a38dd95d 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/dagger/StatusBarPipelineModule.kt +++ b/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/dagger/StatusBarPipelineModule.kt @@ -118,5 +118,12 @@ abstract class StatusBarPipelineModule { fun provideAirplaneTableLogBuffer(factory: TableLogBufferFactory): TableLogBuffer { return factory.create("AirplaneTableLog", 30) } + + @Provides + @SysUISingleton + @MobileSummaryLog + fun provideMobileSummaryLogBuffer(factory: TableLogBufferFactory): TableLogBuffer { + return factory.create("MobileSummaryLog", 100) + } } } diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/mobile/data/model/MobileConnectivityModel.kt b/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/mobile/data/model/MobileConnectivityModel.kt index e61890523ebba..97a537ac0ce67 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/mobile/data/model/MobileConnectivityModel.kt +++ b/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/mobile/data/model/MobileConnectivityModel.kt @@ -17,6 +17,8 @@ package com.android.systemui.statusbar.pipeline.mobile.data.model import android.net.NetworkCapabilities +import com.android.systemui.log.table.Diffable +import com.android.systemui.log.table.TableRowLogger /** Provides information about a mobile network connection */ data class MobileConnectivityModel( @@ -24,4 +26,24 @@ data class MobileConnectivityModel( val isConnected: Boolean = false, /** Whether the mobile transport is validated [NetworkCapabilities.NET_CAPABILITY_VALIDATED] */ val isValidated: Boolean = false, -) +) : Diffable { + // TODO(b/267767715): Can we implement [logDiffs] and [logFull] generically for data classes? + override fun logDiffs(prevVal: MobileConnectivityModel, row: TableRowLogger) { + if (prevVal.isConnected != isConnected) { + row.logChange(COL_IS_CONNECTED, isConnected) + } + if (prevVal.isValidated != isValidated) { + row.logChange(COL_IS_VALIDATED, isValidated) + } + } + + override fun logFull(row: TableRowLogger) { + row.logChange(COL_IS_CONNECTED, isConnected) + row.logChange(COL_IS_VALIDATED, isValidated) + } + + companion object { + private const val COL_IS_CONNECTED = "isConnected" + private const val COL_IS_VALIDATED = "isValidated" + } +} diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/mobile/data/repository/prod/MobileConnectionsRepositoryImpl.kt b/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/mobile/data/repository/prod/MobileConnectionsRepositoryImpl.kt index 10f48a36e7f64..c660d311a9a18 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/mobile/data/repository/prod/MobileConnectionsRepositoryImpl.kt +++ b/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/mobile/data/repository/prod/MobileConnectionsRepositoryImpl.kt @@ -42,6 +42,9 @@ import com.android.systemui.common.coroutine.ConflatedCallbackFlow.conflatedCall import com.android.systemui.dagger.SysUISingleton import com.android.systemui.dagger.qualifiers.Application import com.android.systemui.dagger.qualifiers.Background +import com.android.systemui.log.table.TableLogBuffer +import com.android.systemui.log.table.logDiffsForTable +import com.android.systemui.statusbar.pipeline.dagger.MobileSummaryLog import com.android.systemui.statusbar.pipeline.mobile.data.model.MobileConnectivityModel import com.android.systemui.statusbar.pipeline.mobile.data.model.NetworkNameModel import com.android.systemui.statusbar.pipeline.mobile.data.model.SubscriptionModel @@ -82,6 +85,7 @@ constructor( private val subscriptionManager: SubscriptionManager, private val telephonyManager: TelephonyManager, private val logger: ConnectivityPipelineLogger, + @MobileSummaryLog private val tableLogger: TableLogBuffer, mobileMappingsProxy: MobileMappingsProxy, broadcastDispatcher: BroadcastDispatcher, private val context: Context, @@ -114,6 +118,12 @@ constructor( } } .distinctUntilChanged() + .logDiffsForTable( + tableLogger, + LOGGING_PREFIX, + columnName = "carrierMergedSubId", + initialValue = null, + ) .stateIn(scope, started = SharingStarted.WhileSubscribed(), null) private val mobileSubscriptionsChangeEvent: Flow = conflatedCallbackFlow { @@ -139,8 +149,14 @@ constructor( override val subscriptions: StateFlow> = merge(mobileSubscriptionsChangeEvent, carrierMergedSubId) .mapLatest { fetchSubscriptionsList().map { it.toSubscriptionModel() } } - .logInputChange(logger, "onSubscriptionsChanged") .onEach { infos -> updateRepos(infos) } + .distinctUntilChanged() + .logDiffsForTable( + tableLogger, + LOGGING_PREFIX, + columnName = "subscriptions", + initialValue = listOf(), + ) .stateIn(scope, started = SharingStarted.WhileSubscribed(), listOf()) /** StateFlow that keeps track of the current active mobile data subscription */ @@ -157,7 +173,12 @@ constructor( awaitClose { telephonyManager.unregisterTelephonyCallback(callback) } } .distinctUntilChanged() - .logInputChange(logger, "onActiveDataSubscriptionIdChanged") + .logDiffsForTable( + tableLogger, + LOGGING_PREFIX, + columnName = "activeSubId", + initialValue = INVALID_SUBSCRIPTION_ID, + ) .stateIn(scope, started = SharingStarted.WhileSubscribed(), INVALID_SUBSCRIPTION_ID) private val defaultDataSubIdChangeEvent: MutableSharedFlow = @@ -171,7 +192,12 @@ constructor( intent.getIntExtra(PhoneConstants.SUBSCRIPTION_KEY, INVALID_SUBSCRIPTION_ID) } .distinctUntilChanged() - .logInputChange(logger, "ACTION_DEFAULT_DATA_SUBSCRIPTION_CHANGED") + .logDiffsForTable( + tableLogger, + LOGGING_PREFIX, + columnName = "defaultSubId", + initialValue = SubscriptionManager.getDefaultDataSubscriptionId(), + ) .onEach { defaultDataSubIdChangeEvent.tryEmit(Unit) } .stateIn( scope, @@ -247,7 +273,11 @@ constructor( awaitClose { connectivityManager.unregisterNetworkCallback(callback) } } .distinctUntilChanged() - .logInputChange(logger, "defaultMobileNetworkConnectivity") + .logDiffsForTable( + tableLogger, + columnPrefix = "$LOGGING_PREFIX.defaultConnection", + initialValue = MobileConnectivityModel(), + ) .stateIn(scope, SharingStarted.WhileSubscribed(), MobileConnectivityModel()) /** @@ -321,4 +351,8 @@ constructor( subscriptionId = subscriptionId, isOpportunistic = isOpportunistic, ) + + companion object { + private const val LOGGING_PREFIX = "Repo" + } } diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/mobile/domain/interactor/MobileIconsInteractor.kt b/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/mobile/domain/interactor/MobileIconsInteractor.kt index 0e4a4321bf263..bbb60f919730b 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/mobile/domain/interactor/MobileIconsInteractor.kt +++ b/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/mobile/domain/interactor/MobileIconsInteractor.kt @@ -23,6 +23,9 @@ import com.android.settingslib.SignalIcon.MobileIconGroup import com.android.settingslib.mobile.TelephonyIcons import com.android.systemui.dagger.SysUISingleton import com.android.systemui.dagger.qualifiers.Application +import com.android.systemui.log.table.TableLogBuffer +import com.android.systemui.log.table.logDiffsForTable +import com.android.systemui.statusbar.pipeline.dagger.MobileSummaryLog import com.android.systemui.statusbar.pipeline.mobile.data.model.MobileConnectivityModel import com.android.systemui.statusbar.pipeline.mobile.data.model.SubscriptionModel import com.android.systemui.statusbar.pipeline.mobile.data.repository.MobileConnectionRepository @@ -104,6 +107,7 @@ constructor( private val mobileConnectionsRepo: MobileConnectionsRepository, private val carrierConfigTracker: CarrierConfigTracker, private val logger: ConnectivityPipelineLogger, + @MobileSummaryLog private val tableLogger: TableLogBuffer, userSetupRepo: UserSetupRepository, @Application private val scope: CoroutineScope, ) : MobileIconsInteractor { @@ -173,7 +177,13 @@ constructor( } } .distinctUntilChanged() - .onEach { logger.logFilteredSubscriptionsChanged(it) } + .logDiffsForTable( + tableLogger, + LOGGING_PREFIX, + columnName = "filteredSubscriptions", + initialValue = listOf(), + ) + .stateIn(scope, SharingStarted.WhileSubscribed(), listOf()) override val defaultDataSubId = mobileConnectionsRepo.defaultDataSubId @@ -195,6 +205,12 @@ constructor( delay(2000) emit(false) } + .logDiffsForTable( + tableLogger, + LOGGING_PREFIX, + columnName = "forcingValidation", + initialValue = false, + ) .stateIn(scope, SharingStarted.WhileSubscribed(), false) override val defaultMobileNetworkConnectivity: StateFlow = @@ -211,6 +227,12 @@ constructor( networkConnectivity } } + .distinctUntilChanged() + .logDiffsForTable( + tableLogger, + columnPrefix = "$LOGGING_PREFIX.defaultConnection", + initialValue = mobileConnectionsRepo.defaultMobileNetworkConnectivity.value, + ) .stateIn( scope, SharingStarted.WhileSubscribed(), @@ -259,6 +281,12 @@ constructor( !connectivityModel.isValidated } } + .logDiffsForTable( + tableLogger, + LOGGING_PREFIX, + columnName = "isDefaultConnectionFailed", + initialValue = false, + ) .stateIn(scope, SharingStarted.WhileSubscribed(), false) override val isUserSetup: StateFlow = userSetupRepo.isUserSetupFlow @@ -277,4 +305,8 @@ constructor( isDefaultConnectionFailed, mobileConnectionsRepo.getRepoForSubId(subId), ) + + companion object { + private const val LOGGING_PREFIX = "Intr" + } } diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/shared/ConnectivityPipelineLogger.kt b/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/shared/ConnectivityPipelineLogger.kt index 6796a94cf6a92..45036969aefe9 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/shared/ConnectivityPipelineLogger.kt +++ b/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/shared/ConnectivityPipelineLogger.kt @@ -204,15 +204,6 @@ constructor( // TODO(b/238425913): We should split this class into mobile-specific and wifi-specific loggers. - fun logFilteredSubscriptionsChanged(subs: List) { - buffer.log( - SB_LOGGING_TAG, - LogLevel.INFO, - { str1 = subs.toString() }, - { "Filtered subscriptions updated: $str1" }, - ) - } - fun logUiAdapterSubIdsUpdated(subs: List) { buffer.log( SB_LOGGING_TAG, diff --git a/packages/SystemUI/tests/src/com/android/systemui/statusbar/pipeline/mobile/data/repository/MobileRepositorySwitcherTest.kt b/packages/SystemUI/tests/src/com/android/systemui/statusbar/pipeline/mobile/data/repository/MobileRepositorySwitcherTest.kt index 96cca44407694..4da2104ca32e0 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/statusbar/pipeline/mobile/data/repository/MobileRepositorySwitcherTest.kt +++ b/packages/SystemUI/tests/src/com/android/systemui/statusbar/pipeline/mobile/data/repository/MobileRepositorySwitcherTest.kt @@ -25,6 +25,7 @@ import com.android.systemui.SysuiTestCase import com.android.systemui.demomode.DemoMode import com.android.systemui.demomode.DemoModeController import com.android.systemui.dump.DumpManager +import com.android.systemui.log.table.TableLogBuffer import com.android.systemui.log.table.TableLogBufferFactory import com.android.systemui.statusbar.pipeline.mobile.data.model.SubscriptionModel import com.android.systemui.statusbar.pipeline.mobile.data.repository.demo.DemoMobileConnectionsRepository @@ -81,6 +82,7 @@ class MobileRepositorySwitcherTest : SysuiTestCase() { @Mock private lateinit var subscriptionManager: SubscriptionManager @Mock private lateinit var telephonyManager: TelephonyManager @Mock private lateinit var logger: ConnectivityPipelineLogger + @Mock private lateinit var summaryLogger: TableLogBuffer @Mock private lateinit var demoModeController: DemoModeController @Mock private lateinit var dumpManager: DumpManager @@ -114,6 +116,7 @@ class MobileRepositorySwitcherTest : SysuiTestCase() { subscriptionManager, telephonyManager, logger, + summaryLogger, mobileMappings, fakeBroadcastDispatcher, context, diff --git a/packages/SystemUI/tests/src/com/android/systemui/statusbar/pipeline/mobile/data/repository/prod/MobileConnectionsRepositoryTest.kt b/packages/SystemUI/tests/src/com/android/systemui/statusbar/pipeline/mobile/data/repository/prod/MobileConnectionsRepositoryTest.kt index b73348c185603..fef0981397561 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/statusbar/pipeline/mobile/data/repository/prod/MobileConnectionsRepositoryTest.kt +++ b/packages/SystemUI/tests/src/com/android/systemui/statusbar/pipeline/mobile/data/repository/prod/MobileConnectionsRepositoryTest.kt @@ -84,6 +84,7 @@ class MobileConnectionsRepositoryTest : SysuiTestCase() { @Mock private lateinit var subscriptionManager: SubscriptionManager @Mock private lateinit var telephonyManager: TelephonyManager @Mock private lateinit var logger: ConnectivityPipelineLogger + @Mock private lateinit var summaryLogger: TableLogBuffer @Mock private lateinit var logBufferFactory: TableLogBufferFactory private val mobileMappings = FakeMobileMappingsProxy() @@ -157,6 +158,7 @@ class MobileConnectionsRepositoryTest : SysuiTestCase() { subscriptionManager, telephonyManager, logger, + summaryLogger, mobileMappings, fakeBroadcastDispatcher, context, @@ -616,6 +618,7 @@ class MobileConnectionsRepositoryTest : SysuiTestCase() { subscriptionManager, telephonyManager, logger, + summaryLogger, mobileMappings, fakeBroadcastDispatcher, context, diff --git a/packages/SystemUI/tests/src/com/android/systemui/statusbar/pipeline/mobile/domain/interactor/MobileIconsInteractorTest.kt b/packages/SystemUI/tests/src/com/android/systemui/statusbar/pipeline/mobile/domain/interactor/MobileIconsInteractorTest.kt index bd249221b6cae..23b634c7d2b8a 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/statusbar/pipeline/mobile/domain/interactor/MobileIconsInteractorTest.kt +++ b/packages/SystemUI/tests/src/com/android/systemui/statusbar/pipeline/mobile/domain/interactor/MobileIconsInteractorTest.kt @@ -79,6 +79,7 @@ class MobileIconsInteractorTest : SysuiTestCase() { connectionsRepository, carrierConfigTracker, logger = mock(), + tableLogger = mock(), userSetupRepository, testScope.backgroundScope, )