ref(core): Measure the performance-collection budget on a monotonic ticker (JAVA-579)

CHANGELOG.md

@@ -1,5 +1,11 @@
 # Changelog
 
+## Unreleased
+
+### Internal
+
+- Measure the 30 second performance-collection budget on a monotonic ticker, so that a device time change no longer ends collection early or extends it past the budget ([#6101](https://github.com/getsentry/sentry-java/pull/6101))
+
 ## 8.56.0
 
 ### Behavioral Changes

sentry-test-support/src/main/kotlin/io/sentry/time/TestMonotonicTicker.kt

@@ -9,7 +9,7 @@ import java.util.concurrent.TimeUnit
  * nanosecond ticker is off by a factor of a million and still compiles, whereas `advance(1001,
  * MILLISECONDS)` cannot be.
  */
-class TestMonotonicTicker(private var nanos: Long = 0) : MonotonicTicker {
+class TestMonotonicTicker(@Volatile private var nanos: Long = 0) : MonotonicTicker {
   override fun tickNanos(): Long = nanos
 
   fun advance(amount: Long, unit: TimeUnit) {

sentry/src/main/java/io/sentry/DefaultCompositePerformanceCollector.java

@@ -1,5 +1,7 @@
 package io.sentry;
 
+import io.sentry.time.Deadline;
+import io.sentry.time.MonotonicTicker;
 import io.sentry.util.AutoClosableReentrantLock;
 import io.sentry.util.Objects;
 import java.util.ArrayList;
@@ -26,10 +28,19 @@ public final class DefaultCompositePerformanceCollector implements CompositePerf
   private final boolean hasNoCollectors;
 
   private final @NotNull SentryOptions options;
+  private final @NotNull MonotonicTicker ticker;
   private final @NotNull AtomicBoolean isStarted = new AtomicBoolean(false);
 
   public DefaultCompositePerformanceCollector(final @NotNull SentryOptions options) {
+    this(
+        Objects.requireNonNull(options, "The options object is required."),
+        options.getMonotonicTicker());
+  }
+
+  DefaultCompositePerformanceCollector(
+      final @NotNull SentryOptions options, final @NotNull MonotonicTicker ticker) {
     this.options = Objects.requireNonNull(options, "The options object is required.");
+    this.ticker = Objects.requireNonNull(ticker, "The ticker is required.");
     this.snapshotCollectors = new ArrayList<>();
     this.continuousCollectors = new ArrayList<>();
 
@@ -124,7 +135,7 @@ public void run() {
                 // Add the enriched tempData to all transactions/profiles/objects that collect data.
                 // Then Check if that object timed out.
                 for (CompositeData data : compositeDataMap.values()) {
-                  if (data.addDataAndCheckTimeout(tempData, tempData.getNanoTimestamp())) {
+                  if (data.addDataAndCheckTimeout(tempData)) {
                     // timed out
                     if (data.transaction != null) {
                       timedOutTransactions.add(data.transaction);
@@ -212,33 +223,30 @@ public void close() {
   private class CompositeData {
     private final @NotNull List<PerformanceCollectionData> dataList;
     private final @Nullable ITransaction transaction;
-    private final long startTimestamp;
+    private final @NotNull Deadline collectUntil;
 
     private CompositeData(final @Nullable ITransaction transaction) {
       this.dataList = new ArrayList<>();
       this.transaction = transaction;
-      this.startTimestamp = options.getDateProvider().now().nanoTimestamp();
+      // On a ticker rather than the date provider: this is how long we have been collecting, and
+      // a device time change must not end a collection early or keep a finished one going.
+      this.collectUntil =
+          Deadline.after(ticker, TRANSACTION_COLLECTION_TIMEOUT_MILLIS, TimeUnit.MILLISECONDS);
     }
 
     /**
      * Adds the data to the internal list of PerformanceCollectionData. Then it checks if data
      * collection timed out (for transactions only).
      *
-     * @param nowNanos the timestamp of the current collection, passed in so a single clock reading
-     *     is shared by every transaction in this collection round.
      * @return true if data collection timed out (for transactions only).
      */
-    boolean addDataAndCheckTimeout(
-        final @NotNull PerformanceCollectionData data, final long nowNanos) {
+    boolean addDataAndCheckTimeout(final @NotNull PerformanceCollectionData data) {
       // stop() hands dataList out while this timer thread may still be writing to it, so consumers
       // synchronize on the list while iterating. We must hold the same monitor here.
       synchronized (dataList) {
         dataList.add(data);
       }
-      return transaction != null
-          && nowNanos
-              > startTimestamp
-                  + TimeUnit.MILLISECONDS.toNanos(TRANSACTION_COLLECTION_TIMEOUT_MILLIS);
+      return transaction != null && collectUntil.hasPassed();
     }
   }
 }

sentry/src/test/java/io/sentry/DefaultCompositePerformanceCollectorTest.kt

@@ -3,6 +3,7 @@ package io.sentry
 import io.sentry.test.getCtor
 import io.sentry.test.getProperty
 import io.sentry.test.injectForField
+import io.sentry.time.TestMonotonicTicker
 import io.sentry.util.thread.ThreadChecker
 import java.util.Timer
 import java.util.concurrent.TimeUnit
@@ -34,6 +35,7 @@ class DefaultCompositePerformanceCollectorTest {
     val id1 = "id1"
     val scopes: IScopes = mock()
     val options = SentryOptions()
+    val ticker = TestMonotonicTicker()
     var mockTimer: Timer? = null
 
     val mockCpuCollector: IPerformanceSnapshotCollector =
@@ -65,7 +67,7 @@ class DefaultCompositePerformanceCollectorTest {
       optionsConfiguration.configure(options)
       transaction1 = SentryTracer(TransactionContext("", ""), scopes)
       transaction2 = SentryTracer(TransactionContext("", ""), scopes)
-      val collector = DefaultCompositePerformanceCollector(options)
+      val collector = DefaultCompositePerformanceCollector(options, ticker)
       val timer: Timer = collector.getProperty("timer") ?: Timer(true)
       mockTimer = spy(timer)
       collector.injectForField("timer", mockTimer)
@@ -183,26 +185,19 @@ class DefaultCompositePerformanceCollectorTest {
 
   @Test
   fun `collector times out after 30 seconds`() {
-    val mockDateProvider = mock<SentryDateProvider>()
     val mockCollector = mock<IPerformanceContinuousCollector>()
-    val dates =
-      listOf(
-        SentryNanotimeDate(TimeUnit.SECONDS.toMillis(100), TimeUnit.SECONDS.toNanos(100)),
-        SentryNanotimeDate(TimeUnit.SECONDS.toMillis(131), TimeUnit.SECONDS.toNanos(131)),
-      )
-    whenever(mockDateProvider.now()).thenReturn(dates[0], dates[0], dates[0], dates[1])
-    val collector = fixture.getSut {
-      it.dateProvider = mockDateProvider
-      it.addPerformanceCollector(mockCollector)
-    }
+    val collector = fixture.getSut { it.addPerformanceCollector(mockCollector) }
     collector.start(fixture.transaction1)
     verify(fixture.mockTimer, never())!!.cancel()
 
+    // 31 seconds of collecting have gone by
+    fixture.ticker.advance(31, TimeUnit.SECONDS)
+
     // Let's sleep to make the collector get values
     Thread.sleep(300)
 
-    // When the collector gets the values, it checks the current date, set 31 seconds after the
-    // begin. This means it should stop itself
+    // When the collector gets the values, it checks how long it has been collecting for, which is
+    // now past the 30 second budget. This means it should stop itself
     verify(fixture.mockTimer)!!.cancel()
 
     // When the collector times out, the data collection for spans is stopped, too
@@ -214,23 +209,18 @@ class DefaultCompositePerformanceCollectorTest {
   }
 
   @Test
-  fun `collector collects for 30 seconds`() {
-    val mockDateProvider = mock<SentryDateProvider>()
-    val dates =
-      listOf(
-        SentryNanotimeDate(TimeUnit.SECONDS.toMillis(100), TimeUnit.SECONDS.toNanos(100)),
-        SentryNanotimeDate(TimeUnit.SECONDS.toMillis(130), TimeUnit.SECONDS.toNanos(130)),
-      )
-    whenever(mockDateProvider.now()).thenReturn(dates[0], dates[0], dates[0], dates[1])
-    val collector = fixture.getSut { it.dateProvider = mockDateProvider }
+  fun `collector keeps collecting while inside the 30 second budget`() {
+    val collector = fixture.getSut()
     collector.start(fixture.transaction1)
     verify(fixture.mockTimer, never())!!.cancel()
 
+    // 29 seconds of collecting have gone by
+    fixture.ticker.advance(29, TimeUnit.SECONDS)
+
     // Let's sleep to make the collector get values
     Thread.sleep(300)
 
-    // When the collector gets the values, it checks the current date, set 30 seconds after the
-    // begin. This means it should continue without being cancelled
+    // Still inside the 30 second budget, so it should continue without being cancelled
     verify(fixture.mockTimer, never())!!.cancel()
 
     // Data is deleted after the collector times out