Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

## Unreleased

* Feat: Add Android profiling traces #1897
* Feat: Add Android profiling traces #1897 and its tests #1949

## 5.6.2

Expand Down
15 changes: 15 additions & 0 deletions Sentry/src/test/java/io/sentry/NoOpTransactionProfilerTest.kt
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
package io.sentry

import kotlin.test.Test
import kotlin.test.assertNull

class NoOpTransactionProfilerTest {
private var profiler: NoOpTransactionProfiler = NoOpTransactionProfiler.getInstance()
Comment thread
stefanosiano marked this conversation as resolved.
Outdated

@Test
fun `onTransactionStart is no op`() = profiler.onTransactionStart(NoOpTransaction.getInstance())
Comment thread
marandaneto marked this conversation as resolved.
Outdated

@Test
fun `onTransactionFinish returns null`() =
assertNull(profiler.onTransactionFinish(NoOpTransaction.getInstance()))
}
58 changes: 58 additions & 0 deletions Sentry/src/test/java/io/sentry/util/FileUtilsTest.kt
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
package io.sentry.util

import org.junit.Test
Comment thread
stefanosiano marked this conversation as resolved.
Outdated
import java.io.File
import java.nio.file.Files
import kotlin.test.assertEquals
import kotlin.test.assertFalse
import kotlin.test.assertNull

