Skip to content

Commit

Permalink
Merge abffe29 into fb1c50c
Browse files Browse the repository at this point in the history
  • Loading branch information
adinauer committed Jan 26, 2023
2 parents fb1c50c + abffe29 commit 239f3a1
Show file tree
Hide file tree
Showing 9 changed files with 159 additions and 108 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
19 changes: 15 additions & 4 deletions sentry/api/sentry.api
Original file line number Diff line number Diff line change
Expand Up @@ -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 <init> (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 <init> (Lio/sentry/SentryOptions;Lio/sentry/ILogger;)V
public fun getLogger ()Lio/sentry/ILogger;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -2055,10 +2067,9 @@ public final class io/sentry/TransactionOptions {
public fun setWaitForChildren (Z)V
}

public final class io/sentry/TransactionPerformanceCollector {
public fun <init> (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 {
Expand Down
Original file line number Diff line number Diff line change
@@ -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<String, PerformanceCollectionData> performanceDataMap =
new ConcurrentHashMap<>();
private final @NotNull List<ICollector> 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;
}
}
}
Original file line number Diff line number Diff line change
@@ -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;
}
}
2 changes: 1 addition & 1 deletion sentry/src/main/java/io/sentry/SentryOptions.java
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
@@ -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<String, PerformanceCollectionData> performanceDataMap =
new ConcurrentHashMap<>();
private final @NotNull List<ICollector> 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);
}
2 changes: 1 addition & 1 deletion sentry/src/test/java/io/sentry/SentryTracerTest.kt
Original file line number Diff line number Diff line change
Expand Up @@ -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())
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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<Class<*>> = arrayOf(SentryOptions::class.java)
private val fixture = Fixture()

Expand Down Expand Up @@ -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)
Expand Down

0 comments on commit 239f3a1

Please sign in to comment.