Skip to content

Commit 1f9fc8a

Browse files
adinaueradinauer
authored
Merge pull request #5957 from getsentry/perf/metrics-batch-thread-first-use
perf(core): [Logs and Metrics Enable Flags 10] Avoid unused Metrics worker thread
2 parents 98074e6 + 481c394 commit 1f9fc8a

3 files changed

Lines changed: 124 additions & 7 deletions

File tree

‎sentry-android-core/src/test/java/io/sentry/android/core/AndroidMetricsBatchProcessorTest.kt‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,14 @@
11
package io.sentry.android.core
22

33
import androidx.test.ext.junit.runners.AndroidJUnit4
4+
import com.google.common.truth.Truth.assertThat
45
import io.sentry.ISentryClient
56
import io.sentry.SentryMetricsEvent
67
import io.sentry.SentryOptions
78
import io.sentry.protocol.SentryId
89
import io.sentry.test.ImmediateExecutorService
10+
import io.sentry.test.getProperty
11+
import java.util.concurrent.atomic.AtomicBoolean
912
import kotlin.test.AfterTest
1013
import kotlin.test.BeforeTest
1114
import kotlin.test.Test
@@ -14,6 +17,7 @@ import kotlin.test.assertTrue
1417
import org.junit.runner.RunWith
1518
import org.mockito.kotlin.any
1619
import org.mockito.kotlin.mock
20+
import org.mockito.kotlin.never
1721
import org.mockito.kotlin.verify
1822
import org.mockito.kotlin.whenever
1923

@@ -54,6 +58,16 @@ class AndroidMetricsBatchProcessorTest {
5458
assertNotNull(AppState.getInstance().lifecycleObserver)
5559
}
5660

