fix code smells

amosproj · Dec 13, 2024 · 5b07d68 · 5b07d68
1 parent c2905f9
commit 5b07d68
Show file tree

Hide file tree

Showing 35 changed files with 194 additions and 180 deletions.
diff --git a/.github/workflows/pull_request.yml b/.github/workflows/pull_request.yml
@@ -118,7 +118,7 @@ jobs:
       - name: Detekt
         run: | 
           cd frontend
-          nix develop --command ./gradlew app:detekt \
+          nix develop --command ./gradlew app:detektMain \
             --no-daemon \
             --parallel \
             --build-cache \

diff --git a/frontend/app/src/main/java/de/amosproj3/ziofa/api/events/DataStreamProvider.kt b/frontend/app/src/main/java/de/amosproj3/ziofa/api/events/DataStreamProvider.kt
@@ -7,9 +7,8 @@ package de.amosproj3.ziofa.api.events
 import kotlinx.coroutines.flow.Flow
 
 interface DataStreamProvider {
-    suspend fun counter(ebpfProgramName: String): Flow<UInt>
 
-    suspend fun vfsWriteEvents(pids: List<UInt>?): Flow<BackendEvent.VfsWriteEvent>
+    fun vfsWriteEvents(pids: List<UInt>?): Flow<BackendEvent.VfsWriteEvent>
 
-    suspend fun sendMessageEvents(pids: List<UInt>?): Flow<BackendEvent.SendMessageEvent>
+    fun sendMessageEvents(pids: List<UInt>?): Flow<BackendEvent.SendMessageEvent>
 }
