diff --git a/CHANGELOG.md b/CHANGELOG.md index cfffb41a11..9df47ab186 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,12 @@ # Changelog +## Unreleased + +### Fixes + +- Keep UI profiling running when `startProfiler` is called before a requested stop takes effect at the end of the current chunk ([#6291](https://github.com/getsentry/sentry-java/pull/6291)) + - Before, the start call was ignored and the profiler still stopped at the end of the chunk, even in `TRACE` mode while sampled spans were active. On API 34 and lower, every later profile also stopped after one chunk. + ## 8.61.1 ### Fixes diff --git a/sentry-android-core/src/main/java/io/sentry/android/core/AndroidContinuousProfiler.java b/sentry-android-core/src/main/java/io/sentry/android/core/AndroidContinuousProfiler.java index 20fea39a28..4f115ca9d7 100644 --- a/sentry-android-core/src/main/java/io/sentry/android/core/AndroidContinuousProfiler.java +++ b/sentry-android-core/src/main/java/io/sentry/android/core/AndroidContinuousProfiler.java @@ -131,6 +131,8 @@ public void startProfiler( logger.log(SentryLevel.DEBUG, "Profiler was not started due to sampling decision."); return; } + // A stop only takes effect when the current chunk ends, so a start before then cancels it. + shouldStop = false; switch (profileLifecycle) { case TRACE: // rootSpanCounter should never be negative, unless the user changed profile lifecycle diff --git a/sentry-android-core/src/main/java/io/sentry/android/core/PerfettoContinuousProfiler.java b/sentry-android-core/src/main/java/io/sentry/android/core/PerfettoContinuousProfiler.java index 58065c708e..15ae88f22c 100644 --- a/sentry-android-core/src/main/java/io/sentry/android/core/PerfettoContinuousProfiler.java +++ b/sentry-android-core/src/main/java/io/sentry/android/core/PerfettoContinuousProfiler.java @@ -141,6 +141,8 @@ public void startProfiler( logger.log(SentryLevel.DEBUG, "Profiler was not started due to sampling decision."); return; } + // A stop only takes effect when the current chunk ends, so a start before then cancels it. + shouldStop = false; switch (profileLifecycle) { case TRACE: activeTraceCount = Math.max(0, activeTraceCount); // safety check. @@ -148,16 +150,13 @@ public void startProfiler( break; case MANUAL: if (isRunning()) { - logger.log( - SentryLevel.WARNING, - "Unexpected call to startProfiler(MANUAL) while profiler already running. Skipping."); + logger.log(SentryLevel.DEBUG, "Profiler is already running."); return; } break; } if (!isRunning()) { logger.log(SentryLevel.DEBUG, "Started Profiler."); - shouldStop = false; startInternal(); } } diff --git a/sentry-android-core/src/test/java/io/sentry/android/core/AndroidContinuousProfilerTest.kt b/sentry-android-core/src/test/java/io/sentry/android/core/AndroidContinuousProfilerTest.kt index caaa30152a..a13f3fe124 100644 --- a/sentry-android-core/src/test/java/io/sentry/android/core/AndroidContinuousProfilerTest.kt +++ b/sentry-android-core/src/test/java/io/sentry/android/core/AndroidContinuousProfilerTest.kt @@ -278,6 +278,18 @@ class AndroidContinuousProfilerTest { } .testDoesNotStartWhenOffline(mocks) + @Test + fun `manual start cancels a pending stop`() = + fixture.getSut().testManualStartCancelsPendingStop(mocks) + + @Test + fun `trace start cancels a pending stop`() = + fixture.getSut().testTraceStartCancelsPendingStop(mocks) + + @Test + fun `manual profiler can be started again after a full start-stop cycle`() = + fixture.getSut().testCanBeStartedAgainAfterStopCycle(mocks) + // -- Legacy-specific tests (AndroidContinuousProfiler only) -- @Test diff --git a/sentry-android-core/src/test/java/io/sentry/android/core/ContinuousProfilerTestCases.kt b/sentry-android-core/src/test/java/io/sentry/android/core/ContinuousProfilerTestCases.kt index 5e4e0504dd..267529fd33 100644 --- a/sentry-android-core/src/test/java/io/sentry/android/core/ContinuousProfilerTestCases.kt +++ b/sentry-android-core/src/test/java/io/sentry/android/core/ContinuousProfilerTestCases.kt @@ -192,3 +192,23 @@ fun IContinuousProfiler.testCanBeStartedAgainAfterStopCycle(mocks: ProfilerMocks mocks.executor.runAll() assertTrue(isRunning, "shouldStop must be reset on start") } + +fun IContinuousProfiler.testManualStartCancelsPendingStop(mocks: ProfilerMocks) { + startProfiler(ProfileLifecycle.MANUAL, mocks.tracesSampler) + val profilerIdBeforeStop = profilerId + stopProfiler(ProfileLifecycle.MANUAL) + startProfiler(ProfileLifecycle.MANUAL, mocks.tracesSampler) + mocks.executor.runAll() + assertTrue(isRunning, "a start before the chunk ends must cancel the pending stop") + assertEquals(profilerIdBeforeStop, profilerId) +} + +fun IContinuousProfiler.testTraceStartCancelsPendingStop(mocks: ProfilerMocks) { + startProfiler(ProfileLifecycle.TRACE, mocks.tracesSampler) + val profilerIdBeforeStop = profilerId + stopProfiler(ProfileLifecycle.TRACE) + startProfiler(ProfileLifecycle.TRACE, mocks.tracesSampler) + mocks.executor.runAll() + assertTrue(isRunning, "a start before the chunk ends must cancel the pending stop") + assertEquals(profilerIdBeforeStop, profilerId) +} diff --git a/sentry-android-core/src/test/java/io/sentry/android/core/PerfettoContinuousProfilerTest.kt b/sentry-android-core/src/test/java/io/sentry/android/core/PerfettoContinuousProfilerTest.kt index d6f29b97b7..3bafc29be3 100644 --- a/sentry-android-core/src/test/java/io/sentry/android/core/PerfettoContinuousProfilerTest.kt +++ b/sentry-android-core/src/test/java/io/sentry/android/core/PerfettoContinuousProfilerTest.kt @@ -178,6 +178,14 @@ class PerfettoContinuousProfilerTest { fun `manual profiler can be started again after a full start-stop cycle`() = fixture.getSut().testCanBeStartedAgainAfterStopCycle(mocks) + @Test + fun `manual start cancels a pending stop`() = + fixture.getSut().testManualStartCancelsPendingStop(mocks) + + @Test + fun `trace start cancels a pending stop`() = + fixture.getSut().testTraceStartCancelsPendingStop(mocks) + // -- Perfetto-specific tests -- @Test @@ -503,10 +511,6 @@ class PerfettoContinuousProfilerTest { assertTrue(profiler.isRunning) profiler.startProfiler(ProfileLifecycle.MANUAL, fixture.mockTracesSampler) assertTrue(profiler.isRunning) - verify(fixture.mockLogger) - .log( - eq(SentryLevel.WARNING), - eq("Unexpected call to startProfiler(MANUAL) while profiler already running. Skipping."), - ) + verify(fixture.mockLogger).log(eq(SentryLevel.DEBUG), eq("Profiler is already running.")) } }