Skip to content

Commit 5d8c793

Browse files
runningcodeclaude
runningcode
and
claude
committed
perf: Collect performance data on shared timer executor (JAVA-653)
DefaultCompositePerformanceCollector created a java.util.Timer thread for every burst of transactions and destroyed it when the last one finished. Schedule the periodic collection on the shared timer executor instead: each run reschedules itself, and a generation counter prevents an in-flight run from rescheduling after the collection was stopped. This removes both the extra thread and the per-transaction thread churn. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 parent ae3e4c7 commit 5d8c793

2 files changed

Lines changed: 133 additions & 96 deletions

File tree

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

Lines changed: 85 additions & 59 deletions
Original file line numberDiff line numberDiff line change
@@ -5,9 +5,8 @@
55
import java.util.ArrayList;
66
import java.util.List;
77
import java.util.Map;
8-
import java.util.Timer;
9-
import java.util.TimerTask;
108
import java.util.concurrent.ConcurrentHashMap;
9+
import java.util.concurrent.Future;
1110
import java.util.concurrent.TimeUnit;
1211
import java.util.concurrent.atomic.AtomicBoolean;
1312
import org.jetbrains.annotations.ApiStatus;
@@ -19,7 +18,14 @@ public final class DefaultCompositePerformanceCollector implements CompositePerf
1918
private static final long TRANSACTION_COLLECTION_INTERVAL_MILLIS = 100;
2019
private static final long TRANSACTION_COLLECTION_TIMEOUT_MILLIS = 30000;
2120
private final @NotNull AutoClosableReentrantLock timerLock = new AutoClosableReentrantLock();
22-
private volatile @Nullable Timer timer = null;
21+
private volatile @Nullable Future<?> collectFuture = null;
22+
23+
/**
24+
* Incremented on close() so an in-flight collect run doesn't reschedule itself after its chain
25+
* was cancelled. Guarded by timerLock.
26+
*/
27+
private long generation = 0;
28+
2329
private final @NotNull Map<String, CompositeData> compositeDataMap = new ConcurrentHashMap<>();
2430
private final @NotNull List<IPerformanceSnapshotCollector> snapshotCollectors;
2531
private final @NotNull List<IPerformanceContinuousCollector> continuousCollectors;
@@ -71,6 +77,7 @@ public void start(final @NotNull ITransaction transaction) {
7177
}
7278

