ttid span is now cancelled only on destroyed or when a new activity s…

…tarts, not anymore if auto-finish option is enabled
getsentry · Dec 13, 2022 · 04bbc33 · 04bbc33
1 parent da34861
commit 04bbc33
Show file tree

Hide file tree

Showing 3 changed files with 48 additions and 29 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -4,7 +4,7 @@
 
 ### Features
 
-- Add ttid span to ActivityLifecycleIntegration ([#2369](https://github.com/getsentry/sentry-java/pull/2369))
+- Add time-to-initial-display span to Activity transactions ([#2369](https://github.com/getsentry/sentry-java/pull/2369))
 
 ### Dependencies
 

diff --git a/sentry-android-core/src/main/java/io/sentry/android/core/ActivityLifecycleIntegration.java b/sentry-android-core/src/main/java/io/sentry/android/core/ActivityLifecycleIntegration.java
@@ -276,8 +276,7 @@ private boolean isRunningTransaction(final @NotNull Activity activity) {
   private void stopTracing(final @NotNull Activity activity, final boolean shouldFinishTracing) {
     if (performanceEnabled && shouldFinishTracing) {
       final ITransaction transaction = activitiesWithOngoingTransactions.get(activity);
-      final ISpan ttidSpan = ttidSpanMap.get(activity);
-      finishTransaction(transaction, ttidSpan);
+      finishTransaction(transaction, null);
     }
   }
 
@@ -291,9 +290,7 @@ private void finishTransaction(
       }
 
       // in case the ttidSpan isn't completed yet, we finish it as cancelled to avoid memory leak
-      if (ttidSpan != null && !ttidSpan.isFinished()) {
-        ttidSpan.finish(SpanStatus.CANCELLED);
-      }
+      finishSpan(ttidSpan, SpanStatus.CANCELLED);
 
       SpanStatus status = transaction.getStatus();
       // status might be set by other integrations, let's not overwrite it
@@ -365,24 +362,11 @@ public synchronized void onActivityResumed(final @NotNull Activity activity) {
     if (buildInfoProvider.getSdkInfoVersion() >= Build.VERSION_CODES.JELLY_BEAN
         && rootView != null) {
       FirstDrawDoneListener.registerForNextDraw(
-          rootView,
-          () -> {
-            // finishes ttidSpan span
-            if (ttidSpan != null && !ttidSpan.isFinished()) {
-              ttidSpan.finish();
-            }
-          },
-          buildInfoProvider);
+          rootView, () -> finishSpan(ttidSpan), buildInfoProvider);
     } else {
       // Posting a task to the main thread's handler will make it executed after it finished
       // its current job. That is, right after the activity draws the layout.
-      mainHandler.post(
-          () -> {
-            // finishes ttidSpan span
-            if (ttidSpan != null && !ttidSpan.isFinished()) {
-              ttidSpan.finish();
-            }
-          });
+      mainHandler.post(() -> finishSpan(ttidSpan));
     }
     addBreadcrumb(activity, "resumed");
 
@@ -436,17 +420,18 @@ public synchronized void onActivityDestroyed(final @NotNull Activity activity) {
 
     // in case the appStartSpan isn't completed yet, we finish it as cancelled to avoid
     // memory leak
-    if (appStartSpan != null && !appStartSpan.isFinished()) {
-      appStartSpan.finish(SpanStatus.CANCELLED);
-    }
+    finishSpan(appStartSpan, SpanStatus.CANCELLED);
+
+    // we finish the ttidSpan as cancelled in case it isn't completed yet
+    final ISpan ttidSpan = ttidSpanMap.get(activity);
+    finishSpan(ttidSpan, SpanStatus.CANCELLED);
 
     // in case people opt-out enableActivityLifecycleTracingAutoFinish and forgot to finish it,
     // we make sure to finish it when the activity gets destroyed.
     stopTracing(activity, true);
 
     // set it to null in case its been just finished as cancelled
     appStartSpan = null;
-    // ttidSpan is finished in the stopTracing() method
     ttidSpanMap.remove(activity);
 
     // clear it up, so we don't start again for the same activity if the activity is in the activity
@@ -457,6 +442,18 @@ public synchronized void onActivityDestroyed(final @NotNull Activity activity) {
     }
   }
 
+  private void finishSpan(@Nullable ISpan span) {
+    if (span != null && !span.isFinished()) {
+      span.finish();
+    }
+  }
+
+  private void finishSpan(@Nullable ISpan span, @NotNull SpanStatus status) {
+    if (span != null && !span.isFinished()) {
+      span.finish(status);
+    }
+  }
+
   @TestOnly
   @NotNull
   WeakHashMap<Activity, ITransaction> getActivitiesWithOngoingTransactions() {

diff --git a/sentry-android-core/src/test/java/io/sentry/android/core/ActivityLifecycleIntegrationTest.kt b/sentry-android-core/src/test/java/io/sentry/android/core/ActivityLifecycleIntegrationTest.kt
@@ -363,13 +363,14 @@ class ActivityLifecycleIntegrationTest {
     }
 
     @Test
-    fun `When tracing auto finish is enabled, it stops the transaction on onActivityPostResumed`() {
+    fun `When tracing auto finish is enabled and ttid span is finished, it stops the transaction on onActivityPostResumed`() {
         val sut = fixture.getSut()
         fixture.options.tracesSampleRate = 1.0
         sut.register(fixture.hub, fixture.options)
 
         val activity = mock<Activity>()
         sut.onActivityCreated(activity, fixture.bundle)
+        sut.ttidSpanMap.values.first().finish()
         sut.onActivityPostResumed(activity)
 
         verify(fixture.hub).captureTransaction(
@@ -381,6 +382,25 @@ class ActivityLifecycleIntegrationTest {
         )
     }
 
+    @Test
+    fun `When tracing auto finish is enabled, it doesn't stop the transaction on onActivityPostResumed`() {
+        val sut = fixture.getSut()
+        fixture.options.tracesSampleRate = 1.0
+        sut.register(fixture.hub, fixture.options)
+
+        val activity = mock<Activity>()
+        sut.onActivityCreated(activity, fixture.bundle)
+        sut.onActivityPostResumed(activity)
+
+        verify(fixture.hub, never()).captureTransaction(
+            check {
+                assertEquals(SpanStatus.OK, it.status)
+            },
+            anyOrNull<TraceContext>(),
+            anyOrNull()
+        )
+    }
+
     @Test
     fun `When tracing has status, do not overwrite it`() {
         val sut = fixture.getSut()
@@ -393,6 +413,7 @@ class ActivityLifecycleIntegrationTest {
         fixture.transaction.status = SpanStatus.UNKNOWN_ERROR
 
         sut.onActivityPostResumed(activity)
+        sut.onActivityDestroyed(activity)
 
         verify(fixture.hub).captureTransaction(
             check {
@@ -510,8 +531,8 @@ class ActivityLifecycleIntegrationTest {
         sut.onActivityCreated(activity, fixture.bundle)
         sut.onActivityDestroyed(activity)
 
-        val span = fixture.transaction.children.first { it.description?.endsWith(".ttid") == true }
-        assertEquals(span.status, SpanStatus.CANCELLED)
+        val span = fixture.transaction.children.first { it.operation == ActivityLifecycleIntegration.TTID_OP }
+        assertEquals(SpanStatus.CANCELLED, span.status)
         assertTrue(span.isFinished)
     }
 
@@ -571,13 +592,14 @@ class ActivityLifecycleIntegrationTest {
     }
 
     @Test
-    fun `stop transaction on resumed if API 29 less than 29`() {
+    fun `stop transaction on resumed if API 29 less than 29 and ttid is finished`() {
         val sut = fixture.getSut(14)
         fixture.options.tracesSampleRate = 1.0
         sut.register(fixture.hub, fixture.options)
 
         val activity = mock<Activity>()
         sut.onActivityCreated(activity, mock())
+        sut.ttidSpanMap.values.first().finish()
         sut.onActivityResumed(activity)
 
         verify(fixture.hub).captureTransaction(any(), anyOrNull<TraceContext>(), anyOrNull())