improve default debouncing mechanism (#2945)

* improve default debouncing mechanism * added execution counter to Debouncer
getsentry · Sep 29, 2023 · 1a21cf4 · 1a21cf4
1 parent 909430d
commit 1a21cf4
Show file tree

Hide file tree

Showing 9 changed files with 119 additions and 12 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -4,6 +4,7 @@
 
 ### Features
 
+- Improve default debouncing mechanism ([#2945](https://github.com/getsentry/sentry-java/pull/2945))
 - Add `sendModules` option for disable sending modules ([#2926](https://github.com/getsentry/sentry-java/pull/2926))
 - Send `db.system` and `db.name` in span data for androidx.sqlite spans ([#2928](https://github.com/getsentry/sentry-java/pull/2928))
 - Check-ins (CRONS) support ([#2952](https://github.com/getsentry/sentry-java/pull/2952))

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
@@ -30,14 +30,19 @@ public final class ScreenshotEventProcessor implements EventProcessor, Integrati
 
   private final @NotNull Debouncer debouncer;
   private static final long DEBOUNCE_WAIT_TIME_MS = 2000;
+  private static final int DEBOUNCE_MAX_EXECUTIONS = 3;
 
   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);
+    this.debouncer =
+        new Debouncer(
+            AndroidCurrentDateProvider.getInstance(),
+            DEBOUNCE_WAIT_TIME_MS,
+            DEBOUNCE_MAX_EXECUTIONS);
 
     if (options.isAttachScreenshot()) {
       addIntegrationToSdkVersion();

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
@@ -41,10 +41,15 @@ public final class ViewHierarchyEventProcessor implements EventProcessor, Integr
 
   private static final long CAPTURE_TIMEOUT_MS = 1000;
   private static final long DEBOUNCE_WAIT_TIME_MS = 2000;
+  private static final int DEBOUNCE_MAX_EXECUTIONS = 3;
 
   public ViewHierarchyEventProcessor(final @NotNull SentryAndroidOptions options) {
     this.options = Objects.requireNonNull(options, "SentryAndroidOptions is required");
-    this.debouncer = new Debouncer(AndroidCurrentDateProvider.getInstance(), DEBOUNCE_WAIT_TIME_MS);
+    this.debouncer =
+        new Debouncer(
+            AndroidCurrentDateProvider.getInstance(),
+            DEBOUNCE_WAIT_TIME_MS,
+            DEBOUNCE_MAX_EXECUTIONS);
 
     if (options.isAttachViewHierarchy()) {
       addIntegrationToSdkVersion();

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
@@ -1,6 +1,8 @@
 package io.sentry.android.core.internal.util;
 
 import io.sentry.transport.ICurrentDateProvider;
+import java.util.concurrent.atomic.AtomicInteger;
+import java.util.concurrent.atomic.AtomicLong;
 import org.jetbrains.annotations.ApiStatus;
 import org.jetbrains.annotations.NotNull;
 
@@ -10,24 +12,35 @@ public class Debouncer {
 
   private final long waitTimeMs;
   private final @NotNull ICurrentDateProvider timeProvider;
+  private final @NotNull AtomicInteger executions = new AtomicInteger(0);
+  private final int maxExecutions;
 
-  private Long lastExecutionTime = null;
+  private final @NotNull AtomicLong lastExecutionTime = new AtomicLong(0);
 
-  public Debouncer(final @NotNull ICurrentDateProvider timeProvider, final long waitTimeMs) {
+  public Debouncer(
+      final @NotNull ICurrentDateProvider timeProvider,
+      final long waitTimeMs,
+      final int maxExecutions) {
     this.timeProvider = timeProvider;
     this.waitTimeMs = waitTimeMs;
+    this.maxExecutions = maxExecutions <= 0 ? 1 : maxExecutions;
   }
 
   /**
-   * @return true if the execution should be debounced due to the last execution being within within
-   *     waitTimeMs, otherwise false.
+   * @return true if the execution should be debounced due to maxExecutions executions being made
+   *     within waitTimeMs, otherwise false.
    */
   public boolean checkForDebounce() {
     final long now = timeProvider.getCurrentTimeMillis();
-    if (lastExecutionTime == null || (lastExecutionTime + waitTimeMs) <= now) {
-      lastExecutionTime = now;
+    if (lastExecutionTime.get() == 0 || (lastExecutionTime.get() + waitTimeMs) <= now) {
+      executions.set(0);
+      lastExecutionTime.set(now);
       return false;
     }
+    if (executions.incrementAndGet() < maxExecutions) {
+      return false;
+    }
+    executions.set(0);
     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
@@ -210,7 +210,13 @@ class ScreenshotEventProcessorTest {
         val event = SentryEvent().apply {
             exceptions = listOf(SentryException())
         }
-        val hint0 = Hint()
+        var hint0 = Hint()
+        processor.process(event, hint0)
+        assertNotNull(hint0.screenshot)
+        hint0 = Hint()
+        processor.process(event, hint0)
+        assertNotNull(hint0.screenshot)
+        hint0 = Hint()
         processor.process(event, hint0)
         assertNotNull(hint0.screenshot)
 
@@ -236,6 +242,10 @@ class ScreenshotEventProcessorTest {
         val hint0 = Hint()
         processor.process(event, hint0)
         assertFalse(debounceFlag)
+        processor.process(event, hint0)
+        assertFalse(debounceFlag)
+        processor.process(event, hint0)
+        assertFalse(debounceFlag)
 
         val hint1 = Hint()
         processor.process(event, hint1)
@@ -256,6 +266,8 @@ class ScreenshotEventProcessorTest {
         }
         val hint0 = Hint()
         processor.process(event, hint0)
+        processor.process(event, hint0)
+        processor.process(event, hint0)
         assertNotNull(hint0.screenshot)
 
         val hint1 = Hint()

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
@@ -321,7 +321,13 @@ class ViewHierarchyEventProcessorTest {
         val event = SentryEvent().apply {
             exceptions = listOf(SentryException())
         }
-        val hint0 = Hint()
+        var hint0 = Hint()
+        processor.process(event, hint0)
+        assertNotNull(hint0.viewHierarchy)
+        hint0 = Hint()
+        processor.process(event, hint0)
+        assertNotNull(hint0.viewHierarchy)
+        hint0 = Hint()
         processor.process(event, hint0)
         assertNotNull(hint0.viewHierarchy)
 
@@ -345,6 +351,10 @@ class ViewHierarchyEventProcessorTest {
         val hint0 = Hint()
         processor.process(event, hint0)
         assertFalse(debounceFlag)
+        processor.process(event, hint0)
+        assertFalse(debounceFlag)
+        processor.process(event, hint0)
+        assertFalse(debounceFlag)
 
         val hint1 = Hint()
         processor.process(event, hint1)
@@ -363,6 +373,8 @@ class ViewHierarchyEventProcessorTest {
         }
         val hint0 = Hint()
         processor.process(event, hint0)
+        processor.process(event, hint0)
+        processor.process(event, hint0)
         assertNotNull(hint0.viewHierarchy)
 
         val hint1 = Hint()

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
@@ -14,8 +14,8 @@ class DebouncerTest {
 
         override fun getCurrentTimeMillis(): Long = currentTimeMs
 
-        fun getDebouncer(waitTimeMs: Long = 3000): Debouncer {
-            return Debouncer(this, waitTimeMs)
+        fun getDebouncer(waitTimeMs: Long = 3000, maxExecutions: Int = 1): Debouncer {
+            return Debouncer(this, waitTimeMs, maxExecutions)
         }
     }
 
@@ -59,4 +59,48 @@ class DebouncerTest {
         fixture.currentTimeMs = 4000
         assertFalse(debouncer.checkForDebounce())
     }
+
+    @Test
+    fun `Debouncer maxExecutions is always greater than 0`() {
+        fixture.currentTimeMs = 1000
+        val debouncer = fixture.getDebouncer(3000, -1)
+        assertFalse(debouncer.checkForDebounce())
+        assertTrue(debouncer.checkForDebounce())
+    }
+
+    @Test
+    fun `Debouncer should signal debounce after maxExecutions calls`() {
+        fixture.currentTimeMs = 1000
+        val debouncer = fixture.getDebouncer(3000, 3)
+        assertFalse(debouncer.checkForDebounce())
+        assertFalse(debouncer.checkForDebounce())
+        assertFalse(debouncer.checkForDebounce())
+        assertTrue(debouncer.checkForDebounce())
+    }
+
+    @Test
+    fun `Debouncer maxExecutions counter resets if the other invocation is late enough`() {
+        fixture.currentTimeMs = 1000
+        val debouncer = fixture.getDebouncer(3000, 3)
+        assertFalse(debouncer.checkForDebounce())
+        assertFalse(debouncer.checkForDebounce())
+
+        // After waitTimeMs passes, the maxExecutions counter is reset
+        fixture.currentTimeMs = 4000
+        assertFalse(debouncer.checkForDebounce())
+        assertFalse(debouncer.checkForDebounce())
+        assertFalse(debouncer.checkForDebounce())
+        assertTrue(debouncer.checkForDebounce())
+    }
+
+    @Test
+    fun `Debouncer maxExecutions counter resets after maxExecutions`() {
+        fixture.currentTimeMs = 1000
+        val debouncer = fixture.getDebouncer(3000, 3)
+        assertFalse(debouncer.checkForDebounce())
+        assertFalse(debouncer.checkForDebounce())
+        assertFalse(debouncer.checkForDebounce())
+        assertTrue(debouncer.checkForDebounce())
+        assertFalse(debouncer.checkForDebounce())
+    }
 }
diff --git a/sentry/src/test/java/io/sentry/NoOpHubTest.kt b/sentry/src/test/java/io/sentry/NoOpHubTest.kt
@@ -84,4 +84,14 @@ class NoOpHubTest {
 
     @Test
     fun `reportFullyDrawn doesnt throw`() = sut.reportFullyDisplayed()
+
+    @Test
+    fun `getBaggage returns null`() {
+        assertNull(sut.baggage)
+    }
+
+    @Test
+    fun `captureCheckIn returns empty id`() {
+        assertEquals(SentryId.EMPTY_ID, sut.captureCheckIn(mock()))
+    }
 }
diff --git a/sentry/src/test/java/io/sentry/NoOpSentryClientTest.kt b/sentry/src/test/java/io/sentry/NoOpSentryClientTest.kt
@@ -50,4 +50,9 @@ class NoOpSentryClientTest {
     @Test
     fun `captureTransaction returns empty SentryId`() =
         assertEquals(SentryId.EMPTY_ID, sut.captureTransaction(mock(), mock()))
+
+    @Test
+    fun `captureCheckIn returns empty id`() {
+        assertEquals(SentryId.EMPTY_ID, sut.captureCheckIn(mock(), mock(), mock()))
+    }
 }