diff --git a/CHANGELOG.md b/CHANGELOG.md index d24d2840e69..d8fb0ca1af8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,7 @@ ### Fixes +- Start performance collection on AppStart continuous profiling ([#4752](https://github.com/getsentry/sentry-java/pull/4752)) - Preserve modifiers in `SentryTraced` ([#4757](https://github.com/getsentry/sentry-java/pull/4757)) ### Improvements diff --git a/sentry-android-core/api/sentry-android-core.api b/sentry-android-core/api/sentry-android-core.api index 5b416486576..0712c78ce91 100644 --- a/sentry-android-core/api/sentry-android-core.api +++ b/sentry-android-core/api/sentry-android-core.api @@ -43,6 +43,7 @@ public final class io/sentry/android/core/ActivityLifecycleIntegration : android public class io/sentry/android/core/AndroidContinuousProfiler : io/sentry/IContinuousProfiler, io/sentry/transport/RateLimiter$IRateLimitObserver { public fun (Lio/sentry/android/core/BuildInfoProvider;Lio/sentry/android/core/internal/util/SentryFrameMetricsCollector;Lio/sentry/ILogger;Ljava/lang/String;ILio/sentry/ISentryExecutorService;)V public fun close (Z)V + public fun getChunkId ()Lio/sentry/protocol/SentryId; public fun getProfilerId ()Lio/sentry/protocol/SentryId; public fun getRootSpanCounter ()I public fun isRunning ()Z 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 a3fb6f6c8db..699e49ba973 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 @@ -208,11 +208,11 @@ private void start() { isRunning = true; - if (profilerId == SentryId.EMPTY_ID) { + if (profilerId.equals(SentryId.EMPTY_ID)) { profilerId = new SentryId(); } - if (chunkId == SentryId.EMPTY_ID) { + if (chunkId.equals(SentryId.EMPTY_ID)) { chunkId = new SentryId(); } @@ -344,6 +344,11 @@ public void close(final boolean isTerminating) { return profilerId; } + @Override + public @NotNull SentryId getChunkId() { + return chunkId; + } + private void sendChunks(final @NotNull IScopes scopes, final @NotNull SentryOptions options) { try { options diff --git a/sentry-android-core/src/main/java/io/sentry/android/core/AndroidOptionsInitializer.java b/sentry-android-core/src/main/java/io/sentry/android/core/AndroidOptionsInitializer.java index 296916bb9ef..70c76c72824 100644 --- a/sentry-android-core/src/main/java/io/sentry/android/core/AndroidOptionsInitializer.java +++ b/sentry-android-core/src/main/java/io/sentry/android/core/AndroidOptionsInitializer.java @@ -5,6 +5,7 @@ import android.app.Application; import android.content.Context; import android.content.pm.PackageInfo; +import io.sentry.CompositePerformanceCollector; import io.sentry.DeduplicateMultithreadedEventProcessor; import io.sentry.DefaultCompositePerformanceCollector; import io.sentry.DefaultVersionDetector; @@ -45,6 +46,7 @@ import io.sentry.internal.gestures.GestureTargetLocator; import io.sentry.internal.modules.NoOpModulesLoader; import io.sentry.internal.viewhierarchy.ViewHierarchyExporter; +import io.sentry.protocol.SentryId; import io.sentry.transport.CurrentDateProvider; import io.sentry.transport.NoOpEnvelopeCache; import io.sentry.transport.NoOpTransportGate; @@ -180,25 +182,7 @@ static void initializeIntegrationsAndProcessors( options.setTransportGate(new AndroidTransportGate(options)); } - // Check if the profiler was already instantiated in the app start. - // We use the Android profiler, that uses a global start/stop api, so we need to preserve the - // state of the profiler, and it's only possible retaining the instance. final @NotNull AppStartMetrics appStartMetrics = AppStartMetrics.getInstance(); - final @Nullable ITransactionProfiler appStartTransactionProfiler; - final @Nullable IContinuousProfiler appStartContinuousProfiler; - try (final @NotNull ISentryLifecycleToken ignored = AppStartMetrics.staticLock.acquire()) { - appStartTransactionProfiler = appStartMetrics.getAppStartProfiler(); - appStartContinuousProfiler = appStartMetrics.getAppStartContinuousProfiler(); - appStartMetrics.setAppStartProfiler(null); - appStartMetrics.setAppStartContinuousProfiler(null); - } - - setupProfiler( - options, - context, - buildInfoProvider, - appStartTransactionProfiler, - appStartContinuousProfiler); if (options.getModulesLoader() instanceof NoOpModulesLoader) { options.setModulesLoader(new AssetsModulesLoader(context, options.getLogger())); @@ -262,6 +246,26 @@ static void initializeIntegrationsAndProcessors( if (options.getCompositePerformanceCollector() instanceof NoOpCompositePerformanceCollector) { options.setCompositePerformanceCollector(new DefaultCompositePerformanceCollector(options)); } + + // Check if the profiler was already instantiated in the app start. + // We use the Android profiler, that uses a global start/stop api, so we need to preserve the + // state of the profiler, and it's only possible retaining the instance. + final @Nullable ITransactionProfiler appStartTransactionProfiler; + final @Nullable IContinuousProfiler appStartContinuousProfiler; + try (final @NotNull ISentryLifecycleToken ignored = AppStartMetrics.staticLock.acquire()) { + appStartTransactionProfiler = appStartMetrics.getAppStartProfiler(); + appStartContinuousProfiler = appStartMetrics.getAppStartContinuousProfiler(); + appStartMetrics.setAppStartProfiler(null); + appStartMetrics.setAppStartContinuousProfiler(null); + } + + setupProfiler( + options, + context, + buildInfoProvider, + appStartTransactionProfiler, + appStartContinuousProfiler, + options.getCompositePerformanceCollector()); } /** Setup the correct profiler (transaction or continuous) based on the options. */ @@ -270,7 +274,8 @@ private static void setupProfiler( final @NotNull Context context, final @NotNull BuildInfoProvider buildInfoProvider, final @Nullable ITransactionProfiler appStartTransactionProfiler, - final @Nullable IContinuousProfiler appStartContinuousProfiler) { + final @Nullable IContinuousProfiler appStartContinuousProfiler, + final @NotNull CompositePerformanceCollector performanceCollector) { if (options.isProfilingEnabled() || options.getProfilesSampleRate() != null) { options.setContinuousProfiler(NoOpContinuousProfiler.getInstance()); // This is a safeguard, but it should never happen, as the app start profiler should be the @@ -299,6 +304,12 @@ private static void setupProfiler( } if (appStartContinuousProfiler != null) { options.setContinuousProfiler(appStartContinuousProfiler); + // If the profiler is running, we start the performance collector too, otherwise we'd miss + // measurements in app launch profiles + final @NotNull SentryId chunkId = appStartContinuousProfiler.getChunkId(); + if (appStartContinuousProfiler.isRunning() && !chunkId.equals(SentryId.EMPTY_ID)) { + performanceCollector.start(chunkId.toString()); + } } else { options.setContinuousProfiler( new AndroidContinuousProfiler( 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 34fc60d3634..60a5ab530fc 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 @@ -30,6 +30,7 @@ import kotlin.test.Test import kotlin.test.assertContains import kotlin.test.assertEquals import kotlin.test.assertFalse +import kotlin.test.assertNotEquals import kotlin.test.assertNotNull import kotlin.test.assertNull import kotlin.test.assertTrue @@ -167,9 +168,13 @@ class AndroidContinuousProfilerTest { // We are scheduling the profiler to stop at the end of the chunk, so it should still be running profiler.stopProfiler(ProfileLifecycle.MANUAL) assertTrue(profiler.isRunning) + assertNotEquals(SentryId.EMPTY_ID, profiler.profilerId) + assertNotEquals(SentryId.EMPTY_ID, profiler.chunkId) // We run the executor service to trigger the chunk finish, and the profiler shouldn't restart fixture.executor.runAll() assertFalse(profiler.isRunning) + assertEquals(SentryId.EMPTY_ID, profiler.profilerId) + assertEquals(SentryId.EMPTY_ID, profiler.chunkId) } @Test @@ -397,6 +402,7 @@ class AndroidContinuousProfilerTest { val profiler = fixture.getSut() profiler.startProfiler(ProfileLifecycle.MANUAL, fixture.mockTracesSampler) assertTrue(profiler.isRunning) + val oldChunkId = profiler.chunkId fixture.executor.runAll() verify(fixture.mockLogger) @@ -407,6 +413,7 @@ class AndroidContinuousProfilerTest { verify(fixture.mockLogger, times(2)) .log(eq(SentryLevel.DEBUG), eq("Profile chunk finished. Starting a new one.")) assertTrue(profiler.isRunning) + assertNotEquals(oldChunkId, profiler.chunkId) } @Test @@ -508,6 +515,7 @@ class AndroidContinuousProfilerTest { profiler.onRateLimitChanged(rateLimiter) assertFalse(profiler.isRunning) assertEquals(SentryId.EMPTY_ID, profiler.profilerId) + assertEquals(SentryId.EMPTY_ID, profiler.chunkId) verify(fixture.mockLogger) .log(eq(SentryLevel.WARNING), eq("SDK is rate limited. Stopping profiler.")) } @@ -523,6 +531,7 @@ class AndroidContinuousProfilerTest { profiler.startProfiler(ProfileLifecycle.MANUAL, fixture.mockTracesSampler) assertFalse(profiler.isRunning) assertEquals(SentryId.EMPTY_ID, profiler.profilerId) + assertEquals(SentryId.EMPTY_ID, profiler.chunkId) verify(fixture.mockLogger) .log(eq(SentryLevel.WARNING), eq("SDK is rate limited. Stopping profiler.")) } @@ -541,6 +550,7 @@ class AndroidContinuousProfilerTest { profiler.startProfiler(ProfileLifecycle.MANUAL, fixture.mockTracesSampler) assertFalse(profiler.isRunning) assertEquals(SentryId.EMPTY_ID, profiler.profilerId) + assertEquals(SentryId.EMPTY_ID, profiler.chunkId) verify(fixture.mockLogger) .log(eq(SentryLevel.WARNING), eq("Device is offline. Stopping profiler.")) } diff --git a/sentry-android-core/src/test/java/io/sentry/android/core/AndroidOptionsInitializerTest.kt b/sentry-android-core/src/test/java/io/sentry/android/core/AndroidOptionsInitializerTest.kt index 79b5ce39be1..d49a905772d 100644 --- a/sentry-android-core/src/test/java/io/sentry/android/core/AndroidOptionsInitializerTest.kt +++ b/sentry-android-core/src/test/java/io/sentry/android/core/AndroidOptionsInitializerTest.kt @@ -12,6 +12,7 @@ import io.sentry.IConnectionStatusProvider import io.sentry.IContinuousProfiler import io.sentry.ILogger import io.sentry.ISocketTagger +import io.sentry.ITransaction import io.sentry.ITransactionProfiler import io.sentry.MainEventProcessor import io.sentry.NoOpContinuousProfiler @@ -34,6 +35,7 @@ import io.sentry.cache.PersistingScopeObserver import io.sentry.compose.gestures.ComposeGestureTargetLocator import io.sentry.internal.debugmeta.IDebugMetaLoader import io.sentry.internal.modules.IModulesLoader +import io.sentry.protocol.SentryId import io.sentry.test.ImmediateExecutorService import io.sentry.transport.ITransportGate import io.sentry.util.thread.IThreadChecker @@ -426,6 +428,33 @@ class AndroidOptionsInitializerTest { assertNull(AppStartMetrics.getInstance().appStartContinuousProfiler) } + @Test + fun `init starts performance collector if continuous profiler of appStartMetrics is running`() { + val appStartContinuousProfiler = mock() + val mockPerformanceCollector = mock() + val chunkId = SentryId() + whenever(appStartContinuousProfiler.isRunning()).thenReturn(true) + whenever(appStartContinuousProfiler.chunkId).thenReturn(chunkId) + + AppStartMetrics.getInstance().appStartContinuousProfiler = appStartContinuousProfiler + fixture.initSut(configureOptions = { compositePerformanceCollector = mockPerformanceCollector }) + + verify(mockPerformanceCollector).start(eq(chunkId.toString())) + } + + @Test + fun `init does not start performance collector if transaction profiler of appStartMetrics is running`() { + val appStartTransactionProfiler = mock() + val mockPerformanceCollector = mock() + whenever(appStartTransactionProfiler.isRunning()).thenReturn(true) + + AppStartMetrics.getInstance().appStartProfiler = appStartTransactionProfiler + fixture.initSut(configureOptions = { compositePerformanceCollector = mockPerformanceCollector }) + + verify(mockPerformanceCollector, never()).start(any()) + verify(mockPerformanceCollector, never()).start(any()) + } + @Test fun `init with transaction profiling closes continuous profiler of appStartMetrics`() { val appStartContinuousProfiler = mock() diff --git a/sentry/api/sentry.api b/sentry/api/sentry.api index 0f478c95d57..e32a335e881 100644 --- a/sentry/api/sentry.api +++ b/sentry/api/sentry.api @@ -760,6 +760,7 @@ public abstract interface class io/sentry/IConnectionStatusProvider$IConnectionS public abstract interface class io/sentry/IContinuousProfiler { public abstract fun close (Z)V + public abstract fun getChunkId ()Lio/sentry/protocol/SentryId; public abstract fun getProfilerId ()Lio/sentry/protocol/SentryId; public abstract fun isRunning ()Z public abstract fun reevaluateSampling ()V @@ -1471,6 +1472,7 @@ public final class io/sentry/NoOpConnectionStatusProvider : io/sentry/IConnectio public final class io/sentry/NoOpContinuousProfiler : io/sentry/IContinuousProfiler { public fun close (Z)V + public fun getChunkId ()Lio/sentry/protocol/SentryId; public static fun getInstance ()Lio/sentry/NoOpContinuousProfiler; public fun getProfilerId ()Lio/sentry/protocol/SentryId; public fun isRunning ()Z diff --git a/sentry/src/main/java/io/sentry/IContinuousProfiler.java b/sentry/src/main/java/io/sentry/IContinuousProfiler.java index 3abca9822aa..f7e59362273 100644 --- a/sentry/src/main/java/io/sentry/IContinuousProfiler.java +++ b/sentry/src/main/java/io/sentry/IContinuousProfiler.java @@ -25,4 +25,7 @@ void startProfiler( @NotNull SentryId getProfilerId(); + + @NotNull + SentryId getChunkId(); } diff --git a/sentry/src/main/java/io/sentry/NoOpContinuousProfiler.java b/sentry/src/main/java/io/sentry/NoOpContinuousProfiler.java index 893eb914ad9..4cda59e7c33 100644 --- a/sentry/src/main/java/io/sentry/NoOpContinuousProfiler.java +++ b/sentry/src/main/java/io/sentry/NoOpContinuousProfiler.java @@ -36,4 +36,9 @@ public void reevaluateSampling() {} public @NotNull SentryId getProfilerId() { return SentryId.EMPTY_ID; } + + @Override + public @NotNull SentryId getChunkId() { + return SentryId.EMPTY_ID; + } } diff --git a/sentry/src/test/java/io/sentry/NoOpContinuousProfilerTest.kt b/sentry/src/test/java/io/sentry/NoOpContinuousProfilerTest.kt index 5c32f4f6a71..eeae9bdcca1 100644 --- a/sentry/src/test/java/io/sentry/NoOpContinuousProfilerTest.kt +++ b/sentry/src/test/java/io/sentry/NoOpContinuousProfilerTest.kt @@ -25,6 +25,11 @@ class NoOpContinuousProfilerTest { assertEquals(profiler.profilerId, SentryId.EMPTY_ID) } + @Test + fun `getChunkId returns Empty SentryId`() { + assertEquals(profiler.chunkId, SentryId.EMPTY_ID) + } + @Test fun `reevaluateSampling does not throw`() { profiler.reevaluateSampling()