61+
@Test
62+
fun `onBackground does not flush before first accepted item`() {
63+
val sut = fixture.getSut(useImmediateExecutor = true)
64+
65+
sut.onBackground()
66+
67+
assertThat(sut.getProperty<AtomicBoolean>("hasScheduled").get()).isFalse()
68+
verify(fixture.client, never()).captureBatchedMetricsEvents(any())
69+
}
70+
5771
@Test
5872
fun `onBackground schedules flush`() {
5973
val sut = fixture.getSut(useImmediateExecutor = true)

‎sentry/src/main/java/io/sentry/metrics/MetricsBatchProcessor.java‎

Lines changed: 19 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -34,16 +34,24 @@ public class MetricsBatchProcessor implements IMetricsBatchProcessor {
3434
private final @NotNull Queue<SentryMetricsEvent> queue;
3535
private final @NotNull ISentryExecutorService executorService;
3636
private final @NotNull AtomicBoolean hasScheduled = new AtomicBoolean(false);
37+
private volatile boolean hasAcceptedItem = false;
3738
private volatile boolean isShuttingDown = false;
3839

3940
private final @NotNull ReusableCountLatch pendingCount = new ReusableCountLatch();
4041

4142
public MetricsBatchProcessor(
4243
final @NotNull SentryOptions options, final @NotNull ISentryClient client) {
44+
this(options, client, new SentryExecutorService(options));
45+
}
46+
47+
MetricsBatchProcessor(
48+
final @NotNull SentryOptions options,
49+
final @NotNull ISentryClient client,
50+
final @NotNull ISentryExecutorService executorService) {
4351
this.options = options;
4452
this.client = client;
4553
this.queue = new ConcurrentLinkedQueue<>();
46-
this.executorService = new SentryExecutorService(options);
54+
this.executorService = executorService;
4755
}
4856

4957
@Override
@@ -65,21 +73,22 @@ public void add(final @NotNull SentryMetricsEvent metricsEvent) {
6573
}
6674
pendingCount.increment();
6775
queue.offer(metricsEvent);
76+
hasAcceptedItem = true;
6877
maybeSchedule(false);
6978
}
7079

7180
@SuppressWarnings("FutureReturnValueIgnored")
7281
@Override
7382
public void close(final boolean isRestarting) {
7483
isShuttingDown = true;
75-
if (isRestarting) {
84+
if (isRestarting && hasAcceptedItem) {
7685
maybeSchedule(true);
7786
executorService.submit(() -> executorService.close(options.getShutdownTimeoutMillis()));
78-
} else {
79-
executorService.close(options.getShutdownTimeoutMillis());
80-
while (!queue.isEmpty()) {
81-
flushBatch();
82-
}
87+
return;
88+
}
89+
executorService.close(options.getShutdownTimeoutMillis());
90+
while (!queue.isEmpty()) {
91+
flushBatch();
8392
}
8493
}
8594

@@ -106,6 +115,9 @@ private void maybeSchedule(boolean immediately) {
106115

107116
@Override
108117
public void flush(long timeoutMillis) {
118+
if (!hasAcceptedItem) {
119+
return;
120+
}
109121
maybeSchedule(true);
110122
try {
111123
pendingCount.waitTillZero(timeoutMillis, TimeUnit.MILLISECONDS);

‎sentry/src/test/java/io/sentry/metrics/MetricsBatchProcessorTest.kt‎

Lines changed: 91 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ package io.sentry.metrics
33
import com.google.common.truth.Truth.assertThat
44
import io.sentry.DataCategory
55
import io.sentry.ISentryClient
6+
import io.sentry.ISentryExecutorService
67
import io.sentry.SentryMetricsEvent
78
import io.sentry.SentryMetricsEvents
89
import io.sentry.SentryNanotimeDate
@@ -12,19 +13,106 @@ import io.sentry.clientreport.DiscardReason
1213
import io.sentry.clientreport.DiscardedEvent
1314
import io.sentry.protocol.SentryId
1415
import io.sentry.test.DeferredExecutorService
16+
import io.sentry.test.getProperty
1517
import io.sentry.test.injectForField
18+
import io.sentry.transport.ReusableCountLatch
1619
import io.sentry.util.JsonSerializationUtils
1720
import kotlin.test.Test
1821
import kotlin.test.assertEquals
1922
import kotlin.test.assertFalse
2023
import kotlin.test.assertTrue
24+
import org.mockito.kotlin.any
2125
import org.mockito.kotlin.argumentCaptor
2226
import org.mockito.kotlin.atLeast
2327
import org.mockito.kotlin.mock
28+
import org.mockito.kotlin.never
2429
import org.mockito.kotlin.times
2530
import org.mockito.kotlin.verify
31+
import org.mockito.kotlin.verifyNoInteractions
2632

2733
class MetricsBatchProcessorTest {
34+
@Test
35+
fun `constructor does not submit processor work`() {
36+
val mockExecutor = mock<ISentryExecutorService>()
37+
38+
MetricsBatchProcessor(SentryOptions(), mock(), mockExecutor)
39+
40+
verifyNoInteractions(mockExecutor)
41+
}
42+
43+
@Test
44+
fun `empty flush does not submit processor work`() {
45+
val mockExecutor = mock<ISentryExecutorService>()
46+
val processor = MetricsBatchProcessor(SentryOptions(), mock(), mockExecutor)
47+
48+
processor.flush(0)
49+
50+
verifyNoInteractions(mockExecutor)
51+
}
52+
53+
@Test
54+
fun `close before first accepted item does not submit processor work`() {
55+
val mockExecutor = mock<ISentryExecutorService>()
56+
val processor = MetricsBatchProcessor(SentryOptions(), mock(), mockExecutor)
57+
58+
processor.close(false)
59+
60+
verify(mockExecutor).close(any())
61+
verify(mockExecutor, never()).schedule(any(), any())
62+
verify(mockExecutor, never()).submit(any<Runnable>())
63+
}
64+
65+
@Test
66+
fun `restart close before first accepted item does not submit processor work`() {
67+
val mockExecutor = mock<ISentryExecutorService>()
68+
val processor = MetricsBatchProcessor(SentryOptions(), mock(), mockExecutor)
69+
70+
processor.close(true)
71+
72+
verify(mockExecutor).close(any())
73+
verify(mockExecutor, never()).schedule(any(), any())
74+
verify(mockExecutor, never()).submit(any<Runnable>())
75+
}
76+
77+
@Test
78+
fun `item rejected during shutdown does not mark processor as used`() {
79+
val mockExecutor = mock<ISentryExecutorService>()
80+
val processor = MetricsBatchProcessor(SentryOptions(), mock(), mockExecutor)
81+
processor.close(false)
82+
83+
processor.add(metricsEvent("rejected"))
84+
processor.flush(0)
85+
86+
verify(mockExecutor, never()).schedule(any(), any())
87+
verify(mockExecutor, never()).submit(any<Runnable>())
88+
}
89+
90+
@Test
91+
fun `item rejected due to queue capacity does not mark processor as used`() {
92+
val mockExecutor = mock<ISentryExecutorService>()
93+
val processor = MetricsBatchProcessor(SentryOptions(), mock(), mockExecutor)
94+
val pendingCount = processor.getProperty<ReusableCountLatch>("pendingCount")
95+
repeat(MetricsBatchProcessor.MAX_QUEUE_SIZE) { pendingCount.increment() }
96+
97+
processor.add(metricsEvent("rejected"))
98+
processor.flush(0)
99+
100+
verifyNoInteractions(mockExecutor)
101+
}
102+
103+
@Test
104+
fun `flush and restart close submit processor work after first accepted item`() {
105+
val mockExecutor = mock<ISentryExecutorService>()
106+
val processor = MetricsBatchProcessor(SentryOptions(), mock(), mockExecutor)
107+
processor.add(metricsEvent("accepted"))
108+
109+
processor.flush(0)
110+
processor.close(true)
111+
112+
verify(mockExecutor, times(3)).schedule(any(), any())
113+
verify(mockExecutor).submit(any<Runnable>())
114+
}
115+
28116
@Test
29117
fun `schedules another flush after previous flush has run`() {
30118
val mockClient = mock<ISentryClient>()
@@ -46,6 +134,9 @@ class MetricsBatchProcessorTest {
46134
.inOrder()
47135
}
48136

137+
private fun metricsEvent(name: String) =
138+
SentryMetricsEvent(SentryId(), SentryNanotimeDate(), name, "gauge", 1.0)
139+
49140
@Test
50141
fun `drops metrics events after reaching MAX_QUEUE_SIZE limit`() {
51142
// given

0 commit comments

Comments
 (0)