From 7027bd7ff015b873e3295c8f66287fc284ec1261 Mon Sep 17 00:00:00 2001 From: Roman Zavarnitsyn Date: Mon, 5 Aug 2024 11:05:44 +0200 Subject: [PATCH 1/4] Persist buffer replay type when switching to session --- .../android/replay/capture/BaseCaptureStrategy.kt | 14 +++++++------- .../replay/capture/BufferCaptureStrategy.kt | 10 ++++++---- .../android/replay/capture/CaptureStrategy.kt | 4 +++- .../replay/capture/SessionCaptureStrategy.kt | 6 ++++-- .../sentry/android/replay/ReplayIntegrationTest.kt | 12 ++++++++---- .../replay/capture/BufferCaptureStrategyTest.kt | 12 ++++++++++++ 6 files changed, 40 insertions(+), 18 deletions(-) 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 index 62bba00cd0..ce45ba9e81 100644 --- 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 @@ -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(propertyName = SEGMENT_KEY_REPLAY_TYPE) + override var replayType by persistableAtomic(propertyName = SEGMENT_KEY_REPLAY_TYPE) protected val currentEvents: LinkedList = 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/sentry-android-replay/src/main/java/io/sentry/android/replay/capture/BufferCaptureStrategy.kt b/sentry-android-replay/src/main/java/io/sentry/android/replay/capture/BufferCaptureStrategy.kt index a49c7bf789..a74e6f1603 100644 --- a/sentry-android-replay/src/main/java/io/sentry/android/replay/capture/BufferCaptureStrategy.kt +++ b/sentry-android-replay/src/main/java/io/sentry/android/replay/capture/BufferCaptureStrategy.kt @@ -8,6 +8,7 @@ import io.sentry.SentryLevel.DEBUG import io.sentry.SentryLevel.ERROR import io.sentry.SentryLevel.INFO import io.sentry.SentryOptions +import io.sentry.SentryReplayEvent.ReplayType import io.sentry.SentryReplayEvent.ReplayType.BUFFER import io.sentry.android.replay.ReplayCache import io.sentry.android.replay.ScreenshotRecorderConfig @@ -46,9 +47,10 @@ internal class BufferCaptureStrategy( override fun start( recorderConfig: ScreenshotRecorderConfig, segmentId: Int, - replayId: SentryId + replayId: SentryId, + replayType: ReplayType? ) { - super.start(recorderConfig, segmentId, replayId) + super.start(recorderConfig, segmentId, replayId, replayType) hub?.configureScope { val screen = it.screen?.substringAfterLast('.') @@ -159,7 +161,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 } @@ -250,7 +252,7 @@ internal class BufferCaptureStrategy( 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 index c3be520b84..e95ad8ce04 100644 --- 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,13 @@ internal interface CaptureStrategy { var currentSegment: Int var currentReplayId: SentryId val replayCacheDir: File? + var replayType: ReplayType fun start( recorderConfig: ScreenshotRecorderConfig, segmentId: Int = 0, - replayId: SentryId = SentryId() + replayId: SentryId = SentryId(), + replayType: ReplayType? = null ) fun stop() diff --git a/sentry-android-replay/src/main/java/io/sentry/android/replay/capture/SessionCaptureStrategy.kt b/sentry-android-replay/src/main/java/io/sentry/android/replay/capture/SessionCaptureStrategy.kt index d0fd2ce1e1..a8517b263d 100644 --- a/sentry-android-replay/src/main/java/io/sentry/android/replay/capture/SessionCaptureStrategy.kt +++ b/sentry-android-replay/src/main/java/io/sentry/android/replay/capture/SessionCaptureStrategy.kt @@ -7,6 +7,7 @@ import io.sentry.IHub import io.sentry.SentryLevel.DEBUG import io.sentry.SentryLevel.INFO import io.sentry.SentryOptions +import io.sentry.SentryReplayEvent.ReplayType import io.sentry.android.replay.ReplayCache import io.sentry.android.replay.ScreenshotRecorderConfig import io.sentry.android.replay.capture.CaptureStrategy.ReplaySegment @@ -31,9 +32,10 @@ internal class SessionCaptureStrategy( override fun start( recorderConfig: ScreenshotRecorderConfig, segmentId: Int, - replayId: SentryId + replayId: SentryId, + replayType: ReplayType? ) { - super.start(recorderConfig, segmentId, replayId) + super.start(recorderConfig, segmentId, replayId, replayType) // only set replayId on the scope if it's a full session, otherwise all events will be // tagged with the replay that might never be sent when we're recording in buffer mode hub?.configureScope { diff --git a/sentry-android-replay/src/test/java/io/sentry/android/replay/ReplayIntegrationTest.kt b/sentry-android-replay/src/test/java/io/sentry/android/replay/ReplayIntegrationTest.kt index a98e344277..03016e349a 100644 --- a/sentry-android-replay/src/test/java/io/sentry/android/replay/ReplayIntegrationTest.kt +++ b/sentry-android-replay/src/test/java/io/sentry/android/replay/ReplayIntegrationTest.kt @@ -41,6 +41,7 @@ import org.junit.runner.RunWith import org.mockito.ArgumentMatchers.anyInt import org.mockito.ArgumentMatchers.anyLong import org.mockito.kotlin.any +import org.mockito.kotlin.anyOrNull import org.mockito.kotlin.argThat import org.mockito.kotlin.check import org.mockito.kotlin.doAnswer @@ -160,7 +161,7 @@ class ReplayIntegrationTest { replay.start() - verify(captureStrategy, never()).start(any(), any(), any()) + verify(captureStrategy, never()).start(any(), any(), any(), anyOrNull()) } @Test @@ -186,7 +187,8 @@ class ReplayIntegrationTest { verify(captureStrategy, times(1)).start( any(), eq(0), - argThat { this != SentryId.EMPTY_ID } + argThat { this != SentryId.EMPTY_ID }, + anyOrNull() ) } @@ -201,7 +203,8 @@ class ReplayIntegrationTest { verify(captureStrategy, never()).start( any(), eq(0), - argThat { this != SentryId.EMPTY_ID } + argThat { this != SentryId.EMPTY_ID }, + anyOrNull() ) } @@ -216,7 +219,8 @@ class ReplayIntegrationTest { verify(captureStrategy, times(1)).start( any(), eq(0), - argThat { this != SentryId.EMPTY_ID } + argThat { this != SentryId.EMPTY_ID }, + anyOrNull() ) } diff --git a/sentry-android-replay/src/test/java/io/sentry/android/replay/capture/BufferCaptureStrategyTest.kt b/sentry-android-replay/src/test/java/io/sentry/android/replay/capture/BufferCaptureStrategyTest.kt index 5e5130aae8..54f0cd1483 100644 --- a/sentry-android-replay/src/test/java/io/sentry/android/replay/capture/BufferCaptureStrategyTest.kt +++ b/sentry-android-replay/src/test/java/io/sentry/android/replay/capture/BufferCaptureStrategyTest.kt @@ -241,6 +241,18 @@ class BufferCaptureStrategyTest { assertEquals(strategy.currentReplayId, fixture.scope.replayId) } + @Test + fun `convert persists buffer replayType when converting to session strategy`() { + val strategy = fixture.getSut() + strategy.start(fixture.recorderConfig) + + val converted = strategy.convert() + assertEquals( + ReplayType.BUFFER, + converted.replayType + ) + } + @Test fun `captureReplay does not replayId to scope when not sampled`() { val strategy = fixture.getSut(errorSampleRate = 0.0) From dd8f9f38b2b6d0c4c02da6653f443330dea605b3 Mon Sep 17 00:00:00 2001 From: Roman Zavarnitsyn Date: Mon, 5 Aug 2024 12:13:37 +0200 Subject: [PATCH 2/4] Ensure no gaps in segment timestamps when converting strategies --- .../io/sentry/android/replay/ReplayIntegration.kt | 3 ++- .../android/replay/capture/BaseCaptureStrategy.kt | 2 +- .../replay/capture/BufferCaptureStrategy.kt | 6 +++--- .../android/replay/capture/CaptureStrategy.kt | 5 ++--- .../replay/capture/SessionCaptureStrategy.kt | 8 ++++---- .../replay/capture/BufferCaptureStrategyTest.kt | 14 ++++++++++++++ 6 files changed, 26 insertions(+), 12 deletions(-) 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 index e99aec2c90..ce0f70e10b 100644 --- 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 @@ -176,8 +176,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() } 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 index ce45ba9e81..d888c2e33d 100644 --- 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(propertyName = SEGMENT_KEY_TIMESTAMP) { _, _, newValue -> + override var segmentTimestamp by persistableAtomicNullable(propertyName = SEGMENT_KEY_TIMESTAMP) { _, _, newValue -> cache?.persistSegmentValues(SEGMENT_KEY_TIMESTAMP, if (newValue == null) null else DateUtils.getTimestamp(newValue)) } protected val replayStartTimestamp = AtomicLong() diff --git a/sentry-android-replay/src/main/java/io/sentry/android/replay/capture/BufferCaptureStrategy.kt b/sentry-android-replay/src/main/java/io/sentry/android/replay/capture/BufferCaptureStrategy.kt index a74e6f1603..a70c35a5f6 100644 --- a/sentry-android-replay/src/main/java/io/sentry/android/replay/capture/BufferCaptureStrategy.kt +++ b/sentry-android-replay/src/main/java/io/sentry/android/replay/capture/BufferCaptureStrategy.kt @@ -21,6 +21,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( @@ -92,7 +93,7 @@ internal class BufferCaptureStrategy( override fun captureReplay( isTerminating: Boolean, - onSegmentSent: () -> Unit + onSegmentSent: (Date) -> Unit ) { val sampled = random.sample(options.experimental.sessionReplay.errorSampleRate) @@ -123,8 +124,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) } } } 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 index e95ad8ce04..7e9168df22 100644 --- 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 @@ -26,6 +26,7 @@ internal interface CaptureStrategy { var currentReplayId: SentryId val replayCacheDir: File? var replayType: ReplayType + var segmentTimestamp: Date? fun start( recorderConfig: ScreenshotRecorderConfig, @@ -40,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) @@ -196,7 +197,6 @@ internal interface CaptureStrategy { replay.urls = urls return ReplaySegment.Created( - videoDuration = videoDuration, replay = replay, recording = recording ) @@ -221,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() { diff --git a/sentry-android-replay/src/main/java/io/sentry/android/replay/capture/SessionCaptureStrategy.kt b/sentry-android-replay/src/main/java/io/sentry/android/replay/capture/SessionCaptureStrategy.kt index a8517b263d..b50ed98929 100644 --- a/sentry-android-replay/src/main/java/io/sentry/android/replay/capture/SessionCaptureStrategy.kt +++ b/sentry-android-replay/src/main/java/io/sentry/android/replay/capture/SessionCaptureStrategy.kt @@ -1,7 +1,6 @@ package io.sentry.android.replay.capture import android.graphics.Bitmap -import io.sentry.DateUtils import io.sentry.IConnectionStatusProvider.ConnectionStatus.DISCONNECTED import io.sentry.IHub import io.sentry.SentryLevel.DEBUG @@ -15,6 +14,7 @@ import io.sentry.android.replay.util.submitSafely import io.sentry.protocol.SentryId import io.sentry.transport.ICurrentDateProvider import io.sentry.util.FileUtils +import java.util.Date import java.util.concurrent.ScheduledExecutorService internal class SessionCaptureStrategy( @@ -67,7 +67,7 @@ internal class SessionCaptureStrategy( super.stop() } - override fun captureReplay(isTerminating: Boolean, onSegmentSent: () -> Unit) { + override fun captureReplay(isTerminating: Boolean, onSegmentSent: (Date) -> Unit) { options.logger.log(DEBUG, "Replay is already running in 'session' mode, not capturing for event") this.isTerminating.set(isTerminating) } @@ -112,7 +112,7 @@ internal class SessionCaptureStrategy( segment.capture(hub) currentSegment++ // set next segment timestamp as close to the previous one as possible to avoid gaps - segmentTimestamp = DateUtils.getDateTime(currentSegmentTimestamp.time + segment.videoDuration) + segmentTimestamp = segment.replay.timestamp } } @@ -131,7 +131,7 @@ internal class SessionCaptureStrategy( currentSegment++ // set next segment timestamp as close to the previous one as possible to avoid gaps - segmentTimestamp = DateUtils.getDateTime(currentSegmentTimestamp.time + segment.videoDuration) + segmentTimestamp = segment.replay.timestamp } } diff --git a/sentry-android-replay/src/test/java/io/sentry/android/replay/capture/BufferCaptureStrategyTest.kt b/sentry-android-replay/src/test/java/io/sentry/android/replay/capture/BufferCaptureStrategyTest.kt index 54f0cd1483..bbad02444d 100644 --- a/sentry-android-replay/src/test/java/io/sentry/android/replay/capture/BufferCaptureStrategyTest.kt +++ b/sentry-android-replay/src/test/java/io/sentry/android/replay/capture/BufferCaptureStrategyTest.kt @@ -16,6 +16,7 @@ import io.sentry.android.replay.ReplayCache.Companion.SEGMENT_KEY_REPLAY_TYPE import io.sentry.android.replay.ReplayCache.Companion.SEGMENT_KEY_TIMESTAMP import io.sentry.android.replay.ReplayFrame import io.sentry.android.replay.ScreenshotRecorderConfig +import io.sentry.android.replay.capture.BufferCaptureStrategyTest.Fixture.Companion.VIDEO_DURATION import io.sentry.protocol.SentryId import io.sentry.transport.CurrentDateProvider import io.sentry.transport.ICurrentDateProvider @@ -279,4 +280,17 @@ class BufferCaptureStrategyTest { assertEquals(strategy.currentReplayId, fixture.scope.replayId) assertTrue(called) } + + @Test + fun `captureReplay sets new segment timestamp to new strategy after successful creation`() { + val strategy = fixture.getSut() + strategy.start(fixture.recorderConfig) + val oldTimestamp = strategy.segmentTimestamp + + strategy.captureReplay(false) { newTimestamp -> + assertEquals(oldTimestamp!!.time + VIDEO_DURATION, newTimestamp.time) + } + + verify(fixture.hub).captureReplay(any(), any()) + } } From 0729e6d0afe6c55006237e8207f02d8b8f935876 Mon Sep 17 00:00:00 2001 From: Roman Zavarnitsyn Date: Mon, 5 Aug 2024 19:48:36 +0200 Subject: [PATCH 3/4] Properly store screen name at start for buffer mode --- .../api/sentry-android-replay.api | 5 +- .../io/sentry/android/replay/ReplayCache.kt | 18 ++++--- .../android/replay/ReplayIntegration.kt | 12 ++--- .../replay/capture/BufferCaptureStrategy.kt | 51 +------------------ .../replay/capture/SessionCaptureStrategy.kt | 2 +- .../sentry/android/replay/ReplayCacheTest.kt | 17 +++++++ .../android/replay/ReplayIntegrationTest.kt | 28 +++++++++- 7 files changed, 63 insertions(+), 70 deletions(-) diff --git a/sentry-android-replay/api/sentry-android-replay.api b/sentry-android-replay/api/sentry-android-replay.api index 2a81b45b82..caf0a3e37c 100644 --- 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 (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 index 21dbd6ec21..c1bfeb1e52 100644 --- 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 index ce0f70e10b..6e6d9ea052 100644 --- 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) @@ -213,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/BufferCaptureStrategy.kt b/sentry-android-replay/src/main/java/io/sentry/android/replay/capture/BufferCaptureStrategy.kt index a70c35a5f6..9acbbe6c11 100644 --- a/sentry-android-replay/src/main/java/io/sentry/android/replay/capture/BufferCaptureStrategy.kt +++ b/sentry-android-replay/src/main/java/io/sentry/android/replay/capture/BufferCaptureStrategy.kt @@ -8,7 +8,6 @@ import io.sentry.SentryLevel.DEBUG import io.sentry.SentryLevel.ERROR import io.sentry.SentryLevel.INFO import io.sentry.SentryOptions -import io.sentry.SentryReplayEvent.ReplayType import io.sentry.SentryReplayEvent.ReplayType.BUFFER import io.sentry.android.replay.ReplayCache import io.sentry.android.replay.ScreenshotRecorderConfig @@ -36,42 +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() - // TODO: rework this bs, it doesn't work with sending replay on restart - private val bufferedScreensLock = Any() - private val bufferedScreens = mutableListOf>() - 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, - replayType: ReplayType? - ) { - super.start(recorderConfig, segmentId, replayId, replayType) - - 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) { @@ -138,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) } } @@ -171,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 @@ -248,8 +201,6 @@ 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) diff --git a/sentry-android-replay/src/main/java/io/sentry/android/replay/capture/SessionCaptureStrategy.kt b/sentry-android-replay/src/main/java/io/sentry/android/replay/capture/SessionCaptureStrategy.kt index b50ed98929..61a98b6914 100644 --- a/sentry-android-replay/src/main/java/io/sentry/android/replay/capture/SessionCaptureStrategy.kt +++ b/sentry-android-replay/src/main/java/io/sentry/android/replay/capture/SessionCaptureStrategy.kt @@ -40,7 +40,7 @@ internal class SessionCaptureStrategy( // tagged with the replay that might never be sent when we're recording in buffer mode hub?.configureScope { it.replayId = currentReplayId - screenAtStart = it.screen + screenAtStart = it.screen?.substringAfterLast('.') } } diff --git a/sentry-android-replay/src/test/java/io/sentry/android/replay/ReplayCacheTest.kt b/sentry-android-replay/src/test/java/io/sentry/android/replay/ReplayCacheTest.kt index 99a308f53a..91a17f5192 100644 --- a/sentry-android-replay/src/test/java/io/sentry/android/replay/ReplayCacheTest.kt +++ b/sentry-android-replay/src/test/java/io/sentry/android/replay/ReplayCacheTest.kt @@ -266,6 +266,23 @@ class ReplayCacheTest { assertTrue(replayCache.replayCacheDir!!.listFiles()!!.none { it.name == "1.jpg" || it.name == "1001.jpg" }) } + @Test + fun `rotate returns first screen in buffer`() { + val replayCache = fixture.getSut( + tmpDir, + frameRate = 1 + ) + + val bitmap = Bitmap.createBitmap(1, 1, ARGB_8888) + replayCache.addFrame(bitmap, 1, "MainActivity") + replayCache.addFrame(bitmap, 1001, "SecondActivity") + replayCache.addFrame(bitmap, 2001, "ThirdActivity") + replayCache.addFrame(bitmap, 3001, "FourthActivity") + + val screen = replayCache.rotate(2000) + assertEquals("ThirdActivity", screen) + } + @Test fun `does not persist segment if already closed`() { val replayId = SentryId() diff --git a/sentry-android-replay/src/test/java/io/sentry/android/replay/ReplayIntegrationTest.kt b/sentry-android-replay/src/test/java/io/sentry/android/replay/ReplayIntegrationTest.kt index 03016e349a..e96375dfa6 100644 --- a/sentry-android-replay/src/test/java/io/sentry/android/replay/ReplayIntegrationTest.kt +++ b/sentry-android-replay/src/test/java/io/sentry/android/replay/ReplayIntegrationTest.kt @@ -10,6 +10,8 @@ import io.sentry.Breadcrumb import io.sentry.DateUtils import io.sentry.Hint import io.sentry.IHub +import io.sentry.Scope +import io.sentry.ScopeCallback import io.sentry.SentryEvent import io.sentry.SentryIntegrationPackageStorage import io.sentry.SentryOptions @@ -78,7 +80,12 @@ class ReplayIntegrationTest { }.whenever(mock).submit(any()) } } - val hub = mock() + val scope = Scope(options) + val hub = mock { + doAnswer { + ((it.arguments[0]) as ScopeCallback).run(scope) + }.whenever(mock).configureScope(any()) + } val replayCache = mock { on { frames }.thenReturn(mutableListOf(ReplayFrame(File("1720693523997.jpg"), 1720693523997))) @@ -149,7 +156,6 @@ class ReplayIntegrationTest { replay.register(fixture.hub, fixture.options) assertTrue(replay.isEnabled.get()) - assertEquals(1, fixture.options.scopeObservers.size) assertTrue(recorderCreated) assertTrue(SentryIntegrationPackageStorage.getInstance().integrations.contains("Replay")) } @@ -533,4 +539,22 @@ class ReplayIntegrationTest { assertTrue(scopeCache.exists()) assertFalse(evenOlderReplay.exists()) } + + @Test + fun `onScreenshotRecorded supplies screen from scope to replay cache`() { + val captureStrategy = mock { + doAnswer { + ((it.arguments[1] as ReplayCache.(frameTimestamp: Long) -> Unit)).invoke(fixture.replayCache, 1720693523997) + }.whenever(mock).onScreenshotRecorded(anyOrNull(), any()) + } + val replay = fixture.getSut(context, replayCaptureStrategyProvider = { captureStrategy }) + + fixture.hub.configureScope { it.screen = "MainActivity" } + replay.register(fixture.hub, fixture.options) + replay.start() + + replay.onScreenshotRecorded(mock()) + + verify(fixture.replayCache).addFrame(any(), any(), eq("MainActivity")) + } } From 76539893cf7d1f4e956d4f0b170181aed5ae7053 Mon Sep 17 00:00:00 2001 From: Roman Zavarnitsyn Date: Tue, 6 Aug 2024 10:20:39 +0200 Subject: [PATCH 4/4] Changelog --- CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 25cbbdffba..ca4cf89943 100644 --- 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