diff --git a/frontend/app/src/main/java/de/amosproj3/ziofa/bl/configuration/ConfigurationManager.kt b/frontend/app/src/main/java/de/amosproj3/ziofa/bl/configuration/ConfigurationManager.kt
@@ -45,6 +45,7 @@ class ConfigurationManager(val clientFactory: ClientFactory) :
             .map { it ?: ConfigurationUpdate.Unknown }
 
     init {
+        @Suppress("TooGenericExceptionCaught") // we want to display all exceptions
         coroutineScope.launch {
             try {
                 client = clientFactory.connect()
@@ -65,55 +66,59 @@ class ConfigurationManager(val clientFactory: ClientFactory) :
         _localConfiguration.update { prev ->
             Timber.e("changeFeatureConfigurationForPIDs.prev $prev")
             Timber.e(
-                "changeFeatureConfigurationForPIDs() $vfsWriteFeature, $sendMessageFeature, $uprobesFeature, $jniReferencesFeature"
+                "changeFeatureConfigurationForPIDs() " +
+                    "vfs=$vfsWriteFeature, sendMsg=$sendMessageFeature, " +
+                    "uprobes=$uprobesFeature, jni=$jniReferencesFeature"
             )
             // the configuration shall not be changed from the UI if there is none received from
             // backend
             if (prev != null && prev is ConfigurationUpdate.Valid) {
                 val previousConfiguration = prev.configuration
                 previousConfiguration
                     .copy(
-                        vfsWrite =
-                            vfsWriteFeature?.let { requestedChanges ->
-                                previousConfiguration.vfsWrite.updatePIDs(
-                                    pidsToAdd =
-                                        if (enable) requestedChanges.entries.entries else setOf(),
-                                    pidsToRemove =
-                                        if (!enable) requestedChanges.entries.entries else setOf(),
-                                )
-                            } ?: previousConfiguration.vfsWrite,
-                        sysSendmsg =
-                            sendMessageFeature?.let { requestedChanges ->
-                                previousConfiguration.sysSendmsg.updatePIDs(
-                                    pidsToAdd =
-                                        if (enable) requestedChanges.entries.entries else setOf(),
-                                    pidsToRemove =
-                                        if (!enable) requestedChanges.entries.entries else setOf(),
-                                )
-                            } ?: previousConfiguration.sysSendmsg,
-                        uprobes =
-                            uprobesFeature.let { requestedChanges ->
-                                if (requestedChanges == null)
-                                    return@let previousConfiguration.uprobes
-                                previousConfiguration.uprobes.updateUProbes(
-                                    pidsToAdd = if (enable) requestedChanges else listOf(),
-                                    pidsToRemove = if (!enable) requestedChanges else listOf(),
-                                )
-                            },
-                        jniReferences =
-                            jniReferencesFeature?.let { requestedChanges ->
-                                previousConfiguration.jniReferences.updatePIDs(
-                                    pidsToAdd = if (enable) requestedChanges.pids else listOf(),
-                                    pidsToRemove = if (!enable) requestedChanges.pids else listOf(),
-                                )
-                            } ?: previousConfiguration.jniReferences,
+                        vfsWrite = previousConfiguration.merge(vfsWriteFeature, enable),
+                        sysSendmsg = previousConfiguration.merge(sendMessageFeature, enable),
+                        uprobes = previousConfiguration.merge(uprobesFeature, enable),
+                        jniReferences = previousConfiguration.merge(jniReferencesFeature, enable),
                     )
                     .also { Timber.i("new local configuration = $it") }
                     .let { ConfigurationUpdate.Valid(it) }
             } else return@update prev
         }
     }
 
+    fun Configuration.merge(vfsWriteConfig: VfsWriteConfig?, enable: Boolean) =
+        vfsWriteConfig?.let { requestedChanges ->
+            this.vfsWrite.updatePIDs(
+                pidsToAdd = if (enable) requestedChanges.entries.entries else setOf(),
+                pidsToRemove = if (!enable) requestedChanges.entries.entries else setOf(),
+            )
+        } ?: this.vfsWrite
+
+    fun Configuration.merge(sysSendmsgConfig: SysSendmsgConfig?, enable: Boolean) =
+        sysSendmsgConfig?.let { requestedChanges ->
+            this.sysSendmsg.updatePIDs(
+                pidsToAdd = if (enable) requestedChanges.entries.entries else setOf(),
+                pidsToRemove = if (!enable) requestedChanges.entries.entries else setOf(),
+            )
+        } ?: this.sysSendmsg
+
+    fun Configuration.merge(uprobeConfigs: List<UprobeConfig>?, enable: Boolean) =
+        uprobeConfigs?.let { requestedChanges ->
+            this.uprobes.updateUProbes(
+                pidsToAdd = if (enable) requestedChanges else listOf(),
+                pidsToRemove = if (!enable) requestedChanges else listOf(),
+            )
+        } ?: this.uprobes
+
+    fun Configuration.merge(jniReferencesConfig: JniReferencesConfig?, enable: Boolean) =
+        jniReferencesConfig?.let { requestedChanges ->
+            this.jniReferences.updatePIDs(
+                pidsToAdd = if (enable) requestedChanges.pids else listOf(),
+                pidsToRemove = if (!enable) requestedChanges.pids else listOf(),
+            )
+        } ?: this.jniReferences
+
     override fun submitConfiguration() {
         coroutineScope.launch {
             sendLocalToBackend()
@@ -130,6 +135,7 @@ class ConfigurationManager(val clientFactory: ClientFactory) :
         }
     }
 
+    @Suppress("TooGenericExceptionCaught", "SwallowedException") // initialization mechanism
     private suspend fun initializeConfigurations() {
         val initializedConfiguration =
             try {
@@ -141,6 +147,7 @@ class ConfigurationManager(val clientFactory: ClientFactory) :
     }
 
     // TODO this should be handled on the backend
+    @Suppress("TooGenericExceptionCaught") // we want to display all exceptions
     private suspend fun getOrCreateInitialConfiguration(): ConfigurationUpdate {
         return try {
             // the config may not be initialized, we should try initializing it
@@ -164,6 +171,7 @@ class ConfigurationManager(val clientFactory: ClientFactory) :
         } ?: Timber.e("unsubmittedConfiguration == null -> this should never happen")
     }
 
+    @Suppress("TooGenericExceptionCaught") // we want to display all exceptions
     private suspend fun getFromBackend(): ConfigurationUpdate {
         return try {
             (client?.getConfiguration()?.let { ConfigurationUpdate.Valid(it) }

diff --git a/frontend/app/src/main/java/de/amosproj3/ziofa/bl/events/DataStreamManager.kt b/frontend/app/src/main/java/de/amosproj3/ziofa/bl/events/DataStreamManager.kt
@@ -17,7 +17,6 @@ import kotlinx.coroutines.flow.flow
 import kotlinx.coroutines.flow.map
 import kotlinx.coroutines.flow.mapNotNull
 import kotlinx.coroutines.flow.shareIn
-import timber.log.Timber
 
 class DataStreamManager(private val clientFactory: ClientFactory, coroutineScope: CoroutineScope) :
     DataStreamProvider {
@@ -26,23 +25,7 @@ class DataStreamManager(private val clientFactory: ClientFactory, coroutineScope
         flow { clientFactory.connect().initStream().collect { emit(it) } }
             .shareIn(coroutineScope, SharingStarted.Lazily)
 
-    override suspend fun counter(ebpfProgramName: String): Flow<UInt> {
-        return clientFactory
-            .connect()
-            .also {
-                try {
-                    it.load()
-                    // default wifi interface on android, now configurable
-                    it.attach("wlan0")
-                    it.startCollecting()
-                } catch (e: Exception) {
-                    Timber.e(e.stackTraceToString())
-                }
-            }
-            .serverCount()
-    }
-
-    override suspend fun vfsWriteEvents(pids: List<UInt>?): Flow<BackendEvent.VfsWriteEvent> =
+    override fun vfsWriteEvents(pids: List<UInt>?): Flow<BackendEvent.VfsWriteEvent> =
         dataFlow
             .mapNotNull { it as? Event.VfsWrite }
             .filter { it.pid.isGlobalRequestedOrPidConfigured(pids) }
@@ -55,7 +38,7 @@ class DataStreamManager(private val clientFactory: ClientFactory, coroutineScope
                 )
             }
 
-    override suspend fun sendMessageEvents(pids: List<UInt>?): Flow<BackendEvent.SendMessageEvent> =
+    override fun sendMessageEvents(pids: List<UInt>?): Flow<BackendEvent.SendMessageEvent> =
         dataFlow
             .mapNotNull { it as? Event.SysSendmsg }
             .filter { it.pid.isGlobalRequestedOrPidConfigured(pids) }

diff --git a/frontend/app/src/main/java/de/amosproj3/ziofa/ui/ZiofaApp.kt b/frontend/app/src/main/java/de/amosproj3/ziofa/ui/ZiofaApp.kt
@@ -35,6 +35,7 @@ val GLOBAL_CONFIGURATION_ROUTE =
     "${Routes.IndividualConfiguration.name}?displayName=${Uri.encode("all processes")}?pids=-1"
 
 /** Main application composable. All calls to [NavController] should happen here. */
+@Suppress("ModifierMissing") // Top level composable
 @Composable
 fun ZIOFAApp() {
     val navController = rememberNavController()
@@ -56,16 +57,16 @@ fun ZIOFAApp() {
             }
             screenWithDefaultAnimations(Routes.Reset.name) {
                 ResetScreen(
-                    Modifier.padding(innerPadding),
                     afterResetConfirmed = { navController.popBackStack() },
+                    modifier = Modifier.padding(innerPadding),
                 )
             }
             screenWithDefaultAnimations(Routes.Configuration.name) {
                 ConfigurationMenu(
-                    Modifier.padding(innerPadding),
                     toProcesses = { navController.navigate(Routes.Processes.name) },
                     toGlobalConfiguration = { navController.navigate(GLOBAL_CONFIGURATION_ROUTE) },
                     toReset = { navController.navigate(Routes.Reset.name) },
+                    modifier = Modifier.padding(innerPadding),
                 )
             }
             screenWithDefaultAnimations(Routes.Visualize.name) {
@@ -77,8 +78,8 @@ fun ZIOFAApp() {
             screenWithDefaultAnimations(Routes.Processes.name) {
                 ProcessesScreen(
                     Modifier.padding(innerPadding),
-                    onClickEdit = {
-                        navController.navigate(it.toConfigurationScreenRouteForComponent())
+                    onClickEdit = { component ->
+                        navController.navigate(component.toConfigurationScreenRouteForComponent())
                     },
                 )
             }
@@ -98,7 +99,6 @@ fun ZIOFAApp() {
             ) {
                 ConfigurationScreen(
                     Modifier.padding(innerPadding),
-                    onBack = { navController.popBackStack() },
                     pids = it.arguments?.getString("pids")?.deserializePIDs()?.validPIDsOrNull(),
                     onAddUprobeSelected = {
                         navController.navigate(it.arguments.copyToSymbolsRoute())
@@ -124,8 +124,11 @@ fun ZIOFAApp() {
                     modifier = Modifier.padding(innerPadding),
                     onSymbolsSubmitted = { navController.popBackStack() },
                     pids =
-                        it.arguments?.getString("pids")?.deserializePIDs()?.validPIDsOrNull()
-                            ?: listOf(),
+                        it.arguments
+                            ?.getString("pids")
+                            ?.deserializePIDs()
+                            ?.validPIDsOrNull()
+                            .orEmpty(),
                 )
             }
         }

diff --git a/frontend/app/src/main/java/de/amosproj3/ziofa/ui/about/AboutScreen.kt b/frontend/app/src/main/java/de/amosproj3/ziofa/ui/about/AboutScreen.kt
@@ -12,8 +12,17 @@ import androidx.compose.ui.Modifier
 import androidx.compose.ui.unit.dp
 
 const val PRODUCT_MISSION =
-    "ZIOFA (Zero Instrumentation Observability for Android) aims to implement observability use cases relevant to performance specified by our industry partner using eBPF. Examples include tracing long-running blocking calls, leaking JNI indirect references or signals like SIGKILL sent to processes, all without instrumenting the observed application itself.\n" +
-        "The eBPF programs are loaded and unloaded using a backend daemon running as root that will collect metrics and send them to a client. For displaying these metrics to the user, we are implementing an on-device UI that can display visualizations for these use cases and allow for configuration of the enabled use cases, but using a decoupled Client SDK so that future work may easily make the data accessible the external processing."
+    "ZIOFA (Zero Instrumentation Observability for Android) aims to implement observability use " +
+        "cases relevant to performance specified by our industry partner using eBPF. " +
+        "Examples include tracing long-running blocking calls, leaking JNI indirect" +
+        "references or signals like SIGKILL sent to processes, all without instrumenting" +
+        " the observed application itself.\n" +
+        "The eBPF programs are loaded and unloaded using a backend daemon running as root that " +
+        "will collect metrics and send them to a client. For displaying these metrics to " +
+        "the user, we are implementing an on-device UI that can display visualizations for" +
+        " these use cases and allow for configuration of the enabled use cases, but using a " +
+        "decoupled Client SDK so that future work may easily make the data accessible the " +
+        "external processing."
 
 /**
  * Screen containing information about the project. Might delete later if we need space for another

diff --git a/frontend/app/src/main/java/de/amosproj3/ziofa/ui/configuration/ConfigurationScreen.kt b/frontend/app/src/main/java/de/amosproj3/ziofa/ui/configuration/ConfigurationScreen.kt
@@ -35,7 +35,6 @@ import org.koin.core.parameter.parametersOf
 @Composable
 fun ConfigurationScreen(
     modifier: Modifier = Modifier,
-    onBack: () -> Unit = {},
     onAddUprobeSelected: () -> Unit = {},
     pids: List<UInt>? = listOf(),
 ) {
@@ -62,9 +61,7 @@ fun ConfigurationScreen(
                     SectionTitleRow("Uprobes")
                     EbpfUprobeFeatureOptions(
                         options =
-                            state.options.mapNotNull {
-                                if (it is BackendFeatureOptions.UprobeOption) it else null
-                            },
+                            state.options.mapNotNull { it as? BackendFeatureOptions.UprobeOption },
                         onOptionDeleted = { option ->
                             viewModel.optionChanged(option, active = false)
                         },

diff --git a/...app/src/main/java/de/amosproj3/ziofa/ui/configuration/composables/EbpfIOFeatureOptions.kt b/...app/src/main/java/de/amosproj3/ziofa/ui/configuration/composables/EbpfIOFeatureOptions.kt
@@ -23,8 +23,9 @@ import de.amosproj3.ziofa.ui.configuration.data.BackendFeatureOptions
 fun EbpfIOFeatureOptions(
     options: List<BackendFeatureOptions>,
     onOptionChanged: (BackendFeatureOptions, Boolean) -> Unit,
+    modifier: Modifier = Modifier,
 ) {
-    LazyColumn(modifier = Modifier.padding(horizontal = 20.dp, vertical = 15.dp).fillMaxWidth()) {
+    LazyColumn(modifier = modifier.padding(horizontal = 20.dp, vertical = 15.dp).fillMaxWidth()) {
         items(options) { option ->
             Row(
                 modifier = Modifier.fillMaxWidth(),

diff --git a/...src/main/java/de/amosproj3/ziofa/ui/configuration/composables/EbpfUprobeFeatureOptions.kt b/...src/main/java/de/amosproj3/ziofa/ui/configuration/composables/EbpfUprobeFeatureOptions.kt
@@ -28,8 +28,9 @@ fun EbpfUprobeFeatureOptions(
     options: List<BackendFeatureOptions.UprobeOption>,
     onOptionDeleted: (BackendFeatureOptions.UprobeOption) -> Unit,
     onAddUprobeSelected: () -> Unit,
+    modifier: Modifier = Modifier,
 ) {
-    LazyColumn(modifier = Modifier.padding(horizontal = 20.dp, vertical = 15.dp).fillMaxSize()) {
+    LazyColumn(modifier = modifier.padding(horizontal = 20.dp, vertical = 15.dp).fillMaxSize()) {
         items(options) { option ->
             Row(
                 modifier = Modifier.fillMaxWidth(),

diff --git a/frontend/app/src/main/java/de/amosproj3/ziofa/ui/configuration/composables/ErrorScreen.kt b/frontend/app/src/main/java/de/amosproj3/ziofa/ui/configuration/composables/ErrorScreen.kt
@@ -25,6 +25,8 @@ import androidx.compose.ui.unit.TextUnitType
 import androidx.compose.ui.unit.dp
 import kotlin.system.exitProcess
 
+const val TITLE_TEXT_SIZE = 25f
+
 @Preview(device = Devices.AUTOMOTIVE_1024p)
 @Composable
 fun ErrorScreen(error: String = "No error message available") {
@@ -36,7 +38,7 @@ fun ErrorScreen(error: String = "No error message available") {
             Text(
                 text = "Error while communicating with backend",
                 color = Color.Red,
-                fontSize = TextUnit(25f, TextUnitType.Sp),
+                fontSize = TextUnit(TITLE_TEXT_SIZE, TextUnitType.Sp),
             )
             Text(text = error)
         }

diff --git a/...tend/app/src/main/java/de/amosproj3/ziofa/ui/configuration/composables/SectionTitleRow.kt b/...tend/app/src/main/java/de/amosproj3/ziofa/ui/configuration/composables/SectionTitleRow.kt
@@ -15,8 +15,8 @@ import androidx.compose.ui.text.font.FontWeight
 import androidx.compose.ui.unit.dp
 
 @Composable
-fun SectionTitleRow(title: String) {
-    Row(horizontalArrangement = Arrangement.Center, modifier = Modifier.padding(bottom = 10.dp)) {
+fun SectionTitleRow(title: String, modifier: Modifier = Modifier) {
+    Row(horizontalArrangement = Arrangement.Center, modifier = modifier.padding(bottom = 10.dp)) {
         Text(title, fontWeight = FontWeight.Bold)
     }
     HorizontalDivider(thickness = 5.dp)

diff --git a/...i/configuration/data/EBpfProgramOption.kt → ...nfiguration/data/BackendFeatureOptions.kt b/...i/configuration/data/EBpfProgramOption.kt → ...nfiguration/data/BackendFeatureOptions.kt
diff --git a/frontend/app/src/main/java/de/amosproj3/ziofa/ui/navigation/ConfigurationMenu.kt b/frontend/app/src/main/java/de/amosproj3/ziofa/ui/navigation/ConfigurationMenu.kt
@@ -14,16 +14,16 @@ import androidx.compose.material3.MaterialTheme
 import androidx.compose.runtime.Composable
 import androidx.compose.ui.Modifier
 import androidx.compose.ui.unit.dp
-import de.amosproj3.ziofa.ui.navigation.composables.MenuOptionData
 import de.amosproj3.ziofa.ui.navigation.composables.MenuOptions
 import de.amosproj3.ziofa.ui.navigation.data.Emoji
+import de.amosproj3.ziofa.ui.navigation.data.MenuOptionData
 
 @Composable
 fun ConfigurationMenu(
-    modifier: Modifier = Modifier,
     toProcesses: () -> Unit,
     toGlobalConfiguration: () -> Unit,
     toReset: () -> Unit,
+    modifier: Modifier = Modifier,
 ) {
 
     Box(

diff --git a/frontend/app/src/main/java/de/amosproj3/ziofa/ui/navigation/HomeScreen.kt b/frontend/app/src/main/java/de/amosproj3/ziofa/ui/navigation/HomeScreen.kt
@@ -16,9 +16,9 @@ import androidx.compose.ui.Modifier
 import androidx.compose.ui.tooling.preview.Devices
 import androidx.compose.ui.tooling.preview.Preview
 import androidx.compose.ui.unit.dp
-import de.amosproj3.ziofa.ui.navigation.composables.MenuOptionData
 import de.amosproj3.ziofa.ui.navigation.composables.MenuOptions
 import de.amosproj3.ziofa.ui.navigation.data.Emoji
+import de.amosproj3.ziofa.ui.navigation.data.MenuOptionData
 
 /** Static home screen for navigation */
 @Composable

diff --git a/frontend/app/src/main/java/de/amosproj3/ziofa/ui/navigation/composables/MenuOptions.kt b/frontend/app/src/main/java/de/amosproj3/ziofa/ui/navigation/composables/MenuOptions.kt
@@ -22,11 +22,13 @@ import androidx.compose.ui.Modifier
 import androidx.compose.ui.unit.TextUnit
 import androidx.compose.ui.unit.TextUnitType
 import androidx.compose.ui.unit.dp
+import de.amosproj3.ziofa.ui.navigation.data.MenuOptionData
 
-data class MenuOptionData(val title: String, val logoEmoji: String, val onClick: () -> Unit)
+private const val CARD_EMOJI_SIZE = 120f
+private const val CARD_TITLE_TEXT_SIZE = 120f
 
 @Composable
-fun MenuOptions(modifier: Modifier = Modifier, menuOptions: List<MenuOptionData>) {
+fun MenuOptions(menuOptions: List<MenuOptionData>, modifier: Modifier = Modifier) {
     Row(
         modifier = modifier.fillMaxWidth(),
         horizontalArrangement = Arrangement.SpaceEvenly,
@@ -65,10 +67,10 @@ fun MenuCardWithIcon(
         ) {
             Text(
                 emoji,
-                fontSize = TextUnit(120f, TextUnitType.Sp),
+                fontSize = TextUnit(CARD_EMOJI_SIZE, TextUnitType.Sp),
                 modifier = Modifier.padding(bottom = 20.dp),
             )
-            Text(text, fontSize = TextUnit(40f, TextUnitType.Sp))
+            Text(text, fontSize = TextUnit(CARD_TITLE_TEXT_SIZE, TextUnitType.Sp))
         }
     }
 }