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
getsentry · Jun 13, 2024 · 051ce9b · 051ce9b
1 parent 1e646ca
commit 051ce9b
Show file tree

Hide file tree

Showing 6 changed files with 19 additions and 24 deletions.
diff --git 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
@@ -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<>();
@@ -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> 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
@@ -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
@@ -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
@@ -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
@@ -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()
@@ -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/...ests/sentry-uitest-android/src/androidTest/java/io/sentry/uitest/android/EnvelopeTests.kt b/...ests/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()
         }