From dc210b4a40a3f07bd4d3245c36f44f8875b3050b Mon Sep 17 00:00:00 2001 From: Roman Zavarnitsyn Date: Wed, 6 Nov 2024 23:21:23 +0100 Subject: [PATCH 01/11] Do not capture screenshots in session mode when rate limit is active --- .../replay/capture/SessionCaptureStrategy.kt | 8 +++++++ .../capture/SessionCaptureStrategyTest.kt | 19 +++++++++++++++ .../clientreport/ClientReportRecorder.java | 3 +++ .../java/io/sentry/transport/RateLimiter.java | 2 ++ .../sentry/clientreport/ClientReportTest.kt | 8 +++++-- .../io/sentry/transport/RateLimiterTest.kt | 23 +++++++++++++++++++ 6 files changed, 61 insertions(+), 2 deletions(-) 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 7b416d18b7d..3865d99909c 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,6 +1,8 @@ package io.sentry.android.replay.capture import android.graphics.Bitmap +import io.sentry.DataCategory.All +import io.sentry.DataCategory.Replay import io.sentry.IConnectionStatusProvider.ConnectionStatus.DISCONNECTED import io.sentry.IHub import io.sentry.SentryLevel.DEBUG @@ -78,6 +80,12 @@ internal class SessionCaptureStrategy( bitmap?.recycle() return } + val rateLimiter = hub?.rateLimiter + if (rateLimiter?.isActiveForCategory(All) == true || rateLimiter?.isActiveForCategory(Replay) == true) { + options.logger.log(DEBUG, "Skipping screenshot recording, rate limit is active") + bitmap?.recycle() + return + } // have to do it before submitting, otherwise if the queue is busy, the timestamp won't be // reflecting the exact time of when it was captured val frameTimestamp = dateProvider.currentTimeMillis diff --git a/sentry-android-replay/src/test/java/io/sentry/android/replay/capture/SessionCaptureStrategyTest.kt b/sentry-android-replay/src/test/java/io/sentry/android/replay/capture/SessionCaptureStrategyTest.kt index 12eb10c3f4b..c7b4a9f8d58 100644 --- a/sentry-android-replay/src/test/java/io/sentry/android/replay/capture/SessionCaptureStrategyTest.kt +++ b/sentry-android-replay/src/test/java/io/sentry/android/replay/capture/SessionCaptureStrategyTest.kt @@ -2,6 +2,7 @@ package io.sentry.android.replay.capture import android.graphics.Bitmap import io.sentry.Breadcrumb +import io.sentry.DataCategory import io.sentry.DateUtils import io.sentry.IHub import io.sentry.Scope @@ -27,6 +28,7 @@ import io.sentry.rrweb.RRWebBreadcrumbEvent import io.sentry.rrweb.RRWebMetaEvent import io.sentry.transport.CurrentDateProvider import io.sentry.transport.ICurrentDateProvider +import io.sentry.transport.RateLimiter import org.junit.Rule import org.junit.rules.TemporaryFolder import org.mockito.ArgumentMatchers.anyInt @@ -36,6 +38,8 @@ import org.mockito.kotlin.anyOrNull import org.mockito.kotlin.argThat import org.mockito.kotlin.check import org.mockito.kotlin.doAnswer +import org.mockito.kotlin.doReturn +import org.mockito.kotlin.eq import org.mockito.kotlin.mock import org.mockito.kotlin.never import org.mockito.kotlin.verify @@ -69,6 +73,7 @@ class SessionCaptureStrategyTest { doAnswer { (it.arguments[0] as ScopeCallback).run(scope) }.whenever(it).configureScope(any()) + doReturn(mock()).whenever(it).rateLimiter } var persistedSegment = LinkedHashMap() val replayCache = mock { @@ -367,4 +372,18 @@ class SessionCaptureStrategyTest { "the current replay cache folder is not being deleted." ) } + + @Test + fun `when rate limited does not capture screenshots`() { + val strategy = fixture.getSut() + + whenever(fixture.hub.rateLimiter?.isActiveForCategory(eq(DataCategory.Replay))).thenReturn(true) + + strategy.start(fixture.recorderConfig) + var called = false + strategy.onScreenshotRecorded(mock()) { + called = true + } + assertFalse(called) + } } diff --git a/sentry/src/main/java/io/sentry/clientreport/ClientReportRecorder.java b/sentry/src/main/java/io/sentry/clientreport/ClientReportRecorder.java index 796a17cb3c4..a88df2824fd 100644 --- a/sentry/src/main/java/io/sentry/clientreport/ClientReportRecorder.java +++ b/sentry/src/main/java/io/sentry/clientreport/ClientReportRecorder.java @@ -174,6 +174,9 @@ private DataCategory categoryFromItemType(SentryItemType itemType) { if (SentryItemType.CheckIn.equals(itemType)) { return DataCategory.Monitor; } + if (SentryItemType.ReplayVideo.equals(itemType)) { + return DataCategory.Replay; + } return DataCategory.Default; } diff --git a/sentry/src/main/java/io/sentry/transport/RateLimiter.java b/sentry/src/main/java/io/sentry/transport/RateLimiter.java index c4598820abf..f45313a497e 100644 --- a/sentry/src/main/java/io/sentry/transport/RateLimiter.java +++ b/sentry/src/main/java/io/sentry/transport/RateLimiter.java @@ -177,6 +177,8 @@ private boolean isRetryAfter(final @NotNull String itemType) { return DataCategory.Transaction; case "check_in": return DataCategory.Monitor; + case "replay_video": + return DataCategory.Replay; default: return DataCategory.Unknown; } diff --git a/sentry/src/test/java/io/sentry/clientreport/ClientReportTest.kt b/sentry/src/test/java/io/sentry/clientreport/ClientReportTest.kt index c06e8da1f6f..135cce944be 100644 --- a/sentry/src/test/java/io/sentry/clientreport/ClientReportTest.kt +++ b/sentry/src/test/java/io/sentry/clientreport/ClientReportTest.kt @@ -10,12 +10,14 @@ import io.sentry.Hint import io.sentry.IHub import io.sentry.NoOpLogger import io.sentry.ProfilingTraceData +import io.sentry.ReplayRecording import io.sentry.Sentry import io.sentry.SentryEnvelope import io.sentry.SentryEnvelopeHeader import io.sentry.SentryEnvelopeItem import io.sentry.SentryEvent import io.sentry.SentryOptions +import io.sentry.SentryReplayEvent import io.sentry.SentryTracer import io.sentry.Session import io.sentry.TracesSamplingDecision @@ -71,13 +73,14 @@ class ClientReportTest { SentryEnvelopeItem.fromAttachment(opts.serializer, NoOpLogger.getInstance(), Attachment("{ \"number\": 10 }".toByteArray(), "log.json"), 1000), SentryEnvelopeItem.fromProfilingTrace(ProfilingTraceData(File(""), transaction), 1000, opts.serializer), SentryEnvelopeItem.fromCheckIn(opts.serializer, CheckIn("monitor-slug-1", CheckInStatus.ERROR)), - SentryEnvelopeItem.fromMetrics(EncodedMetrics(emptyMap())) + SentryEnvelopeItem.fromMetrics(EncodedMetrics(emptyMap())), + SentryEnvelopeItem.fromReplay(opts.serializer, opts.logger, SentryReplayEvent(), ReplayRecording(), false) ) clientReportRecorder.recordLostEnvelope(DiscardReason.NETWORK_ERROR, envelope) val clientReportAtEnd = clientReportRecorder.resetCountsAndGenerateClientReport() - testHelper.assertTotalCount(15, clientReportAtEnd) + testHelper.assertTotalCount(16, clientReportAtEnd) testHelper.assertCountFor(DiscardReason.SAMPLE_RATE, DataCategory.Error, 3, clientReportAtEnd) testHelper.assertCountFor(DiscardReason.BEFORE_SEND, DataCategory.Error, 2, clientReportAtEnd) testHelper.assertCountFor(DiscardReason.QUEUE_OVERFLOW, DataCategory.Transaction, 1, clientReportAtEnd) @@ -90,6 +93,7 @@ class ClientReportTest { testHelper.assertCountFor(DiscardReason.NETWORK_ERROR, DataCategory.Profile, 1, clientReportAtEnd) testHelper.assertCountFor(DiscardReason.NETWORK_ERROR, DataCategory.Monitor, 1, clientReportAtEnd) testHelper.assertCountFor(DiscardReason.NETWORK_ERROR, DataCategory.MetricBucket, 1, clientReportAtEnd) + testHelper.assertCountFor(DiscardReason.NETWORK_ERROR, DataCategory.Replay, 1, clientReportAtEnd) } @Test diff --git a/sentry/src/test/java/io/sentry/transport/RateLimiterTest.kt b/sentry/src/test/java/io/sentry/transport/RateLimiterTest.kt index 8346ddf1023..79c80c40412 100644 --- a/sentry/src/test/java/io/sentry/transport/RateLimiterTest.kt +++ b/sentry/src/test/java/io/sentry/transport/RateLimiterTest.kt @@ -5,15 +5,18 @@ import io.sentry.CheckIn import io.sentry.CheckInStatus import io.sentry.Hint import io.sentry.IHub +import io.sentry.ILogger import io.sentry.ISerializer import io.sentry.NoOpLogger import io.sentry.ProfilingTraceData +import io.sentry.ReplayRecording import io.sentry.SentryEnvelope import io.sentry.SentryEnvelopeHeader import io.sentry.SentryEnvelopeItem import io.sentry.SentryEvent import io.sentry.SentryOptions import io.sentry.SentryOptionsManipulator +import io.sentry.SentryReplayEvent import io.sentry.SentryTracer import io.sentry.Session import io.sentry.TransactionContext @@ -354,4 +357,24 @@ class RateLimiterTest { assertTrue(rateLimiter.isAnyRateLimitActive) } + + @Test + fun `drop replay items as lost`() { + val rateLimiter = fixture.getSUT() + val hub = mock() + whenever(hub.options).thenReturn(SentryOptions()) + + val replayItem = SentryEnvelopeItem.fromReplay(fixture.serializer, mock(), SentryReplayEvent(), ReplayRecording(), false) + val attachmentItem = SentryEnvelopeItem.fromAttachment(fixture.serializer, NoOpLogger.getInstance(), Attachment("{ \"number\": 10 }".toByteArray(), "log.json"), 1000) + val envelope = SentryEnvelope(SentryEnvelopeHeader(), arrayListOf(replayItem, attachmentItem)) + + rateLimiter.updateRetryAfterLimits("60:replay:key", null, 1) + val result = rateLimiter.filter(envelope, Hint()) + + assertNotNull(result) + assertEquals(1, result.items.toList().size) + + verify(fixture.clientReportRecorder, times(1)).recordLostEnvelopeItem(eq(DiscardReason.RATELIMIT_BACKOFF), same(replayItem)) + verifyNoMoreInteractions(fixture.clientReportRecorder) + } } From b67c33c5aef084465d04c084a3090883b53ce309 Mon Sep 17 00:00:00 2001 From: Roman Zavarnitsyn Date: Wed, 6 Nov 2024 23:47:47 +0100 Subject: [PATCH 02/11] Changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index da49a05efd6..029e210a0da 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,7 @@ - Using MaxBreadcrumb with value 0 no longer crashes. ([#3836](https://github.com/getsentry/sentry-java/pull/3836)) - Accept manifest integer values when requiring floating values ([#3823](https://github.com/getsentry/sentry-java/pull/3823)) +- Session Replay: Disable replay in session mode when rate limit is active ([#3854](https://github.com/getsentry/sentry-java/pull/3854)) ## 7.16.0 From 3ff77f956281f92cb33aa86c4e19e85d9302a216 Mon Sep 17 00:00:00 2001 From: Roman Zavarnitsyn Date: Tue, 12 Nov 2024 00:47:42 +0100 Subject: [PATCH 03/11] WIP --- .../java/io/sentry/android/replay/Recorder.kt | 2 ++ .../android/replay/ReplayIntegration.kt | 7 ++++++- .../android/replay/ScreenshotRecorder.kt | 21 +++++++++++++++---- .../sentry/android/replay/WindowRecorder.kt | 10 +++++++-- .../replay/capture/SessionCaptureStrategy.kt | 11 ---------- .../capture/SessionCaptureStrategyTest.kt | 14 ------------- 6 files changed, 33 insertions(+), 32 deletions(-) diff --git a/sentry-android-replay/src/main/java/io/sentry/android/replay/Recorder.kt b/sentry-android-replay/src/main/java/io/sentry/android/replay/Recorder.kt index 6cf86b6a7e6..7bbc38c1d32 100644 --- a/sentry-android-replay/src/main/java/io/sentry/android/replay/Recorder.kt +++ b/sentry-android-replay/src/main/java/io/sentry/android/replay/Recorder.kt @@ -15,4 +15,6 @@ interface Recorder : Closeable { fun pause() fun stop() + + fun setReplayStrategy(isSession: Boolean) = Unit } 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 90832585cd8..c5778baf0d1 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 @@ -109,7 +109,7 @@ public class ReplayIntegration( } this.hub = hub - recorder = recorderProvider?.invoke() ?: WindowRecorder(options, this, mainLooperHandler) + recorder = recorderProvider?.invoke() ?: WindowRecorder(options, this, mainLooperHandler, hub.rateLimiter) gestureRecorder = gestureRecorderProvider?.invoke() ?: GestureRecorder(options, this) isEnabled.set(true) @@ -157,6 +157,10 @@ public class ReplayIntegration( captureStrategy?.start(recorderConfig) recorder?.start(recorderConfig) + // TODO: move this here + // TODO: add 2 listeners here: OnRateLimit(category, applied) and OnConnectionChanged(connected) + // TODO: when they are fired, call either recorder?.resume() + strategy.resume() or recorder?.pause() + strategy.pause() + recorder?.setReplayStrategy(captureStrategy is SessionCaptureStrategy) registerRootViewListeners() } @@ -184,6 +188,7 @@ public class ReplayIntegration( captureStrategy?.segmentTimestamp = newTimestamp }) captureStrategy = captureStrategy?.convert() + recorder?.setReplayStrategy(captureStrategy is SessionCaptureStrategy) } override fun getReplayId(): SentryId = captureStrategy?.currentReplayId ?: SentryId.EMPTY_ID diff --git a/sentry-android-replay/src/main/java/io/sentry/android/replay/ScreenshotRecorder.kt b/sentry-android-replay/src/main/java/io/sentry/android/replay/ScreenshotRecorder.kt index 54f92a89587..764dafbf7c8 100644 --- a/sentry-android-replay/src/main/java/io/sentry/android/replay/ScreenshotRecorder.kt +++ b/sentry-android-replay/src/main/java/io/sentry/android/replay/ScreenshotRecorder.kt @@ -17,6 +17,9 @@ import android.view.PixelCopy import android.view.View import android.view.ViewTreeObserver import android.view.WindowManager +import io.sentry.DataCategory.All +import io.sentry.DataCategory.Replay +import io.sentry.IConnectionStatusProvider.ConnectionStatus.DISCONNECTED import io.sentry.SentryLevel.DEBUG import io.sentry.SentryLevel.INFO import io.sentry.SentryLevel.WARNING @@ -30,12 +33,12 @@ import io.sentry.android.replay.util.traverse import io.sentry.android.replay.viewhierarchy.ViewHierarchyNode import io.sentry.android.replay.viewhierarchy.ViewHierarchyNode.ImageViewHierarchyNode import io.sentry.android.replay.viewhierarchy.ViewHierarchyNode.TextViewHierarchyNode +import io.sentry.transport.RateLimiter import java.io.File import java.lang.ref.WeakReference import java.util.concurrent.Executors import java.util.concurrent.ThreadFactory import java.util.concurrent.atomic.AtomicBoolean -import java.util.concurrent.atomic.AtomicReference import kotlin.LazyThreadSafetyMode.NONE import kotlin.math.roundToInt @@ -44,14 +47,14 @@ internal class ScreenshotRecorder( val config: ScreenshotRecorderConfig, val options: SentryOptions, val mainLooperHandler: MainLooperHandler, - private val screenshotRecorderCallback: ScreenshotRecorderCallback? + private val screenshotRecorderCallback: ScreenshotRecorderCallback?, + private val rateLimiter: RateLimiter? ) : ViewTreeObserver.OnDrawListener { private val recorder by lazy { Executors.newSingleThreadScheduledExecutor(RecorderExecutorServiceThreadFactory()) } private var rootView: WeakReference? = null - private val pendingViewHierarchy = AtomicReference() private val maskingPaint by lazy(NONE) { Paint() } private val singlePixelBitmap: Bitmap by lazy(NONE) { Bitmap.createBitmap( @@ -69,6 +72,7 @@ internal class ScreenshotRecorder( private val contentChanged = AtomicBoolean(false) private val isCapturing = AtomicBoolean(true) private var lastScreenshot: Bitmap? = null + internal val isSession = AtomicBoolean(false) fun capture() { if (!isCapturing.get()) { @@ -76,6 +80,16 @@ internal class ScreenshotRecorder( return } + if (isSession.get() && options.connectionStatusProvider.connectionStatus == DISCONNECTED) { + options.logger.log(DEBUG, "Skipping screenshot recording, no internet connection") + return + } + + if (isSession.get() && (rateLimiter?.isActiveForCategory(All) == true || rateLimiter?.isActiveForCategory(Replay) == true)) { + options.logger.log(DEBUG, "Skipping screenshot recording, rate limit is active") + return + } + if (!contentChanged.get() && lastScreenshot != null && !lastScreenshot!!.isRecycled) { options.logger.log(DEBUG, "Content hasn't changed, repeating last known frame") @@ -230,7 +244,6 @@ internal class ScreenshotRecorder( unbind(rootView?.get()) rootView?.clear() lastScreenshot?.recycle() - pendingViewHierarchy.set(null) isCapturing.set(false) recorder.gracefullyShutdown(options) } diff --git a/sentry-android-replay/src/main/java/io/sentry/android/replay/WindowRecorder.kt b/sentry-android-replay/src/main/java/io/sentry/android/replay/WindowRecorder.kt index 9e846dfcf08..c3b7a81a93c 100644 --- a/sentry-android-replay/src/main/java/io/sentry/android/replay/WindowRecorder.kt +++ b/sentry-android-replay/src/main/java/io/sentry/android/replay/WindowRecorder.kt @@ -6,6 +6,7 @@ import io.sentry.SentryOptions import io.sentry.android.replay.util.MainLooperHandler import io.sentry.android.replay.util.gracefullyShutdown import io.sentry.android.replay.util.scheduleAtFixedRateSafely +import io.sentry.transport.RateLimiter import java.lang.ref.WeakReference import java.util.concurrent.Executors import java.util.concurrent.ScheduledFuture @@ -17,7 +18,8 @@ import java.util.concurrent.atomic.AtomicBoolean internal class WindowRecorder( private val options: SentryOptions, private val screenshotRecorderCallback: ScreenshotRecorderCallback? = null, - private val mainLooperHandler: MainLooperHandler + private val mainLooperHandler: MainLooperHandler, + private val rateLimiter: RateLimiter? ) : Recorder, OnRootViewsChangedListener { internal companion object { @@ -52,7 +54,7 @@ internal class WindowRecorder( return } - recorder = ScreenshotRecorder(recorderConfig, options, mainLooperHandler, screenshotRecorderCallback) + recorder = ScreenshotRecorder(recorderConfig, options, mainLooperHandler, screenshotRecorderCallback, rateLimiter) capturingTask = capturer.scheduleAtFixedRateSafely( options, "$TAG.capture", @@ -86,6 +88,10 @@ internal class WindowRecorder( capturer.gracefullyShutdown(options) } + override fun setReplayStrategy(isSession: Boolean) { + recorder?.isSession?.set(isSession) + } + private class RecorderExecutorServiceThreadFactory : ThreadFactory { private var cnt = 0 override fun newThread(r: Runnable): Thread { 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 3865d99909c..281576feec0 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 @@ -75,17 +75,6 @@ internal class SessionCaptureStrategy( } override fun onScreenshotRecorded(bitmap: Bitmap?, store: ReplayCache.(frameTimestamp: Long) -> Unit) { - if (options.connectionStatusProvider.connectionStatus == DISCONNECTED) { - options.logger.log(DEBUG, "Skipping screenshot recording, no internet connection") - bitmap?.recycle() - return - } - val rateLimiter = hub?.rateLimiter - if (rateLimiter?.isActiveForCategory(All) == true || rateLimiter?.isActiveForCategory(Replay) == true) { - options.logger.log(DEBUG, "Skipping screenshot recording, rate limit is active") - bitmap?.recycle() - return - } // have to do it before submitting, otherwise if the queue is busy, the timestamp won't be // reflecting the exact time of when it was captured val frameTimestamp = dateProvider.currentTimeMillis diff --git a/sentry-android-replay/src/test/java/io/sentry/android/replay/capture/SessionCaptureStrategyTest.kt b/sentry-android-replay/src/test/java/io/sentry/android/replay/capture/SessionCaptureStrategyTest.kt index c7b4a9f8d58..cd7767e1c16 100644 --- a/sentry-android-replay/src/test/java/io/sentry/android/replay/capture/SessionCaptureStrategyTest.kt +++ b/sentry-android-replay/src/test/java/io/sentry/android/replay/capture/SessionCaptureStrategyTest.kt @@ -372,18 +372,4 @@ class SessionCaptureStrategyTest { "the current replay cache folder is not being deleted." ) } - - @Test - fun `when rate limited does not capture screenshots`() { - val strategy = fixture.getSut() - - whenever(fixture.hub.rateLimiter?.isActiveForCategory(eq(DataCategory.Replay))).thenReturn(true) - - strategy.start(fixture.recorderConfig) - var called = false - strategy.onScreenshotRecorded(mock()) { - called = true - } - assertFalse(called) - } } From 2be170b088cb24fa56f1590db1dbb812d6e60ff1 Mon Sep 17 00:00:00 2001 From: Sentry Github Bot Date: Mon, 11 Nov 2024 23:52:16 +0000 Subject: [PATCH 04/11] Format code --- .../io/sentry/android/replay/capture/SessionCaptureStrategy.kt | 3 --- .../android/replay/capture/SessionCaptureStrategyTest.kt | 2 -- 2 files changed, 5 deletions(-) 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 281576feec0..08aec7e9f7a 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,9 +1,6 @@ package io.sentry.android.replay.capture import android.graphics.Bitmap -import io.sentry.DataCategory.All -import io.sentry.DataCategory.Replay -import io.sentry.IConnectionStatusProvider.ConnectionStatus.DISCONNECTED import io.sentry.IHub import io.sentry.SentryLevel.DEBUG import io.sentry.SentryLevel.INFO diff --git a/sentry-android-replay/src/test/java/io/sentry/android/replay/capture/SessionCaptureStrategyTest.kt b/sentry-android-replay/src/test/java/io/sentry/android/replay/capture/SessionCaptureStrategyTest.kt index cd7767e1c16..6ae1dac3fec 100644 --- a/sentry-android-replay/src/test/java/io/sentry/android/replay/capture/SessionCaptureStrategyTest.kt +++ b/sentry-android-replay/src/test/java/io/sentry/android/replay/capture/SessionCaptureStrategyTest.kt @@ -2,7 +2,6 @@ package io.sentry.android.replay.capture import android.graphics.Bitmap import io.sentry.Breadcrumb -import io.sentry.DataCategory import io.sentry.DateUtils import io.sentry.IHub import io.sentry.Scope @@ -39,7 +38,6 @@ import org.mockito.kotlin.argThat import org.mockito.kotlin.check import org.mockito.kotlin.doAnswer import org.mockito.kotlin.doReturn -import org.mockito.kotlin.eq import org.mockito.kotlin.mock import org.mockito.kotlin.never import org.mockito.kotlin.verify From 4f8bb4ab9d857faa057f97da303aa0bfee8b9344 Mon Sep 17 00:00:00 2001 From: Roman Zavarnitsyn Date: Tue, 12 Nov 2024 16:33:53 +0100 Subject: [PATCH 05/11] Change approach to rate-limit and offline --- .../java/io/sentry/android/replay/Recorder.kt | 2 - .../android/replay/ReplayIntegration.kt | 68 ++++++++++++++++-- .../android/replay/ScreenshotRecorder.kt | 12 ---- .../sentry/android/replay/WindowRecorder.kt | 7 +- .../android/replay/capture/CaptureStrategy.kt | 6 +- .../sentry/transport/AsyncHttpTransport.java | 1 + .../java/io/sentry/transport/RateLimiter.java | 70 ++++++++++++++++++- 7 files changed, 137 insertions(+), 29 deletions(-) diff --git a/sentry-android-replay/src/main/java/io/sentry/android/replay/Recorder.kt b/sentry-android-replay/src/main/java/io/sentry/android/replay/Recorder.kt index 7bbc38c1d32..6cf86b6a7e6 100644 --- a/sentry-android-replay/src/main/java/io/sentry/android/replay/Recorder.kt +++ b/sentry-android-replay/src/main/java/io/sentry/android/replay/Recorder.kt @@ -15,6 +15,4 @@ interface Recorder : Closeable { fun pause() fun stop() - - fun setReplayStrategy(isSession: Boolean) = Unit } 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 c5778baf0d1..0f92cce5532 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 @@ -7,6 +7,12 @@ import android.graphics.Bitmap import android.os.Build import android.view.MotionEvent import io.sentry.Breadcrumb +import io.sentry.DataCategory +import io.sentry.DataCategory.All +import io.sentry.DataCategory.Replay +import io.sentry.IConnectionStatusProvider.ConnectionStatus +import io.sentry.IConnectionStatusProvider.ConnectionStatus.DISCONNECTED +import io.sentry.IConnectionStatusProvider.IConnectionStatusObserver import io.sentry.IHub import io.sentry.Integration import io.sentry.NoOpReplayBreadcrumbConverter @@ -32,6 +38,7 @@ import io.sentry.cache.PersistingScopeObserver.REPLAY_FILENAME import io.sentry.hints.Backfillable import io.sentry.protocol.SentryId import io.sentry.transport.ICurrentDateProvider +import io.sentry.transport.RateLimiter.IRateLimitObserver import io.sentry.util.FileUtils import io.sentry.util.HintUtils import io.sentry.util.IntegrationUtils.addIntegrationToSdkVersion @@ -48,7 +55,14 @@ public class ReplayIntegration( private val recorderProvider: (() -> Recorder)? = null, private val recorderConfigProvider: ((configChanged: Boolean) -> ScreenshotRecorderConfig)? = null, private val replayCacheProvider: ((replayId: SentryId, recorderConfig: ScreenshotRecorderConfig) -> ReplayCache)? = null -) : Integration, Closeable, ScreenshotRecorderCallback, TouchRecorderCallback, ReplayController, ComponentCallbacks { +) : Integration, + Closeable, + ScreenshotRecorderCallback, + TouchRecorderCallback, + ReplayController, + ComponentCallbacks, + IConnectionStatusObserver, + IRateLimitObserver { // needed for the Java's call site constructor(context: Context, dateProvider: ICurrentDateProvider) : this( @@ -109,10 +123,12 @@ public class ReplayIntegration( } this.hub = hub - recorder = recorderProvider?.invoke() ?: WindowRecorder(options, this, mainLooperHandler, hub.rateLimiter) + recorder = recorderProvider?.invoke() ?: WindowRecorder(options, this, mainLooperHandler) gestureRecorder = gestureRecorderProvider?.invoke() ?: GestureRecorder(options, this) isEnabled.set(true) + options.connectionStatusProvider.addConnectionStatusObserver(this) + hub.rateLimiter?.addRateLimitObserver(this) try { context.registerComponentCallbacks(this) } catch (e: Throwable) { @@ -157,10 +173,6 @@ public class ReplayIntegration( captureStrategy?.start(recorderConfig) recorder?.start(recorderConfig) - // TODO: move this here - // TODO: add 2 listeners here: OnRateLimit(category, applied) and OnConnectionChanged(connected) - // TODO: when they are fired, call either recorder?.resume() + strategy.resume() or recorder?.pause() + strategy.pause() - recorder?.setReplayStrategy(captureStrategy is SessionCaptureStrategy) registerRootViewListeners() } @@ -188,7 +200,6 @@ public class ReplayIntegration( captureStrategy?.segmentTimestamp = newTimestamp }) captureStrategy = captureStrategy?.convert() - recorder?.setReplayStrategy(captureStrategy is SessionCaptureStrategy) } override fun getReplayId(): SentryId = captureStrategy?.currentReplayId ?: SentryId.EMPTY_ID @@ -227,12 +238,14 @@ public class ReplayIntegration( hub?.configureScope { screen = it.screen?.substringAfterLast('.') } captureStrategy?.onScreenshotRecorded(bitmap) { frameTimeStamp -> addFrame(bitmap, frameTimeStamp, screen) + checkCanRecord() } } override fun onScreenshotRecorded(screenshot: File, frameTimestamp: Long) { captureStrategy?.onScreenshotRecorded { _ -> addFrame(screenshot, frameTimestamp) + checkCanRecord() } } @@ -241,6 +254,8 @@ public class ReplayIntegration( return } + options.connectionStatusProvider.removeConnectionStatusObserver(this) + hub?.rateLimiter?.removeRateLimitObserver(this) try { context.unregisterComponentCallbacks(this) } catch (ignored: Throwable) { @@ -264,12 +279,51 @@ public class ReplayIntegration( recorder?.start(recorderConfig) } + override fun onConnectionStatusChanged(status: ConnectionStatus) { + if (captureStrategy !is SessionCaptureStrategy) { + // we only want to stop recording when offline for session mode + return + } + + if (status == DISCONNECTED) { + pause() + } else { + // being positive for other states, even if it's NO_PERMISSION + resume() + } + } + + override fun onRateLimitChanged(applied: Boolean) { + if (captureStrategy !is SessionCaptureStrategy) { + // we only want to stop recording when rate-limited for session mode + return + } + + if (hub?.rateLimiter?.isActiveForCategories(All, Replay) == true) { + pause() + } else { + resume() + } + } + override fun onLowMemory() = Unit override fun onTouchEvent(event: MotionEvent) { captureStrategy?.onTouchEvent(event) } + /** + * Check if we're offline or rate-limited and pause for session mode to not overflow the + * envelope cache. + */ + private fun checkCanRecord() { + if (captureStrategy is SessionCaptureStrategy && + (options.connectionStatusProvider.connectionStatus == DISCONNECTED || + hub?.rateLimiter?.isActiveForCategories(All, Replay) == true)) { + pause() + } + } + private fun registerRootViewListeners() { if (recorder is OnRootViewsChangedListener) { rootViewsSpy.listeners += (recorder as OnRootViewsChangedListener) diff --git a/sentry-android-replay/src/main/java/io/sentry/android/replay/ScreenshotRecorder.kt b/sentry-android-replay/src/main/java/io/sentry/android/replay/ScreenshotRecorder.kt index 764dafbf7c8..fa7617770ac 100644 --- a/sentry-android-replay/src/main/java/io/sentry/android/replay/ScreenshotRecorder.kt +++ b/sentry-android-replay/src/main/java/io/sentry/android/replay/ScreenshotRecorder.kt @@ -48,7 +48,6 @@ internal class ScreenshotRecorder( val options: SentryOptions, val mainLooperHandler: MainLooperHandler, private val screenshotRecorderCallback: ScreenshotRecorderCallback?, - private val rateLimiter: RateLimiter? ) : ViewTreeObserver.OnDrawListener { private val recorder by lazy { @@ -72,7 +71,6 @@ internal class ScreenshotRecorder( private val contentChanged = AtomicBoolean(false) private val isCapturing = AtomicBoolean(true) private var lastScreenshot: Bitmap? = null - internal val isSession = AtomicBoolean(false) fun capture() { if (!isCapturing.get()) { @@ -80,16 +78,6 @@ internal class ScreenshotRecorder( return } - if (isSession.get() && options.connectionStatusProvider.connectionStatus == DISCONNECTED) { - options.logger.log(DEBUG, "Skipping screenshot recording, no internet connection") - return - } - - if (isSession.get() && (rateLimiter?.isActiveForCategory(All) == true || rateLimiter?.isActiveForCategory(Replay) == true)) { - options.logger.log(DEBUG, "Skipping screenshot recording, rate limit is active") - return - } - if (!contentChanged.get() && lastScreenshot != null && !lastScreenshot!!.isRecycled) { options.logger.log(DEBUG, "Content hasn't changed, repeating last known frame") diff --git a/sentry-android-replay/src/main/java/io/sentry/android/replay/WindowRecorder.kt b/sentry-android-replay/src/main/java/io/sentry/android/replay/WindowRecorder.kt index c3b7a81a93c..1d642a195d6 100644 --- a/sentry-android-replay/src/main/java/io/sentry/android/replay/WindowRecorder.kt +++ b/sentry-android-replay/src/main/java/io/sentry/android/replay/WindowRecorder.kt @@ -19,7 +19,6 @@ internal class WindowRecorder( private val options: SentryOptions, private val screenshotRecorderCallback: ScreenshotRecorderCallback? = null, private val mainLooperHandler: MainLooperHandler, - private val rateLimiter: RateLimiter? ) : Recorder, OnRootViewsChangedListener { internal companion object { @@ -54,7 +53,7 @@ internal class WindowRecorder( return } - recorder = ScreenshotRecorder(recorderConfig, options, mainLooperHandler, screenshotRecorderCallback, rateLimiter) + recorder = ScreenshotRecorder(recorderConfig, options, mainLooperHandler, screenshotRecorderCallback) capturingTask = capturer.scheduleAtFixedRateSafely( options, "$TAG.capture", @@ -88,10 +87,6 @@ internal class WindowRecorder( capturer.gracefullyShutdown(options) } - override fun setReplayStrategy(isSession: Boolean) { - recorder?.isSession?.set(isSession) - } - private class RecorderExecutorServiceThreadFactory : ThreadFactory { private var cnt = 0 override fun newThread(r: Runnable): Thread { 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 7e9168df227..53f5c666831 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 @@ -56,6 +56,7 @@ internal interface CaptureStrategy { fun close() companion object { + private const val BREADCRUMB_START_OFFSET = 100L internal val currentEventsLock = Any() fun createSegment( @@ -161,7 +162,10 @@ internal interface CaptureStrategy { val urls = LinkedList() breadcrumbs.forEach { breadcrumb -> - if (breadcrumb.timestamp.time >= segmentTimestamp.time && + // we add some fixed breadcrumb offset to make sure we don't miss any + // breadcrumbs that might be relevant for the current segment, but just happened + // earlier than the current segment (e.g. network connectivity changed) + if ((breadcrumb.timestamp.time + BREADCRUMB_START_OFFSET) >= segmentTimestamp.time && breadcrumb.timestamp.time < endTimestamp.time ) { val rrwebEvent = options diff --git a/sentry/src/main/java/io/sentry/transport/AsyncHttpTransport.java b/sentry/src/main/java/io/sentry/transport/AsyncHttpTransport.java index 985bbcccbbe..24f954c0c10 100644 --- a/sentry/src/main/java/io/sentry/transport/AsyncHttpTransport.java +++ b/sentry/src/main/java/io/sentry/transport/AsyncHttpTransport.java @@ -170,6 +170,7 @@ public void close() throws IOException { @Override public void close(final boolean isRestarting) throws IOException { + rateLimiter.close(); executor.shutdown(); options.getLogger().log(SentryLevel.DEBUG, "Shutting down"); try { diff --git a/sentry/src/main/java/io/sentry/transport/RateLimiter.java b/sentry/src/main/java/io/sentry/transport/RateLimiter.java index f45313a497e..d89e4f92b61 100644 --- a/sentry/src/main/java/io/sentry/transport/RateLimiter.java +++ b/sentry/src/main/java/io/sentry/transport/RateLimiter.java @@ -5,6 +5,7 @@ import io.sentry.DataCategory; import io.sentry.Hint; +import io.sentry.IConnectionStatusProvider; import io.sentry.SentryEnvelope; import io.sentry.SentryEnvelopeItem; import io.sentry.SentryLevel; @@ -15,16 +16,21 @@ import io.sentry.util.CollectionUtils; import io.sentry.util.HintUtils; import io.sentry.util.StringUtils; +import java.io.Closeable; +import java.io.IOException; import java.util.ArrayList; import java.util.Date; import java.util.List; import java.util.Map; +import java.util.Timer; +import java.util.TimerTask; import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.CopyOnWriteArrayList; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; /** Controls retry limits on different category types sent to Sentry. */ -public final class RateLimiter { +public final class RateLimiter implements Closeable { private static final int HTTP_RETRY_AFTER_DEFAULT_DELAY_MILLIS = 60000; @@ -32,6 +38,15 @@ public final class RateLimiter { private final @NotNull SentryOptions options; private final @NotNull Map sentryRetryAfterLimit = new ConcurrentHashMap<>(); + private final @NotNull List rateLimitObservers = new CopyOnWriteArrayList<>(); + private final @NotNull TimerTask timerTask = new TimerTask() { + @Override public void run() { + notifyRateLimitObservers(false); + } + }; + private @Nullable Timer timer = null; + private final @NotNull Object timerLock = new Object(); + public RateLimiter( final @NotNull ICurrentDateProvider currentDateProvider, @@ -88,6 +103,15 @@ public RateLimiter(final @NotNull SentryOptions options) { return envelope; } + public boolean isActiveForCategories(final @NotNull DataCategory... dataCategories) { + for (DataCategory dataCategory : dataCategories) { + if (isActiveForCategory(dataCategory)) { + return true; + } + } + return false; + } + @SuppressWarnings({"JdkObsolete", "JavaUtilDate"}) public boolean isActiveForCategory(final @NotNull DataCategory dataCategory) { final Date currentDate = new Date(currentDateProvider.getCurrentTimeMillis()); @@ -282,6 +306,16 @@ private void applyRetryAfterOnlyIfLonger( // only overwrite its previous date if the limit is even longer if (oldDate == null || date.after(oldDate)) { sentryRetryAfterLimit.put(dataCategory, date); + + notifyRateLimitObservers(true); + + synchronized (timerLock) { + if (timer == null) { + timer = new Timer(true); + } + + timer.schedule(timerTask, date); + } } } @@ -303,4 +337,38 @@ private long parseRetryAfterOrDefault(final @Nullable String retryAfterHeader) { } return retryAfterMillis; } + + private void notifyRateLimitObservers(final boolean applied) { + for (IRateLimitObserver observer : rateLimitObservers) { + observer.onRateLimitChanged(applied); + } + } + + public void addRateLimitObserver(@NotNull final IRateLimitObserver observer) { + rateLimitObservers.add(observer); + } + + public void removeRateLimitObserver(@NotNull final IRateLimitObserver observer) { + rateLimitObservers.remove(observer); + } + + @Override + public void close() throws IOException { + synchronized (timerLock) { + if (timer != null) { + timer.cancel(); + timer = null; + } + } + } + + public interface IRateLimitObserver { + /** + * Invoked whenever the rate limit changed. You should use {@link RateLimiter#isActiveForCategory(DataCategory)} + * to check whether the category you're interested in has changed. + * + * @param applied true if the rate limit was applied, false if it was lifted + */ + void onRateLimitChanged(boolean applied); + } } From 169ed679311ce229d3eba2f50c2a5f4f062e3f8e Mon Sep 17 00:00:00 2001 From: Roman Zavarnitsyn Date: Tue, 12 Nov 2024 16:38:00 +0100 Subject: [PATCH 06/11] Clean up --- .../java/io/sentry/android/replay/ScreenshotRecorder.kt | 6 +----- .../main/java/io/sentry/android/replay/WindowRecorder.kt | 3 +-- 2 files changed, 2 insertions(+), 7 deletions(-) diff --git a/sentry-android-replay/src/main/java/io/sentry/android/replay/ScreenshotRecorder.kt b/sentry-android-replay/src/main/java/io/sentry/android/replay/ScreenshotRecorder.kt index fa7617770ac..60249103d09 100644 --- a/sentry-android-replay/src/main/java/io/sentry/android/replay/ScreenshotRecorder.kt +++ b/sentry-android-replay/src/main/java/io/sentry/android/replay/ScreenshotRecorder.kt @@ -17,9 +17,6 @@ import android.view.PixelCopy import android.view.View import android.view.ViewTreeObserver import android.view.WindowManager -import io.sentry.DataCategory.All -import io.sentry.DataCategory.Replay -import io.sentry.IConnectionStatusProvider.ConnectionStatus.DISCONNECTED import io.sentry.SentryLevel.DEBUG import io.sentry.SentryLevel.INFO import io.sentry.SentryLevel.WARNING @@ -33,7 +30,6 @@ import io.sentry.android.replay.util.traverse import io.sentry.android.replay.viewhierarchy.ViewHierarchyNode import io.sentry.android.replay.viewhierarchy.ViewHierarchyNode.ImageViewHierarchyNode import io.sentry.android.replay.viewhierarchy.ViewHierarchyNode.TextViewHierarchyNode -import io.sentry.transport.RateLimiter import java.io.File import java.lang.ref.WeakReference import java.util.concurrent.Executors @@ -47,7 +43,7 @@ internal class ScreenshotRecorder( val config: ScreenshotRecorderConfig, val options: SentryOptions, val mainLooperHandler: MainLooperHandler, - private val screenshotRecorderCallback: ScreenshotRecorderCallback?, + private val screenshotRecorderCallback: ScreenshotRecorderCallback? ) : ViewTreeObserver.OnDrawListener { private val recorder by lazy { diff --git a/sentry-android-replay/src/main/java/io/sentry/android/replay/WindowRecorder.kt b/sentry-android-replay/src/main/java/io/sentry/android/replay/WindowRecorder.kt index 1d642a195d6..9e846dfcf08 100644 --- a/sentry-android-replay/src/main/java/io/sentry/android/replay/WindowRecorder.kt +++ b/sentry-android-replay/src/main/java/io/sentry/android/replay/WindowRecorder.kt @@ -6,7 +6,6 @@ import io.sentry.SentryOptions import io.sentry.android.replay.util.MainLooperHandler import io.sentry.android.replay.util.gracefullyShutdown import io.sentry.android.replay.util.scheduleAtFixedRateSafely -import io.sentry.transport.RateLimiter import java.lang.ref.WeakReference import java.util.concurrent.Executors import java.util.concurrent.ScheduledFuture @@ -18,7 +17,7 @@ import java.util.concurrent.atomic.AtomicBoolean internal class WindowRecorder( private val options: SentryOptions, private val screenshotRecorderCallback: ScreenshotRecorderCallback? = null, - private val mainLooperHandler: MainLooperHandler, + private val mainLooperHandler: MainLooperHandler ) : Recorder, OnRootViewsChangedListener { internal companion object { From 30968fb51b6aa2a0caf47d152b4019a384769045 Mon Sep 17 00:00:00 2001 From: Roman Zavarnitsyn Date: Tue, 12 Nov 2024 23:41:33 +0100 Subject: [PATCH 07/11] Tests --- .../android/replay/ReplayIntegration.kt | 6 +- .../android/replay/ReplayIntegrationTest.kt | 121 ++++++++++++++++++ .../capture/SessionCaptureStrategyTest.kt | 3 - .../java/io/sentry/transport/RateLimiter.java | 9 -- .../transport/AsyncHttpTransportTest.kt | 8 ++ .../io/sentry/transport/RateLimiterTest.kt | 48 +++++++ 6 files changed, 181 insertions(+), 14 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 0f92cce5532..011f14585c8 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 @@ -299,7 +299,8 @@ public class ReplayIntegration( return } - if (hub?.rateLimiter?.isActiveForCategories(All, Replay) == true) { + if (hub?.rateLimiter?.isActiveForCategory(All) == true || + hub?.rateLimiter?.isActiveForCategory(Replay) == true) { pause() } else { resume() @@ -319,7 +320,8 @@ public class ReplayIntegration( private fun checkCanRecord() { if (captureStrategy is SessionCaptureStrategy && (options.connectionStatusProvider.connectionStatus == DISCONNECTED || - hub?.rateLimiter?.isActiveForCategories(All, Replay) == true)) { + hub?.rateLimiter?.isActiveForCategory(All) == true || + hub?.rateLimiter?.isActiveForCategory(Replay) == true)) { pause() } } 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 f503268dfff..ab4af426064 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 @@ -9,6 +9,8 @@ import androidx.test.ext.junit.runners.AndroidJUnit4 import io.sentry.Breadcrumb import io.sentry.DateUtils import io.sentry.Hint +import io.sentry.IConnectionStatusProvider.ConnectionStatus.CONNECTED +import io.sentry.IConnectionStatusProvider.ConnectionStatus.DISCONNECTED import io.sentry.IHub import io.sentry.Scope import io.sentry.ScopeCallback @@ -26,6 +28,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.ReplayCache.Companion.SEGMENT_KEY_WIDTH import io.sentry.android.replay.capture.CaptureStrategy +import io.sentry.android.replay.capture.SessionCaptureStrategy import io.sentry.android.replay.capture.SessionCaptureStrategyTest.Fixture.Companion.VIDEO_DURATION import io.sentry.android.replay.gestures.GestureRecorder import io.sentry.cache.PersistingScopeObserver @@ -38,6 +41,7 @@ import io.sentry.rrweb.RRWebMetaEvent import io.sentry.rrweb.RRWebVideoEvent import io.sentry.transport.CurrentDateProvider import io.sentry.transport.ICurrentDateProvider +import io.sentry.transport.RateLimiter import org.junit.Rule import org.junit.rules.TemporaryFolder import org.junit.runner.RunWith @@ -48,6 +52,7 @@ import org.mockito.kotlin.anyOrNull import org.mockito.kotlin.argThat import org.mockito.kotlin.check import org.mockito.kotlin.doAnswer +import org.mockito.kotlin.doReturn import org.mockito.kotlin.eq import org.mockito.kotlin.mock import org.mockito.kotlin.never @@ -82,10 +87,12 @@ class ReplayIntegrationTest { } } val scope = Scope(options) + val rateLimiter = mock() val hub = mock { doAnswer { ((it.arguments[0]) as ScopeCallback).run(scope) }.whenever(mock).configureScope(any()) + on { rateLimiter }.thenReturn(rateLimiter) } val replayCache = mock { @@ -98,6 +105,8 @@ class ReplayIntegrationTest { context: Context, sessionSampleRate: Double = 1.0, onErrorSampleRate: Double = 1.0, + isOffline: Boolean = false, + isRateLimited: Boolean = false, recorderProvider: (() -> Recorder)? = null, replayCaptureStrategyProvider: ((isFullSession: Boolean) -> CaptureStrategy)? = null, recorderConfigProvider: ((configChanged: Boolean) -> ScreenshotRecorderConfig)? = null, @@ -107,6 +116,12 @@ class ReplayIntegrationTest { options.run { experimental.sessionReplay.onErrorSampleRate = onErrorSampleRate experimental.sessionReplay.sessionSampleRate = sessionSampleRate + connectionStatusProvider = mock { + on { connectionStatus }.thenReturn(if (isOffline) DISCONNECTED else CONNECTED) + } + } + if (isRateLimited) { + whenever(rateLimiter.isActiveForCategory(any())).thenReturn(true) } return ReplayIntegration( context, @@ -567,4 +582,110 @@ class ReplayIntegrationTest { verify(fixture.replayCache).addFrame(any(), any(), eq("MainActivity")) } + + @Test + fun `onScreenshotRecorded pauses replay when offline for sessions`() { + val captureStrategy = SessionCaptureStrategy(fixture.options, null, CurrentDateProvider.getInstance()) + val recorder = mock() + val replay = fixture.getSut( + context, + recorderProvider = { recorder }, + replayCaptureStrategyProvider = { captureStrategy }, + isOffline = true + ) + + replay.register(fixture.hub, fixture.options) + replay.start() + replay.onScreenshotRecorded(mock()) + + verify(recorder).pause() + } + + @Test + fun `onScreenshotRecorded pauses replay when rate-limited for sessions`() { + val captureStrategy = SessionCaptureStrategy(fixture.options, null, CurrentDateProvider.getInstance()) + val recorder = mock() + val replay = fixture.getSut( + context, + recorderProvider = { recorder }, + replayCaptureStrategyProvider = { captureStrategy }, + isRateLimited = true + ) + + replay.register(fixture.hub, fixture.options) + replay.start() + replay.onScreenshotRecorded(mock()) + + verify(recorder).pause() + } + + @Test + fun `onConnectionStatusChanged pauses replay when offline for sessions`() { + val captureStrategy = SessionCaptureStrategy(fixture.options, null, CurrentDateProvider.getInstance()) + val recorder = mock() + val replay = fixture.getSut( + context, + recorderProvider = { recorder }, + replayCaptureStrategyProvider = { captureStrategy } + ) + + replay.register(fixture.hub, fixture.options) + replay.start() + replay.onConnectionStatusChanged(DISCONNECTED) + + verify(recorder).pause() + } + + @Test + fun `onConnectionStatusChanged resumes replay when back-online for sessions`() { + val captureStrategy = SessionCaptureStrategy(fixture.options, null, CurrentDateProvider.getInstance()) + val recorder = mock() + val replay = fixture.getSut( + context, + recorderProvider = { recorder }, + replayCaptureStrategyProvider = { captureStrategy } + ) + + replay.register(fixture.hub, fixture.options) + replay.start() + replay.onConnectionStatusChanged(CONNECTED) + + verify(recorder).resume() + } + + @Test + fun `onRateLimitChanged pauses replay when rate-limited for sessions`() { + val captureStrategy = SessionCaptureStrategy(fixture.options, null, CurrentDateProvider.getInstance()) + val recorder = mock() + val replay = fixture.getSut( + context, + recorderProvider = { recorder }, + replayCaptureStrategyProvider = { captureStrategy }, + isRateLimited = true + ) + + replay.register(fixture.hub, fixture.options) + replay.start() + replay.onRateLimitChanged(true) + + verify(recorder).pause() + } + + @Test + fun `onRateLimitChanged resumes replay when rate-limit lifted for sessions`() { + val captureStrategy = SessionCaptureStrategy(fixture.options, null, CurrentDateProvider.getInstance()) + val recorder = mock() + val replay = fixture.getSut( + context, + recorderProvider = { recorder }, + replayCaptureStrategyProvider = { captureStrategy }, + isRateLimited = false + ) + + replay.register(fixture.hub, fixture.options) + replay.start() + replay.onRateLimitChanged(false) + + verify(recorder).resume() + } } diff --git a/sentry-android-replay/src/test/java/io/sentry/android/replay/capture/SessionCaptureStrategyTest.kt b/sentry-android-replay/src/test/java/io/sentry/android/replay/capture/SessionCaptureStrategyTest.kt index 6ae1dac3fec..12eb10c3f4b 100644 --- a/sentry-android-replay/src/test/java/io/sentry/android/replay/capture/SessionCaptureStrategyTest.kt +++ b/sentry-android-replay/src/test/java/io/sentry/android/replay/capture/SessionCaptureStrategyTest.kt @@ -27,7 +27,6 @@ import io.sentry.rrweb.RRWebBreadcrumbEvent import io.sentry.rrweb.RRWebMetaEvent import io.sentry.transport.CurrentDateProvider import io.sentry.transport.ICurrentDateProvider -import io.sentry.transport.RateLimiter import org.junit.Rule import org.junit.rules.TemporaryFolder import org.mockito.ArgumentMatchers.anyInt @@ -37,7 +36,6 @@ import org.mockito.kotlin.anyOrNull import org.mockito.kotlin.argThat import org.mockito.kotlin.check import org.mockito.kotlin.doAnswer -import org.mockito.kotlin.doReturn import org.mockito.kotlin.mock import org.mockito.kotlin.never import org.mockito.kotlin.verify @@ -71,7 +69,6 @@ class SessionCaptureStrategyTest { doAnswer { (it.arguments[0] as ScopeCallback).run(scope) }.whenever(it).configureScope(any()) - doReturn(mock()).whenever(it).rateLimiter } var persistedSegment = LinkedHashMap() val replayCache = mock { diff --git a/sentry/src/main/java/io/sentry/transport/RateLimiter.java b/sentry/src/main/java/io/sentry/transport/RateLimiter.java index d89e4f92b61..68e6c63d626 100644 --- a/sentry/src/main/java/io/sentry/transport/RateLimiter.java +++ b/sentry/src/main/java/io/sentry/transport/RateLimiter.java @@ -103,15 +103,6 @@ public RateLimiter(final @NotNull SentryOptions options) { return envelope; } - public boolean isActiveForCategories(final @NotNull DataCategory... dataCategories) { - for (DataCategory dataCategory : dataCategories) { - if (isActiveForCategory(dataCategory)) { - return true; - } - } - return false; - } - @SuppressWarnings({"JdkObsolete", "JavaUtilDate"}) public boolean isActiveForCategory(final @NotNull DataCategory dataCategory) { final Date currentDate = new Date(currentDateProvider.getCurrentTimeMillis()); diff --git a/sentry/src/test/java/io/sentry/transport/AsyncHttpTransportTest.kt b/sentry/src/test/java/io/sentry/transport/AsyncHttpTransportTest.kt index abaa965175c..ae479ed6cb8 100644 --- a/sentry/src/test/java/io/sentry/transport/AsyncHttpTransportTest.kt +++ b/sentry/src/test/java/io/sentry/transport/AsyncHttpTransportTest.kt @@ -329,6 +329,14 @@ class AsyncHttpTransportTest { ) } + @Test + fun `close closes the rate limiter`() { + val sut = fixture.getSUT() + sut.close() + + verify(fixture.rateLimiter).close() + } + @Test fun `close uses flushTimeoutMillis option to schedule termination`() { fixture.sentryOptions.flushTimeoutMillis = 123 diff --git a/sentry/src/test/java/io/sentry/transport/RateLimiterTest.kt b/sentry/src/test/java/io/sentry/transport/RateLimiterTest.kt index 79c80c40412..01c3f767e34 100644 --- a/sentry/src/test/java/io/sentry/transport/RateLimiterTest.kt +++ b/sentry/src/test/java/io/sentry/transport/RateLimiterTest.kt @@ -27,6 +27,7 @@ import io.sentry.metrics.EncodedMetrics import io.sentry.protocol.SentryId import io.sentry.protocol.SentryTransaction import io.sentry.protocol.User +import org.awaitility.kotlin.await import org.mockito.kotlin.eq import org.mockito.kotlin.mock import org.mockito.kotlin.same @@ -36,6 +37,8 @@ import org.mockito.kotlin.verifyNoMoreInteractions import org.mockito.kotlin.whenever import java.io.File import java.util.UUID +import java.util.concurrent.TimeUnit.MILLISECONDS +import java.util.concurrent.atomic.AtomicBoolean import kotlin.test.Test import kotlin.test.assertEquals import kotlin.test.assertFalse @@ -377,4 +380,49 @@ class RateLimiterTest { verify(fixture.clientReportRecorder, times(1)).recordLostEnvelopeItem(eq(DiscardReason.RATELIMIT_BACKOFF), same(replayItem)) verifyNoMoreInteractions(fixture.clientReportRecorder) } + + @Test + fun `apply rate limits notifies observers`() { + val rateLimiter = fixture.getSUT() + + var applied = false + rateLimiter.addRateLimitObserver { + applied = it + } + rateLimiter.updateRetryAfterLimits("60:replay:key", null, 1) + + assertTrue(applied) + } + + @Test + fun `apply rate limits schedules a timer to notify observers of lifted limits`() { + val rateLimiter = fixture.getSUT() + + var applied = true + rateLimiter.addRateLimitObserver { + applied = it + } + rateLimiter.updateRetryAfterLimits("1:replay:key", null, 1) + + // wait for 1.5s to ensure the timer has run after 1s + await.atLeast(1500L, MILLISECONDS) + assertFalse(applied) + } + + @Test + fun `close cancels the timer`() { + val rateLimiter = fixture.getSUT() + + var applied = true + rateLimiter.addRateLimitObserver { + applied = it + } + + rateLimiter.updateRetryAfterLimits("1:replay:key", null, 1) + rateLimiter.close() + + // wait for 1.5s to ensure the timer has run after 1s + await.atLeast(1500L, MILLISECONDS) + assertTrue(applied) + } } From 177543a5e83cab7e43fea07147e25f4b719be484 Mon Sep 17 00:00:00 2001 From: Roman Zavarnitsyn Date: Tue, 12 Nov 2024 23:42:15 +0100 Subject: [PATCH 08/11] Api dump --- .../api/sentry-android-replay.api | 4 +++- .../android/replay/ReplayIntegration.kt | 13 ++++++++----- .../android/replay/ReplayIntegrationTest.kt | 1 - sentry/api/sentry.api | 9 ++++++++- .../java/io/sentry/transport/RateLimiter.java | 19 ++++++++++--------- .../io/sentry/transport/RateLimiterTest.kt | 1 - 6 files changed, 29 insertions(+), 18 deletions(-) diff --git a/sentry-android-replay/api/sentry-android-replay.api b/sentry-android-replay/api/sentry-android-replay.api index a08fb1dd988..afed52a8d5b 100644 --- a/sentry-android-replay/api/sentry-android-replay.api +++ b/sentry-android-replay/api/sentry-android-replay.api @@ -57,7 +57,7 @@ public final class io/sentry/android/replay/ReplayCache$Companion { public final fun makeReplayCacheDir (Lio/sentry/SentryOptions;Lio/sentry/protocol/SentryId;)Ljava/io/File; } -public final class io/sentry/android/replay/ReplayIntegration : android/content/ComponentCallbacks, io/sentry/Integration, io/sentry/ReplayController, io/sentry/android/replay/ScreenshotRecorderCallback, io/sentry/android/replay/gestures/TouchRecorderCallback, java/io/Closeable { +public final class io/sentry/android/replay/ReplayIntegration : android/content/ComponentCallbacks, io/sentry/IConnectionStatusProvider$IConnectionStatusObserver, io/sentry/Integration, io/sentry/ReplayController, io/sentry/android/replay/ScreenshotRecorderCallback, io/sentry/android/replay/gestures/TouchRecorderCallback, io/sentry/transport/RateLimiter$IRateLimitObserver, java/io/Closeable { public static final field $stable I public fun (Landroid/content/Context;Lio/sentry/transport/ICurrentDateProvider;)V public fun (Landroid/content/Context;Lio/sentry/transport/ICurrentDateProvider;Lkotlin/jvm/functions/Function0;Lkotlin/jvm/functions/Function1;Lkotlin/jvm/functions/Function2;)V @@ -69,7 +69,9 @@ public final class io/sentry/android/replay/ReplayIntegration : android/content/ public fun getReplayId ()Lio/sentry/protocol/SentryId; public fun isRecording ()Z public fun onConfigurationChanged (Landroid/content/res/Configuration;)V + public fun onConnectionStatusChanged (Lio/sentry/IConnectionStatusProvider$ConnectionStatus;)V public fun onLowMemory ()V + public fun onRateLimitChanged (Z)V public fun onScreenshotRecorded (Landroid/graphics/Bitmap;)V public fun onScreenshotRecorded (Ljava/io/File;J)V public fun onTouchEvent (Landroid/view/MotionEvent;)V 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 011f14585c8..81a470df786 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 @@ -7,7 +7,6 @@ import android.graphics.Bitmap import android.os.Build import android.view.MotionEvent import io.sentry.Breadcrumb -import io.sentry.DataCategory import io.sentry.DataCategory.All import io.sentry.DataCategory.Replay import io.sentry.IConnectionStatusProvider.ConnectionStatus @@ -300,7 +299,8 @@ public class ReplayIntegration( } if (hub?.rateLimiter?.isActiveForCategory(All) == true || - hub?.rateLimiter?.isActiveForCategory(Replay) == true) { + hub?.rateLimiter?.isActiveForCategory(Replay) == true + ) { pause() } else { resume() @@ -319,9 +319,12 @@ public class ReplayIntegration( */ private fun checkCanRecord() { if (captureStrategy is SessionCaptureStrategy && - (options.connectionStatusProvider.connectionStatus == DISCONNECTED || - hub?.rateLimiter?.isActiveForCategory(All) == true || - hub?.rateLimiter?.isActiveForCategory(Replay) == true)) { + ( + options.connectionStatusProvider.connectionStatus == DISCONNECTED || + hub?.rateLimiter?.isActiveForCategory(All) == true || + hub?.rateLimiter?.isActiveForCategory(Replay) == true + ) + ) { pause() } } 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 ab4af426064..d7c202c4cfb 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 @@ -52,7 +52,6 @@ import org.mockito.kotlin.anyOrNull import org.mockito.kotlin.argThat import org.mockito.kotlin.check import org.mockito.kotlin.doAnswer -import org.mockito.kotlin.doReturn import org.mockito.kotlin.eq import org.mockito.kotlin.mock import org.mockito.kotlin.never diff --git a/sentry/api/sentry.api b/sentry/api/sentry.api index 8e266bcdba6..294de4c2462 100644 --- a/sentry/api/sentry.api +++ b/sentry/api/sentry.api @@ -5564,15 +5564,22 @@ public final class io/sentry/transport/NoOpTransportGate : io/sentry/transport/I public fun isConnected ()Z } -public final class io/sentry/transport/RateLimiter { +public final class io/sentry/transport/RateLimiter : java/io/Closeable { public fun (Lio/sentry/SentryOptions;)V public fun (Lio/sentry/transport/ICurrentDateProvider;Lio/sentry/SentryOptions;)V + public fun addRateLimitObserver (Lio/sentry/transport/RateLimiter$IRateLimitObserver;)V + public fun close ()V public fun filter (Lio/sentry/SentryEnvelope;Lio/sentry/Hint;)Lio/sentry/SentryEnvelope; public fun isActiveForCategory (Lio/sentry/DataCategory;)Z public fun isAnyRateLimitActive ()Z + public fun removeRateLimitObserver (Lio/sentry/transport/RateLimiter$IRateLimitObserver;)V public fun updateRetryAfterLimits (Ljava/lang/String;Ljava/lang/String;I)V } +public abstract interface class io/sentry/transport/RateLimiter$IRateLimitObserver { + public abstract fun onRateLimitChanged (Z)V +} + public final class io/sentry/transport/ReusableCountLatch { public fun ()V public fun (I)V diff --git a/sentry/src/main/java/io/sentry/transport/RateLimiter.java b/sentry/src/main/java/io/sentry/transport/RateLimiter.java index 68e6c63d626..87551835881 100644 --- a/sentry/src/main/java/io/sentry/transport/RateLimiter.java +++ b/sentry/src/main/java/io/sentry/transport/RateLimiter.java @@ -5,7 +5,6 @@ import io.sentry.DataCategory; import io.sentry.Hint; -import io.sentry.IConnectionStatusProvider; import io.sentry.SentryEnvelope; import io.sentry.SentryEnvelopeItem; import io.sentry.SentryLevel; @@ -39,15 +38,16 @@ public final class RateLimiter implements Closeable { private final @NotNull Map sentryRetryAfterLimit = new ConcurrentHashMap<>(); private final @NotNull List rateLimitObservers = new CopyOnWriteArrayList<>(); - private final @NotNull TimerTask timerTask = new TimerTask() { - @Override public void run() { - notifyRateLimitObservers(false); - } - }; + private final @NotNull TimerTask timerTask = + new TimerTask() { + @Override + public void run() { + notifyRateLimitObservers(false); + } + }; private @Nullable Timer timer = null; private final @NotNull Object timerLock = new Object(); - public RateLimiter( final @NotNull ICurrentDateProvider currentDateProvider, final @NotNull SentryOptions options) { @@ -355,8 +355,9 @@ public void close() throws IOException { public interface IRateLimitObserver { /** - * Invoked whenever the rate limit changed. You should use {@link RateLimiter#isActiveForCategory(DataCategory)} - * to check whether the category you're interested in has changed. + * Invoked whenever the rate limit changed. You should use {@link + * RateLimiter#isActiveForCategory(DataCategory)} to check whether the category you're + * interested in has changed. * * @param applied true if the rate limit was applied, false if it was lifted */ diff --git a/sentry/src/test/java/io/sentry/transport/RateLimiterTest.kt b/sentry/src/test/java/io/sentry/transport/RateLimiterTest.kt index 01c3f767e34..97f84bb2795 100644 --- a/sentry/src/test/java/io/sentry/transport/RateLimiterTest.kt +++ b/sentry/src/test/java/io/sentry/transport/RateLimiterTest.kt @@ -38,7 +38,6 @@ import org.mockito.kotlin.whenever import java.io.File import java.util.UUID import java.util.concurrent.TimeUnit.MILLISECONDS -import java.util.concurrent.atomic.AtomicBoolean import kotlin.test.Test import kotlin.test.assertEquals import kotlin.test.assertFalse From 717e80215097bb62290e7a2313d4aedcaf8017a6 Mon Sep 17 00:00:00 2001 From: Roman Zavarnitsyn Date: Wed, 13 Nov 2024 11:49:45 +0100 Subject: [PATCH 09/11] Fix tests --- .../java/io/sentry/transport/RateLimiter.java | 16 ++++++++-------- .../io/sentry/transport/RateLimiterTest.kt | 18 +++++++++--------- 2 files changed, 17 insertions(+), 17 deletions(-) diff --git a/sentry/src/main/java/io/sentry/transport/RateLimiter.java b/sentry/src/main/java/io/sentry/transport/RateLimiter.java index 87551835881..2dc342dc70e 100644 --- a/sentry/src/main/java/io/sentry/transport/RateLimiter.java +++ b/sentry/src/main/java/io/sentry/transport/RateLimiter.java @@ -38,13 +38,6 @@ public final class RateLimiter implements Closeable { private final @NotNull Map sentryRetryAfterLimit = new ConcurrentHashMap<>(); private final @NotNull List rateLimitObservers = new CopyOnWriteArrayList<>(); - private final @NotNull TimerTask timerTask = - new TimerTask() { - @Override - public void run() { - notifyRateLimitObservers(false); - } - }; private @Nullable Timer timer = null; private final @NotNull Object timerLock = new Object(); @@ -305,7 +298,14 @@ private void applyRetryAfterOnlyIfLonger( timer = new Timer(true); } - timer.schedule(timerTask, date); + timer.schedule( + new TimerTask() { + @Override + public void run() { + notifyRateLimitObservers(false); + } + }, + date); } } } diff --git a/sentry/src/test/java/io/sentry/transport/RateLimiterTest.kt b/sentry/src/test/java/io/sentry/transport/RateLimiterTest.kt index 97f84bb2795..fe1362ddc3d 100644 --- a/sentry/src/test/java/io/sentry/transport/RateLimiterTest.kt +++ b/sentry/src/test/java/io/sentry/transport/RateLimiterTest.kt @@ -37,7 +37,7 @@ import org.mockito.kotlin.verifyNoMoreInteractions import org.mockito.kotlin.whenever import java.io.File import java.util.UUID -import java.util.concurrent.TimeUnit.MILLISECONDS +import java.util.concurrent.atomic.AtomicBoolean import kotlin.test.Test import kotlin.test.assertEquals import kotlin.test.assertFalse @@ -397,31 +397,31 @@ class RateLimiterTest { fun `apply rate limits schedules a timer to notify observers of lifted limits`() { val rateLimiter = fixture.getSUT() - var applied = true + val applied = AtomicBoolean(true) rateLimiter.addRateLimitObserver { - applied = it + applied.set(it) } rateLimiter.updateRetryAfterLimits("1:replay:key", null, 1) // wait for 1.5s to ensure the timer has run after 1s - await.atLeast(1500L, MILLISECONDS) - assertFalse(applied) + await.untilFalse(applied) + assertFalse(applied.get()) } @Test fun `close cancels the timer`() { val rateLimiter = fixture.getSUT() - var applied = true + val applied = AtomicBoolean(true) rateLimiter.addRateLimitObserver { - applied = it + applied.set(it) } rateLimiter.updateRetryAfterLimits("1:replay:key", null, 1) rateLimiter.close() // wait for 1.5s to ensure the timer has run after 1s - await.atLeast(1500L, MILLISECONDS) - assertTrue(applied) + await.untilTrue(applied) + assertTrue(applied.get()) } } From 9b70c97eec12983bebe87472e6a8fc3b856f9b19 Mon Sep 17 00:00:00 2001 From: Roman Zavarnitsyn Date: Fri, 15 Nov 2024 15:39:36 +0100 Subject: [PATCH 10/11] Address PR feedback --- .../api/sentry-android-replay.api | 2 +- .../android/replay/ReplayIntegration.kt | 7 ++--- .../android/replay/ReplayIntegrationTest.kt | 29 ++++++++++++++----- sentry/api/sentry.api | 2 +- .../java/io/sentry/transport/RateLimiter.java | 14 +++++---- 5 files changed, 34 insertions(+), 20 deletions(-) diff --git a/sentry-android-replay/api/sentry-android-replay.api b/sentry-android-replay/api/sentry-android-replay.api index afed52a8d5b..7e2db5248f0 100644 --- a/sentry-android-replay/api/sentry-android-replay.api +++ b/sentry-android-replay/api/sentry-android-replay.api @@ -71,7 +71,7 @@ public final class io/sentry/android/replay/ReplayIntegration : android/content/ public fun onConfigurationChanged (Landroid/content/res/Configuration;)V public fun onConnectionStatusChanged (Lio/sentry/IConnectionStatusProvider$ConnectionStatus;)V public fun onLowMemory ()V - public fun onRateLimitChanged (Z)V + public fun onRateLimitChanged (Lio/sentry/transport/RateLimiter;)V public fun onScreenshotRecorded (Landroid/graphics/Bitmap;)V public fun onScreenshotRecorded (Ljava/io/File;J)V public fun onTouchEvent (Landroid/view/MotionEvent;)V 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 81a470df786..d644ad096a6 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 @@ -37,6 +37,7 @@ import io.sentry.cache.PersistingScopeObserver.REPLAY_FILENAME import io.sentry.hints.Backfillable import io.sentry.protocol.SentryId import io.sentry.transport.ICurrentDateProvider +import io.sentry.transport.RateLimiter import io.sentry.transport.RateLimiter.IRateLimitObserver import io.sentry.util.FileUtils import io.sentry.util.HintUtils @@ -292,15 +293,13 @@ public class ReplayIntegration( } } - override fun onRateLimitChanged(applied: Boolean) { + override fun onRateLimitChanged(rateLimiter: RateLimiter) { if (captureStrategy !is SessionCaptureStrategy) { // we only want to stop recording when rate-limited for session mode return } - if (hub?.rateLimiter?.isActiveForCategory(All) == true || - hub?.rateLimiter?.isActiveForCategory(Replay) == true - ) { + if (rateLimiter.isActiveForCategory(All) || rateLimiter.isActiveForCategory(Replay)) { pause() } else { resume() 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 d7c202c4cfb..ff396d04c10 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 @@ -584,7 +584,7 @@ class ReplayIntegrationTest { @Test fun `onScreenshotRecorded pauses replay when offline for sessions`() { - val captureStrategy = SessionCaptureStrategy(fixture.options, null, CurrentDateProvider.getInstance()) + val captureStrategy = getSessionCaptureStrategy(fixture.options) val recorder = mock() val replay = fixture.getSut( context, @@ -602,7 +602,7 @@ class ReplayIntegrationTest { @Test fun `onScreenshotRecorded pauses replay when rate-limited for sessions`() { - val captureStrategy = SessionCaptureStrategy(fixture.options, null, CurrentDateProvider.getInstance()) + val captureStrategy = getSessionCaptureStrategy(fixture.options) val recorder = mock() val replay = fixture.getSut( context, @@ -620,7 +620,7 @@ class ReplayIntegrationTest { @Test fun `onConnectionStatusChanged pauses replay when offline for sessions`() { - val captureStrategy = SessionCaptureStrategy(fixture.options, null, CurrentDateProvider.getInstance()) + val captureStrategy = getSessionCaptureStrategy(fixture.options) val recorder = mock() val replay = fixture.getSut( context, @@ -637,7 +637,7 @@ class ReplayIntegrationTest { @Test fun `onConnectionStatusChanged resumes replay when back-online for sessions`() { - val captureStrategy = SessionCaptureStrategy(fixture.options, null, CurrentDateProvider.getInstance()) + val captureStrategy = getSessionCaptureStrategy(fixture.options) val recorder = mock() val replay = fixture.getSut( context, @@ -654,7 +654,7 @@ class ReplayIntegrationTest { @Test fun `onRateLimitChanged pauses replay when rate-limited for sessions`() { - val captureStrategy = SessionCaptureStrategy(fixture.options, null, CurrentDateProvider.getInstance()) + val captureStrategy = getSessionCaptureStrategy(fixture.options) val recorder = mock() val replay = fixture.getSut( context, @@ -665,14 +665,14 @@ class ReplayIntegrationTest { replay.register(fixture.hub, fixture.options) replay.start() - replay.onRateLimitChanged(true) + replay.onRateLimitChanged(fixture.rateLimiter) verify(recorder).pause() } @Test fun `onRateLimitChanged resumes replay when rate-limit lifted for sessions`() { - val captureStrategy = SessionCaptureStrategy(fixture.options, null, CurrentDateProvider.getInstance()) + val captureStrategy = getSessionCaptureStrategy(fixture.options) val recorder = mock() val replay = fixture.getSut( context, @@ -683,8 +683,21 @@ class ReplayIntegrationTest { replay.register(fixture.hub, fixture.options) replay.start() - replay.onRateLimitChanged(false) + replay.onRateLimitChanged(fixture.rateLimiter) verify(recorder).resume() } + + private fun getSessionCaptureStrategy(options: SentryOptions): SessionCaptureStrategy { + return SessionCaptureStrategy( + options, + null, + CurrentDateProvider.getInstance(), + executor = mock { + doAnswer { + (it.arguments[0] as Runnable).run() + }.whenever(mock).submit(any()) + } + ) + } } diff --git a/sentry/api/sentry.api b/sentry/api/sentry.api index 294de4c2462..c356b298b68 100644 --- a/sentry/api/sentry.api +++ b/sentry/api/sentry.api @@ -5577,7 +5577,7 @@ public final class io/sentry/transport/RateLimiter : java/io/Closeable { } public abstract interface class io/sentry/transport/RateLimiter$IRateLimitObserver { - public abstract fun onRateLimitChanged (Z)V + public abstract fun onRateLimitChanged (Lio/sentry/transport/RateLimiter;)V } public final class io/sentry/transport/ReusableCountLatch { diff --git a/sentry/src/main/java/io/sentry/transport/RateLimiter.java b/sentry/src/main/java/io/sentry/transport/RateLimiter.java index 2dc342dc70e..191e8cbe7ef 100644 --- a/sentry/src/main/java/io/sentry/transport/RateLimiter.java +++ b/sentry/src/main/java/io/sentry/transport/RateLimiter.java @@ -291,7 +291,7 @@ private void applyRetryAfterOnlyIfLonger( if (oldDate == null || date.after(oldDate)) { sentryRetryAfterLimit.put(dataCategory, date); - notifyRateLimitObservers(true); + notifyRateLimitObservers(); synchronized (timerLock) { if (timer == null) { @@ -302,7 +302,7 @@ private void applyRetryAfterOnlyIfLonger( new TimerTask() { @Override public void run() { - notifyRateLimitObservers(false); + notifyRateLimitObservers(); } }, date); @@ -329,9 +329,9 @@ private long parseRetryAfterOrDefault(final @Nullable String retryAfterHeader) { return retryAfterMillis; } - private void notifyRateLimitObservers(final boolean applied) { + private void notifyRateLimitObservers() { for (IRateLimitObserver observer : rateLimitObservers) { - observer.onRateLimitChanged(applied); + observer.onRateLimitChanged(this); } } @@ -351,6 +351,7 @@ public void close() throws IOException { timer = null; } } + rateLimitObservers.clear(); } public interface IRateLimitObserver { @@ -359,8 +360,9 @@ public interface IRateLimitObserver { * RateLimiter#isActiveForCategory(DataCategory)} to check whether the category you're * interested in has changed. * - * @param applied true if the rate limit was applied, false if it was lifted + * @param rateLimiter this {@link RateLimiter} instance which you can use to check if the rate + * limit is active for a specific category */ - void onRateLimitChanged(boolean applied); + void onRateLimitChanged(@NotNull RateLimiter rateLimiter); } } From 8ccd0bbf77c6b4da13e65f6d1d8b1480693d3def Mon Sep 17 00:00:00 2001 From: Roman Zavarnitsyn Date: Fri, 15 Nov 2024 16:25:40 +0100 Subject: [PATCH 11/11] Fix tests --- .../test/java/io/sentry/transport/RateLimiterTest.kt | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/sentry/src/test/java/io/sentry/transport/RateLimiterTest.kt b/sentry/src/test/java/io/sentry/transport/RateLimiterTest.kt index fe1362ddc3d..8d8fb9601ef 100644 --- a/sentry/src/test/java/io/sentry/transport/RateLimiterTest.kt +++ b/sentry/src/test/java/io/sentry/transport/RateLimiterTest.kt @@ -3,6 +3,7 @@ package io.sentry.transport import io.sentry.Attachment import io.sentry.CheckIn import io.sentry.CheckInStatus +import io.sentry.DataCategory.Replay import io.sentry.Hint import io.sentry.IHub import io.sentry.ILogger @@ -386,7 +387,7 @@ class RateLimiterTest { var applied = false rateLimiter.addRateLimitObserver { - applied = it + applied = rateLimiter.isActiveForCategory(Replay) } rateLimiter.updateRetryAfterLimits("60:replay:key", null, 1) @@ -396,14 +397,14 @@ class RateLimiterTest { @Test fun `apply rate limits schedules a timer to notify observers of lifted limits`() { val rateLimiter = fixture.getSUT() + whenever(fixture.currentDateProvider.currentTimeMillis).thenReturn(0, 1, 2001) val applied = AtomicBoolean(true) rateLimiter.addRateLimitObserver { - applied.set(it) + applied.set(rateLimiter.isActiveForCategory(Replay)) } rateLimiter.updateRetryAfterLimits("1:replay:key", null, 1) - // wait for 1.5s to ensure the timer has run after 1s await.untilFalse(applied) assertFalse(applied.get()) } @@ -411,10 +412,11 @@ class RateLimiterTest { @Test fun `close cancels the timer`() { val rateLimiter = fixture.getSUT() + whenever(fixture.currentDateProvider.currentTimeMillis).thenReturn(0, 1, 2001) val applied = AtomicBoolean(true) rateLimiter.addRateLimitObserver { - applied.set(it) + applied.set(rateLimiter.isActiveForCategory(Replay)) } rateLimiter.updateRetryAfterLimits("1:replay:key", null, 1)