From 9246ed4aa0cf28a080ed3461e5fd9c7466ffc88a Mon Sep 17 00:00:00 2001 From: Markus Hintersteiner Date: Mon, 19 Jun 2023 13:45:05 +0200 Subject: [PATCH] Add debouncing mechanism and before-capture callbacks for screenshots/vh (#2773) --- .github/workflows/agp-matrix.yml | 2 +- CHANGELOG.md | 6 ++ .../api/sentry-android-core.api | 8 ++ .../core/ScreenshotEventProcessor.java | 20 ++++ .../android/core/SentryAndroidOptions.java | 73 +++++++++++++- .../core/ViewHierarchyEventProcessor.java | 20 ++++ .../android/core/internal/util/Debouncer.java | 33 +++++++ .../core/ScreenshotEventProcessorTest.kt | 99 +++++++++++++++++++ .../core/ViewHierarchyEventProcessorTest.kt | 85 ++++++++++++++++ .../core/internal/util/DebouncerTest.kt | 62 ++++++++++++ 10 files changed, 405 insertions(+), 3 deletions(-) create mode 100644 sentry-android-core/src/main/java/io/sentry/android/core/internal/util/Debouncer.java create mode 100644 sentry-android-core/src/test/java/io/sentry/android/core/internal/util/DebouncerTest.kt diff --git a/.github/workflows/agp-matrix.yml b/.github/workflows/agp-matrix.yml index b5e28a0470..eb5f8ed2d6 100644 --- a/.github/workflows/agp-matrix.yml +++ b/.github/workflows/agp-matrix.yml @@ -17,7 +17,7 @@ jobs: access_token: ${{ github.token }} agp-matrix-compatibility: - timeout-minutes: 25 + timeout-minutes: 30 runs-on: macos-latest strategy: fail-fast: false diff --git a/CHANGELOG.md b/CHANGELOG.md index 1bfca335cf..2bd46b6a4d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,11 @@ # Changelog +## Unreleased + +### Features + +- Add debouncing mechanism and before-capture callbacks for screenshots and view hierarchies ([#2773](https://github.com/getsentry/sentry-java/pull/2773)) + ## 6.23.0 ### Features diff --git a/sentry-android-core/api/sentry-android-core.api b/sentry-android-core/api/sentry-android-core.api index 2304b56cf4..7fd64ed8c5 100644 --- a/sentry-android-core/api/sentry-android-core.api +++ b/sentry-android-core/api/sentry-android-core.api @@ -207,6 +207,8 @@ public final class io/sentry/android/core/SentryAndroidOptions : io/sentry/Sentr public fun ()V public fun enableAllAutoBreadcrumbs (Z)V public fun getAnrTimeoutIntervalMillis ()J + public fun getBeforeScreenshotCaptureCallback ()Lio/sentry/android/core/SentryAndroidOptions$BeforeCaptureCallback; + public fun getBeforeViewHierarchyCaptureCallback ()Lio/sentry/android/core/SentryAndroidOptions$BeforeCaptureCallback; public fun getDebugImagesLoader ()Lio/sentry/android/core/IDebugImagesLoader; public fun getNativeSdkName ()Ljava/lang/String; public fun getProfilingTracesHz ()I @@ -231,6 +233,8 @@ public final class io/sentry/android/core/SentryAndroidOptions : io/sentry/Sentr public fun setAnrTimeoutIntervalMillis (J)V public fun setAttachScreenshot (Z)V public fun setAttachViewHierarchy (Z)V + public fun setBeforeScreenshotCaptureCallback (Lio/sentry/android/core/SentryAndroidOptions$BeforeCaptureCallback;)V + public fun setBeforeViewHierarchyCaptureCallback (Lio/sentry/android/core/SentryAndroidOptions$BeforeCaptureCallback;)V public fun setCollectAdditionalContext (Z)V public fun setDebugImagesLoader (Lio/sentry/android/core/IDebugImagesLoader;)V public fun setEnableActivityLifecycleBreadcrumbs (Z)V @@ -247,6 +251,10 @@ public final class io/sentry/android/core/SentryAndroidOptions : io/sentry/Sentr public fun setProfilingTracesIntervalMillis (I)V } +public abstract interface class io/sentry/android/core/SentryAndroidOptions$BeforeCaptureCallback { + public abstract fun execute (Lio/sentry/SentryEvent;Lio/sentry/Hint;Z)Z +} + public final class io/sentry/android/core/SentryInitProvider { public fun ()V public fun attachInfo (Landroid/content/Context;Landroid/content/pm/ProviderInfo;)V diff --git a/sentry-android-core/src/main/java/io/sentry/android/core/ScreenshotEventProcessor.java b/sentry-android-core/src/main/java/io/sentry/android/core/ScreenshotEventProcessor.java index 85cb134778..0812e55015 100644 --- a/sentry-android-core/src/main/java/io/sentry/android/core/ScreenshotEventProcessor.java +++ b/sentry-android-core/src/main/java/io/sentry/android/core/ScreenshotEventProcessor.java @@ -10,6 +10,8 @@ import io.sentry.IntegrationName; import io.sentry.SentryEvent; import io.sentry.SentryLevel; +import io.sentry.android.core.internal.util.AndroidCurrentDateProvider; +import io.sentry.android.core.internal.util.Debouncer; import io.sentry.util.HintUtils; import io.sentry.util.Objects; import org.jetbrains.annotations.ApiStatus; @@ -26,12 +28,17 @@ public final class ScreenshotEventProcessor implements EventProcessor, Integrati private final @NotNull SentryAndroidOptions options; private final @NotNull BuildInfoProvider buildInfoProvider; + private final @NotNull Debouncer debouncer; + private static final long DEBOUNCE_WAIT_TIME_MS = 2000; + public ScreenshotEventProcessor( final @NotNull SentryAndroidOptions options, final @NotNull BuildInfoProvider buildInfoProvider) { this.options = Objects.requireNonNull(options, "SentryAndroidOptions is required"); this.buildInfoProvider = Objects.requireNonNull(buildInfoProvider, "BuildInfoProvider is required"); + this.debouncer = new Debouncer(AndroidCurrentDateProvider.getInstance(), DEBOUNCE_WAIT_TIME_MS); + if (options.isAttachScreenshot()) { addIntegrationToSdkVersion(); } @@ -52,6 +59,19 @@ public ScreenshotEventProcessor( return event; } + // skip capturing in case of debouncing (=too many frequent capture requests) + // the BeforeCaptureCallback may overrules the debouncing decision + final boolean shouldDebounce = debouncer.checkForDebounce(); + final @Nullable SentryAndroidOptions.BeforeCaptureCallback beforeCaptureCallback = + options.getBeforeScreenshotCaptureCallback(); + if (beforeCaptureCallback != null) { + if (!beforeCaptureCallback.execute(event, hint, shouldDebounce)) { + return event; + } + } else if (shouldDebounce) { + return event; + } + final byte[] screenshot = takeScreenshot( activity, options.getMainThreadChecker(), options.getLogger(), buildInfoProvider); diff --git a/sentry-android-core/src/main/java/io/sentry/android/core/SentryAndroidOptions.java b/sentry-android-core/src/main/java/io/sentry/android/core/SentryAndroidOptions.java index 216a3ea137..f28112ca11 100644 --- a/sentry-android-core/src/main/java/io/sentry/android/core/SentryAndroidOptions.java +++ b/sentry-android-core/src/main/java/io/sentry/android/core/SentryAndroidOptions.java @@ -1,8 +1,10 @@ package io.sentry.android.core; +import io.sentry.Hint; import io.sentry.ISpan; import io.sentry.Scope; import io.sentry.Sentry; +import io.sentry.SentryEvent; import io.sentry.SentryOptions; import io.sentry.SpanStatus; import io.sentry.android.core.internal.util.RootChecker; @@ -98,10 +100,18 @@ public final class SentryAndroidOptions extends SentryOptions { /** Interface that loads the debug images list */ private @NotNull IDebugImagesLoader debugImagesLoader = NoOpDebugImagesLoader.getInstance(); - /** Enables or disables the attach screenshot feature when an error happened. */ + /** + * Enables or disables the attach screenshot feature when an error happened. Use {@link + * SentryAndroidOptions#setBeforeScreenshotCaptureCallback(BeforeCaptureCallback)} ()} to control + * when a screenshot should be captured. + */ private boolean attachScreenshot; - /** Enables or disables the attach view hierarchy feature when an error happened. */ + /** + * Enables or disables the attach view hierarchy feature when an error happened. Use {@link + * SentryAndroidOptions#setBeforeViewHierarchyCaptureCallback(BeforeCaptureCallback)} ()} to + * control when a view hierarchy should be captured. + */ private boolean attachViewHierarchy; /** @@ -143,6 +153,35 @@ public final class SentryAndroidOptions extends SentryOptions { */ private boolean enableRootCheck = true; + private @Nullable BeforeCaptureCallback beforeScreenshotCaptureCallback; + + private @Nullable BeforeCaptureCallback beforeViewHierarchyCaptureCallback; + + public interface BeforeCaptureCallback { + + /** + * A callback which can be used to suppress capturing of screenshots or view hierarchies. This + * gives more fine grained control when capturing should be performed. E.g. - only capture + * screenshots for fatal events - overrule any debouncing for important events
+ * As capturing can be resource-intensive, the debounce parameter should be respected if + * possible. + * + * 
+     *  if (debounce) {
+     *    return false;
+     *  } else {
+     *    // check event and hint
+     *  }
+     *
+ * + * @param event the event + * @param hint the hints + * @param debounce true if capturing is marked for being debounced + * @return true if capturing should be performed, false otherwise + */ + boolean execute(@NotNull SentryEvent event, @NotNull Hint hint, boolean debounce); + } + public SentryAndroidOptions() { setSentryClientName(BuildConfig.SENTRY_ANDROID_SDK_NAME + "/" + BuildConfig.VERSION_NAME); setSdkVersion(createSdkVersion()); @@ -441,4 +480,34 @@ public boolean isEnableRootCheck() { public void setEnableRootCheck(final boolean enableRootCheck) { this.enableRootCheck = enableRootCheck; } + + public @Nullable BeforeCaptureCallback getBeforeScreenshotCaptureCallback() { + return beforeScreenshotCaptureCallback; + } + + /** + * Sets a callback which is executed before capturing screenshots. Only relevant if + * attachScreenshot is set to true. + * + * @param beforeScreenshotCaptureCallback the callback to execute + */ + public void setBeforeScreenshotCaptureCallback( + final @NotNull BeforeCaptureCallback beforeScreenshotCaptureCallback) { + this.beforeScreenshotCaptureCallback = beforeScreenshotCaptureCallback; + } + + public @Nullable BeforeCaptureCallback getBeforeViewHierarchyCaptureCallback() { + return beforeViewHierarchyCaptureCallback; + } + + /** + * Sets a callback which is executed before capturing view hierarchies. Only relevant if + * attachViewHierarchy is set to true. + * + * @param beforeViewHierarchyCaptureCallback the callback to execute + */ + public void setBeforeViewHierarchyCaptureCallback( + final @NotNull BeforeCaptureCallback beforeViewHierarchyCaptureCallback) { + this.beforeViewHierarchyCaptureCallback = beforeViewHierarchyCaptureCallback; + } } diff --git a/sentry-android-core/src/main/java/io/sentry/android/core/ViewHierarchyEventProcessor.java b/sentry-android-core/src/main/java/io/sentry/android/core/ViewHierarchyEventProcessor.java index 3cd1874abb..b667e04888 100644 --- a/sentry-android-core/src/main/java/io/sentry/android/core/ViewHierarchyEventProcessor.java +++ b/sentry-android-core/src/main/java/io/sentry/android/core/ViewHierarchyEventProcessor.java @@ -13,7 +13,9 @@ import io.sentry.SentryEvent; import io.sentry.SentryLevel; import io.sentry.android.core.internal.gestures.ViewUtils; +import io.sentry.android.core.internal.util.AndroidCurrentDateProvider; import io.sentry.android.core.internal.util.AndroidMainThreadChecker; +import io.sentry.android.core.internal.util.Debouncer; import io.sentry.internal.viewhierarchy.ViewHierarchyExporter; import io.sentry.protocol.ViewHierarchy; import io.sentry.protocol.ViewHierarchyNode; @@ -35,10 +37,15 @@ public final class ViewHierarchyEventProcessor implements EventProcessor, IntegrationName { private final @NotNull SentryAndroidOptions options; + private final @NotNull Debouncer debouncer; + private static final long CAPTURE_TIMEOUT_MS = 1000; + private static final long DEBOUNCE_WAIT_TIME_MS = 2000; public ViewHierarchyEventProcessor(final @NotNull SentryAndroidOptions options) { this.options = Objects.requireNonNull(options, "SentryAndroidOptions is required"); + this.debouncer = new Debouncer(AndroidCurrentDateProvider.getInstance(), DEBOUNCE_WAIT_TIME_MS); + if (options.isAttachViewHierarchy()) { addIntegrationToSdkVersion(); } @@ -59,6 +66,19 @@ public ViewHierarchyEventProcessor(final @NotNull SentryAndroidOptions options) return event; } + // skip capturing in case of debouncing (=too many frequent capture requests) + // the BeforeCaptureCallback may overrules the debouncing decision + final boolean shouldDebounce = debouncer.checkForDebounce(); + final @Nullable SentryAndroidOptions.BeforeCaptureCallback beforeCaptureCallback = + options.getBeforeViewHierarchyCaptureCallback(); + if (beforeCaptureCallback != null) { + if (!beforeCaptureCallback.execute(event, hint, shouldDebounce)) { + return event; + } + } else if (shouldDebounce) { + return event; + } + final @Nullable Activity activity = CurrentActivityHolder.getInstance().getActivity(); final @Nullable ViewHierarchy viewHierarchy = snapshotViewHierarchy( diff --git a/sentry-android-core/src/main/java/io/sentry/android/core/internal/util/Debouncer.java b/sentry-android-core/src/main/java/io/sentry/android/core/internal/util/Debouncer.java new file mode 100644 index 0000000000..738904df24 --- /dev/null +++ b/sentry-android-core/src/main/java/io/sentry/android/core/internal/util/Debouncer.java @@ -0,0 +1,33 @@ +package io.sentry.android.core.internal.util; + +import io.sentry.transport.ICurrentDateProvider; +import org.jetbrains.annotations.ApiStatus; +import org.jetbrains.annotations.NotNull; + +/** A simple time-based debouncing mechanism */ +@ApiStatus.Internal +public class Debouncer { + + private final long waitTimeMs; + private final @NotNull ICurrentDateProvider timeProvider; + + private Long lastExecutionTime = null; + + public Debouncer(final @NotNull ICurrentDateProvider timeProvider, final long waitTimeMs) { + this.timeProvider = timeProvider; + this.waitTimeMs = waitTimeMs; + } + + /** + * @return true if the execution should be debounced due to the last execution being within within + * waitTimeMs, otherwise false. + */ + public boolean checkForDebounce() { + final long now = timeProvider.getCurrentTimeMillis(); + if (lastExecutionTime == null || (lastExecutionTime + waitTimeMs) <= now) { + lastExecutionTime = now; + return false; + } + return true; + } +} diff --git a/sentry-android-core/src/test/java/io/sentry/android/core/ScreenshotEventProcessorTest.kt b/sentry-android-core/src/test/java/io/sentry/android/core/ScreenshotEventProcessorTest.kt index 60d61394f2..015a0454cf 100644 --- a/sentry-android-core/src/test/java/io/sentry/android/core/ScreenshotEventProcessorTest.kt +++ b/sentry-android-core/src/test/java/io/sentry/android/core/ScreenshotEventProcessorTest.kt @@ -10,6 +10,7 @@ import io.sentry.MainEventProcessor import io.sentry.SentryEvent import io.sentry.SentryIntegrationPackageStorage import io.sentry.TypeCheckHint.ANDROID_ACTIVITY +import io.sentry.protocol.SentryException import io.sentry.util.thread.IMainThreadChecker import org.junit.runner.RunWith import org.mockito.kotlin.any @@ -66,6 +67,7 @@ class ScreenshotEventProcessorTest { @BeforeTest fun `set up`() { fixture = Fixture() + CurrentActivityHolder.getInstance().clearActivity() } @Test @@ -200,5 +202,102 @@ class ScreenshotEventProcessorTest { assertFalse(fixture.options.sdkVersion!!.integrationSet.contains("Screenshot")) } + @Test + fun `when screenshots are captured rapidly, capturing should be debounced`() { + CurrentActivityHolder.getInstance().setActivity(fixture.activity) + + val processor = fixture.getSut(true) + val event = SentryEvent().apply { + exceptions = listOf(SentryException()) + } + val hint0 = Hint() + processor.process(event, hint0) + assertNotNull(hint0.screenshot) + + val hint1 = Hint() + processor.process(event, hint1) + assertNull(hint1.screenshot) + } + + @Test + fun `when screenshots are captured rapidly, debounce flag should be propagated`() { + CurrentActivityHolder.getInstance().setActivity(fixture.activity) + + var debounceFlag = false + fixture.options.setBeforeScreenshotCaptureCallback { _, _, debounce -> + debounceFlag = debounce + true + } + + val processor = fixture.getSut(true) + val event = SentryEvent().apply { + exceptions = listOf(SentryException()) + } + val hint0 = Hint() + processor.process(event, hint0) + assertFalse(debounceFlag) + + val hint1 = Hint() + processor.process(event, hint1) + assertTrue(debounceFlag) + } + + @Test + fun `when screenshots are captured rapidly, capture callback can still overrule debouncing`() { + CurrentActivityHolder.getInstance().setActivity(fixture.activity) + + val processor = fixture.getSut(true) + + fixture.options.setBeforeScreenshotCaptureCallback { _, _, _ -> + true + } + val event = SentryEvent().apply { + exceptions = listOf(SentryException()) + } + val hint0 = Hint() + processor.process(event, hint0) + assertNotNull(hint0.screenshot) + + val hint1 = Hint() + processor.process(event, hint1) + assertNotNull(hint1.screenshot) + } + + @Test + fun `when capture callback returns false, no screenshot should be captured`() { + CurrentActivityHolder.getInstance().setActivity(fixture.activity) + + fixture.options.setBeforeScreenshotCaptureCallback { _, _, _ -> + false + } + val processor = fixture.getSut(true) + + val event = SentryEvent().apply { + exceptions = listOf(SentryException()) + } + val hint = Hint() + + processor.process(event, hint) + assertNull(hint.screenshot) + } + + @Test + fun `when capture callback returns true, a screenshot should be captured`() { + CurrentActivityHolder.getInstance().setActivity(fixture.activity) + + fixture.options.setBeforeViewHierarchyCaptureCallback { _, _, _ -> + true + } + val processor = fixture.getSut(true) + + val event = SentryEvent().apply { + exceptions = listOf(SentryException()) + } + val hint = Hint() + + processor.process(event, hint) + assertNotNull(hint.screenshot) + } + private fun getEvent(): SentryEvent = SentryEvent(Throwable("Throwable")) } diff --git a/sentry-android-core/src/test/java/io/sentry/android/core/ViewHierarchyEventProcessorTest.kt b/sentry-android-core/src/test/java/io/sentry/android/core/ViewHierarchyEventProcessorTest.kt index c00bb08b3e..87c9da8538 100644 --- a/sentry-android-core/src/test/java/io/sentry/android/core/ViewHierarchyEventProcessorTest.kt +++ b/sentry-android-core/src/test/java/io/sentry/android/core/ViewHierarchyEventProcessorTest.kt @@ -315,6 +315,91 @@ class ViewHierarchyEventProcessorTest { assertFalse(fixture.options.sdkVersion!!.integrationSet.contains("ViewHierarchy")) } + @Test + fun `when view hierarchies are captured rapidly, capturing should be debounced`() { + val processor = fixture.getSut(true) + val event = SentryEvent().apply { + exceptions = listOf(SentryException()) + } + val hint0 = Hint() + processor.process(event, hint0) + assertNotNull(hint0.viewHierarchy) + + val hint1 = Hint() + processor.process(event, hint1) + assertNull(hint1.viewHierarchy) + } + + @Test + fun `when view hierarchies are captured rapidly, debounced flag should be propagated`() { + val processor = fixture.getSut(true) + + var debounceFlag = false + fixture.options.setBeforeViewHierarchyCaptureCallback { _, _, debounce -> + debounceFlag = debounce + true + } + val event = SentryEvent().apply { + exceptions = listOf(SentryException()) + } + val hint0 = Hint() + processor.process(event, hint0) + assertFalse(debounceFlag) + + val hint1 = Hint() + processor.process(event, hint1) + assertTrue(debounceFlag) + } + + @Test + fun `when view hierarchies are captured rapidly, capture callback can still overrule debouncing`() { + val processor = fixture.getSut(true) + + fixture.options.setBeforeViewHierarchyCaptureCallback { _, _, _ -> + true + } + val event = SentryEvent().apply { + exceptions = listOf(SentryException()) + } + val hint0 = Hint() + processor.process(event, hint0) + assertNotNull(hint0.viewHierarchy) + + val hint1 = Hint() + processor.process(event, hint1) + assertNotNull(hint1.viewHierarchy) + } + + @Test + fun `when capture callback returns false, no view hierarchy should be captured`() { + fixture.options.setBeforeViewHierarchyCaptureCallback { _, _, _ -> + false + } + val (_, hint) = fixture.process( + true, + SentryEvent().apply { + exceptions = listOf(SentryException()) + } + ) + + assertNull(hint.viewHierarchy) + } + + @Test + fun `when capture callback returns true, a view hierarchy should be captured`() { + fixture.options.setBeforeViewHierarchyCaptureCallback { _, _, _ -> + true + } + val (_, hint) = fixture.process( + true, + SentryEvent().apply { + exceptions = listOf(SentryException()) + } + ) + + assertNotNull(hint.viewHierarchy) + } + private fun mockedView( x: Float, y: Float, diff --git a/sentry-android-core/src/test/java/io/sentry/android/core/internal/util/DebouncerTest.kt b/sentry-android-core/src/test/java/io/sentry/android/core/internal/util/DebouncerTest.kt new file mode 100644 index 0000000000..37d64ac508 --- /dev/null +++ b/sentry-android-core/src/test/java/io/sentry/android/core/internal/util/DebouncerTest.kt @@ -0,0 +1,62 @@ +package io.sentry.android.core.internal.util + +import io.sentry.transport.ICurrentDateProvider +import org.junit.Assert.assertFalse +import org.junit.Assert.assertTrue +import kotlin.test.BeforeTest +import kotlin.test.Test + +class DebouncerTest { + + private class Fixture : ICurrentDateProvider { + + var currentTimeMs: Long = 0 + + override fun getCurrentTimeMillis(): Long = currentTimeMs + + fun getDebouncer(waitTimeMs: Long = 3000): Debouncer { + return Debouncer(this, waitTimeMs) + } + } + + private val fixture = Fixture() + + @BeforeTest + fun `set up`() { + fixture.currentTimeMs = 0 + } + + @Test + fun `Debouncer should not debounce on the first check`() { + val debouncer = fixture.getDebouncer() + assertFalse(debouncer.checkForDebounce()) + } + + @Test + fun `Debouncer should not debounce if wait time is 0`() { + val debouncer = fixture.getDebouncer(0) + assertFalse(debouncer.checkForDebounce()) + assertFalse(debouncer.checkForDebounce()) + assertFalse(debouncer.checkForDebounce()) + } + + @Test + fun `Debouncer should signal debounce if the second invocation is too early`() { + fixture.currentTimeMs = 1000 + val debouncer = fixture.getDebouncer(3000) + assertFalse(debouncer.checkForDebounce()) + + fixture.currentTimeMs = 3999 + assertTrue(debouncer.checkForDebounce()) + } + + @Test + fun `Debouncer should not signal debounce if the second invocation is late enough`() { + fixture.currentTimeMs = 1000 + val debouncer = fixture.getDebouncer(3000) + assertFalse(debouncer.checkForDebounce()) + + fixture.currentTimeMs = 4000 + assertFalse(debouncer.checkForDebounce()) + } +}