Skip to content

Commit f8c8b31

Browse files
stefanosianostefanosiano
authored
Merge cde1aa1 into 1f3a214 
2 parents 1f3a214 + cde1aa1 commit f8c8b31

File tree

3 files changed

+108
-57
lines changed

3 files changed

+108
-57
lines changed

‎CHANGELOG.md‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,10 @@
22

33
## Unreleased
44

5+
### Fixes
6+
7+
- Avoid ExecutorService for DefaultCompositePerformanceCollector timeout ([#4841](https://github.com/getsentry/sentry-java/pull/4841))
8+
59
### Dependencies
610

711
- Bump Native SDK from v0.11.2 to v0.11.3 ([#4810](https://github.com/getsentry/sentry-java/pull/4810))

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

Lines changed: 50 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@
88
import java.util.Timer;
99
import java.util.TimerTask;
1010
import java.util.concurrent.ConcurrentHashMap;
11-
import java.util.concurrent.RejectedExecutionException;
11+
import java.util.concurrent.TimeUnit;
1212
import java.util.concurrent.atomic.AtomicBoolean;
1313
import org.jetbrains.annotations.ApiStatus;
1414
import org.jetbrains.annotations.NotNull;
@@ -20,8 +20,7 @@ public final class DefaultCompositePerformanceCollector implements CompositePerf
2020
private static final long TRANSACTION_COLLECTION_TIMEOUT_MILLIS = 30000;
2121
private final @NotNull AutoClosableReentrantLock timerLock = new AutoClosableReentrantLock();
2222
private volatile @Nullable Timer timer = null;
23-
private final @NotNull Map<String, List<PerformanceCollectionData>> performanceDataMap =
24-
new ConcurrentHashMap<>();
23+
private final @NotNull Map<String, CompositeData> compositeDataMap = new ConcurrentHashMap<>();
2524
private final @NotNull List<IPerformanceSnapshotCollector> snapshotCollectors;
2625
private final @NotNull List<IPerformanceContinuousCollector> continuousCollectors;
2726
private final boolean hasNoCollectors;
@@ -65,23 +64,11 @@ public void start(final @NotNull ITransaction transaction) {
6564
collector.onSpanStarted(transaction);
6665
}
6766

68-
if (!performanceDataMap.containsKey(transaction.getEventId().toString())) {
69-
performanceDataMap.put(transaction.getEventId().toString(), new ArrayList<>());
70-
// We schedule deletion of collected performance data after a timeout
71-
try {
72-
options
73-
.getExecutorService()
74-
.schedule(() -> stop(transaction), TRANSACTION_COLLECTION_TIMEOUT_MILLIS);
75-
} catch (RejectedExecutionException e) {
76-
options
77-
.getLogger()
78-
.log(
79-
SentryLevel.ERROR,
80-
"Failed to call the executor. Performance collector will not be automatically finished. Did you call Sentry.close()?",
81-
e);
82-
}
67+
final @NotNull String id = transaction.getEventId().toString();
68+
if (!compositeDataMap.containsKey(id)) {
69+
compositeDataMap.put(id, new CompositeData(id, true));
8370
}
84-
start(transaction.getEventId().toString());
71+
start(id);
8572
}
8673

8774
@Override
@@ -95,8 +82,10 @@ public void start(final @NotNull String id) {
9582
return;
9683
}
9784

98-
if (!performanceDataMap.containsKey(id)) {
99-
performanceDataMap.put(id, new ArrayList<>());
85+
if (!compositeDataMap.containsKey(id)) {
86+
// Transactions are added in start(ITransaction). If we are here, it means we don't come from
87+
// a transaction
88+
compositeDataMap.put(id, new CompositeData(id, false));
10089
}
10190
if (!isStarted.getAndSet(true)) {
10291
try (final @NotNull ISentryLifecycleToken ignored = timerLock.acquire()) {
@@ -131,14 +120,17 @@ public void run() {
131120
}
132121
lastCollectionTimestamp = now;
133122
final @NotNull PerformanceCollectionData tempData =
134-
new PerformanceCollectionData(new SentryNanotimeDate().nanoTimestamp());
123+
new PerformanceCollectionData(options.getDateProvider().now().nanoTimestamp());
135124

136125
for (IPerformanceSnapshotCollector collector : snapshotCollectors) {
137126
collector.collect(tempData);
138127
}
139128

140-
for (List<PerformanceCollectionData> data : performanceDataMap.values()) {
141-
data.add(tempData);
129+
for (CompositeData data : compositeDataMap.values()) {
130+
if (data.addDataAndCheckTimeout(tempData)) {
131+
// timed out
132+
stop(data.id);
133+
}
142134
}
143135
}
144136
};
@@ -183,13 +175,14 @@ public void onSpanFinished(@NotNull ISpan span) {
183175

184176
@Override
185177
public @Nullable List<PerformanceCollectionData> stop(final @NotNull String id) {
186-
final @Nullable List<PerformanceCollectionData> data = performanceDataMap.remove(id);
178+
final @Nullable CompositeData data = compositeDataMap.remove(id);
179+
options.getLogger().log(SentryLevel.DEBUG, "stop collecting performance info for " + id);
187180

188-
// close if they are no more running requests
189-
if (performanceDataMap.isEmpty()) {
181+
// close if there are no more running requests
182+
if (compositeDataMap.isEmpty()) {
190183
close();
191184
}
192-
return data;
185+
return data != null ? data.dataList : null;
193186
}
194187

195188
@Override
@@ -198,7 +191,7 @@ public void close() {
198191
.getLogger()
199192
.log(SentryLevel.DEBUG, "stop collecting all performance info for transactions");
200193

201-
performanceDataMap.clear();
194+
compositeDataMap.clear();
202195
for (final @NotNull IPerformanceContinuousCollector collector : continuousCollectors) {
203196
collector.clear();
204197
}
@@ -211,4 +204,32 @@ public void close() {
211204
}
212205
}
213206
}
207+
208+
private class CompositeData {
209+
private final @NotNull List<PerformanceCollectionData> dataList;
210+
private final @NotNull String id;
211+
private final boolean isTransaction;
212+
private final long startTimestamp;
213+
214+
private CompositeData(final @NotNull String id, final boolean isTransaction) {
215+
this.dataList = new ArrayList<>();
216+
this.id = id;
217+
this.isTransaction = isTransaction;
218+
this.startTimestamp = options.getDateProvider().now().nanoTimestamp();
219+
}
220+
221+
/**
222+
* Adds the data to the internal list of PerformanceCollectionData. Then it checks if data
223+
* collection timed out (for transactions only).
224+
*
225+
* @return true if data collection timed out (for transactions only).
226+
*/
227+
boolean addDataAndCheckTimeout(final @NotNull PerformanceCollectionData data) {
228+
dataList.add(data);
229+
return isTransaction
230+
&& options.getDateProvider().now().nanoTimestamp()
231+
> startTimestamp
232+
+ TimeUnit.MILLISECONDS.toNanos(TRANSACTION_COLLECTION_TIMEOUT_MILLIS);
233+
}
234+
}
214235
}

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

Lines changed: 54 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,12 @@
11
package io.sentry
22

3-
import io.sentry.test.DeferredExecutorService
43
import io.sentry.test.getCtor
54
import io.sentry.test.getProperty
65
import io.sentry.test.injectForField
76
import io.sentry.util.thread.ThreadChecker
7+
import java.util.Date
88
import java.util.Timer
9-
import java.util.concurrent.RejectedExecutionException
9+
import java.util.concurrent.TimeUnit
1010
import kotlin.test.Test
1111
import kotlin.test.assertEquals
1212
import kotlin.test.assertFailsWith
@@ -36,7 +36,6 @@ class DefaultCompositePerformanceCollectorTest {
3636
val scopes: IScopes = mock()
3737
val options = SentryOptions()
3838
var mockTimer: Timer? = null
39-
val deferredExecutorService = DeferredExecutorService()
4039

4140
val mockCpuCollector: IPerformanceSnapshotCollector =
4241
object : IPerformanceSnapshotCollector {
@@ -54,16 +53,17 @@ class DefaultCompositePerformanceCollectorTest {
5453
fun getSut(
5554
memoryCollector: IPerformanceSnapshotCollector? = JavaMemoryCollector(),
5655
cpuCollector: IPerformanceSnapshotCollector? = mockCpuCollector,
57-
executorService: ISentryExecutorService = deferredExecutorService,
56+
optionsConfiguration: Sentry.OptionsConfiguration<SentryOptions> =
57+
Sentry.OptionsConfiguration {},
5858
): CompositePerformanceCollector {
5959
options.dsn = "https://[email protected]/proj"
60-
options.executorService = executorService
6160
if (cpuCollector != null) {
6261
options.addPerformanceCollector(cpuCollector)
6362
}
6463
if (memoryCollector != null) {
6564
options.addPerformanceCollector(memoryCollector)
6665
}
66+
optionsConfiguration.configure(options)
6767
transaction1 = SentryTracer(TransactionContext("", ""), scopes)
6868
transaction2 = SentryTracer(TransactionContext("", ""), scopes)
6969
val collector = DefaultCompositePerformanceCollector(options)
@@ -184,21 +184,66 @@ class DefaultCompositePerformanceCollectorTest {
184184

185185
@Test
186186
fun `collector times out after 30 seconds`() {
187-
val collector = fixture.getSut()
187+
val mockDateProvider = mock<SentryDateProvider>()
188+
val dates =
189+
listOf(
190+
SentryNanotimeDate(
191+
Date().apply { time = TimeUnit.SECONDS.toMillis(100) },
192+
TimeUnit.SECONDS.toNanos(100),
193+
),
194+
SentryNanotimeDate(
195+
Date().apply { time = TimeUnit.SECONDS.toMillis(131) },
196+
TimeUnit.SECONDS.toNanos(131),
197+
),
198+
)
199+
whenever(mockDateProvider.now()).thenReturn(dates[0], dates[0], dates[0], dates[1])
200+
val collector = fixture.getSut { it.dateProvider = mockDateProvider }
188201
collector.start(fixture.transaction1)
202+
verify(fixture.mockTimer, never())!!.cancel()
203+
189204
// Let's sleep to make the collector get values
190205
Thread.sleep(300)
191-
verify(fixture.mockTimer, never())!!.cancel()
192206

193-
// Let the timeout job stop the collector
194-
fixture.deferredExecutorService.runAll()
207+
// When the collector gets the values, it checks the current date, set 31 seconds after the
208+
// begin. This means it should stop itself
195209
verify(fixture.mockTimer)!!.cancel()
196210

197211
// Data is deleted after the collector times out
198212
val data1 = collector.stop(fixture.transaction1)
199213
assertNull(data1)
200214
}
201215

216+
@Test
217+
fun `collector collects for 30 seconds`() {
218+
val mockDateProvider = mock<SentryDateProvider>()
219+
val dates =
220+
listOf(
221+
SentryNanotimeDate(
222+
Date().apply { time = TimeUnit.SECONDS.toMillis(100) },
223+
TimeUnit.SECONDS.toNanos(100),
224+
),
225+
SentryNanotimeDate(
226+
Date().apply { time = TimeUnit.SECONDS.toMillis(130) },
227+
TimeUnit.SECONDS.toNanos(130),
228+
),
229+
)
230+
whenever(mockDateProvider.now()).thenReturn(dates[0], dates[0], dates[0], dates[1])
231+
val collector = fixture.getSut { it.dateProvider = mockDateProvider }
232+
collector.start(fixture.transaction1)
233+
verify(fixture.mockTimer, never())!!.cancel()
234+
235+
// Let's sleep to make the collector get values
236+
Thread.sleep(300)
237+
238+
// When the collector gets the values, it checks the current date, set 30 seconds after the
239+
// begin. This means it should continue without being cancelled
240+
verify(fixture.mockTimer, never())!!.cancel()
241+
242+
// Data is deleted after the collector times out
243+
val data1 = collector.stop(fixture.transaction1)
244+
assertNotNull(data1)
245+
}
246+
202247
@Test
203248
fun `collector has no IPerformanceCollector by default`() {
204249
val collector = fixture.getSut(null, null)
@@ -270,25 +315,6 @@ class DefaultCompositePerformanceCollectorTest {
270315
assertNull(collector.stop(fixture.transaction1))
271316
}
272317

273-
@Test
274-
fun `start does not throw on executor shut down`() {
275-
val executorService = mock<ISentryExecutorService>()
276-
whenever(executorService.schedule(any(), any())).thenThrow(RejectedExecutionException())
277-
val logger = mock<ILogger>()
278-
fixture.options.setLogger(logger)
279-
fixture.options.isDebug = true
280-
val sut = fixture.getSut(executorService = executorService)
281-
sut.start(fixture.transaction1)
282-
verify(logger)
283-
.log(
284-
eq(SentryLevel.ERROR),
285-
eq(
286-
"Failed to call the executor. Performance collector will not be automatically finished. Did you call Sentry.close()?"
287-
),
288-
any(),
289-
)
290-
}
291-
292318
@Test
293319
fun `Continuous collectors are notified properly`() {
294320
val collector = mock<IPerformanceContinuousCollector>()

0 commit comments

Comments
 (0)