diff --git a/.craft.yml b/.craft.yml index 763f85252d5..7b791bc5671 100644 --- a/.craft.yml +++ b/.craft.yml @@ -41,3 +41,5 @@ targets: maven:io.sentry:sentry-apollo: maven:io.sentry:sentry-jdbc: maven:io.sentry:sentry-graphql: + maven:io.sentry:sentry-android-navigation: + maven:io.sentry:sentry-compose: diff --git a/build.gradle.kts b/build.gradle.kts index 401adc72928..b2d5d2a1e04 100644 --- a/build.gradle.kts +++ b/build.gradle.kts @@ -1,6 +1,7 @@ import com.diffplug.spotless.LineEnding import com.vanniktech.maven.publish.MavenPublishPlugin import com.vanniktech.maven.publish.MavenPublishPluginExtension +import io.gitlab.arturbosch.detekt.extensions.DetektExtension import org.gradle.api.tasks.testing.logging.TestExceptionFormat import org.gradle.api.tasks.testing.logging.TestLogEvent @@ -89,6 +90,14 @@ allprojects { } subprojects { + plugins.withId(Config.QualityPlugins.detektPlugin) { + configure { + buildUponDefaultConfig = true + allRules = true + config.setFrom("${rootProject.rootDir}/detekt.yml") + } + } + if (!this.name.contains("sample") && !this.name.contains("integration-tests") && this.name != "sentry-test-support") { apply() diff --git a/detekt.yml b/detekt.yml new file mode 100644 index 00000000000..49f91ae64fb --- /dev/null +++ b/detekt.yml @@ -0,0 +1,3 @@ +style: + ReturnCount: + max: 3 diff --git a/sentry-android-fragment/build.gradle.kts b/sentry-android-fragment/build.gradle.kts index a0bab5910ee..1cd48226c04 100644 --- a/sentry-android-fragment/build.gradle.kts +++ b/sentry-android-fragment/build.gradle.kts @@ -1,5 +1,4 @@ import io.gitlab.arturbosch.detekt.Detekt -import io.gitlab.arturbosch.detekt.extensions.DetektExtension plugins { id("com.android.library") @@ -79,8 +78,3 @@ tasks.withType { // Target version of the generated JVM bytecode. It is used for type resolution. jvmTarget = JavaVersion.VERSION_1_8.toString() } - -configure { - buildUponDefaultConfig = true - allRules = true -} diff --git a/sentry-android-integration-tests/sentry-uitest-android-benchmark/build.gradle.kts b/sentry-android-integration-tests/sentry-uitest-android-benchmark/build.gradle.kts index ed39396aa51..3393321f528 100644 --- a/sentry-android-integration-tests/sentry-uitest-android-benchmark/build.gradle.kts +++ b/sentry-android-integration-tests/sentry-uitest-android-benchmark/build.gradle.kts @@ -1,5 +1,4 @@ import io.gitlab.arturbosch.detekt.Detekt -import io.gitlab.arturbosch.detekt.extensions.DetektExtension import net.ltgt.gradle.errorprone.errorprone plugins { @@ -122,11 +121,6 @@ tasks.withType { jvmTarget = JavaVersion.VERSION_1_8.toString() } -configure { - buildUponDefaultConfig = true - allRules = true -} - kotlin { explicitApi() } diff --git a/sentry-android-integration-tests/sentry-uitest-android/build.gradle.kts b/sentry-android-integration-tests/sentry-uitest-android/build.gradle.kts index f1cea81b1c6..60b95b1c703 100644 --- a/sentry-android-integration-tests/sentry-uitest-android/build.gradle.kts +++ b/sentry-android-integration-tests/sentry-uitest-android/build.gradle.kts @@ -1,5 +1,4 @@ import io.gitlab.arturbosch.detekt.Detekt -import io.gitlab.arturbosch.detekt.extensions.DetektExtension import net.ltgt.gradle.errorprone.errorprone plugins { @@ -120,11 +119,6 @@ tasks.withType { jvmTarget = JavaVersion.VERSION_1_8.toString() } -configure { - buildUponDefaultConfig = true - allRules = true -} - kotlin { explicitApi() } diff --git a/sentry-android-navigation/api/sentry-android-navigation.api b/sentry-android-navigation/api/sentry-android-navigation.api index dbb4f9b544e..1ae144d5ddf 100644 --- a/sentry-android-navigation/api/sentry-android-navigation.api +++ b/sentry-android-navigation/api/sentry-android-navigation.api @@ -7,9 +7,16 @@ public final class io/sentry/android/navigation/BuildConfig { } public final class io/sentry/android/navigation/SentryNavigationListener : androidx/navigation/NavController$OnDestinationChangedListener { + public static final field Companion Lio/sentry/android/navigation/SentryNavigationListener$Companion; + public static final field NAVIGATION_OP Ljava/lang/String; public fun ()V public fun (Lio/sentry/IHub;)V - public synthetic fun (Lio/sentry/IHub;ILkotlin/jvm/internal/DefaultConstructorMarker;)V + public fun (Lio/sentry/IHub;Z)V + public fun (Lio/sentry/IHub;ZZ)V + public synthetic fun (Lio/sentry/IHub;ZZILkotlin/jvm/internal/DefaultConstructorMarker;)V public fun onDestinationChanged (Landroidx/navigation/NavController;Landroidx/navigation/NavDestination;Landroid/os/Bundle;)V } +public final class io/sentry/android/navigation/SentryNavigationListener$Companion { +} + diff --git a/sentry-android-navigation/build.gradle.kts b/sentry-android-navigation/build.gradle.kts index c30519b12bc..52501ef4188 100644 --- a/sentry-android-navigation/build.gradle.kts +++ b/sentry-android-navigation/build.gradle.kts @@ -1,5 +1,4 @@ import io.gitlab.arturbosch.detekt.Detekt -import io.gitlab.arturbosch.detekt.extensions.DetektExtension plugins { id("com.android.library") @@ -88,8 +87,3 @@ tasks.withType { // Target version of the generated JVM bytecode. It is used for type resolution. jvmTarget = JavaVersion.VERSION_1_8.toString() } - -configure { - buildUponDefaultConfig = true - allRules = true -} diff --git a/sentry-android-navigation/src/main/java/io/sentry/android/navigation/SentryNavigationListener.kt b/sentry-android-navigation/src/main/java/io/sentry/android/navigation/SentryNavigationListener.kt index 3e4d408858f..d2a82f7b43b 100644 --- a/sentry-android-navigation/src/main/java/io/sentry/android/navigation/SentryNavigationListener.kt +++ b/sentry-android-navigation/src/main/java/io/sentry/android/navigation/SentryNavigationListener.kt @@ -1,5 +1,6 @@ package io.sentry.android.navigation +import android.content.res.Resources.NotFoundException import android.os.Bundle import androidx.navigation.NavController import androidx.navigation.NavDestination @@ -7,58 +8,144 @@ import io.sentry.Breadcrumb import io.sentry.Hint import io.sentry.HubAdapter import io.sentry.IHub +import io.sentry.ITransaction +import io.sentry.SentryLevel.DEBUG import io.sentry.SentryLevel.INFO +import io.sentry.SpanStatus import io.sentry.TypeCheckHint import java.lang.ref.WeakReference class SentryNavigationListener @JvmOverloads constructor( - private val hub: IHub = HubAdapter.getInstance() + private val hub: IHub = HubAdapter.getInstance(), + private val enableNavigationBreadcrumbs: Boolean = true, + private val enableNavigationTracing: Boolean = true ) : NavController.OnDestinationChangedListener { private var previousDestinationRef: WeakReference? = null private var previousArgs: Bundle? = null + private val isPerformanceEnabled get() = hub.options.isTracingEnabled && enableNavigationTracing + + private var activeTransaction: ITransaction? = null + override fun onDestinationChanged( controller: NavController, destination: NavDestination, arguments: Bundle? ) { - addBreadcrumb(destination, arguments) + val toArguments = arguments.refined() + + addBreadcrumb(destination, toArguments) + startTracing(controller, destination, toArguments) previousDestinationRef = WeakReference(destination) previousArgs = arguments } - private fun addBreadcrumb(destination: NavDestination, arguments: Bundle?) { + private fun addBreadcrumb(destination: NavDestination, arguments: Map) { + if (!enableNavigationBreadcrumbs) { + return + } val breadcrumb = Breadcrumb().apply { - type = "navigation" - category = "navigation" + type = NAVIGATION_OP + category = NAVIGATION_OP val from = previousDestinationRef?.get()?.route - from?.let { data["from"] = it } - previousArgs?.let { args -> - val fromArguments = args.keySet().filter { - it != NavController.KEY_DEEP_LINK_INTENT // there's a lot of unrelated stuff - }.associateWith { args[it] } - if (fromArguments.isNotEmpty()) { - data["from_arguments"] = fromArguments - } + from?.let { data["from"] = "/$it" } + val fromArguments = previousArgs.refined() + if (fromArguments.isNotEmpty()) { + data["from_arguments"] = fromArguments } val to = destination.route - to?.let { data["to"] = it } - arguments?.let { args -> - val toArguments = args.keySet().filter { - it != NavController.KEY_DEEP_LINK_INTENT // there's a lot of unrelated stuff - }.associateWith { args[it] } - if (toArguments.isNotEmpty()) { - data["to_arguments"] = toArguments - } + to?.let { data["to"] = "/$it" } + if (arguments.isNotEmpty()) { + data["to_arguments"] = arguments } level = INFO } val hint = Hint() hint.set(TypeCheckHint.ANDROID_NAV_DESTINATION, destination) - hub.addBreadcrumb(breadcrumb) + hub.addBreadcrumb(breadcrumb, hint) + } + + private fun startTracing( + controller: NavController, + destination: NavDestination, + arguments: Map + ) { + if (!isPerformanceEnabled) { + return + } + + // we can only have one nav transaction at a time + if (activeTransaction != null) { + stopTracing() + } + + if (destination.navigatorName == "activity") { + // we do not trace navigation between activities to avoid clashing with activity lifecycle tracing + hub.options.logger.log( + DEBUG, + "Navigating to activity destination, no transaction captured." + ) + return + } + + @Suppress("SwallowedException") // we swallow it on purpose + var name = destination.route ?: try { + controller.context.resources.getResourceEntryName(destination.id) + } catch (e: NotFoundException) { + hub.options.logger.log( + DEBUG, + "Destination id cannot be retrieved from Resources, no transaction captured." + ) + return + } + + // we add '/' to the name to match dart and web pattern + name = "/" + name.substringBefore('/') // strip out arguments from the tx name + + val transaction = + hub.startTransaction(name, NAVIGATION_OP, true, hub.options.idleTimeout, true) + + if (arguments.isNotEmpty()) { + transaction.setData("arguments", arguments) + } + hub.configureScope { scope -> + scope.withTransaction { tx -> + if (tx == null) { + scope.transaction = transaction + } + } + } + activeTransaction = transaction + } + + private fun stopTracing() { + val status = activeTransaction?.status ?: SpanStatus.OK + activeTransaction?.finish(status) + + // clear transaction from scope so others can bind to it + hub.configureScope { scope -> + scope.withTransaction { tx -> + if (tx == activeTransaction) { + scope.clearTransaction() + } + } + } + + activeTransaction = null + } + + private fun Bundle?.refined(): Map = + this?.let { args -> + args.keySet().filter { + it != NavController.KEY_DEEP_LINK_INTENT // there's a lot of unrelated stuff + }.associateWith { args[it] } + } ?: emptyMap() + + companion object { + const val NAVIGATION_OP = "navigation" } } diff --git a/sentry-android-navigation/src/test/java/io/sentry/android/navigation/SentryNavigationListenerTest.kt b/sentry-android-navigation/src/test/java/io/sentry/android/navigation/SentryNavigationListenerTest.kt index 8654c7ba4cd..72d9a298c7a 100644 --- a/sentry-android-navigation/src/test/java/io/sentry/android/navigation/SentryNavigationListenerTest.kt +++ b/sentry-android-navigation/src/test/java/io/sentry/android/navigation/SentryNavigationListenerTest.kt @@ -1,17 +1,29 @@ package io.sentry.android.navigation +import android.content.Context +import android.content.res.Resources import androidx.core.os.bundleOf import androidx.navigation.NavController import androidx.navigation.NavDestination import androidx.test.ext.junit.runners.AndroidJUnit4 +import com.nhaarman.mockitokotlin2.any +import com.nhaarman.mockitokotlin2.anyOrNull +import com.nhaarman.mockitokotlin2.argumentCaptor import com.nhaarman.mockitokotlin2.check import com.nhaarman.mockitokotlin2.mock -import com.nhaarman.mockitokotlin2.reset +import com.nhaarman.mockitokotlin2.never +import com.nhaarman.mockitokotlin2.times import com.nhaarman.mockitokotlin2.verify import com.nhaarman.mockitokotlin2.whenever import io.sentry.Breadcrumb import io.sentry.IHub +import io.sentry.Scope +import io.sentry.Scope.IWithTransaction +import io.sentry.ScopeCallback import io.sentry.SentryLevel +import io.sentry.SentryOptions +import io.sentry.SentryTracer +import io.sentry.TransactionContext import org.junit.runner.RunWith import org.robolectric.annotation.Config import kotlin.test.Test @@ -27,9 +39,55 @@ class SentryNavigationListenerTest { val destination = mock() val navController = mock() - fun getSut(toRoute: String = "route"): SentryNavigationListener { + val context = mock() + val resources = mock() + val scope = mock() + + lateinit var transaction: SentryTracer + + @Suppress("LongParameterList") + fun getSut( + toRoute: String? = "route", + toId: String? = "destination-id-1", + enableBreadcrumbs: Boolean = true, + enableTracing: Boolean = true, + tracesSampleRate: Double? = 1.0, + hasViewIdInRes: Boolean = true, + transaction: SentryTracer = SentryTracer( + TransactionContext( + "/$toRoute", + SentryNavigationListener.NAVIGATION_OP + ), + hub + ) + ): SentryNavigationListener { + this.transaction = transaction + + whenever(hub.startTransaction(any(), any(), any(), anyOrNull(), any())) + .thenReturn(transaction) + whenever(hub.options).thenReturn( + SentryOptions().apply { + setTracesSampleRate( + tracesSampleRate + ) + } + ) + whenever(hub.configureScope(any())).thenAnswer { + (it.arguments[0] as ScopeCallback).run(scope) + } + + whenever(destination.id).thenReturn(1) + if (hasViewIdInRes) { + whenever(resources.getResourceEntryName(1)).thenReturn(toId) + } else { + whenever(resources.getResourceEntryName(destination.id)).thenThrow( + Resources.NotFoundException() + ) + } + whenever(context.resources).thenReturn(resources) + whenever(navController.context).thenReturn(context) whenever(destination.route).thenReturn(toRoute) - return SentryNavigationListener(hub) + return SentryNavigationListener(hub, enableBreadcrumbs, enableTracing) } } @@ -45,9 +103,10 @@ class SentryNavigationListenerTest { check { assertEquals("navigation", it.type) assertEquals("navigation", it.category) - assertEquals("route", it.data["to"]) + assertEquals("/route", it.data["to"]) assertEquals(SentryLevel.INFO, it.level) - } + }, + any() ) } @@ -63,9 +122,10 @@ class SentryNavigationListenerTest { verify(fixture.hub).addBreadcrumb( check { - assertEquals("route", it.data["to"]) + assertEquals("/route", it.data["to"]) assertEquals(mapOf("arg1" to "foo", "arg2" to "bar"), it.data["to_arguments"]) - } + }, + any() ) } @@ -81,9 +141,10 @@ class SentryNavigationListenerTest { verify(fixture.hub).addBreadcrumb( check { - assertEquals("route", it.data["to"]) + assertEquals("/route", it.data["to"]) assertNull(it.data["to_arguments"]) - } + }, + any() ) } @@ -96,7 +157,6 @@ class SentryNavigationListenerTest { fixture.destination, bundleOf("from_arg1" to "from_foo") ) - reset(fixture.hub) val toDestination = mock { whenever(mock.route).thenReturn("route_to") @@ -106,14 +166,181 @@ class SentryNavigationListenerTest { toDestination, bundleOf("to_arg1" to "to_foo") ) - verify(fixture.hub).addBreadcrumb( - check { - assertEquals("route_from", it.data["from"]) - assertEquals(mapOf("from_arg1" to "from_foo"), it.data["from_arguments"]) + val captor = argumentCaptor() + verify(fixture.hub, times(2)).addBreadcrumb(captor.capture(), any()) + captor.secondValue.let { + assertEquals("/route_from", it.data["from"]) + assertEquals(mapOf("from_arg1" to "from_foo"), it.data["from_arguments"]) - assertEquals("route_to", it.data["to"]) - assertEquals(mapOf("to_arg1" to "to_foo"), it.data["to_arguments"]) - } + assertEquals("/route_to", it.data["to"]) + assertEquals(mapOf("to_arg1" to "to_foo"), it.data["to_arguments"]) + } + } + + @Test + fun `onDestinationChanged does not capture a breadcrumb when breadcrumbs are disabled`() { + val sut = fixture.getSut(enableBreadcrumbs = false) + + sut.onDestinationChanged(fixture.navController, fixture.destination, null) + + verify(fixture.hub, never()).addBreadcrumb(any()) + } + + @Test + fun `onDestinationChanged does not start tracing when tracing is disabled`() { + val sut = fixture.getSut(enableTracing = false) + + sut.onDestinationChanged(fixture.navController, fixture.destination, null) + + verify(fixture.hub, never()).startTransaction( + any(), + any(), + any(), + anyOrNull(), + any() + ) + } + + @Test + fun `onDestinationChanged does not start tracing when tracesSampleRate is not set`() { + val sut = fixture.getSut(enableTracing = true, tracesSampleRate = null) + + sut.onDestinationChanged(fixture.navController, fixture.destination, null) + + verify(fixture.hub, never()).startTransaction( + any(), + any(), + any(), + anyOrNull(), + any() + ) + } + + @Test + fun `onDestinationChanged does not start tracing when navigating between activities`() { + val sut = fixture.getSut() + whenever(fixture.destination.navigatorName).thenReturn("activity") + + sut.onDestinationChanged(fixture.navController, fixture.destination, null) + + verify(fixture.hub, never()).startTransaction( + any(), + any(), + any(), + anyOrNull(), + any() + ) + } + + @Test + fun `onDestinationChanged does not start tracing when route and id are not available`() { + val sut = fixture.getSut(toRoute = null, hasViewIdInRes = false) + + sut.onDestinationChanged(fixture.navController, fixture.destination, null) + + verify(fixture.hub, never()).startTransaction( + any(), + any(), + any(), + anyOrNull(), + any() + ) + } + + @Test + fun `onDestinationChanged starts tracing with the route name as transaction name`() { + val sut = fixture.getSut() + + sut.onDestinationChanged(fixture.navController, fixture.destination, null) + + verify(fixture.hub).startTransaction( + check { assertEquals("/route", it) }, + check { assertEquals(SentryNavigationListener.NAVIGATION_OP, it) }, + any(), anyOrNull(), any() + ) + } + + @Test + fun `onDestinationChanged strips out route parameters from transaction name`() { + val sut = fixture.getSut(toRoute = "github/{user_id}?per_page={per_page}") + + sut.onDestinationChanged(fixture.navController, fixture.destination, null) + + verify(fixture.hub).startTransaction( + check { assertEquals("/github", it) }, + any(), any(), anyOrNull(), any() + ) + } + + @Test + fun `onDestinationChanged starts tracing with destination id if route is not available`() { + val sut = fixture.getSut(toRoute = null, hasViewIdInRes = true) + + sut.onDestinationChanged(fixture.navController, fixture.destination, null) + + verify(fixture.hub).startTransaction( + check { assertEquals("/destination-id-1", it) }, + any(), any(), anyOrNull(), any() + ) + } + + @Test + fun `onDestinationChanged captures arguments as additional data for transaction`() { + val sut = fixture.getSut(toRoute = "github/{user_id}?per_page={per_page}") + + sut.onDestinationChanged( + fixture.navController, + fixture.destination, + bundleOf("user_id" to 123, "per_page" to 10) ) + + verify(fixture.hub).startTransaction( + check { assertEquals("/github", it) }, + any(), any(), anyOrNull(), any() + ) + + val capturedArgs = fixture.transaction.data!!["arguments"] + require(capturedArgs is Map<*, *>) + assertEquals(123, capturedArgs["user_id"]) + assertEquals(10, capturedArgs["per_page"]) + } + + @Test + fun `onDestinationChanged binds transaction to the Scope`() { + val sut = fixture.getSut() + + sut.onDestinationChanged(fixture.navController, fixture.destination, null) + + val captor = argumentCaptor() + verify(fixture.scope).withTransaction(captor.capture()) + captor.firstValue.accept(null) + verify(fixture.scope).transaction = fixture.transaction + } + + @Test + fun `onDestinationChanged does not replace existing transaction on the Scope`() { + val sut = fixture.getSut() + + sut.onDestinationChanged(fixture.navController, fixture.destination, null) + + val captor = argumentCaptor() + verify(fixture.scope).withTransaction(captor.capture()) + captor.firstValue.accept(mock()) + verify(fixture.scope, never()).transaction = fixture.transaction + } + + @Test + fun `onDestinationChanged finishes previous navigation transaction before starting a new one`() { + val sut = fixture.getSut() + + sut.onDestinationChanged(fixture.navController, fixture.destination, null) + sut.onDestinationChanged(fixture.navController, fixture.destination, null) + + assertEquals(true, fixture.transaction.isFinished) + val captor = argumentCaptor() + verify(fixture.scope, times(4)).withTransaction(captor.capture()) + // 1st time - bind to scope, 2nd time - in SentryTracer when finish, 3rd time - in the nav listener + captor.thirdValue.accept(fixture.transaction) + verify(fixture.scope).clearTransaction() } } diff --git a/sentry-android-okhttp/build.gradle.kts b/sentry-android-okhttp/build.gradle.kts index 53243225084..65a29cce96c 100644 --- a/sentry-android-okhttp/build.gradle.kts +++ b/sentry-android-okhttp/build.gradle.kts @@ -1,5 +1,4 @@ import io.gitlab.arturbosch.detekt.Detekt -import io.gitlab.arturbosch.detekt.extensions.DetektExtension import org.jetbrains.kotlin.config.KotlinCompilerVersion plugins { @@ -86,8 +85,3 @@ tasks.withType { // Target version of the generated JVM bytecode. It is used for type resolution. jvmTarget = JavaVersion.VERSION_1_8.toString() } - -configure { - buildUponDefaultConfig = true - allRules = true -} diff --git a/sentry-android-timber/build.gradle.kts b/sentry-android-timber/build.gradle.kts index 16649360d2b..5d193aecb43 100644 --- a/sentry-android-timber/build.gradle.kts +++ b/sentry-android-timber/build.gradle.kts @@ -1,5 +1,4 @@ import io.gitlab.arturbosch.detekt.Detekt -import io.gitlab.arturbosch.detekt.extensions.DetektExtension import org.jetbrains.kotlin.config.KotlinCompilerVersion plugins { @@ -87,8 +86,3 @@ tasks.withType { // Target version of the generated JVM bytecode. It is used for type resolution. jvmTarget = JavaVersion.VERSION_1_8.toString() } - -configure { - buildUponDefaultConfig = true - allRules = true -} diff --git a/sentry-compose/build.gradle.kts b/sentry-compose/build.gradle.kts index 2be16fceca2..baf80736669 100644 --- a/sentry-compose/build.gradle.kts +++ b/sentry-compose/build.gradle.kts @@ -1,5 +1,4 @@ import io.gitlab.arturbosch.detekt.Detekt -import io.gitlab.arturbosch.detekt.extensions.DetektExtension plugins { kotlin("multiplatform") @@ -114,8 +113,3 @@ tasks.withType { // Target version of the generated JVM bytecode. It is used for type resolution. jvmTarget = JavaVersion.VERSION_1_8.toString() } - -configure { - buildUponDefaultConfig = true - allRules = true -} diff --git a/sentry-kotlin-extensions/build.gradle.kts b/sentry-kotlin-extensions/build.gradle.kts index 27bd5273ed9..fe10716085e 100644 --- a/sentry-kotlin-extensions/build.gradle.kts +++ b/sentry-kotlin-extensions/build.gradle.kts @@ -1,5 +1,4 @@ import io.gitlab.arturbosch.detekt.Detekt -import io.gitlab.arturbosch.detekt.extensions.DetektExtension import org.jetbrains.kotlin.gradle.tasks.KotlinCompile plugins { @@ -71,11 +70,6 @@ tasks.withType { jvmTarget = JavaVersion.VERSION_1_8.toString() } -configure { - buildUponDefaultConfig = true - allRules = true -} - kotlin { explicitApi() }