class FileUtilsTest {
Comment thread
stefanosiano marked this conversation as resolved.

@Test
fun `deleteRecursively returns true on non-existing file or null`() {
assert(FileUtils.deleteRecursively(null))
assert(FileUtils.deleteRecursively(File("")))
Comment thread
stefanosiano marked this conversation as resolved.
Outdated
}

@Test
fun `deleteRecursively deletes a simple file`() {
val f = Files.createTempFile("here", "test").toFile()
assert(f.exists())
assert(FileUtils.deleteRecursively(f))
assertFalse(f.exists())
}

@Test
fun `deleteRecursively deletes a folder`() {
val d = Files.createTempDirectory("here").toFile()
val f = File(d, "test")
val d2 = File(d, "dir2")
val f2 = File(d2, "test")
Comment thread
stefanosiano marked this conversation as resolved.
Outdated
f.createNewFile()
d2.mkdir()
f2.createNewFile()
assert(d.exists() && d.isDirectory && f.exists() && d2.exists() && d2.isDirectory)
assert(f2.exists())
assert(FileUtils.deleteRecursively(d))
assertFalse(f.exists() || d.exists() || f2.exists() || d2.exists())
}

@Test
fun `readText returns null on null, non existing or unreadable file`() {
val f = File("here", "test")
val unreadableFile = Files.createTempFile("here", "test").toFile()
unreadableFile.setReadable(false)
assertNull(FileUtils.readText(null))
assertNull(FileUtils.readText(f))
assertNull(FileUtils.readText(unreadableFile))
}

@Test
fun `readText returns the content of a file`() {
val f = Files.createTempFile("here", "test").toFile()
val text = "Lorem ipsum dolor sit amet\nLorem ipsum dolor sit amet"
f.writeText(text)
assertEquals(text, FileUtils.readText(f))
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@
import java.util.concurrent.Future;
import org.jetbrains.annotations.NotNull;
import org.jetbrains.annotations.Nullable;
import org.jetbrains.annotations.TestOnly;
import org.jetbrains.annotations.VisibleForTesting;

final class AndroidTransactionProfiler implements ITransactionProfiler {

Expand All @@ -41,7 +43,7 @@ final class AndroidTransactionProfiler implements ITransactionProfiler {
private @Nullable File traceFile = null;
private @Nullable File traceFilesDir = null;
private @Nullable Future<?> scheduledFinish = null;
private volatile @Nullable ITransaction activeTransaction = null;
@VisibleForTesting volatile @Nullable ITransaction activeTransaction = null;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we test the class without VisibleForTesting? I see this almost in every class.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Avoiding it would make it harder/less readable to test the code without increasing the visibility of the field.
I realized that when using org.jetbrains.annotations.VisibleForTesting Android studio doesn't care whether you use it in tests or outside them, and builds in any case.
However, using androidx.annotation.VisibleForTesting makes Android studio complain if you access that field outside tests.
Of course, that would work in sentry-android-* only. Would it make any difference?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem is that we could not use androidx.annotation in non-Android projects and that would make 2 different packages of annotations across the project, so we preferred to keep the standard.

You can configure AS to warn in such cases well.

You can also test the behavior of the method without using VisibleForTesting, for example, when you finish the transaction 2 times, you can verify that the captureTransaction method was called only once, so the activeTransaction was actually null during the 2nd time, instead of asserting the field directly.

Usually VisibleForTesting is only necessary when you are working with static fields or when its technically not possible to test the behaviour, it does not like this use case for this field.

private volatile @Nullable ProfilingTraceData timedOutProfilingData = null;
private final @NotNull Context context;
private final @NotNull SentryAndroidOptions options;
Expand All @@ -59,6 +61,10 @@ public AndroidTransactionProfiler(
Objects.requireNonNull(buildInfoProvider, "The BuildInfoProvider is required.");
this.packageInfo = ContextUtils.getPackageInfo(context, options.getLogger());
final String tracesFilesDirPath = options.getProfilingTracesDirPath();
if (!options.isProfilingEnabled()) {
options.getLogger().log(SentryLevel.INFO, "Profiling is disabled in options.");
return;
}
if (tracesFilesDirPath == null || tracesFilesDirPath.isEmpty()) {
options
.getLogger()
Expand Down Expand Up @@ -90,7 +96,7 @@ public synchronized void onTransactionStart(@NotNull ITransaction transaction) {

// traceFilesDir is null or intervalUs is 0 only if there was a problem in the constructor, but
// we already logged that
if (traceFilesDir == null || intervalUs == 0) {
if (traceFilesDir == null || intervalUs == 0 || !traceFilesDir.exists()) {
return;
}

Expand Down Expand Up @@ -225,7 +231,7 @@ public synchronized void onTransactionStart(@NotNull ITransaction transaction) {
buildInfoProvider.getModel(),
buildInfoProvider.getVersionRelease(),
buildInfoProvider.isEmulator(),
CpuInfoUtils.readMaxFrequencies(),
CpuInfoUtils.getInstance().readMaxFrequencies(),
totalMem,
options.getProguardUuid(),
versionName,
Expand Down Expand Up @@ -253,4 +259,9 @@ public synchronized void onTransactionStart(@NotNull ITransaction transaction) {
return null;
}
}

@TestOnly
void setTimedOutProfilingData(@Nullable ProfilingTraceData data) {
this.timedOutProfilingData = data;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -7,26 +7,38 @@
import java.util.List;
import org.jetbrains.annotations.ApiStatus;
import org.jetbrains.annotations.NotNull;
import org.jetbrains.annotations.TestOnly;
import org.jetbrains.annotations.VisibleForTesting;

@ApiStatus.Internal
public final class CpuInfoUtils {

private static final @NotNull String SYSTEM_CPU_PATH = "/sys/devices/system/cpu/";
private static final @NotNull String CPUINFO_MAX_FREQ_PATH = "cpufreq/cpuinfo_max_freq";
private static final CpuInfoUtils instance = new CpuInfoUtils();

public static CpuInfoUtils getInstance() {
return instance;
}

private CpuInfoUtils() {}

private static final @NotNull String SYSTEM_CPU_PATH = "/sys/devices/system/cpu";

@VisibleForTesting
static final @NotNull String CPUINFO_MAX_FREQ_PATH = "cpufreq/cpuinfo_max_freq";

/** Cached max frequencies to avoid reading files multiple times */
private static @NotNull List<String> cpuMaxFrequenciesMhz = new ArrayList<>();
@VisibleForTesting final @NotNull List<Integer> cpuMaxFrequenciesMhz = new ArrayList<>();

/**
* Read the max frequency of each core of the cpu and returns it in Mhz
*
* @return A list with the frequency of each core of the cpu in Mhz
*/
public static @NotNull List<String> readMaxFrequencies() {
public @NotNull List<Integer> readMaxFrequencies() {
if (!cpuMaxFrequenciesMhz.isEmpty()) {
return cpuMaxFrequenciesMhz;
}
File[] cpuDirs = new File(SYSTEM_CPU_PATH).listFiles();
File[] cpuDirs = new File(getSystemCpuPath()).listFiles();
if (cpuDirs == null) {
return new ArrayList<>();
}
Expand All @@ -37,7 +49,7 @@ public final class CpuInfoUtils {

if (!cpuMaxFreqFile.exists() || !cpuMaxFreqFile.canRead()) continue;

long khz = 0;
long khz;
try {
String content = FileUtils.readText(cpuMaxFreqFile);
if (content == null) continue;
Expand All @@ -47,8 +59,19 @@ public final class CpuInfoUtils {
} catch (IOException e) {
continue;
}
cpuMaxFrequenciesMhz.add(Long.toString(khz / 1000));
cpuMaxFrequenciesMhz.add((int) (khz / 1000));
}
return cpuMaxFrequenciesMhz;
}

@VisibleForTesting
@NotNull
String getSystemCpuPath() {
return SYSTEM_CPU_PATH;
}

@TestOnly
final void clear() {
cpuMaxFrequenciesMhz.clear();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,29 @@ class AndroidOptionsInitializerTest {
assertTrue(sentryOptions.cacheDirPath?.endsWith("${File.separator}cache${File.separator}sentry")!!)
}

@Test
fun `profilingTracesDirPath should be set at initialization`() {
val sentryOptions = SentryAndroidOptions()
val mockContext = createMockContext()

AndroidOptionsInitializer.init(sentryOptions, mockContext)

assertTrue(sentryOptions.profilingTracesDirPath?.endsWith("${File.separator}cache${File.separator}sentry${File.separator}profiling_traces")!!)
assertFalse(File(sentryOptions.profilingTracesDirPath!!).exists())
}

@Test
fun `profilingTracesDirPath should be created and cleared when profiling is enabled`() {
val sentryOptions = SentryAndroidOptions()
val mockContext = createMockContext()
sentryOptions.isProfilingEnabled = true

AndroidOptionsInitializer.init(sentryOptions, mockContext)

assertTrue(File(sentryOptions.profilingTracesDirPath!!).exists())
assertTrue(File(sentryOptions.profilingTracesDirPath!!).list()!!.isEmpty())
}

@Test
fun `outboxDir should be set at initialization`() {
val sentryOptions = SentryAndroidOptions()
Expand Down Expand Up @@ -181,6 +204,17 @@ class AndroidOptionsInitializerTest {
assertTrue(sentryOptions.transportGate is AndroidTransportGate)
}

@Test
fun `init should set Android transaction profiler`() {
val sentryOptions = SentryAndroidOptions()
val mockContext = createMockContext()

AndroidOptionsInitializer.init(sentryOptions, mockContext)

assertNotNull(sentryOptions.transactionProfiler)
assertTrue(sentryOptions.transactionProfiler is AndroidTransactionProfiler)
}

@Test
fun `NdkIntegration will load SentryNdk class and add to the integration list`() {
val mockContext = ContextUtilsTest.mockMetaData(metaData = createBundleWithDsn())
Expand Down
Loading