7379
@Override
80+
@SuppressWarnings("FutureReturnValueIgnored")
7481
public void start(final @NotNull String id) {
7582
if (hasNoCollectors) {
7683
options
@@ -88,60 +95,78 @@ public void start(final @NotNull String id) {
8895
}
8996
if (!isStarted.getAndSet(true)) {
9097
try (final @NotNull ISentryLifecycleToken ignored = timerLock.acquire()) {
91-
if (timer == null) {
92-
timer = new Timer(true);
93-
}
94-
// We schedule the timer to call setup() on collectors immediately in the background.
95-
timer.schedule(
96-
new TimerTask() {
97-
@Override
98-
public void run() {
99-
for (IPerformanceSnapshotCollector collector : snapshotCollectors) {
100-
collector.setup();
101-
}
102-
}
103-
},
104-
0L);
105-
// We schedule the timer to start after a delay, so we let some time pass between setup()
106-
// and collect() calls.
107-
// This way ICollectors that collect average stats based on time intervals, like
108-
// AndroidCpuCollector, can have an actual time interval to evaluate.
109-
final @NotNull List<ITransaction> timedOutTransactions = new ArrayList<>();
110-
TimerTask timerTask =
111-
new TimerTask() {
112-
@Override
113-
public void run() {
114-
timedOutTransactions.clear();
115-
116-
final @NotNull PerformanceCollectionData tempData =
117-
new PerformanceCollectionData(options.getDateProvider().now().nanoTimestamp());
118-
119-
// Enrich tempData using collectors
120-
for (IPerformanceSnapshotCollector collector : snapshotCollectors) {
121-
collector.collect(tempData);
122-
}
123-
124-
// Add the enriched tempData to all transactions/profiles/objects that collect data.
125-
// Then Check if that object timed out.
126-
for (CompositeData data : compositeDataMap.values()) {
127-
if (data.addDataAndCheckTimeout(tempData)) {
128-
// timed out
129-
if (data.transaction != null) {
130-
timedOutTransactions.add(data.transaction);
98+
final long currentGeneration = generation;
99+
try {
100+
// We schedule the executor to call setup() on collectors immediately in the background.
101+
options
102+
.getTimerExecutorService()
103+
.schedule(
104+
() -> {
105+
for (IPerformanceSnapshotCollector collector : snapshotCollectors) {
106+
collector.setup();
131107
}
132-
}
133-
}
134-
// Stop timed out transactions outside compositeDataMap loop, as stop() modifies the
135-
// map
136-
for (final @NotNull ITransaction t : timedOutTransactions) {
137-
stop(t);
138-
}
139-
}
140-
};
141-
timer.schedule(
142-
timerTask,
143-
TRANSACTION_COLLECTION_INTERVAL_MILLIS,
144-
TRANSACTION_COLLECTION_INTERVAL_MILLIS);
108+
},
109+
0L);
110+
// We schedule the collection to start after a delay, so we let some time pass between
111+
// setup() and collect() calls.
112+
// This way ICollectors that collect average stats based on time intervals, like
113+
// AndroidCpuCollector, can have an actual time interval to evaluate.
114+
collectFuture =
115+
options
116+
.getTimerExecutorService()
117+
.schedule(
118+
() -> collectAndReschedule(currentGeneration),
119+
TRANSACTION_COLLECTION_INTERVAL_MILLIS);
120+
} catch (Throwable t) {
121+
options
122+
.getLogger()
123+
.log(SentryLevel.WARNING, "Failed to schedule performance collection.", t);
124+
}
125+
}
126+
}
127+
}
128+
129+
private void collectAndReschedule(final long scheduledGeneration) {
130+
final @NotNull PerformanceCollectionData tempData =
131+
new PerformanceCollectionData(options.getDateProvider().now().nanoTimestamp());
132+
133+
// Enrich tempData using collectors
134+
for (IPerformanceSnapshotCollector collector : snapshotCollectors) {
135+
collector.collect(tempData);
136+
}
137+
138+
// Add the enriched tempData to all transactions/profiles/objects that collect data.
139+
// Then Check if that object timed out.
140+
final @NotNull List<ITransaction> timedOutTransactions = new ArrayList<>();
141+
for (CompositeData data : compositeDataMap.values()) {
142+
if (data.addDataAndCheckTimeout(tempData)) {
143+
// timed out
144+
if (data.transaction != null) {
145+
timedOutTransactions.add(data.transaction);
146+
}
147+
}
148+
}
149+
// Stop timed out transactions outside compositeDataMap loop, as stop() modifies the map
150+
for (final @NotNull ITransaction t : timedOutTransactions) {
151+
stop(t);
152+
}
153+
154+
try (final @NotNull ISentryLifecycleToken ignored = timerLock.acquire()) {
155+
// stopping a timed out transaction above may have closed this collector; only reschedule if
156+
// this run still belongs to the current collection chain
157+
if (scheduledGeneration == generation) {
158+
try {
159+
collectFuture =
160+
options
161+
.getTimerExecutorService()
162+
.schedule(
163+
() -> collectAndReschedule(scheduledGeneration),
164+
TRANSACTION_COLLECTION_INTERVAL_MILLIS);
165+
} catch (Throwable t) {
166+
options
167+
.getLogger()
168+
.log(SentryLevel.WARNING, "Failed to reschedule performance collection.", t);
169+
}
145170
}
146171
}
147172
}
@@ -201,9 +226,10 @@ public void close() {
201226
}
202227
if (isStarted.getAndSet(false)) {
203228
try (final @NotNull ISentryLifecycleToken ignored = timerLock.acquire()) {
204-
if (timer != null) {
205-
timer.cancel();
206-
timer = null;
229+
generation++;
230+
if (collectFuture != null) {
231+
collectFuture.cancel(false);
232+
collectFuture = null;
207233
}
208234
}
209235
}

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

Lines changed: 48 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -2,9 +2,9 @@ package io.sentry
22

33
import io.sentry.test.getCtor
44
import io.sentry.test.getProperty
5-
import io.sentry.test.injectForField
65
import io.sentry.util.thread.ThreadChecker
7-
import java.util.Timer
6+
import java.util.concurrent.CopyOnWriteArrayList
7+
import java.util.concurrent.Future
88
import java.util.concurrent.TimeUnit
99
import kotlin.test.Test
1010
import kotlin.test.assertEquals
@@ -34,7 +34,8 @@ class DefaultCompositePerformanceCollectorTest {
3434
val id1 = "id1"
3535
val scopes: IScopes = mock()
3636
val options = SentryOptions()
37-
var mockTimer: Timer? = null
37+
// a real executor so scheduled collection tasks actually run, recording schedule delays
38+
val executorService = RecordingExecutorService(SentryExecutorService(options))
3839

3940
val mockCpuCollector: IPerformanceSnapshotCollector =
4041
object : IPerformanceSnapshotCollector {
@@ -47,6 +48,7 @@ class DefaultCompositePerformanceCollectorTest {
4748

4849
init {
4950
whenever(scopes.options).thenReturn(options)
51+
options.setTimerExecutorService(executorService)
5052
}
5153

5254
fun getSut(
@@ -65,14 +67,23 @@ class DefaultCompositePerformanceCollectorTest {
6567
optionsConfiguration.configure(options)
6668
transaction1 = SentryTracer(TransactionContext("", ""), scopes)
6769
transaction2 = SentryTracer(TransactionContext("", ""), scopes)
68-
val collector = DefaultCompositePerformanceCollector(options)
69-
val timer: Timer = collector.getProperty("timer") ?: Timer(true)
70-
mockTimer = spy(timer)
71-
collector.injectForField("timer", mockTimer)
72-
return collector
70+
return DefaultCompositePerformanceCollector(options)
7371
}
7472
}
7573

74+
private class RecordingExecutorService(private val delegate: ISentryExecutorService) :
75+
ISentryExecutorService by delegate {
76+
val scheduledDelays = CopyOnWriteArrayList<Long>()
77+
78+
override fun schedule(runnable: Runnable, delayMillis: Long): Future<*> {
79+
scheduledDelays.add(delayMillis)
80+
return delegate.schedule(runnable, delayMillis)
81+
}
82+
}
83+
84+
private fun CompositePerformanceCollector.collectFuture(): Future<*>? =
85+
getProperty<Future<*>?>("collectFuture")
86+
7687
@Test
7788
fun `when null param is provided, invalid argument is thrown`() {
7889
val ctor = className.getCtor(ctorTypes)
@@ -85,7 +96,7 @@ class DefaultCompositePerformanceCollectorTest {
8596
val collector = fixture.getSut(null, null)
8697
assertTrue(fixture.options.performanceCollectors.isEmpty())
8798
collector.start(fixture.transaction1)
88-
verify(fixture.mockTimer, never())!!.schedule(any(), any<Long>(), any())
99+
assertTrue(fixture.executorService.scheduledDelays.isEmpty())
89100
}
90101

91102
@Test
@@ -100,52 +111,52 @@ class DefaultCompositePerformanceCollectorTest {
100111
}
101112

102113
@Test
103-
fun `when start, timer is scheduled every 100 milliseconds`() {
114+
fun `when start, collection is scheduled every 100 milliseconds`() {
104115
val collector = fixture.getSut()
105116
collector.start(fixture.transaction1)
106-
verify(fixture.mockTimer)!!.schedule(any(), any<Long>(), eq(100))
117+
assertTrue(fixture.executorService.scheduledDelays.contains(100L))
107118
}
108119

109120
@Test
110-
fun `when start with a string, timer is scheduled every 100 milliseconds`() {
121+
fun `when start with a string, collection is scheduled every 100 milliseconds`() {
111122
val collector = fixture.getSut()
112123
collector.start(fixture.id1)
113-
verify(fixture.mockTimer)!!.schedule(any(), any<Long>(), eq(100))
124+
assertTrue(fixture.executorService.scheduledDelays.contains(100L))
114125
}
115126

116127
@Test
117-
fun `when stop, timer is stopped`() {
128+
fun `when stop, collection is stopped`() {
118129
val collector = fixture.getSut()
119130
collector.start(fixture.transaction1)
131+
assertNotNull(collector.collectFuture())
120132
collector.stop(fixture.transaction1)
121-
verify(fixture.mockTimer)!!.schedule(any(), any<Long>(), eq(100))
122-
verify(fixture.mockTimer)!!.cancel()
133+
assertNull(collector.collectFuture())
123134
}
124135

125136
@Test
126-
fun `when stop with a string, timer is stopped`() {
137+
fun `when stop with a string, collection is stopped`() {
127138
val collector = fixture.getSut()
128139
collector.start(fixture.id1)
140+
assertNotNull(collector.collectFuture())
129141
collector.stop(fixture.id1)
130-
verify(fixture.mockTimer)!!.schedule(any(), any<Long>(), eq(100))
131-
verify(fixture.mockTimer)!!.cancel()
142+
assertNull(collector.collectFuture())
132143
}
133144

134145
@Test
135146
fun `stopping a not collected transaction return null`() {
136147
val collector = fixture.getSut()
137148
val data = collector.stop(fixture.transaction1)
138-
verify(fixture.mockTimer, never())!!.schedule(any(), any<Long>(), eq(100))
139-
verify(fixture.mockTimer, never())!!.cancel()
149+
assertTrue(fixture.executorService.scheduledDelays.isEmpty())
150+
assertNull(collector.collectFuture())
140151
assertNull(data)
141152
}
142153

143154
@Test
144155
fun `stopping a not collected id return null`() {
145156
val collector = fixture.getSut()
146157
val data = collector.stop(fixture.id1)
147-
verify(fixture.mockTimer, never())!!.schedule(any(), any<Long>(), eq(100))
148-
verify(fixture.mockTimer, never())!!.cancel()
158+
assertTrue(fixture.executorService.scheduledDelays.isEmpty())
159+
assertNull(collector.collectFuture())
149160
assertNull(data)
150161
}
151162

@@ -159,16 +170,16 @@ class DefaultCompositePerformanceCollectorTest {
159170
Thread.sleep(300)
160171

161172
val data1 = collector.stop(fixture.transaction1)
162-
// There is still a transaction and an id running: the timer shouldn't stop now
163-
verify(fixture.mockTimer, never())!!.cancel()
173+
// There is still a transaction and an id running: the collection shouldn't stop now
174+
assertNotNull(collector.collectFuture())
164175

165176
val data2 = collector.stop(fixture.transaction2)
166-
// There is still an id running: the timer shouldn't stop now
167-
verify(fixture.mockTimer, never())!!.cancel()
177+
// There is still an id running: the collection shouldn't stop now
178+
assertNotNull(collector.collectFuture())
168179

169180
val data3 = collector.stop(fixture.id1)
170-
// There are no more transactions or ids running: the time should stop now
171-
verify(fixture.mockTimer)!!.cancel()
181+
// There are no more transactions or ids running: the collection should stop now
182+
assertNull(collector.collectFuture())
172183

173184
assertNotNull(data1)
174185
assertNotNull(data2)
@@ -196,14 +207,14 @@ class DefaultCompositePerformanceCollectorTest {
196207
it.addPerformanceCollector(mockCollector)
197208
}
198209
collector.start(fixture.transaction1)
199-
verify(fixture.mockTimer, never())!!.cancel()
210+
assertNotNull(collector.collectFuture())
200211

201212
// Let's sleep to make the collector get values
202213
Thread.sleep(300)
203214

204215
// When the collector gets the values, it checks the current date, set 31 seconds after the
205216
// begin. This means it should stop itself
206-
verify(fixture.mockTimer)!!.cancel()
217+
assertNull(collector.collectFuture())
207218

208219
// When the collector times out, the data collection for spans is stopped, too
209220
verify(mockCollector).onSpanFinished(eq(fixture.transaction1))
@@ -224,14 +235,14 @@ class DefaultCompositePerformanceCollectorTest {
224235
whenever(mockDateProvider.now()).thenReturn(dates[0], dates[0], dates[0], dates[1])
225236
val collector = fixture.getSut { it.dateProvider = mockDateProvider }
226237
collector.start(fixture.transaction1)
227-
verify(fixture.mockTimer, never())!!.cancel()
238+
assertNotNull(collector.collectFuture())
228239

229240
// Let's sleep to make the collector get values
230241
Thread.sleep(300)
231242

232243
// When the collector gets the values, it checks the current date, set 30 seconds after the
233244
// begin. This means it should continue without being cancelled
234-
verify(fixture.mockTimer, never())!!.cancel()
245+
assertNotNull(collector.collectFuture())
235246

236247
// Data is deleted after the collector times out
237248
val data1 = collector.stop(fixture.transaction1)
@@ -296,14 +307,14 @@ class DefaultCompositePerformanceCollectorTest {
296307
}
297308

298309
@Test
299-
fun `when close, timer is stopped and data is cleared`() {
310+
fun `when close, collection is stopped and data is cleared`() {
300311
val collector = fixture.getSut()
301312
collector.start(fixture.transaction1)
302313
collector.close()
303314

304-
// Timer was canceled
305-
verify(fixture.mockTimer)!!.schedule(any(), any<Long>(), eq(100))
306-
verify(fixture.mockTimer)!!.cancel()
315+
// Collection was canceled
316+
assertTrue(fixture.executorService.scheduledDelays.contains(100L))
317+
assertNull(collector.collectFuture())
307318

308319
// Data was cleared
309320
assertNull(collector.stop(fixture.transaction1))

0 commit comments

Comments
 (0)