Skip to content

Commit

Permalink
code review changes
Browse files Browse the repository at this point in the history
  • Loading branch information
adinauer committed Oct 31, 2024
1 parent a6afc13 commit 5863675
Show file tree
Hide file tree
Showing 3 changed files with 27 additions and 23 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,6 @@
import io.sentry.protocol.SentryTransaction;
import io.sentry.protocol.User;
import io.sentry.util.HintUtils;
import io.sentry.util.Random;
import io.sentry.util.SentryRandom;
import java.io.File;
import java.util.ArrayList;
Expand Down Expand Up @@ -84,24 +83,13 @@ public final class AnrV2EventProcessor implements BackfillingEventProcessor {

private final @NotNull SentryExceptionFactory sentryExceptionFactory;

private final @Nullable Random random;

public AnrV2EventProcessor(
final @NotNull Context context,
final @NotNull SentryAndroidOptions options,
final @NotNull BuildInfoProvider buildInfoProvider) {
this(context, options, buildInfoProvider, null);
}

AnrV2EventProcessor(
final @NotNull Context context,
final @NotNull SentryAndroidOptions options,
final @NotNull BuildInfoProvider buildInfoProvider,
final @Nullable Random random) {
this.context = ContextUtils.getApplicationContext(context);
this.options = options;
this.buildInfoProvider = buildInfoProvider;
this.random = random;

final SentryStackTraceFactory sentryStackTraceFactory =
new SentryStackTraceFactory(this.options);
Expand Down Expand Up @@ -181,9 +169,8 @@ private boolean sampleReplay(final @NotNull SentryEvent event) {

try {
// we have to sample here with the old sample rate, because it may change between app launches
final @NotNull Random random = this.random != null ? this.random : SentryRandom.current();
final double replayErrorSampleRateDouble = Double.parseDouble(replayErrorSampleRate);
if (replayErrorSampleRateDouble < random.nextDouble()) {
if (replayErrorSampleRateDouble < SentryRandom.current().nextDouble()) {
options
.getLogger()
.log(
Expand Down
19 changes: 18 additions & 1 deletion sentry/src/main/java/io/sentry/util/SentryRandom.java
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,27 @@

import org.jetbrains.annotations.NotNull;

/**
* This SentryRandom is a compromise used for improving performance of the SDK.
*
* <p>We did some testing where using Random from multiple threads degrades performance
* significantly. We opted for this approach as it wasn't easily possible to vendor
* ThreadLocalRandom since it's using advanced features that can cause java.lang.IllegalAccessError.
*/
public final class SentryRandom {

private static final SentryRandomThreadLocal instance = new SentryRandomThreadLocal();
private static final @NotNull SentryRandomThreadLocal instance = new SentryRandomThreadLocal();

/**
* Returns the current threads instance of {@link Random}. An instance of {@link Random} will be
* created the first time this is invoked on each thread.
*
* <p>NOTE: Avoid holding a reference to the returned {@link Random} instance as sharing a
* reference across threads (while being thread-safe) will likely degrade performance
* significantly.
*
* @return random
*/
public static @NotNull Random current() {
return instance.get();
}
Expand Down
16 changes: 8 additions & 8 deletions sentry/src/test/java/io/sentry/util/SentryRandomTest.kt
Original file line number Diff line number Diff line change
@@ -1,16 +1,16 @@
package io.sentry.util

import kotlin.test.Test
import kotlin.test.assertEquals
import kotlin.test.assertNotEquals
import kotlin.test.assertNotSame
import kotlin.test.assertSame

class SentryRandomTest {

@Test
fun `thread local creates a new instance per thread but keeps re-using it for the same thread`() {
val mainThreadRandom1 = SentryRandom.current()
val mainThreadRandom2 = SentryRandom.current()
assertEquals(mainThreadRandom1.toString(), mainThreadRandom2.toString())
assertSame(mainThreadRandom1, mainThreadRandom2)

var thread1Random1: Random? = null
var thread1Random2: Random? = null
Expand All @@ -33,13 +33,13 @@ class SentryRandomTest {
thread1.join()
thread2.join()

assertEquals(thread1Random1.toString(), thread1Random2.toString())
assertNotEquals(mainThreadRandom1.toString(), thread1Random1.toString())
assertSame(thread1Random1, thread1Random2)
assertNotSame(mainThreadRandom1, thread1Random1)

assertEquals(thread2Random1.toString(), thread2Random2.toString())
assertNotEquals(mainThreadRandom1.toString(), thread2Random1.toString())
assertSame(thread2Random1, thread2Random2)
assertNotSame(mainThreadRandom1, thread2Random1)

val mainThreadRandom3 = SentryRandom.current()
assertEquals(mainThreadRandom1.toString(), mainThreadRandom3.toString())
assertSame(mainThreadRandom1, mainThreadRandom3)
}
}

0 comments on commit 5863675

Please sign in to comment.