Skip to content

Commit d503d47

Browse files
runningcodeclaude
andcommitted
ref(core): Measure the performance-collection budget on a monotonic ticker (JAVA-579)
DefaultCompositePerformanceCollector decided a transaction had been collecting for 30s by subtracting two options.getDateProvider() readings. Those are wall-clock on every platform, including Android: SentryNanotimeDate.nanoTimestamp() returns millisToNanos(unixDateMillis), not its nanoTime component, so the extra precision that type exists for never entered this comparison. A device time change therefore ended collection early or kept it running past the budget. Each CompositeData now holds a Deadline on a MonotonicTicker. The budget is not a serialized value, so this only changes when the collector stops. Two side effects worth review: the 30s boundary is now inclusive, where the old strict `>` let a sample land exactly at 30s; and addDataAndCheckTimeout no longer takes the shared clock reading, since each transaction owns its own deadline. TestMonotonicTicker's field is now volatile, as the timer thread reads a ticker the test thread advances. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent 7bf5574 commit d503d47

3 files changed

Lines changed: 35 additions & 37 deletions

File tree

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@ import java.util.concurrent.TimeUnit
99
* nanosecond ticker is off by a factor of a million and still compiles, whereas `advance(1001,
1010
* MILLISECONDS)` cannot be.
1111
*/
12-
class TestMonotonicTicker(private var nanos: Long = 0) : MonotonicTicker {
12+
class TestMonotonicTicker(@Volatile private var nanos: Long = 0) : MonotonicTicker {
1313
override fun tickNanos(): Long = nanos
1414

1515
fun advance(amount: Long, unit: TimeUnit) {

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

Lines changed: 19 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,7 @@
11
package io.sentry;
22

3+
import io.sentry.time.Deadline;
4+
import io.sentry.time.MonotonicTicker;
35
import io.sentry.util.AutoClosableReentrantLock;
46
import io.sentry.util.Objects;
57
import java.util.ArrayList;
@@ -26,10 +28,19 @@ public final class DefaultCompositePerformanceCollector implements CompositePerf
2628
private final boolean hasNoCollectors;
2729

2830
private final @NotNull SentryOptions options;
31+
private final @NotNull MonotonicTicker ticker;
2932
private final @NotNull AtomicBoolean isStarted = new AtomicBoolean(false);
3033

3134
public DefaultCompositePerformanceCollector(final @NotNull SentryOptions options) {
35+
this(
36+
Objects.requireNonNull(options, "The options object is required."),
37+
options.getMonotonicTicker());
38+
}
39+
40+
DefaultCompositePerformanceCollector(
41+
final @NotNull SentryOptions options, final @NotNull MonotonicTicker ticker) {
3242
this.options = Objects.requireNonNull(options, "The options object is required.");
43+
this.ticker = Objects.requireNonNull(ticker, "The ticker is required.");
3344
this.snapshotCollectors = new ArrayList<>();
3445
this.continuousCollectors = new ArrayList<>();
3546

@@ -124,7 +135,7 @@ public void run() {
124135
// Add the enriched tempData to all transactions/profiles/objects that collect data.
125136
// Then Check if that object timed out.
126137
for (CompositeData data : compositeDataMap.values()) {
127-
if (data.addDataAndCheckTimeout(tempData, tempData.getNanoTimestamp())) {
138+
if (data.addDataAndCheckTimeout(tempData)) {
128139
// timed out
129140
if (data.transaction != null) {
130141
timedOutTransactions.add(data.transaction);
@@ -212,33 +223,30 @@ public void close() {
212223
private class CompositeData {
213224
private final @NotNull List<PerformanceCollectionData> dataList;
214225
private final @Nullable ITransaction transaction;
215-
private final long startTimestamp;
226+
private final @NotNull Deadline collectUntil;
216227

217228
private CompositeData(final @Nullable ITransaction transaction) {
218229
this.dataList = new ArrayList<>();
219230
this.transaction = transaction;
220-
this.startTimestamp = options.getDateProvider().now().nanoTimestamp();
231+
// On a ticker rather than the date provider: this is how long we have been collecting, and
232+
// a device time change must not end a collection early or keep a finished one going.
233+
this.collectUntil =
234+
Deadline.after(ticker, TRANSACTION_COLLECTION_TIMEOUT_MILLIS, TimeUnit.MILLISECONDS);
221235
}
222236

223237
/**
224238
* Adds the data to the internal list of PerformanceCollectionData. Then it checks if data
225239
* collection timed out (for transactions only).
226240
*
227-
* @param nowNanos the timestamp of the current collection, passed in so a single clock reading
228-
* is shared by every transaction in this collection round.
229241
* @return true if data collection timed out (for transactions only).
230242
*/
231-
boolean addDataAndCheckTimeout(
232-
final @NotNull PerformanceCollectionData data, final long nowNanos) {
243+
boolean addDataAndCheckTimeout(final @NotNull PerformanceCollectionData data) {
233244
// stop() hands dataList out while this timer thread may still be writing to it, so consumers
234245
// synchronize on the list while iterating. We must hold the same monitor here.
235246
synchronized (dataList) {
236247
dataList.add(data);
237248
}
238-
return transaction != null
239-
&& nowNanos
240-
> startTimestamp
241-
+ TimeUnit.MILLISECONDS.toNanos(TRANSACTION_COLLECTION_TIMEOUT_MILLIS);
249+
return transaction != null && collectUntil.hasPassed();
242250
}
243251
}
244252
}

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

Lines changed: 15 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ package io.sentry
33
import io.sentry.test.getCtor
44
import io.sentry.test.getProperty
55
import io.sentry.test.injectForField
6+
import io.sentry.time.TestMonotonicTicker
67
import io.sentry.util.thread.ThreadChecker
78
import java.util.Timer
89
import java.util.concurrent.TimeUnit
@@ -34,6 +35,7 @@ class DefaultCompositePerformanceCollectorTest {
3435
val id1 = "id1"
3536
val scopes: IScopes = mock()
3637
val options = SentryOptions()
38+
val ticker = TestMonotonicTicker()
3739
var mockTimer: Timer? = null
3840

3941
val mockCpuCollector: IPerformanceSnapshotCollector =
@@ -65,7 +67,7 @@ class DefaultCompositePerformanceCollectorTest {
6567
optionsConfiguration.configure(options)
6668
transaction1 = SentryTracer(TransactionContext("", ""), scopes)
6769
transaction2 = SentryTracer(TransactionContext("", ""), scopes)
68-
val collector = DefaultCompositePerformanceCollector(options)
70+
val collector = DefaultCompositePerformanceCollector(options, ticker)
6971
val timer: Timer = collector.getProperty("timer") ?: Timer(true)
7072
mockTimer = spy(timer)
7173
collector.injectForField("timer", mockTimer)
@@ -183,26 +185,19 @@ class DefaultCompositePerformanceCollectorTest {
183185

184186
@Test
185187
fun `collector times out after 30 seconds`() {
186-
val mockDateProvider = mock<SentryDateProvider>()
187188
val mockCollector = mock<IPerformanceContinuousCollector>()
188-
val dates =
189-
listOf(
190-
SentryNanotimeDate(TimeUnit.SECONDS.toMillis(100), TimeUnit.SECONDS.toNanos(100)),
191-
SentryNanotimeDate(TimeUnit.SECONDS.toMillis(131), TimeUnit.SECONDS.toNanos(131)),
192-
)
193-
whenever(mockDateProvider.now()).thenReturn(dates[0], dates[0], dates[0], dates[1])
194-
val collector = fixture.getSut {
195-
it.dateProvider = mockDateProvider
196-
it.addPerformanceCollector(mockCollector)
197-
}
189+
val collector = fixture.getSut { it.addPerformanceCollector(mockCollector) }
198190
collector.start(fixture.transaction1)
199191
verify(fixture.mockTimer, never())!!.cancel()
200192

193+
// 31 seconds of collecting have gone by
194+
fixture.ticker.advance(31, TimeUnit.SECONDS)
195+
201196
// Let's sleep to make the collector get values
202197
Thread.sleep(300)
203198

204-
// When the collector gets the values, it checks the current date, set 31 seconds after the
205-
// begin. This means it should stop itself
199+
// When the collector gets the values, it checks how long it has been collecting for, which is
200+
// now past the 30 second budget. This means it should stop itself
206201
verify(fixture.mockTimer)!!.cancel()
207202

208203
// When the collector times out, the data collection for spans is stopped, too
@@ -214,23 +209,18 @@ class DefaultCompositePerformanceCollectorTest {
214209
}
215210

216211
@Test
217-
fun `collector collects for 30 seconds`() {
218-
val mockDateProvider = mock<SentryDateProvider>()
219-
val dates =
220-
listOf(
221-
SentryNanotimeDate(TimeUnit.SECONDS.toMillis(100), TimeUnit.SECONDS.toNanos(100)),
222-
SentryNanotimeDate(TimeUnit.SECONDS.toMillis(130), TimeUnit.SECONDS.toNanos(130)),
223-
)
224-
whenever(mockDateProvider.now()).thenReturn(dates[0], dates[0], dates[0], dates[1])
225-
val collector = fixture.getSut { it.dateProvider = mockDateProvider }
212+
fun `collector keeps collecting while inside the 30 second budget`() {
213+
val collector = fixture.getSut()
226214
collector.start(fixture.transaction1)
227215
verify(fixture.mockTimer, never())!!.cancel()
228216

217+
// 29 seconds of collecting have gone by
218+
fixture.ticker.advance(29, TimeUnit.SECONDS)
219+
229220
// Let's sleep to make the collector get values
230221
Thread.sleep(300)
231222

232-
// When the collector gets the values, it checks the current date, set 30 seconds after the
233-
// begin. This means it should continue without being cancelled
223+
// Still inside the 30 second budget, so it should continue without being cancelled
234224
verify(fixture.mockTimer, never())!!.cancel()
235225

236226
// Data is deleted after the collector times out

0 commit comments

Comments
 (0)