Only include span snapshots when process is not crashing (#1600)

## Goal We were including span snapshots for a session end caused by an app crash. How silly! ## Testing Add unit test and modify integration test to verify this
embrace-io · Oct 30, 2024 · 9b33dda · 9b33dda
2 parents b92c57a + 0163f2e
commit 9b33dda
Show file tree

Hide file tree

Showing 3 changed files with 22 additions and 7 deletions.
diff --git a/...otlin/io/embrace/android/embracesdk/internal/envelope/session/SessionPayloadSourceImpl.kt b/...otlin/io/embrace/android/embracesdk/internal/envelope/session/SessionPayloadSourceImpl.kt
@@ -25,9 +25,16 @@ internal class SessionPayloadSourceImpl(
     override fun getSessionPayload(endType: SessionSnapshotType, startNewSession: Boolean, crashId: String?): SessionPayload {
         val sharedLibSymbolMapping = captureDataSafely(logger, symbolMapProvider)
         val isCacheAttempt = endType == SessionSnapshotType.PERIODIC_CACHE
+        val includeSnapshots = endType != SessionSnapshotType.JVM_CRASH
+
+        // Snapshots should only be included if the process is expected to last beyond the current session
+        val snapshots: List<Span>? = if (includeSnapshots) {
+            retrieveSpanSnapshots(isCacheAttempt)
+        } else {
+            emptyList()
+        }
 
         // Ensure the span retrieving is last as that potentially ends the session span, which effectively ends the session
-        val snapshots: List<Span>? = retrieveSpanSnapshots(isCacheAttempt)
         val spans: List<Span>? = retrieveSpanData(isCacheAttempt, endType, startNewSession, crashId)
 
         return SessionPayload(

diff --git a/...a/io/embrace/android/embracesdk/internal/envelope/session/SessionPayloadSourceImplTest.kt b/...a/io/embrace/android/embracesdk/internal/envelope/session/SessionPayloadSourceImplTest.kt
@@ -13,6 +13,7 @@ import io.embrace.android.embracesdk.internal.spans.SpanSinkImpl
 import io.embrace.android.embracesdk.internal.spans.hasFixedAttribute
 import org.junit.Assert.assertEquals
 import org.junit.Assert.assertNotNull
+import org.junit.Assert.assertNull
 import org.junit.Before
 import org.junit.Test
 
@@ -55,28 +56,29 @@ internal class SessionPayloadSourceImplTest {
     @Test
     fun `session crash`() {
         val payload = impl.getSessionPayload(SessionSnapshotType.JVM_CRASH, false)
-        assertPayloadPopulated(payload = payload, hasSessionSnapshot = false)
+        assertPayloadPopulated(payload = payload, hasSessionSnapshot = false, hasNonSessionSnapshots = false)
         assertNotNull(payload.spans?.single())
     }
 
     @Test
     fun `session cache`() {
         val payload = impl.getSessionPayload(SessionSnapshotType.PERIODIC_CACHE, false)
-        assertPayloadPopulated(payload = payload, hasSessionSnapshot = true)
+        assertPayloadPopulated(payload = payload, hasSessionSnapshot = true, hasNonSessionSnapshots = true)
         val span = checkNotNull(payload.spans?.single())
         assertEquals("cache-span", span.name)
     }
 
     @Test
     fun `session lifecycle change`() {
         val payload = impl.getSessionPayload(SessionSnapshotType.NORMAL_END, true)
-        assertPayloadPopulated(payload = payload, hasSessionSnapshot = false)
+        assertPayloadPopulated(payload = payload, hasSessionSnapshot = false, hasNonSessionSnapshots = true)
         assertNotNull(payload.spans?.single())
     }
 
     private fun assertPayloadPopulated(
         payload: SessionPayload,
-        hasSessionSnapshot: Boolean
+        hasSessionSnapshot: Boolean,
+        hasNonSessionSnapshots: Boolean
     ) {
         assertEquals(mapOf("armeabi-v7a" to "my-symbols"), payload.sharedLibSymbolMapping)
         val snapshots = checkNotNull(payload.spanSnapshots)
@@ -86,6 +88,10 @@ internal class SessionPayloadSourceImplTest {
             assertEquals(0, snapshots.filter { it.hasFixedAttribute(EmbType.Ux.Session) }.size)
         }
 
-        assertNotNull(snapshots.single { !it.hasFixedAttribute(EmbType.Ux.Session) })
+        if (hasNonSessionSnapshots) {
+            assertNotNull(snapshots.single { !it.hasFixedAttribute(EmbType.Ux.Session) })
+        } else {
+            assertNull(snapshots.singleOrNull { !it.hasFixedAttribute(EmbType.Ux.Session) })
+        }
     }
 }
diff --git a/...rationTest/kotlin/io/embrace/android/embracesdk/testcases/features/JvmCrashFeatureTest.kt b/...rationTest/kotlin/io/embrace/android/embracesdk/testcases/features/JvmCrashFeatureTest.kt
@@ -1,7 +1,6 @@
 package io.embrace.android.embracesdk.testcases.features
 
 import androidx.test.ext.junit.runners.AndroidJUnit4
-import io.embrace.android.embracesdk.Embrace
 import io.embrace.android.embracesdk.internal.opentelemetry.embCrashId
 import io.embrace.android.embracesdk.internal.opentelemetry.embState
 import io.embrace.android.embracesdk.internal.payload.AppFramework
@@ -20,6 +19,7 @@ import io.embrace.android.embracesdk.testframework.assertions.assertOtelLogRecei
 import io.embrace.android.embracesdk.testframework.assertions.getLastLog
 import io.opentelemetry.api.logs.Severity
 import io.opentelemetry.semconv.incubating.LogIncubatingAttributes
+import org.junit.Assert.assertEquals
 import org.junit.Assert.assertFalse
 import org.junit.Assert.assertNotNull
 import org.junit.Rule
@@ -46,6 +46,7 @@ internal class JvmCrashFeatureTest {
             },
             assertAction = {
                 val session = getSingleSessionEnvelope()
+                assertEquals(0, session.data.spanSnapshots?.size)
                 getSingleLogEnvelope().getLastLog().assertCrash(
                     state = "foreground",
                     crashId = session.getCrashedId()
@@ -62,6 +63,7 @@ internal class JvmCrashFeatureTest {
             },
             assertAction = {
                 val ba = getSingleSessionEnvelope(ApplicationState.BACKGROUND)
+                assertEquals(0, ba.data.spanSnapshots?.size)
                 getSingleLogEnvelope().getLastLog().assertCrash(
                     crashId = ba.getCrashedId()
                 )