diff --git a/CHANGELOG.md b/CHANGELOG.md index 3191648b50..13d4891cc2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,7 @@ ### Fixes - Expand guard against CVE-2018-9492 "Privilege Escalation via Content Provider" ([#2482](https://github.com/getsentry/sentry-java/pull/2482)) +- Prevent OOM by disabling TransactionPerformanceCollector for now ([#2498](https://github.com/getsentry/sentry-java/pull/2498)) ## 6.12.1 diff --git a/sentry-android-integration-tests/sentry-uitest-android/src/androidTest/java/io/sentry/uitest/android/EnvelopeTests.kt b/sentry-android-integration-tests/sentry-uitest-android/src/androidTest/java/io/sentry/uitest/android/EnvelopeTests.kt index 61e8cb711f..d02e3a827f 100644 --- a/sentry-android-integration-tests/sentry-uitest-android/src/androidTest/java/io/sentry/uitest/android/EnvelopeTests.kt +++ b/sentry-android-integration-tests/sentry-uitest-android/src/androidTest/java/io/sentry/uitest/android/EnvelopeTests.kt @@ -101,12 +101,12 @@ class EnvelopeTests : BaseUiTest() { // There could be no slow/frozen frames, but we expect at least one frame rate assertEquals(ProfileMeasurement.UNIT_HZ, frameRates.unit) assertTrue(frameRates.values.isNotEmpty()) - assertEquals(ProfileMeasurement.UNIT_BYTES, memoryStats.unit) - assertTrue(memoryStats.values.isNotEmpty()) - assertEquals(ProfileMeasurement.UNIT_BYTES, memoryNativeStats.unit) - assertTrue(memoryNativeStats.values.isNotEmpty()) - assertEquals(ProfileMeasurement.UNIT_PERCENT, cpuStats.unit) - assertTrue(cpuStats.values.isNotEmpty()) +// assertEquals(ProfileMeasurement.UNIT_BYTES, memoryStats.unit) +// assertTrue(memoryStats.values.isNotEmpty()) +// assertEquals(ProfileMeasurement.UNIT_BYTES, memoryNativeStats.unit) +// assertTrue(memoryNativeStats.values.isNotEmpty()) +// assertEquals(ProfileMeasurement.UNIT_PERCENT, cpuStats.unit) +// assertTrue(cpuStats.values.isNotEmpty()) // We should find the transaction id that started the profiling in the list of transactions val transactionData = profilingTraceData.transactions diff --git a/sentry/api/sentry.api b/sentry/api/sentry.api index 97a42ff173..81bbb428ae 100644 --- a/sentry/api/sentry.api +++ b/sentry/api/sentry.api @@ -183,6 +183,12 @@ public final class io/sentry/DateUtils { public static fun toUtilDateNotNull (Lio/sentry/SentryDate;)Ljava/util/Date; } +public final class io/sentry/DefaultTransactionPerformanceCollector : io/sentry/TransactionPerformanceCollector { + public fun (Lio/sentry/SentryOptions;)V + public fun start (Lio/sentry/ITransaction;)V + public fun stop (Lio/sentry/ITransaction;)Lio/sentry/PerformanceCollectionData; +} + public final class io/sentry/DiagnosticLogger : io/sentry/ILogger { public fun (Lio/sentry/SentryOptions;Lio/sentry/ILogger;)V public fun getLogger ()Lio/sentry/ILogger; @@ -853,6 +859,12 @@ public final class io/sentry/NoOpTransaction : io/sentry/ITransaction { public fun traceContext ()Lio/sentry/TraceContext; } +public final class io/sentry/NoOpTransactionPerformanceCollector : io/sentry/TransactionPerformanceCollector { + public static fun getInstance ()Lio/sentry/NoOpTransactionPerformanceCollector; + public fun start (Lio/sentry/ITransaction;)V + public fun stop (Lio/sentry/ITransaction;)Lio/sentry/PerformanceCollectionData; +} + public final class io/sentry/NoOpTransactionProfiler : io/sentry/ITransactionProfiler { public static fun getInstance ()Lio/sentry/NoOpTransactionProfiler; public fun onTransactionFinish (Lio/sentry/ITransaction;Lio/sentry/PerformanceCollectionData;)Lio/sentry/ProfilingTraceData; @@ -2055,10 +2067,9 @@ public final class io/sentry/TransactionOptions { public fun setWaitForChildren (Z)V } -public final class io/sentry/TransactionPerformanceCollector { - public fun (Lio/sentry/SentryOptions;)V - public fun start (Lio/sentry/ITransaction;)V - public fun stop (Lio/sentry/ITransaction;)Lio/sentry/PerformanceCollectionData; +public abstract interface class io/sentry/TransactionPerformanceCollector { + public abstract fun start (Lio/sentry/ITransaction;)V + public abstract fun stop (Lio/sentry/ITransaction;)Lio/sentry/PerformanceCollectionData; } public final class io/sentry/TypeCheckHint { diff --git a/sentry/src/main/java/io/sentry/DefaultTransactionPerformanceCollector.java b/sentry/src/main/java/io/sentry/DefaultTransactionPerformanceCollector.java new file mode 100644 index 0000000000..ce97904a9e --- /dev/null +++ b/sentry/src/main/java/io/sentry/DefaultTransactionPerformanceCollector.java @@ -0,0 +1,105 @@ +package io.sentry; + +import io.sentry.util.Objects; +import java.util.List; +import java.util.Map; +import java.util.Timer; +import java.util.TimerTask; +import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.atomic.AtomicBoolean; +import org.jetbrains.annotations.ApiStatus; +import org.jetbrains.annotations.NotNull; +import org.jetbrains.annotations.Nullable; + +@ApiStatus.Internal +public final class DefaultTransactionPerformanceCollector + implements TransactionPerformanceCollector { + private static final long TRANSACTION_COLLECTION_INTERVAL_MILLIS = 100; + private static final long TRANSACTION_COLLECTION_TIMEOUT_MILLIS = 30000; + private final @NotNull Object timerLock = new Object(); + private volatile @Nullable Timer timer = null; + private final @NotNull Map performanceDataMap = + new ConcurrentHashMap<>(); + private final @NotNull List collectors; + private final @NotNull SentryOptions options; + private final @NotNull AtomicBoolean isStarted = new AtomicBoolean(false); + + public DefaultTransactionPerformanceCollector(final @NotNull SentryOptions options) { + this.options = Objects.requireNonNull(options, "The options object is required."); + this.collectors = options.getCollectors(); + } + + @Override + @SuppressWarnings("FutureReturnValueIgnored") + public void start(final @NotNull ITransaction transaction) { + if (collectors.isEmpty()) { + options + .getLogger() + .log( + SentryLevel.INFO, + "No collector found. Performance stats will not be captured during transactions."); + return; + } + + if (!performanceDataMap.containsKey(transaction.getEventId().toString())) { + performanceDataMap.put(transaction.getEventId().toString(), new PerformanceCollectionData()); + options + .getExecutorService() + .schedule( + () -> { + PerformanceCollectionData data = stop(transaction); + if (data != null) { + performanceDataMap.put(transaction.getEventId().toString(), data); + } + }, + TRANSACTION_COLLECTION_TIMEOUT_MILLIS); + } + if (!isStarted.getAndSet(true)) { + synchronized (timerLock) { + for (ICollector collector : collectors) { + collector.setup(); + } + if (timer == null) { + timer = new Timer(true); + } + // We schedule the timer to start after a delay, so we let some time pass between setup() + // and collect() calls. + // This way ICollectors that collect average stats based on time intervals, like + // AndroidCpuCollector, can have an actual time interval to evaluate. + timer.scheduleAtFixedRate( + new TimerTask() { + @Override + public void run() { + synchronized (timerLock) { + for (ICollector collector : collectors) { + collector.collect(performanceDataMap.values()); + } + // We commit data after calling all collectors. + // This way we avoid issues caused by having multiple cpu or memory collectors. + for (PerformanceCollectionData data : performanceDataMap.values()) { + data.commitData(); + } + } + } + }, + TRANSACTION_COLLECTION_INTERVAL_MILLIS, + TRANSACTION_COLLECTION_INTERVAL_MILLIS); + } + } + } + + @Override + public @Nullable PerformanceCollectionData stop(final @NotNull ITransaction transaction) { + synchronized (timerLock) { + PerformanceCollectionData data = + performanceDataMap.remove(transaction.getEventId().toString()); + if (performanceDataMap.isEmpty() && isStarted.getAndSet(false)) { + if (timer != null) { + timer.cancel(); + timer = null; + } + } + return data; + } + } +} diff --git a/sentry/src/main/java/io/sentry/NoOpTransactionPerformanceCollector.java b/sentry/src/main/java/io/sentry/NoOpTransactionPerformanceCollector.java new file mode 100644 index 0000000000..8a0bd40b59 --- /dev/null +++ b/sentry/src/main/java/io/sentry/NoOpTransactionPerformanceCollector.java @@ -0,0 +1,24 @@ +package io.sentry; + +import org.jetbrains.annotations.NotNull; +import org.jetbrains.annotations.Nullable; + +public final class NoOpTransactionPerformanceCollector implements TransactionPerformanceCollector { + + private static final NoOpTransactionPerformanceCollector instance = + new NoOpTransactionPerformanceCollector(); + + public static NoOpTransactionPerformanceCollector getInstance() { + return instance; + } + + private NoOpTransactionPerformanceCollector() {} + + @Override + public void start(@NotNull ITransaction transaction) {} + + @Override + public @Nullable PerformanceCollectionData stop(@NotNull ITransaction transaction) { + return null; + } +} diff --git a/sentry/src/main/java/io/sentry/SentryOptions.java b/sentry/src/main/java/io/sentry/SentryOptions.java index 9ea5d720c4..5427f0749b 100644 --- a/sentry/src/main/java/io/sentry/SentryOptions.java +++ b/sentry/src/main/java/io/sentry/SentryOptions.java @@ -397,7 +397,7 @@ public class SentryOptions { /** Performance collector that collect performance stats while transactions run. */ private final @NotNull TransactionPerformanceCollector transactionPerformanceCollector = - new TransactionPerformanceCollector(this); + NoOpTransactionPerformanceCollector.getInstance(); /** * Adds an event processor diff --git a/sentry/src/main/java/io/sentry/TransactionPerformanceCollector.java b/sentry/src/main/java/io/sentry/TransactionPerformanceCollector.java index 6ca9bff85c..81a6803be8 100644 --- a/sentry/src/main/java/io/sentry/TransactionPerformanceCollector.java +++ b/sentry/src/main/java/io/sentry/TransactionPerformanceCollector.java @@ -1,102 +1,12 @@ package io.sentry; -import io.sentry.util.Objects; -import java.util.List; -import java.util.Map; -import java.util.Timer; -import java.util.TimerTask; -import java.util.concurrent.ConcurrentHashMap; -import java.util.concurrent.atomic.AtomicBoolean; -import org.jetbrains.annotations.ApiStatus; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; -@ApiStatus.Internal -public final class TransactionPerformanceCollector { - private static final long TRANSACTION_COLLECTION_INTERVAL_MILLIS = 100; - private static final long TRANSACTION_COLLECTION_TIMEOUT_MILLIS = 30000; - private final @NotNull Object timerLock = new Object(); - private volatile @Nullable Timer timer = null; - private final @NotNull Map performanceDataMap = - new ConcurrentHashMap<>(); - private final @NotNull List collectors; - private final @NotNull SentryOptions options; - private final @NotNull AtomicBoolean isStarted = new AtomicBoolean(false); +public interface TransactionPerformanceCollector { - public TransactionPerformanceCollector(final @NotNull SentryOptions options) { - this.options = Objects.requireNonNull(options, "The options object is required."); - this.collectors = options.getCollectors(); - } + void start(@NotNull ITransaction transaction); - @SuppressWarnings("FutureReturnValueIgnored") - public void start(final @NotNull ITransaction transaction) { - if (collectors.isEmpty()) { - options - .getLogger() - .log( - SentryLevel.INFO, - "No collector found. Performance stats will not be captured during transactions."); - return; - } - - if (!performanceDataMap.containsKey(transaction.getEventId().toString())) { - performanceDataMap.put(transaction.getEventId().toString(), new PerformanceCollectionData()); - options - .getExecutorService() - .schedule( - () -> { - PerformanceCollectionData data = stop(transaction); - if (data != null) { - performanceDataMap.put(transaction.getEventId().toString(), data); - } - }, - TRANSACTION_COLLECTION_TIMEOUT_MILLIS); - } - if (!isStarted.getAndSet(true)) { - synchronized (timerLock) { - for (ICollector collector : collectors) { - collector.setup(); - } - if (timer == null) { - timer = new Timer(true); - } - // We schedule the timer to start after a delay, so we let some time pass between setup() - // and collect() calls. - // This way ICollectors that collect average stats based on time intervals, like - // AndroidCpuCollector, can have an actual time interval to evaluate. - timer.scheduleAtFixedRate( - new TimerTask() { - @Override - public void run() { - synchronized (timerLock) { - for (ICollector collector : collectors) { - collector.collect(performanceDataMap.values()); - } - // We commit data after calling all collectors. - // This way we avoid issues caused by having multiple cpu or memory collectors. - for (PerformanceCollectionData data : performanceDataMap.values()) { - data.commitData(); - } - } - } - }, - TRANSACTION_COLLECTION_INTERVAL_MILLIS, - TRANSACTION_COLLECTION_INTERVAL_MILLIS); - } - } - } - - public @Nullable PerformanceCollectionData stop(final @NotNull ITransaction transaction) { - synchronized (timerLock) { - PerformanceCollectionData data = - performanceDataMap.remove(transaction.getEventId().toString()); - if (performanceDataMap.isEmpty() && isStarted.getAndSet(false)) { - if (timer != null) { - timer.cancel(); - timer = null; - } - } - return data; - } - } + @Nullable + PerformanceCollectionData stop(@NotNull ITransaction transaction); } diff --git a/sentry/src/test/java/io/sentry/SentryTracerTest.kt b/sentry/src/test/java/io/sentry/SentryTracerTest.kt index 6d30385af3..d25631a5a3 100644 --- a/sentry/src/test/java/io/sentry/SentryTracerTest.kt +++ b/sentry/src/test/java/io/sentry/SentryTracerTest.kt @@ -34,7 +34,7 @@ class SentryTracerTest { options.environment = "environment" options.release = "release@3.0.0" hub = spy(Hub(options)) - transactionPerformanceCollector = spy(TransactionPerformanceCollector(options)) + transactionPerformanceCollector = spy(DefaultTransactionPerformanceCollector(options)) hub.bindClient(mock()) } diff --git a/sentry/src/test/java/io/sentry/TransactionPerformanceCollectorTest.kt b/sentry/src/test/java/io/sentry/TransactionPerformanceCollectorTest.kt index 8b2f09cc75..c9ce81406e 100644 --- a/sentry/src/test/java/io/sentry/TransactionPerformanceCollectorTest.kt +++ b/sentry/src/test/java/io/sentry/TransactionPerformanceCollectorTest.kt @@ -24,7 +24,7 @@ import kotlin.test.assertTrue class TransactionPerformanceCollectorTest { - private val className = "io.sentry.TransactionPerformanceCollector" + private val className = "io.sentry.DefaultTransactionPerformanceCollector" private val ctorTypes: Array> = arrayOf(SentryOptions::class.java) private val fixture = Fixture() @@ -70,7 +70,7 @@ class TransactionPerformanceCollectorTest { } transaction1 = SentryTracer(TransactionContext("", ""), hub) transaction2 = SentryTracer(TransactionContext("", ""), hub) - val collector = TransactionPerformanceCollector(options) + val collector = DefaultTransactionPerformanceCollector(options) val timer: Timer? = collector.getProperty("timer") ?: Timer(true) mockTimer = spy(timer) collector.injectForField("timer", mockTimer)