[SR] Buffer mode improvements (#3622)

* Persist buffer replay type when switching to session * Ensure no gaps in segment timestamps when converting strategies * Properly store screen name at start for buffer mode * Changelog
getsentry · Aug 9, 2024 · 9486895 · 9486895
1 parent d4b1f82
commit 9486895
Show file tree

Hide file tree

Showing 11 changed files with 129 additions and 96 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -6,6 +6,10 @@
 
 - Avoid ArrayIndexOutOfBoundsException on Android cpu data collection ([#3598](https://github.com/getsentry/sentry-java/pull/3598))
 - Fix lazy select queries instrumentation ([#3604](https://github.com/getsentry/sentry-java/pull/3604))
+- Session Replay: buffer mode improvements ([#3622](https://github.com/getsentry/sentry-java/pull/3622))
+  - Align next segment timestamp with the end of the buffered segment when converting from buffer mode to session mode
+  - Persist `buffer` replay type for the entire replay when converting from buffer mode to session mode
+  - Properly store screen names for `buffer` mode
 
 ### Chores
 

diff --git a/sentry-android-replay/api/sentry-android-replay.api b/sentry-android-replay/api/sentry-android-replay.api
@@ -36,12 +36,13 @@ public abstract interface class io/sentry/android/replay/Recorder : java/io/Clos
 public final class io/sentry/android/replay/ReplayCache : java/io/Closeable {
 	public static final field Companion Lio/sentry/android/replay/ReplayCache$Companion;
 	public fun <init> (Lio/sentry/SentryOptions;Lio/sentry/protocol/SentryId;Lio/sentry/android/replay/ScreenshotRecorderConfig;)V
-	public final fun addFrame (Ljava/io/File;J)V
+	public final fun addFrame (Ljava/io/File;JLjava/lang/String;)V
+	public static synthetic fun addFrame$default (Lio/sentry/android/replay/ReplayCache;Ljava/io/File;JLjava/lang/String;ILjava/lang/Object;)V
 	public fun close ()V
 	public final fun createVideoOf (JJIIILjava/io/File;)Lio/sentry/android/replay/GeneratedVideo;
 	public static synthetic fun createVideoOf$default (Lio/sentry/android/replay/ReplayCache;JJIIILjava/io/File;ILjava/lang/Object;)Lio/sentry/android/replay/GeneratedVideo;
 	public final fun persistSegmentValues (Ljava/lang/String;Ljava/lang/String;)V
-	public final fun rotate (J)V
+	public final fun rotate (J)Ljava/lang/String;
 }
 
 public final class io/sentry/android/replay/ReplayCache$Companion {

diff --git a/sentry-android-replay/src/main/java/io/sentry/android/replay/ReplayCache.kt b/sentry-android-replay/src/main/java/io/sentry/android/replay/ReplayCache.kt
@@ -76,7 +76,7 @@ public class ReplayCache(
      * @param bitmap the frame screenshot
      * @param frameTimestamp the timestamp when the frame screenshot was taken
      */
-    internal fun addFrame(bitmap: Bitmap, frameTimestamp: Long) {
+    internal fun addFrame(bitmap: Bitmap, frameTimestamp: Long, screen: String? = null) {
         if (replayCacheDir == null || bitmap.isRecycled) {
             return
         }
@@ -89,7 +89,7 @@ public class ReplayCache(
             it.flush()
         }
 
-        addFrame(screenshot, frameTimestamp)
+        addFrame(screenshot, frameTimestamp, screen)
     }
 
     /**
@@ -101,8 +101,8 @@ public class ReplayCache(
      * @param screenshot file containing the frame screenshot
      * @param frameTimestamp the timestamp when the frame screenshot was taken
      */
-    public fun addFrame(screenshot: File, frameTimestamp: Long) {
-        val frame = ReplayFrame(screenshot, frameTimestamp)
+    public fun addFrame(screenshot: File, frameTimestamp: Long, screen: String? = null) {
+        val frame = ReplayFrame(screenshot, frameTimestamp, screen)
         frames += frame
     }
 
@@ -233,15 +233,20 @@ public class ReplayCache(
      * Removes frames from the in-memory and disk cache from start to [until].
      *
      * @param until value until whose the frames should be removed, represented as unix timestamp
+     * @return the first screen in the rotated buffer, if any
      */
-    fun rotate(until: Long) {
+    fun rotate(until: Long): String? {
+        var screen: String? = null
         frames.removeAll {
             if (it.timestamp < until) {
                 deleteFile(it.screenshot)
                 return@removeAll true
+            } else if (screen == null) {
+                screen = it.screen
             }
             return@removeAll false
         }
+        return screen
     }
 
     override fun close() {
@@ -426,7 +431,8 @@ internal data class LastSegmentData(
 
 internal data class ReplayFrame(
     val screenshot: File,
-    val timestamp: Long
+    val timestamp: Long,
+    val screen: String? = null
 )
 
 public data class GeneratedVideo(

diff --git a/sentry-android-replay/src/main/java/io/sentry/android/replay/ReplayIntegration.kt b/sentry-android-replay/src/main/java/io/sentry/android/replay/ReplayIntegration.kt
@@ -12,7 +12,6 @@ import io.sentry.Integration
 import io.sentry.NoOpReplayBreadcrumbConverter
 import io.sentry.ReplayBreadcrumbConverter
 import io.sentry.ReplayController
-import io.sentry.ScopeObserverAdapter
 import io.sentry.SentryIntegrationPackageStorage
 import io.sentry.SentryLevel.DEBUG
 import io.sentry.SentryLevel.INFO
@@ -28,7 +27,6 @@ import io.sentry.cache.PersistingScopeObserver
 import io.sentry.cache.PersistingScopeObserver.BREADCRUMBS_FILENAME
 import io.sentry.cache.PersistingScopeObserver.REPLAY_FILENAME
 import io.sentry.hints.Backfillable
-import io.sentry.protocol.Contexts
 import io.sentry.protocol.SentryId
 import io.sentry.transport.ICurrentDateProvider
 import io.sentry.util.FileUtils
@@ -102,12 +100,6 @@ public class ReplayIntegration(
         }
 
         this.hub = hub
-        this.options.addScopeObserver(object : ScopeObserverAdapter() {
-            override fun setContexts(contexts: Contexts) {
-                // scope screen has fully-qualified name
-                captureStrategy?.onScreenChanged(contexts.app?.viewNames?.lastOrNull()?.substringAfterLast('.'))
-            }
-        })
         recorder = recorderProvider?.invoke() ?: WindowRecorder(options, this, this, mainLooperHandler)
         isEnabled.set(true)
 
@@ -176,8 +168,9 @@ public class ReplayIntegration(
             return
         }
 
-        captureStrategy?.captureReplay(isTerminating == true, onSegmentSent = {
+        captureStrategy?.captureReplay(isTerminating == true, onSegmentSent = { newTimestamp ->
             captureStrategy?.currentSegment = captureStrategy?.currentSegment!! + 1
+            captureStrategy?.segmentTimestamp = newTimestamp
         })
         captureStrategy = captureStrategy?.convert()
     }
@@ -212,8 +205,10 @@ public class ReplayIntegration(
     }
 
     override fun onScreenshotRecorded(bitmap: Bitmap) {
+        var screen: String? = null
+        hub?.configureScope { screen = it.screen?.substringAfterLast('.') }
         captureStrategy?.onScreenshotRecorded(bitmap) { frameTimeStamp ->
-            addFrame(bitmap, frameTimeStamp)
+            addFrame(bitmap, frameTimeStamp, screen)
         }
     }
 

diff --git a/sentry-android-replay/src/main/java/io/sentry/android/replay/capture/BaseCaptureStrategy.kt b/sentry-android-replay/src/main/java/io/sentry/android/replay/capture/BaseCaptureStrategy.kt
@@ -78,7 +78,7 @@ internal abstract class BaseCaptureStrategy(
         cache?.persistSegmentValues(SEGMENT_KEY_FRAME_RATE, newValue.frameRate.toString())
         cache?.persistSegmentValues(SEGMENT_KEY_BIT_RATE, newValue.bitRate.toString())
     }
-    protected var segmentTimestamp by persistableAtomicNullable<Date>(propertyName = SEGMENT_KEY_TIMESTAMP) { _, _, newValue ->
+    override var segmentTimestamp by persistableAtomicNullable<Date>(propertyName = SEGMENT_KEY_TIMESTAMP) { _, _, newValue ->
         cache?.persistSegmentValues(SEGMENT_KEY_TIMESTAMP, if (newValue == null) null else DateUtils.getTimestamp(newValue))
     }
     protected val replayStartTimestamp = AtomicLong()
@@ -87,7 +87,7 @@ internal abstract class BaseCaptureStrategy(
     override var currentSegment: Int by persistableAtomic(initialValue = -1, propertyName = SEGMENT_KEY_ID)
     override val replayCacheDir: File? get() = cache?.replayCacheDir
 
-    private var replayType by persistableAtomic<ReplayType>(propertyName = SEGMENT_KEY_REPLAY_TYPE)
+    override var replayType by persistableAtomic<ReplayType>(propertyName = SEGMENT_KEY_REPLAY_TYPE)
     protected val currentEvents: LinkedList<RRWebEvent> = PersistableLinkedList(
         propertyName = SEGMENT_KEY_REPLAY_RECORDING,
         options,
@@ -105,15 +105,15 @@ internal abstract class BaseCaptureStrategy(
     override fun start(
         recorderConfig: ScreenshotRecorderConfig,
         segmentId: Int,
-        replayId: SentryId
+        replayId: SentryId,
+        replayType: ReplayType?
     ) {
         cache = replayCacheProvider?.invoke(replayId, recorderConfig) ?: ReplayCache(options, replayId, recorderConfig)
 
-        // TODO: this should be persisted even after conversion
-        replayType = if (this is SessionCaptureStrategy) SESSION else BUFFER
+        this.replayType = replayType ?: (if (this is SessionCaptureStrategy) SESSION else BUFFER)
         this.recorderConfig = recorderConfig
-        currentSegment = segmentId
-        currentReplayId = replayId
+        this.currentSegment = segmentId
+        this.currentReplayId = replayId
 
         segmentTimestamp = DateUtils.getCurrentDateTime()
         replayStartTimestamp.set(dateProvider.currentTimeMillis)
@@ -140,7 +140,7 @@ internal abstract class BaseCaptureStrategy(
         segmentId: Int,
         height: Int,
         width: Int,
-        replayType: ReplayType = SESSION,
+        replayType: ReplayType = this.replayType,
         cache: ReplayCache? = this.cache,
         frameRate: Int = recorderConfig.frameRate,
         screenAtStart: String? = this.screenAtStart,

diff --git a/...ry-android-replay/src/main/java/io/sentry/android/replay/capture/BufferCaptureStrategy.kt b/...ry-android-replay/src/main/java/io/sentry/android/replay/capture/BufferCaptureStrategy.kt
@@ -20,6 +20,7 @@ import io.sentry.transport.ICurrentDateProvider
 import io.sentry.util.FileUtils
 import java.io.File
 import java.security.SecureRandom
+import java.util.Date
 import java.util.concurrent.ScheduledExecutorService
 
 internal class BufferCaptureStrategy(
@@ -34,41 +35,11 @@ internal class BufferCaptureStrategy(
     // TODO: capture envelopes for buffered segments instead, but don't send them until buffer is triggered
     private val bufferedSegments = mutableListOf<ReplaySegment.Created>()
 
-    // TODO: rework this bs, it doesn't work with sending replay on restart
-    private val bufferedScreensLock = Any()
-    private val bufferedScreens = mutableListOf<Pair<String, Long>>()
-
     internal companion object {
         private const val TAG = "BufferCaptureStrategy"
         private const val ENVELOPE_PROCESSING_DELAY: Long = 100L
     }
 
-    override fun start(
-        recorderConfig: ScreenshotRecorderConfig,
-        segmentId: Int,
-        replayId: SentryId
-    ) {
-        super.start(recorderConfig, segmentId, replayId)
-
-        hub?.configureScope {
-            val screen = it.screen?.substringAfterLast('.')
-            if (screen != null) {
-                synchronized(bufferedScreensLock) {
-                    bufferedScreens.add(screen to dateProvider.currentTimeMillis)
-                }
-            }
-        }
-    }
-
-    override fun onScreenChanged(screen: String?) {
-        synchronized(bufferedScreensLock) {
-            val lastKnownScreen = bufferedScreens.lastOrNull()?.first
-            if (screen != null && lastKnownScreen != screen) {
-                bufferedScreens.add(screen to dateProvider.currentTimeMillis)
-            }
-        }
-    }
-
     override fun pause() {
         createCurrentSegment("pause") { segment ->
             if (segment is ReplaySegment.Created) {
@@ -90,7 +61,7 @@ internal class BufferCaptureStrategy(
 
     override fun captureReplay(
         isTerminating: Boolean,
-        onSegmentSent: () -> Unit
+        onSegmentSent: (Date) -> Unit
     ) {
         val sampled = random.sample(options.experimental.sessionReplay.errorSampleRate)
 
@@ -121,8 +92,7 @@ internal class BufferCaptureStrategy(
                 // we only want to increment segment_id in the case of success, but currentSegment
                 // might be irrelevant since we changed strategies, so in the callback we increment
                 // it on the new strategy already
-                // TODO: also pass new segmentTimestamp to the new strategy
-                onSegmentSent()
+                onSegmentSent(segment.replay.timestamp)
             }
         }
     }
@@ -136,7 +106,7 @@ internal class BufferCaptureStrategy(
 
             val now = dateProvider.currentTimeMillis
             val bufferLimit = now - options.experimental.sessionReplay.errorReplayDuration
-            cache?.rotate(bufferLimit)
+            screenAtStart = cache?.rotate(bufferLimit)
             bufferedSegments.rotate(bufferLimit)
         }
     }
@@ -159,7 +129,7 @@ internal class BufferCaptureStrategy(
         }
         // we hand over replayExecutor to the new strategy to preserve order of execution
         val captureStrategy = SessionCaptureStrategy(options, hub, dateProvider, replayExecutor)
-        captureStrategy.start(recorderConfig, segmentId = currentSegment, replayId = currentReplayId)
+        captureStrategy.start(recorderConfig, segmentId = currentSegment, replayId = currentReplayId, replayType = BUFFER)
         return captureStrategy
     }
 
@@ -169,21 +139,6 @@ internal class BufferCaptureStrategy(
         rotateEvents(currentEvents, bufferLimit)
     }
 
-    private fun findAndSetStartScreen(segmentStart: Long) {
-        synchronized(bufferedScreensLock) {
-            val startScreen = bufferedScreens.lastOrNull { (_, timestamp) ->
-                timestamp <= segmentStart
-            }?.first
-            // if no screen is found before the segment start, this likely means the buffer is from the
-            // app start, and the start screen will be taken from the navigation crumbs
-            if (startScreen != null) {
-                screenAtStart = startScreen
-            }
-            // can clear as we switch to session mode and don't care anymore about buffering
-            bufferedScreens.clear()
-        }
-    }
-
     private fun deleteFile(file: File?) {
         if (file == null) {
             return
@@ -246,11 +201,9 @@ internal class BufferCaptureStrategy(
         val height = this.recorderConfig.recordingHeight
         val width = this.recorderConfig.recordingWidth
 
-        findAndSetStartScreen(currentSegmentTimestamp.time)
-
         replayExecutor.submitSafely(options, "$TAG.$taskName") {
             val segment =
-                createSegmentInternal(duration, currentSegmentTimestamp, replayId, segmentId, height, width, BUFFER)
+                createSegmentInternal(duration, currentSegmentTimestamp, replayId, segmentId, height, width)
             onSegmentCreated(segment)
         }
     }

diff --git a/sentry-android-replay/src/main/java/io/sentry/android/replay/capture/CaptureStrategy.kt b/sentry-android-replay/src/main/java/io/sentry/android/replay/capture/CaptureStrategy.kt
@@ -25,11 +25,14 @@ internal interface CaptureStrategy {
     var currentSegment: Int
     var currentReplayId: SentryId
     val replayCacheDir: File?
+    var replayType: ReplayType
+    var segmentTimestamp: Date?
 
     fun start(
         recorderConfig: ScreenshotRecorderConfig,
         segmentId: Int = 0,
-        replayId: SentryId = SentryId()
+        replayId: SentryId = SentryId(),
+        replayType: ReplayType? = null
     )
 
     fun stop()
@@ -38,7 +41,7 @@ internal interface CaptureStrategy {
 
     fun resume()
 
-    fun captureReplay(isTerminating: Boolean, onSegmentSent: () -> Unit)
+    fun captureReplay(isTerminating: Boolean, onSegmentSent: (Date) -> Unit)
 
     fun onScreenshotRecorded(bitmap: Bitmap? = null, store: ReplayCache.(frameTimestamp: Long) -> Unit)
 
@@ -194,7 +197,6 @@ internal interface CaptureStrategy {
 
             replay.urls = urls
             return ReplaySegment.Created(
-                videoDuration = videoDuration,
                 replay = replay,
                 recording = recording
             )
@@ -219,7 +221,6 @@ internal interface CaptureStrategy {
     sealed class ReplaySegment {
         object Failed : ReplaySegment()
         data class Created(
-            val videoDuration: Long,
             val replay: SentryReplayEvent,
             val recording: ReplayRecording
         ) : ReplaySegment() {