From 051ce9bce330e4a690fd59db52e506219a35eae9 Mon Sep 17 00:00:00 2001 From: Stefano Date: Thu, 13 Jun 2024 15:37:52 +0200 Subject: [PATCH] Remove profiling timeout logic and disable profiling on API 21 (#3478) * removed timeout logic from AndroidProfiler, as unused by the backend anyway, to fix potential profiling always or never running when one times out * removed API 21 from supported profiling versions, due to a crash --- CHANGELOG.md | 5 +++++ .../java/io/sentry/android/core/AndroidProfiler.java | 8 +------- .../android/core/AndroidTransactionProfiler.java | 9 +++++---- .../io/sentry/android/core/AndroidProfilerTest.kt | 4 ++-- .../android/core/AndroidTransactionProfilerTest.kt | 11 +++++------ .../java/io/sentry/uitest/android/EnvelopeTests.kt | 6 +----- 6 files changed, 19 insertions(+), 24 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4a106dcf6e..49b1043474 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -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 diff --git a/sentry-android-core/src/main/java/io/sentry/android/core/AndroidProfiler.java b/sentry-android-core/src/main/java/io/sentry/android/core/AndroidProfiler.java index 0018f8b75f..95e0e0146e 100644 --- a/sentry-android-core/src/main/java/io/sentry/android/core/AndroidProfiler.java +++ b/sentry-android-core/src/main/java/io/sentry/android/core/AndroidProfiler.java @@ -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 screenFrameRateMeasurements = new ArrayDeque<>(); @@ -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, @@ -216,10 +214,6 @@ public void onFrameMetricCollected( public synchronized @Nullable ProfileEndData endAndCollect( final boolean isTimeout, final @Nullable List performanceCollectionData) { - // check if profiling timed out - if (timedOutProfilingData != null) { - return timedOutProfilingData; - } if (!isRunning) { logger.log(SentryLevel.WARNING, "Profiler not running"); diff --git a/sentry-android-core/src/main/java/io/sentry/android/core/AndroidTransactionProfiler.java b/sentry-android-core/src/main/java/io/sentry/android/core/AndroidTransactionProfiler.java index 3abfe1306a..0fadaebb6b 100644 --- a/sentry-android-core/src/main/java/io/sentry/android/core/AndroidTransactionProfiler.java +++ b/sentry-android-core/src/main/java/io/sentry/android/core/AndroidTransactionProfiler.java @@ -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(); @@ -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 diff --git a/sentry-android-core/src/test/java/io/sentry/android/core/AndroidProfilerTest.kt b/sentry-android-core/src/test/java/io/sentry/android/core/AndroidProfilerTest.kt index a726b2c55b..9478c9a7b7 100644 --- a/sentry-android-core/src/test/java/io/sentry/android/core/AndroidProfilerTest.kt +++ b/sentry-android-core/src/test/java/io/sentry/android/core/AndroidProfilerTest.kt @@ -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 @@ -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 diff --git a/sentry-android-core/src/test/java/io/sentry/android/core/AndroidTransactionProfilerTest.kt b/sentry-android-core/src/test/java/io/sentry/android/core/AndroidTransactionProfilerTest.kt index 405aa6dc98..fd03d34631 100644 --- a/sentry-android-core/src/test/java/io/sentry/android/core/AndroidTransactionProfilerTest.kt +++ b/sentry-android-core/src/test/java/io/sentry/android/core/AndroidTransactionProfilerTest.kt @@ -53,7 +53,7 @@ class AndroidTransactionProfilerTest { private class Fixture { private val mockDsn = "http://key@localhost/proj" val buildInfo = mock { - whenever(it.sdkInfoVersion).thenReturn(Build.VERSION_CODES.LOLLIPOP) + whenever(it.sdkInfoVersion).thenReturn(Build.VERSION_CODES.LOLLIPOP_MR1) } val mockLogger = mock() var lastScheduledRunnable: Runnable? = null @@ -224,9 +224,9 @@ class AndroidTransactionProfilerTest { } @Test - fun `profiler works only on api 21+`() { + fun `profiler works only on api 22+`() { val buildInfo = mock { - whenever(it.sdkInfoVersion).thenReturn(Build.VERSION_CODES.KITKAT) + whenever(it.sdkInfoVersion).thenReturn(Build.VERSION_CODES.LOLLIPOP) } val profiler = fixture.getSut(context, buildInfo) profiler.start() @@ -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 @@ -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 diff --git a/sentry-android-integration-tests/sentry-uitest-android/src/androidTest/java/io/sentry/uitest/android/EnvelopeTests.kt b/sentry-android-integration-tests/sentry-uitest-android/src/androidTest/java/io/sentry/uitest/android/EnvelopeTests.kt index ebf729f6b6..c8ebc87264 100644 --- a/sentry-android-integration-tests/sentry-uitest-android/src/androidTest/java/io/sentry/uitest/android/EnvelopeTests.kt +++ b/sentry-android-integration-tests/sentry-uitest-android/src/androidTest/java/io/sentry/uitest/android/EnvelopeTests.kt @@ -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() }