Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

all operations involving file reads were moved to background threads #1959

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Expand Up @@ -3,6 +3,7 @@
## Unreleased

* Feat: Add Android profiling traces #1897 and its tests #1949
- All operations involving file reads for profiling were moved to the background #1959

## 5.6.2

Expand Down
Expand Up @@ -199,13 +199,8 @@ public synchronized void onTransactionStart(@NotNull ITransaction transaction) {
scheduledFinish = null;
}

if (traceFile == null || !traceFile.exists()) {
options
.getLogger()
.log(
SentryLevel.ERROR,
"Trace file %s does not exists",
traceFile == null ? "null" : traceFile.getPath());
if (traceFile == null) {
options.getLogger().log(SentryLevel.ERROR, "Trace file does not exists");
return null;
}

Expand All @@ -221,16 +216,18 @@ public synchronized void onTransactionStart(@NotNull ITransaction transaction) {
totalMem = Long.toString(memInfo.totalMem);
}

// cpu max frequencies are read with a lambda because reading files is involved, so it will be
// done in the background when the trace file is read
return new ProfilingTraceData(
traceFile,
transaction,
Long.toString(transactionDurationNanos),
buildInfoProvider.getSdkInfoVersion(),
() -> CpuInfoUtils.getInstance().readMaxFrequencies(),
buildInfoProvider.getManufacturer(),
buildInfoProvider.getModel(),
buildInfoProvider.getVersionRelease(),
buildInfoProvider.isEmulator(),
CpuInfoUtils.getInstance().readMaxFrequencies(),
totalMem,
options.getProguardUuid(),
versionName,
Expand Down
3 changes: 2 additions & 1 deletion sentry/api/sentry.api
Expand Up @@ -517,7 +517,7 @@ public final class io/sentry/OutboxSender : io/sentry/IEnvelopeSender {
}

public final class io/sentry/ProfilingTraceData {
public fun <init> (Ljava/io/File;Lio/sentry/ITransaction;Ljava/lang/String;ILjava/lang/String;Ljava/lang/String;Ljava/lang/String;Ljava/lang/Boolean;Ljava/util/List;Ljava/lang/String;Ljava/lang/String;Ljava/lang/String;Ljava/lang/String;Ljava/lang/String;)V
public fun <init> (Ljava/io/File;Lio/sentry/ITransaction;Ljava/lang/String;ILjava/util/concurrent/Callable;Ljava/lang/String;Ljava/lang/String;Ljava/lang/String;Ljava/lang/Boolean;Ljava/lang/String;Ljava/lang/String;Ljava/lang/String;Ljava/lang/String;Ljava/lang/String;)V
public fun getAndroid_api_level ()I
public fun getBuild_id ()Ljava/lang/String;
public fun getDevice_cpu_frequencies ()Ljava/util/List;
Expand All @@ -541,6 +541,7 @@ public final class io/sentry/ProfilingTraceData {
public fun getVersion_code ()Ljava/lang/String;
public fun getVersion_name ()Ljava/lang/String;
public fun isDevice_is_emulator ()Z
public fun readDeviceCpuFrequencies ()V
public fun setSampled_profile (Ljava/lang/String;)V
}

Expand Down
17 changes: 14 additions & 3 deletions sentry/src/main/java/io/sentry/ProfilingTraceData.java
@@ -1,9 +1,11 @@
package io.sentry;

import java.io.File;
import java.util.ArrayList;
import java.util.List;
import java.util.Locale;
import java.util.UUID;
import java.util.concurrent.Callable;
import org.jetbrains.annotations.ApiStatus;
import org.jetbrains.annotations.NotNull;
import org.jetbrains.annotations.Nullable;
Expand All @@ -13,6 +15,7 @@ public final class ProfilingTraceData {

// This field is transient so that it's ignored by Gson
private final transient @NotNull File traceFile;
private final transient @NotNull Callable<List<Integer>> deviceCpuFrequenciesReader;

// Device metadata
private final int android_api_level;
Expand All @@ -23,7 +26,7 @@ public final class ProfilingTraceData {
private final @NotNull String device_os_name;
private final @NotNull String device_os_version;
private final boolean device_is_emulator;
private final @NotNull List<Integer> device_cpu_frequencies;
private @NotNull List<Integer> device_cpu_frequencies = new ArrayList<>();
private final @NotNull String device_physical_memory_bytes;
private final @NotNull String platform;
private final @NotNull String build_id;
Expand Down Expand Up @@ -52,17 +55,18 @@ public ProfilingTraceData(
@NotNull ITransaction transaction,
@NotNull String durationNanos,
int sdkInt,
@NotNull Callable<List<Integer>> deviceCpuFrequenciesReader,
@Nullable String deviceManufacturer,
@Nullable String deviceModel,
@Nullable String deviceOsVersion,
@Nullable Boolean deviceIsEmulator,
@NotNull List<Integer> deviceCpuFrequencies,
@Nullable String devicePhysicalMemoryBytes,
@Nullable String buildId,
@Nullable String versionName,
@Nullable String versionCode,
@Nullable String environment) {
this.traceFile = traceFile;
this.deviceCpuFrequenciesReader = deviceCpuFrequenciesReader;

// Device metadata
this.android_api_level = sdkInt;
Expand All @@ -71,7 +75,6 @@ public ProfilingTraceData(
this.device_model = deviceModel != null ? deviceModel : "";
this.device_os_version = deviceOsVersion != null ? deviceOsVersion : "";
this.device_is_emulator = deviceIsEmulator != null ? deviceIsEmulator : false;
this.device_cpu_frequencies = deviceCpuFrequencies;
this.device_physical_memory_bytes =
devicePhysicalMemoryBytes != null ? devicePhysicalMemoryBytes : "0";
this.device_os_build_number = "";
Expand Down Expand Up @@ -189,4 +192,12 @@ public boolean isDevice_is_emulator() {
public void setSampled_profile(@Nullable String sampledProfile) {
this.sampled_profile = sampledProfile;
}

public void readDeviceCpuFrequencies() {
try {
this.device_cpu_frequencies = deviceCpuFrequenciesReader.call();
} catch (Throwable ignored) {
// should never happen
}
}
}
4 changes: 1 addition & 3 deletions sentry/src/main/java/io/sentry/SentryClient.java
Expand Up @@ -185,9 +185,7 @@ private boolean shouldApplyScopeData(
final SentryEnvelopeItem profilingTraceItem =
SentryEnvelopeItem.fromProfilingTrace(
profilingTraceData, options.getMaxTraceFileSize(), options.getSerializer());
if (profilingTraceItem != null) {
envelopeItems.add(profilingTraceItem);
}
envelopeItems.add(profilingTraceItem);
}

if (attachments != null) {
Expand Down
12 changes: 6 additions & 6 deletions sentry/src/main/java/io/sentry/SentryEnvelopeItem.java
Expand Up @@ -204,29 +204,29 @@ public static SentryEnvelopeItem fromAttachment(
return new SentryEnvelopeItem(itemHeader, () -> cachedItem.getBytes());
}

public static @Nullable SentryEnvelopeItem fromProfilingTrace(
public static @NotNull SentryEnvelopeItem fromProfilingTrace(
final @NotNull ProfilingTraceData profilingTraceData,
final long maxTraceFileSize,
final @NotNull ISerializer serializer)
throws SentryEnvelopeException {

File traceFile = profilingTraceData.getTraceFile();
if (!traceFile.exists()) {
return null;
}

// Using CachedItem, so we read the trace file in the background
final CachedItem cachedItem =
new CachedItem(
() -> {
if (!traceFile.exists()) {
return null;
throw new SentryEnvelopeException(
String.format(
"Dropping profiling trace data, because the file '%s' doesn't exists",
traceFile.getName()));
}
// The payload of the profile item is a json including the trace file encoded with
// base64
byte[] traceFileBytes = readBytesFromFile(traceFile.getPath(), maxTraceFileSize);
String base64Trace = Base64.encodeToString(traceFileBytes, NO_WRAP | NO_PADDING);
profilingTraceData.setSampled_profile(base64Trace);
profilingTraceData.readDeviceCpuFrequencies();

try (final ByteArrayOutputStream stream = new ByteArrayOutputStream();
final Writer writer = new BufferedWriter(new OutputStreamWriter(stream, UTF_8))) {
Expand Down
6 changes: 3 additions & 3 deletions sentry/src/test/java/io/sentry/SentryClientTest.kt
Expand Up @@ -78,8 +78,8 @@ class SentryClientTest {

var attachment = Attachment("hello".toByteArray(), "hello.txt", "text/plain", true)
val profilingTraceFile = Files.createTempFile("trace", ".trace").toFile()
var profilingTraceData = ProfilingTraceData(profilingTraceFile, sentryTracer, "1", 0, "", "", "", false, emptyList(), "", "", "", "", "")
var profilingNonExistingTraceData = ProfilingTraceData(File("non_existent.trace"), sentryTracer, "1", 0, "", "", "", false, emptyList(), "", "", "", "", "")
var profilingTraceData = ProfilingTraceData(profilingTraceFile, sentryTracer, "1", 0, { emptyList() }, "", "", "", false, "", "", "", "", "")
var profilingNonExistingTraceData = ProfilingTraceData(File("non_existent.trace"), sentryTracer, "1", 0, { emptyList() }, "", "", "", false, "", "", "", "", "")

fun getSut() = SentryClient(sentryOptions)
}
Expand Down Expand Up @@ -1328,7 +1328,7 @@ class SentryClientTest {
val profilingTraceItem = actual.items.firstOrNull { item ->
item.header.type == SentryItemType.Profile
}
assertNotNull(profilingTraceItem)
assertNotNull(profilingTraceItem?.data)
},
isNull()
)
Expand Down
18 changes: 11 additions & 7 deletions sentry/src/test/java/io/sentry/SentryEnvelopeItemTest.kt
Expand Up @@ -220,7 +220,7 @@ class SentryEnvelopeItemTest {
}

file.writeBytes(fixture.bytes)
val traceDataBytes = SentryEnvelopeItem.fromProfilingTrace(profilingTraceData, fixture.maxAttachmentSize, serializer)?.data
val traceDataBytes = SentryEnvelopeItem.fromProfilingTrace(profilingTraceData, fixture.maxAttachmentSize, serializer).data
val reader = BufferedReader(InputStreamReader(ByteArrayInputStream(traceDataBytes)))
val deserData = serializer.deserialize(reader, ProfilingTraceData::class.java)
assertNotNull(deserData)
Expand All @@ -238,22 +238,26 @@ class SentryEnvelopeItemTest {
assert(file.exists())
val traceData = SentryEnvelopeItem.fromProfilingTrace(profilingTraceData, fixture.maxAttachmentSize, mock())
assert(file.exists())
traceData?.data
traceData.data
assertFalse(file.exists())
}

@Test
fun `fromProfilingTrace with invalid file returns null`() {
fun `fromProfilingTrace with invalid file throws`() {
val file = File(fixture.pathname)
val profilingTraceData = mock<ProfilingTraceData> {
whenever(it.traceFile).thenReturn(file)
}

assertNull(SentryEnvelopeItem.fromProfilingTrace(profilingTraceData, fixture.maxAttachmentSize, mock())?.data)
assertFailsWith<SentryEnvelopeException>("Dropping profiling trace data, because the file ${file.path} doesn't exists") {
SentryEnvelopeItem.fromProfilingTrace(profilingTraceData, fixture.maxAttachmentSize, mock()).data
}
file.writeBytes(fixture.bytes)
assertNotNull(SentryEnvelopeItem.fromProfilingTrace(profilingTraceData, fixture.maxAttachmentSize, mock())?.data)
assertNotNull(SentryEnvelopeItem.fromProfilingTrace(profilingTraceData, fixture.maxAttachmentSize, mock()).data)
file.setReadable(false)
assertNull(SentryEnvelopeItem.fromProfilingTrace(profilingTraceData, fixture.maxAttachmentSize, mock())?.data)
assertFailsWith<SentryEnvelopeException>("Dropping profiling trace data, because the file ${file.path} doesn't exists") {
SentryEnvelopeItem.fromProfilingTrace(profilingTraceData, fixture.maxAttachmentSize, mock()).data
}
}

@Test
Expand All @@ -265,7 +269,7 @@ class SentryEnvelopeItemTest {
}

val exception = assertFailsWith<SentryEnvelopeException> {
SentryEnvelopeItem.fromProfilingTrace(profilingTraceData, fixture.maxAttachmentSize, mock())?.data
SentryEnvelopeItem.fromProfilingTrace(profilingTraceData, fixture.maxAttachmentSize, mock()).data
}

assertEquals(
Expand Down