From 54d788831699b549f49fbc8023de1a4f2643b0ac Mon Sep 17 00:00:00 2001 From: Nik Clayton Date: Sun, 4 Feb 2024 15:17:46 +0100 Subject: [PATCH] feat: Include extra logs in error reports from orange release builds (#414) Release builds normally strip out all logging to reduce the number of disk writes and reduce UI jank. These logs would still be useful in user error reports from orange builds. To preseve them: - Implement a simple `RingBuffer`. - Create `TreeRing`, a `Timber` `Tree` logger that logs to a `RingBuffer` instance in orange release builds. - Create `TreeRingCollector`, called when ACRA reports are generated, which includes the contents of the ring buffer in the report. - Enable desugaring to allow the use of java.time libraries on older Android versions. - Disable ProGuard obfuscation of class names as the obfuscation adds additional de-obfuscation steps when handling error reports from users. --- app/build.gradle.kts | 6 ++ app/lint-baseline.xml | 84 ++++++++-------- app/proguard-rules.pro | 5 + .../main/java/app/pachli/PachliApplication.kt | 6 +- core/activity/build.gradle.kts | 8 ++ .../app/pachli/core/activity/CrashReporter.kt | 7 ++ .../app/pachli/core/activity/CrashReporter.kt | 70 ++++++++++++++ .../app/pachli/core/activity/RingBuffer.kt | 62 ++++++++++++ .../src/testOrange/kotlin/RingBufferTest.kt | 95 +++++++++++++++++++ gradle/libs.versions.toml | 6 ++ 10 files changed, 306 insertions(+), 43 deletions(-) create mode 100644 core/activity/src/orange/kotlin/app/pachli/core/activity/RingBuffer.kt create mode 100644 core/activity/src/testOrange/kotlin/RingBufferTest.kt diff --git a/app/build.gradle.kts b/app/build.gradle.kts index c6e45e908..2fe017edd 100644 --- a/app/build.gradle.kts +++ b/app/build.gradle.kts @@ -53,6 +53,10 @@ android { } } + compileOptions { + isCoreLibraryDesugaringEnabled = true + } + packaging { resources.excludes.apply { add("LICENSE_OFL") @@ -112,6 +116,8 @@ configurations { } dependencies { + coreLibraryDesugaring(libs.desugar.jdk.libs) + // CachedTimelineRemoteMediator needs the @Transaction annotation from Room compileOnly(libs.bundles.room) testCompileOnly(libs.bundles.room) diff --git a/app/lint-baseline.xml b/app/lint-baseline.xml index fa5f205af..60d4abae7 100644 --- a/app/lint-baseline.xml +++ b/app/lint-baseline.xml @@ -2621,7 +2621,7 @@ errorLine2=" ~~~~~~~~~~~~~~~~~"> @@ -2632,7 +2632,7 @@ errorLine2=" ~~~~~~~"> @@ -2643,7 +2643,7 @@ errorLine2=" ~~~~~~~~~~~~~~~~~"> @@ -2654,7 +2654,7 @@ errorLine2=" ~~~~~~~"> @@ -2665,7 +2665,7 @@ errorLine2=" ~~~~~~~~~~~~~~~~~"> @@ -2676,7 +2676,7 @@ errorLine2=" ~~~~~~~"> @@ -2687,7 +2687,7 @@ errorLine2=" ~~~~~~~~~~~~~~~~~"> @@ -2698,7 +2698,7 @@ errorLine2=" ~~~~~~~"> @@ -2709,7 +2709,7 @@ errorLine2=" ~~~~~~~~~~~~~~~~~"> @@ -2720,7 +2720,7 @@ errorLine2=" ~~~~~~~"> @@ -2731,7 +2731,7 @@ errorLine2=" ~~~~~~~~~~~~~~~~~"> @@ -2742,7 +2742,7 @@ errorLine2=" ~~~~~~~"> @@ -2753,7 +2753,7 @@ errorLine2=" ~~~~~~~~~~~~~~~~~"> @@ -2764,7 +2764,7 @@ errorLine2=" ~~~~~~~"> @@ -2775,7 +2775,7 @@ errorLine2=" ~~~~~~~~~~~~~~~~~"> @@ -2786,7 +2786,7 @@ errorLine2=" ~~~~~~~"> @@ -2797,7 +2797,7 @@ errorLine2=" ~~~~~~~~~~~~~~~~~~~"> @@ -2808,7 +2808,7 @@ errorLine2=" ~~~~~~~"> @@ -2819,7 +2819,7 @@ errorLine2=" ~~~~~~~~~~~~~~~~~~~"> @@ -2830,7 +2830,7 @@ errorLine2=" ~~~~~~~"> @@ -2841,7 +2841,7 @@ errorLine2=" ~~~~~~~~~~~~~~~~~~~"> @@ -2852,7 +2852,7 @@ errorLine2=" ~~~~~~~"> @@ -2863,7 +2863,7 @@ errorLine2=" ~~~~~~~~~~~~~~~~~~~"> @@ -2874,7 +2874,7 @@ errorLine2=" ~~~~~~~"> @@ -2885,7 +2885,7 @@ errorLine2=" ~~~~~~~~~~~~~~~~~"> @@ -2896,7 +2896,7 @@ errorLine2=" ~~~~~~~"> @@ -2907,7 +2907,7 @@ errorLine2=" ~~~~~~~~~~~~~~~~~"> @@ -2918,7 +2918,7 @@ errorLine2=" ~~~~~~~"> @@ -2929,7 +2929,7 @@ errorLine2=" ~~~~~~~~~~~~~~~~~~~"> @@ -2940,7 +2940,7 @@ errorLine2=" ~~~~~~~"> @@ -2949,17 +2949,6 @@ message="Access to `private` method `getBinding` of class `MainActivity` requires synthetic accessor" errorLine1=" binding.mainToolbar.title = tab.contentDescription" errorLine2=" ~~~~~~~"> - - - - + + + + diff --git a/app/proguard-rules.pro b/app/proguard-rules.pro index 1a4d2c0f6..7047a3b26 100644 --- a/app/proguard-rules.pro +++ b/app/proguard-rules.pro @@ -52,6 +52,11 @@ public *; } +# Keep class names. Obfuscating them serves no purpose in an open source +# project and adds an additional step to de-obfuscate them when managing user +# error reports +-keepnames class * + # https://github.com/google/gson/blob/master/examples/android-proguard-example/proguard.cfg # Prevent proguard from stripping interface information from TypeAdapter, TypeAdapterFactory, diff --git a/app/src/main/java/app/pachli/PachliApplication.kt b/app/src/main/java/app/pachli/PachliApplication.kt index 3de7802b9..8871a55c7 100644 --- a/app/src/main/java/app/pachli/PachliApplication.kt +++ b/app/src/main/java/app/pachli/PachliApplication.kt @@ -25,6 +25,7 @@ import androidx.work.ExistingPeriodicWorkPolicy import androidx.work.PeriodicWorkRequestBuilder import androidx.work.WorkManager import app.pachli.components.notifications.createWorkerNotificationChannel +import app.pachli.core.activity.TreeRing import app.pachli.core.activity.initCrashReporter import app.pachli.core.preferences.AppTheme import app.pachli.core.preferences.NEW_INSTALL_SCHEMA_VERSION @@ -81,7 +82,10 @@ class PachliApplication : Application() { AutoDisposePlugins.setHideProxies(false) // a small performance optimization - if (BuildConfig.DEBUG) Timber.plant(Timber.DebugTree()) + when { + BuildConfig.DEBUG -> Timber.plant(Timber.DebugTree()) + BuildConfig.FLAVOR_color == "orange" -> Timber.plant(TreeRing) + } // Migrate shared preference keys and defaults from version to version. val oldVersion = sharedPreferencesRepository.getInt(PrefKeys.SCHEMA_VERSION, NEW_INSTALL_SCHEMA_VERSION) diff --git a/core/activity/build.gradle.kts b/core/activity/build.gradle.kts index c1f6635a2..5cada3f67 100644 --- a/core/activity/build.gradle.kts +++ b/core/activity/build.gradle.kts @@ -27,6 +27,10 @@ android { testInstrumentationRunner = "androidx.test.runner.AndroidJUnitRunner" testInstrumentationRunnerArguments["disableAnalytics"] = "true" } + + compileOptions { + isCoreLibraryDesugaringEnabled = true + } } dependencies { @@ -50,6 +54,10 @@ dependencies { // Crash reporting in orange (Pachli Current) builds only orangeImplementation(libs.bundles.acra) + coreLibraryDesugaring(libs.desugar.jdk.libs) + orangeCompileOnly(libs.auto.service.annotations) + kspOrange(libs.auto.service.ksp) + // BottomSheetActivityTest uses mockito testImplementation(libs.bundles.mockito) } diff --git a/core/activity/src/blue/kotlin/app/pachli/core/activity/CrashReporter.kt b/core/activity/src/blue/kotlin/app/pachli/core/activity/CrashReporter.kt index 9aa890234..af76f53a4 100644 --- a/core/activity/src/blue/kotlin/app/pachli/core/activity/CrashReporter.kt +++ b/core/activity/src/blue/kotlin/app/pachli/core/activity/CrashReporter.kt @@ -18,7 +18,14 @@ package app.pachli.core.activity import android.app.Application +import timber.log.Timber /** Do nothing in blue builds */ fun initCrashReporter(app: Application) {} fun triggerCrashReport() {} + +object TreeRing : Timber.Tree() { + override fun log(priority: Int, tag: String?, message: String, t: Throwable?) { + // Do nothing + } +} diff --git a/core/activity/src/orange/kotlin/app/pachli/core/activity/CrashReporter.kt b/core/activity/src/orange/kotlin/app/pachli/core/activity/CrashReporter.kt index 5a3016218..8ec30f620 100644 --- a/core/activity/src/orange/kotlin/app/pachli/core/activity/CrashReporter.kt +++ b/core/activity/src/orange/kotlin/app/pachli/core/activity/CrashReporter.kt @@ -18,12 +18,21 @@ package app.pachli.core.activity import android.app.Application +import android.content.Context +import android.util.Log import app.pachli.core.designsystem.R as DR +import com.google.auto.service.AutoService +import java.time.Instant import org.acra.ACRA +import org.acra.builder.ReportBuilder +import org.acra.collector.Collector +import org.acra.config.CoreConfiguration import org.acra.config.dialog import org.acra.config.mailSender +import org.acra.data.CrashReportData import org.acra.data.StringFormat import org.acra.ktx.initAcra +import timber.log.Timber /** * Initialise ACRA. @@ -54,3 +63,64 @@ fun initCrashReporter(app: Application) { fun triggerCrashReport() { ACRA.errorReporter.handleException(null, false) } + +/** + * [Timber.Tree] that logs in to ring buffer, keeping the most recent 1,000 log + * entries. + */ +object TreeRing : Timber.DebugTree() { + /** + * Store the components of a log line without doing any formatting or other + * work at logging time. + */ + data class LogEntry( + val instant: Instant, + val priority: Int, + val tag: String?, + val message: String, + val t: Throwable?, + ) + + val buffer = RingBuffer(1000) + + override fun log(priority: Int, tag: String?, message: String, t: Throwable?) { + buffer.add(LogEntry(Instant.now(), priority, tag, message, t)) + } +} + +/** + * Acra collector that appends the contents of the [TreeRing] log to the Acra + * report. + */ +@AutoService(Collector::class) +class TreeRingCollector : Collector { + /** Map log priority values to characters to use when displaying the log */ + private val priority = mapOf( + Log.VERBOSE to 'V', + Log.DEBUG to 'D', + Log.INFO to 'I', + Log.WARN to 'W', + Log.ERROR to 'E', + Log.ASSERT to 'A', + ) + + override fun collect( + context: Context, + config: CoreConfiguration, + reportBuilder: ReportBuilder, + crashReportData: CrashReportData, + ) { + crashReportData.put( + "TreeRing", + TreeRing.buffer.toList().joinToString("\n") { + "%s %c/%s: %s%s".format( + it.instant.toString(), + priority[it.priority] ?: '?', + it.tag, + it.message, + it.t?.let { t -> " $t" } ?: "", + ) + }, + ) + } +} diff --git a/core/activity/src/orange/kotlin/app/pachli/core/activity/RingBuffer.kt b/core/activity/src/orange/kotlin/app/pachli/core/activity/RingBuffer.kt new file mode 100644 index 000000000..f3c3266de --- /dev/null +++ b/core/activity/src/orange/kotlin/app/pachli/core/activity/RingBuffer.kt @@ -0,0 +1,62 @@ +/* + * Copyright 2024 Pachli Association + * + * This file is a part of Pachli. + * + * This program is free software; you can redistribute it and/or modify it under the terms of the + * GNU General Public License as published by the Free Software Foundation; either version 3 of the + * License, or (at your option) any later version. + * + * Pachli is distributed in the hope that it will be useful, but WITHOUT ANY WARRANTY; without even + * the implied warranty of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU General + * Public License for more details. + * + * You should have received a copy of the GNU General Public License along with Pachli; if not, + * see . + */ + +package app.pachli.core.activity + +import java.util.concurrent.atomic.AtomicInteger + +/** + * A ring buffer with space for [capacity] items. Adding new items to the buffer will + * drop older items. + */ +class RingBuffer(private val capacity: Int) : Iterable { + private val data: Array = arrayOfNulls(capacity) + private var nItems: Int = 0 + private var tail: Int = -1 + + private val head: Int + get() = if (nItems == data.size) (tail + 1) % nItems else 0 + + /** Number of items in the buffer */ + val size: Int + get() = nItems + + /** Add an item to the buffer, overwriting the oldest item in the buffer */ + fun add(item: T) { + tail = (tail + 1) % data.size + data[tail] = item + if (nItems < data.size) nItems++ + } + + /** Get an item from the buffer */ + @Suppress("UNCHECKED_CAST") + operator fun get(index: Int): T = when { + nItems == 0 || index > nItems || index < 0 -> throw IndexOutOfBoundsException("$index") + nItems == data.size -> data[(head + index) % data.size] + else -> data[index] + } as T + + /** Buffer as a list. */ + @Suppress("UNCHECKED_CAST") + fun toList(): List = iterator().asSequence().toList() + + override fun iterator(): Iterator = object : Iterator { + private val index: AtomicInteger = AtomicInteger(0) + override fun hasNext(): Boolean = index.get() < size + override fun next(): T = get(index.getAndIncrement()) + } +} diff --git a/core/activity/src/testOrange/kotlin/RingBufferTest.kt b/core/activity/src/testOrange/kotlin/RingBufferTest.kt new file mode 100644 index 000000000..701f44d0b --- /dev/null +++ b/core/activity/src/testOrange/kotlin/RingBufferTest.kt @@ -0,0 +1,95 @@ + +import app.pachli.core.activity.RingBuffer +import com.google.common.truth.Truth.assertThat +import org.junit.Test + +/* + * Copyright 2024 Pachli Association + * + * This file is a part of Pachli. + * + * This program is free software; you can redistribute it and/or modify it under the terms of the + * GNU General Public License as published by the Free Software Foundation; either version 3 of the + * License, or (at your option) any later version. + * + * Pachli is distributed in the hope that it will be useful, but WITHOUT ANY WARRANTY; without even + * the implied warranty of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU General + * Public License for more details. + * + * You should have received a copy of the GNU General Public License along with Pachli; if not, + * see . + */ + +class RingBufferTest { + @Test + fun initialSize() { + val buffer = RingBuffer(10) + assertThat(buffer.size).isEqualTo(0) + } + + @Test + fun `size increases when there is capacity`() { + val buffer = RingBuffer(10) + buffer.add(1) + assertThat(buffer.size).isEqualTo(1) + } + + @Test + fun `size is bounded by capacity`() { + val buffer = RingBuffer(10) + repeat(20) { buffer.add(it) } + assertThat(buffer.size).isEqualTo(10) + } + + @Test + fun `inserting elements cycles around`() { + val buffer = RingBuffer(3) + buffer.add(1) + assertThat(buffer[0]).isEqualTo(1) + + buffer.add(2) + assertThat(buffer[0]).isEqualTo(1) + assertThat(buffer[1]).isEqualTo(2) + + buffer.add(3) + assertThat(buffer[0]).isEqualTo(1) + assertThat(buffer[1]).isEqualTo(2) + assertThat(buffer[2]).isEqualTo(3) + + // Exceed capacity, first item added is dropped + buffer.add(4) + assertThat(buffer[0]).isEqualTo(2) + assertThat(buffer[1]).isEqualTo(3) + assertThat(buffer[2]).isEqualTo(4) + } + + @Test + fun `toList() returns expected list`() { + val buffer = RingBuffer(3) + repeat(3) { buffer.add(it) } + + assertThat(buffer.toList()).isEqualTo(listOf(0, 1, 2)) + + buffer.add(3) + assertThat(buffer.toList()).isEqualTo(listOf(1, 2, 3)) + } + + @Test(expected = IndexOutOfBoundsException::class) + fun getZeroSizeException() { + RingBuffer(1)[0] + } + + @Test(expected = IndexOutOfBoundsException::class) + fun getIndexOutOfBounds() { + val buffer = RingBuffer(1) + buffer.add(1) + buffer[10] + } + + @Test(expected = IndexOutOfBoundsException::class) + fun getIndexNegative() { + val buffer = RingBuffer(1) + buffer.add(1) + buffer[-1] + } +} diff --git a/gradle/libs.versions.toml b/gradle/libs.versions.toml index 37888dff0..fd82ec246 100644 --- a/gradle/libs.versions.toml +++ b/gradle/libs.versions.toml @@ -26,9 +26,12 @@ androidx-work = "2.8.1" androidx-room = "2.6.1" app-update = "2.1.0" autodispose = "2.2.1" +auto-service = "1.1.1" +auto-service-ksp = "1.1.0" bouncycastle = "1.70" conscrypt = "2.5.2" coroutines = "1.7.3" +desugar_jdk_libs = "2.0.3" diffx = "1.1.1" emoji2 = "1.3.0" espresso = "3.5.1" @@ -135,8 +138,11 @@ app-update = { module = "com.google.android.play:app-update", version.ref = "app app-update-ktx = { module = "com.google.android.play:app-update-ktx", version.ref = "app-update" } autodispose-android-lifecycle = { module = "com.uber.autodispose2:autodispose-androidx-lifecycle", version.ref = "autodispose" } autodispose-core = { module = "com.uber.autodispose2:autodispose", version.ref = "autodispose" } +auto-service-annotations = { module = "com.google.auto.service:auto-service-annotations", version.ref = "auto-service"} +auto-service-ksp = { module = "dev.zacsweers.autoservice:auto-service-ksp", version.ref = "auto-service-ksp"} bouncycastle = { module = "org.bouncycastle:bcprov-jdk15on", version.ref = "bouncycastle" } conscrypt-android = { module = "org.conscrypt:conscrypt-android", version.ref = "conscrypt" } +desugar_jdk_libs = { module = "com.android.tools:desugar_jdk_libs", version.ref = "desugar_jdk_libs" } diffx = { module = "org.pageseeder.diffx:pso-diffx", version.ref = "diffx" } espresso-core = { module = "androidx.test.espresso:espresso-core", version.ref = "espresso" } filemojicompat-core = { module = "de.c1710:filemojicompat", version.ref = "filemoji-compat" }