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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 3 commits into from
Mar 28, 2022
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9f90ba0
all operations involving file reads were moved to background threads
stefanosiano Mar 24, 2022
7836c79
all operations involving file reads were moved to background threads
stefanosiano Mar 25, 2022
41e1766
all operations involving file reads were moved to background threads
stefanosiano Mar 25, 2022
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
13 changes: 5 additions & 8 deletions sentry-android-core/src/main/java/io/sentry/android/core/AndroidTransactionProfiler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
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
View file
Open in desktop
Original file line number Diff line number Diff line change
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
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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 (Exception e) {
this.device_cpu_frequencies = new ArrayList<>();
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need that? device_cpu_frequencies is already created in the field declaration.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's just because I cannot leave exception block empty. Since there's no logger to log the exception i don't know if it's fine to call e.printStackTrace()

Copy link
Contributor

Choose a reason for hiding this comment

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

You can, do this:

} catch (Throwable ignored) {
      // should never happen
}

just name it ignored and add a comment.

Copy link
Member Author

Choose a reason for hiding this comment

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

I previously did

} catch (Throwable ignored) {
}

without the comment and it was complaining. I added the comment and now it's fine, thanks!

}
}
}
4 changes: 1 addition & 3 deletions sentry/src/main/java/io/sentry/SentryClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
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
View file
Open in desktop
Original file line number Diff line number Diff line change
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
View file
Open in desktop
Original file line number Diff line number Diff line change
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
View file
Open in desktop
Original file line number Diff line number Diff line change
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