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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove profiling timeout logic and disable profiling on API 21 #3478

Merged
merged 3 commits into from
Jun 13, 2024
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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,10 @@
# Changelog

## Unreleased

### Fixes

- Remove profiling timeout logic and disable profiling on API 21 ([#3478](https://github.com/getsentry/sentry-java/pull/3478))

## 7.10.0

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,6 @@ public ProfileEndData(
private @Nullable Future<?> scheduledFinish = null;
private @Nullable File traceFile = null;
private @Nullable String frameMetricsCollectorId;
private volatile @Nullable ProfileEndData timedOutProfilingData = null;
private final @NotNull SentryFrameMetricsCollector frameMetricsCollector;
private final @NotNull ArrayDeque<ProfileMeasurementValue> screenFrameRateMeasurements =
new ArrayDeque<>();
Expand Down Expand Up @@ -182,8 +181,7 @@ public void onFrameMetricCollected(
// We stop profiling after a timeout to avoid huge profiles to be sent
try {
scheduledFinish =
executorService.schedule(
() -> timedOutProfilingData = endAndCollect(true, null), PROFILING_TIMEOUT_MILLIS);
executorService.schedule(() -> endAndCollect(true, null), PROFILING_TIMEOUT_MILLIS);
} catch (RejectedExecutionException e) {
logger.log(
SentryLevel.ERROR,
Expand Down Expand Up @@ -216,10 +214,6 @@ public void onFrameMetricCollected(
public synchronized @Nullable ProfileEndData endAndCollect(
final boolean isTimeout,
final @Nullable List<PerformanceCollectionData> performanceCollectionData) {
// check if profiling timed out
if (timedOutProfilingData != null) {
return timedOutProfilingData;
}

if (!isRunning) {
logger.log(SentryLevel.WARNING, "Profiler not running");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -133,8 +133,9 @@ private void init() {

@Override
public synchronized void start() {
// Debug.startMethodTracingSampling() is only available since Lollipop
if (buildInfoProvider.getSdkInfoVersion() < Build.VERSION_CODES.LOLLIPOP) return;
// Debug.startMethodTracingSampling() is only available since Lollipop, but Android Profiler
// causes crashes on api 21 -> https://github.com/getsentry/sentry-java/issues/3392
if (buildInfoProvider.getSdkInfoVersion() < Build.VERSION_CODES.LOLLIPOP_MR1) return;

// Let's initialize trace folder and profiling interval
init();
Expand Down Expand Up @@ -204,9 +205,9 @@ public synchronized void bindTransaction(final @NotNull ITransaction transaction
return null;
}

// onTransactionStart() is only available since Lollipop
// onTransactionStart() is only available since Lollipop_MR1
// and SystemClock.elapsedRealtimeNanos() since Jelly Bean
if (buildInfoProvider.getSdkInfoVersion() < Build.VERSION_CODES.LOLLIPOP) return null;
if (buildInfoProvider.getSdkInfoVersion() < Build.VERSION_CODES.LOLLIPOP_MR1) return null;

// Transaction finished, but it's not in the current profile
if (currentProfilingTransactionData == null
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -218,7 +218,7 @@ class AndroidProfilerTest {
}

@Test
fun `timedOutData has timeout flag`() {
fun `timedOutData is not recorded`() {
val profiler = fixture.getSut()

// Start and finish first transaction profiling
Expand All @@ -229,7 +229,7 @@ class AndroidProfilerTest {

// First transaction finishes: timed out data is returned
val endData = profiler.endAndCollect(false, null)
assert(endData!!.didTimeout)
assertNull(endData)
}

@Test
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ class AndroidTransactionProfilerTest {
private class Fixture {
private val mockDsn = "http://key@localhost/proj"
val buildInfo = mock<BuildInfoProvider> {
whenever(it.sdkInfoVersion).thenReturn(Build.VERSION_CODES.LOLLIPOP)
whenever(it.sdkInfoVersion).thenReturn(Build.VERSION_CODES.LOLLIPOP_MR1)
}
val mockLogger = mock<ILogger>()
var lastScheduledRunnable: Runnable? = null
Expand Down Expand Up @@ -224,9 +224,9 @@ class AndroidTransactionProfilerTest {
}

@Test
fun `profiler works only on api 21+`() {
fun `profiler works only on api 22+`() {
val buildInfo = mock<BuildInfoProvider> {
whenever(it.sdkInfoVersion).thenReturn(Build.VERSION_CODES.KITKAT)
whenever(it.sdkInfoVersion).thenReturn(Build.VERSION_CODES.LOLLIPOP)
}
val profiler = fixture.getSut(context, buildInfo)
profiler.start()
Expand Down Expand Up @@ -379,7 +379,7 @@ class AndroidTransactionProfilerTest {
}

@Test
fun `timedOutData has timeout truncation reason`() {
fun `timedOutData is not recorded`() {
val profiler = fixture.getSut(context)

// Start and finish first transaction profiling
Expand All @@ -391,8 +391,7 @@ class AndroidTransactionProfilerTest {

// First transaction finishes: timed out data is returned
val profilingTraceData = profiler.onTransactionFinish(fixture.transaction1, null, fixture.options)
assertEquals(profilingTraceData!!.transactionId, fixture.transaction1.eventId.toString())
assertEquals(ProfilingTraceData.TRUNCATION_REASON_TIMEOUT, profilingTraceData.truncationReason)
assertNull(profilingTraceData)
}

@Test
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -201,13 +201,9 @@ class EnvelopeTests : BaseUiTest() {
assertEnvelopeTransaction(it.items.toList(), AndroidLogger()).transaction == "timedOutProfile"
}.assert {
val transactionItem: SentryTransaction = it.assertTransaction()
val profilingTraceData: ProfilingTraceData = it.assertProfile()
// Profile should not be present, as it timed out and is discarded
it.assertNoOtherItems()
assertEquals("timedOutProfile", transactionItem.transaction)
assertEquals("timedOutProfile", profilingTraceData.transactionName)
// The profile should timeout after 30 seconds
assertTrue(profilingTraceData.durationNs.toLong() < TimeUnit.SECONDS.toNanos(31), "Profile duration expected to be less than 31 seconds. It was ${profilingTraceData.durationNs.toLong()} ns")
assertEquals(ProfilingTraceData.TRUNCATION_REASON_TIMEOUT, profilingTraceData.truncationReason)
}
assertNoOtherEnvelopes()
}
Expand Down
Loading