From 6a4b1ad68588eedc6b0d3b9ac89ee8caff68727c Mon Sep 17 00:00:00 2001 From: stefanosiano Date: Tue, 25 Mar 2025 15:58:34 +0100 Subject: [PATCH 1/9] added `platform` to SentryEnvelopeItemHeader Added platform "android" in ProfileChunk envelope items --- sentry/api/sentry.api | 4 ++- .../java/io/sentry/SentryEnvelopeItem.java | 4 ++- .../io/sentry/SentryEnvelopeItemHeader.java | 31 +++++++++++++++++-- .../java/io/sentry/SentryEnvelopeItemTest.kt | 11 +++++++ ...ntryEnvelopeItemHeaderSerializationTest.kt | 3 +- .../json/sentry_envelope_item_header.json | 1 + 6 files changed, 49 insertions(+), 5 deletions(-) diff --git a/sentry/api/sentry.api b/sentry/api/sentry.api index ebc10beb0cd..8ea97a7430c 100644 --- a/sentry/api/sentry.api +++ b/sentry/api/sentry.api @@ -2762,11 +2762,12 @@ public final class io/sentry/SentryEnvelopeItem { } public final class io/sentry/SentryEnvelopeItemHeader : io/sentry/JsonSerializable, io/sentry/JsonUnknown { - public fun (Lio/sentry/SentryItemType;ILjava/lang/String;Ljava/lang/String;Ljava/lang/String;)V + public fun (Lio/sentry/SentryItemType;ILjava/lang/String;Ljava/lang/String;Ljava/lang/String;Ljava/lang/String;)V public fun getAttachmentType ()Ljava/lang/String; public fun getContentType ()Ljava/lang/String; public fun getFileName ()Ljava/lang/String; public fun getLength ()I + public fun getPlatform ()Ljava/lang/String; public fun getType ()Lio/sentry/SentryItemType; public fun getUnknown ()Ljava/util/Map; public fun serialize (Lio/sentry/ObjectWriter;Lio/sentry/ILogger;)V @@ -2784,6 +2785,7 @@ public final class io/sentry/SentryEnvelopeItemHeader$JsonKeys { public static final field CONTENT_TYPE Ljava/lang/String; public static final field FILENAME Ljava/lang/String; public static final field LENGTH Ljava/lang/String; + public static final field PLATFORM Ljava/lang/String; public static final field TYPE Ljava/lang/String; public fun ()V } diff --git a/sentry/src/main/java/io/sentry/SentryEnvelopeItem.java b/sentry/src/main/java/io/sentry/SentryEnvelopeItem.java index 62892e3ed44..353e3eb1ecb 100644 --- a/sentry/src/main/java/io/sentry/SentryEnvelopeItem.java +++ b/sentry/src/main/java/io/sentry/SentryEnvelopeItem.java @@ -302,7 +302,9 @@ private static void ensureAttachmentSizeLimit( SentryItemType.ProfileChunk, () -> cachedItem.getBytes().length, "application-json", - traceFile.getName()); + traceFile.getName(), + null, + "android"); // avoid method refs on Android due to some issues with older AGP setups // noinspection Convert2MethodRef diff --git a/sentry/src/main/java/io/sentry/SentryEnvelopeItemHeader.java b/sentry/src/main/java/io/sentry/SentryEnvelopeItemHeader.java index 6903d9b1bb9..b999ddca416 100644 --- a/sentry/src/main/java/io/sentry/SentryEnvelopeItemHeader.java +++ b/sentry/src/main/java/io/sentry/SentryEnvelopeItemHeader.java @@ -15,6 +15,7 @@ public final class SentryEnvelopeItemHeader implements JsonSerializable, JsonUnk private final @Nullable String contentType; private final @Nullable String fileName; + private final @Nullable String platform; private final @NotNull SentryItemType type; private final int length; @Nullable private final Callable getLength; @@ -46,19 +47,25 @@ public int getLength() { return fileName; } + public @Nullable String getPlatform() { + return platform; + } + @ApiStatus.Internal public SentryEnvelopeItemHeader( final @NotNull SentryItemType type, int length, final @Nullable String contentType, final @Nullable String fileName, - final @Nullable String attachmentType) { + final @Nullable String attachmentType, + final @Nullable String platform) { this.type = Objects.requireNonNull(type, "type is required"); this.contentType = contentType; this.length = length; this.fileName = fileName; this.getLength = null; this.attachmentType = attachmentType; + this.platform = platform; } SentryEnvelopeItemHeader( @@ -67,12 +74,23 @@ public SentryEnvelopeItemHeader( final @Nullable String contentType, final @Nullable String fileName, final @Nullable String attachmentType) { + this(type, getLength, contentType, fileName, attachmentType, null); + } + + SentryEnvelopeItemHeader( + final @NotNull SentryItemType type, + final @Nullable Callable getLength, + final @Nullable String contentType, + final @Nullable String fileName, + final @Nullable String attachmentType, + final @Nullable String platform) { this.type = Objects.requireNonNull(type, "type is required"); this.contentType = contentType; this.length = -1; this.fileName = fileName; this.getLength = getLength; this.attachmentType = attachmentType; + this.platform = platform; } SentryEnvelopeItemHeader( @@ -100,6 +118,7 @@ public static final class JsonKeys { public static final String TYPE = "type"; public static final String ATTACHMENT_TYPE = "attachment_type"; public static final String LENGTH = "length"; + public static final String PLATFORM = "platform"; } @Override @@ -116,6 +135,9 @@ public void serialize(final @NotNull ObjectWriter writer, final @NotNull ILogger if (attachmentType != null) { writer.name(JsonKeys.ATTACHMENT_TYPE).value(attachmentType); } + if (platform != null) { + writer.name(JsonKeys.PLATFORM).value(platform); + } writer.name(JsonKeys.LENGTH).value(getLength()); if (unknown != null) { for (String key : unknown.keySet()) { @@ -138,6 +160,7 @@ public static final class Deserializer implements JsonDeserializer unknown = null; while (reader.peek() == JsonToken.NAME) { @@ -158,6 +181,9 @@ public static final class Deserializer implements JsonDeserializer(); @@ -170,7 +196,8 @@ public static final class Deserializer implements JsonDeserializer { + whenever(it.traceFile).thenReturn(file) + } + + val chunk = SentryEnvelopeItem.fromProfileChunk(profileChunk, mock()) + assertEquals("android", chunk.header.platform) + } + @Test fun `fromProfileChunk saves file as Base64`() { val file = File(fixture.pathname) diff --git a/sentry/src/test/java/io/sentry/protocol/SentryEnvelopeItemHeaderSerializationTest.kt b/sentry/src/test/java/io/sentry/protocol/SentryEnvelopeItemHeaderSerializationTest.kt index 5456b27e8a4..3e303be2d28 100644 --- a/sentry/src/test/java/io/sentry/protocol/SentryEnvelopeItemHeaderSerializationTest.kt +++ b/sentry/src/test/java/io/sentry/protocol/SentryEnvelopeItemHeaderSerializationTest.kt @@ -23,7 +23,8 @@ class SentryEnvelopeItemHeaderSerializationTest { 345, "5def420f-3dac-4d7b-948b-49de6e551aef", "54cf4644-8610-4ff3-a535-34ac1f367501", - "6f49ad85-a017-4d94-a5d7-6477251da602" + "6f49ad85-a017-4d94-a5d7-6477251da602", + "android" ) } private val fixture = Fixture() diff --git a/sentry/src/test/resources/json/sentry_envelope_item_header.json b/sentry/src/test/resources/json/sentry_envelope_item_header.json index e4e8e173ca8..a3130fc1b38 100644 --- a/sentry/src/test/resources/json/sentry_envelope_item_header.json +++ b/sentry/src/test/resources/json/sentry_envelope_item_header.json @@ -3,5 +3,6 @@ "filename": "54cf4644-8610-4ff3-a535-34ac1f367501", "type": "event", "attachment_type": "6f49ad85-a017-4d94-a5d7-6477251da602", + "platform": "android", "length": 345 } From f21dc6caa9de4b3267b0a7f7909717f2275e903e Mon Sep 17 00:00:00 2001 From: stefanosiano Date: Tue, 25 Mar 2025 17:40:51 +0100 Subject: [PATCH 2/9] updated changelog --- CHANGELOG.md | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 792816b4edd..760b600d506 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,11 @@ - Do not override user-defined `SentryOptions` ([#4262](https://github.com/getsentry/sentry-java/pull/4262)) - Session Replay: Change bitmap config to `ARGB_8888` for screenshots ([#4282](https://github.com/getsentry/sentry-java/pull/4282)) +### Internal + +- Added `platform` to SentryEnvelopeItemHeader ([#4287](https://github.com/getsentry/sentry-java/pull/4287)) + - Set `android` platform to ProfileChunk envelope item header + ### Dependencies - Bump Native SDK from v0.8.1 to v0.8.2 ([#4267](https://github.com/getsentry/sentry-java/pull/4267)) From a9d2ae4248808c626ccf0e0cd9032ffa7f645dbb Mon Sep 17 00:00:00 2001 From: stefanosiano Date: Wed, 26 Mar 2025 12:50:39 +0100 Subject: [PATCH 3/9] updated item header to chunk platform --- sentry/src/main/java/io/sentry/SentryEnvelopeItem.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sentry/src/main/java/io/sentry/SentryEnvelopeItem.java b/sentry/src/main/java/io/sentry/SentryEnvelopeItem.java index 353e3eb1ecb..9a76d118a92 100644 --- a/sentry/src/main/java/io/sentry/SentryEnvelopeItem.java +++ b/sentry/src/main/java/io/sentry/SentryEnvelopeItem.java @@ -304,7 +304,7 @@ private static void ensureAttachmentSizeLimit( "application-json", traceFile.getName(), null, - "android"); + profileChunk.getPlatform()); // avoid method refs on Android due to some issues with older AGP setups // noinspection Convert2MethodRef From 94079a455cde27bc524ee6b4d0afd795646f8e5e Mon Sep 17 00:00:00 2001 From: stefanosiano Date: Wed, 26 Mar 2025 13:07:18 +0100 Subject: [PATCH 4/9] fixed test --- sentry/src/test/java/io/sentry/SentryEnvelopeItemTest.kt | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/sentry/src/test/java/io/sentry/SentryEnvelopeItemTest.kt b/sentry/src/test/java/io/sentry/SentryEnvelopeItemTest.kt index f9c6df4e52f..48df7ff0903 100644 --- a/sentry/src/test/java/io/sentry/SentryEnvelopeItemTest.kt +++ b/sentry/src/test/java/io/sentry/SentryEnvelopeItemTest.kt @@ -467,10 +467,11 @@ class SentryEnvelopeItemTest { val file = File(fixture.pathname) val profileChunk = mock { whenever(it.traceFile).thenReturn(file) + whenever(it.platform).thenReturn("chunk platform") } val chunk = SentryEnvelopeItem.fromProfileChunk(profileChunk, mock()) - assertEquals("android", chunk.header.platform) + assertEquals("chunk platform", chunk.header.platform) } @Test From 96c57200a8826b6e64ec119910d4a91f286cc201 Mon Sep 17 00:00:00 2001 From: stefanosiano Date: Thu, 27 Mar 2025 11:29:44 +0100 Subject: [PATCH 5/9] replaced synchronized blocks with AutoClosableReentrantLock in AndroidContinuousProfiler Added "delayed" stop of profiler, which stops the profiler after the current chunk is finished Added default span data (profiler id, thread name and thread id) to transaction root span --- .../core/AndroidContinuousProfiler.java | 237 ++++++++++-------- .../core/AndroidContinuousProfilerTest.kt | 100 ++++---- .../src/main/kotlin/io/sentry/test/Mocks.kt | 5 +- .../src/main/java/io/sentry/SentryTracer.java | 24 +- .../test/java/io/sentry/SentryTracerTest.kt | 9 + 5 files changed, 198 insertions(+), 177 deletions(-) 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 0c66e1adfdf..d5f5f6a3543 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 @@ -11,6 +11,7 @@ import io.sentry.ILogger; import io.sentry.IScopes; import io.sentry.ISentryExecutorService; +import io.sentry.ISentryLifecycleToken; import io.sentry.NoOpScopes; import io.sentry.PerformanceCollectionData; import io.sentry.ProfileChunk; @@ -24,6 +25,7 @@ import io.sentry.android.core.internal.util.SentryFrameMetricsCollector; import io.sentry.protocol.SentryId; import io.sentry.transport.RateLimiter; +import io.sentry.util.AutoClosableReentrantLock; import io.sentry.util.SentryRandom; import java.util.ArrayList; import java.util.List; @@ -58,9 +60,13 @@ public class AndroidContinuousProfiler private final @NotNull AtomicBoolean isClosed = new AtomicBoolean(false); private @NotNull SentryDate startProfileChunkTimestamp = new SentryNanotimeDate(); private boolean shouldSample = true; + private boolean shouldStop = false; private boolean isSampled = false; private int rootSpanCounter = 0; + private final AutoClosableReentrantLock lock = new AutoClosableReentrantLock(); + private final AutoClosableReentrantLock payloadLock = new AutoClosableReentrantLock(); + public AndroidContinuousProfiler( final @NotNull BuildInfoProvider buildInfoProvider, final @NotNull SentryFrameMetricsCollector frameMetricsCollector, @@ -106,42 +112,46 @@ private void init() { } @Override - public synchronized void startProfiler( + public void startProfiler( final @NotNull ProfileLifecycle profileLifecycle, final @NotNull TracesSampler tracesSampler) { - if (shouldSample) { - isSampled = tracesSampler.sampleSessionProfile(SentryRandom.current().nextDouble()); - shouldSample = false; - } - if (!isSampled) { - logger.log(SentryLevel.DEBUG, "Profiler was not started due to sampling decision."); - return; - } - switch (profileLifecycle) { - case TRACE: - // rootSpanCounter should never be negative, unless the user changed profile lifecycle while - // the profiler is running or close() is called. This is just a safety check. - if (rootSpanCounter < 0) { - rootSpanCounter = 0; - } - rootSpanCounter++; - break; - case MANUAL: - // We check if the profiler is already running and log a message only in manual mode, since - // in trace mode we can have multiple concurrent traces - if (isRunning()) { - logger.log(SentryLevel.DEBUG, "Profiler is already running."); - return; - } - break; - } - if (!isRunning()) { - logger.log(SentryLevel.DEBUG, "Started Profiler."); - start(); + try (final @NotNull ISentryLifecycleToken ignored = lock.acquire()) { + if (shouldSample) { + isSampled = tracesSampler.sampleSessionProfile(SentryRandom.current().nextDouble()); + shouldSample = false; + } + if (!isSampled) { + logger.log(SentryLevel.DEBUG, "Profiler was not started due to sampling decision."); + return; + } + switch (profileLifecycle) { + case TRACE: + // rootSpanCounter should never be negative, unless the user changed profile lifecycle + // while + // the profiler is running or close() is called. This is just a safety check. + if (rootSpanCounter < 0) { + rootSpanCounter = 0; + } + rootSpanCounter++; + break; + case MANUAL: + // We check if the profiler is already running and log a message only in manual mode, + // since + // in trace mode we can have multiple concurrent traces + if (isRunning()) { + logger.log(SentryLevel.DEBUG, "Profiler is already running."); + return; + } + break; + } + if (!isRunning()) { + logger.log(SentryLevel.DEBUG, "Started Profiler."); + start(); + } } } - private synchronized void start() { + private void start() { if ((scopes == null || scopes == NoOpScopes.getInstance()) && Sentry.getCurrentScopes() != NoOpScopes.getInstance()) { this.scopes = Sentry.getCurrentScopes(); @@ -213,103 +223,112 @@ private synchronized void start() { SentryLevel.ERROR, "Failed to schedule profiling chunk finish. Did you call Sentry.close()?", e); + shouldStop = true; } } @Override - public synchronized void stopProfiler(final @NotNull ProfileLifecycle profileLifecycle) { - switch (profileLifecycle) { - case TRACE: - rootSpanCounter--; - // If there are active spans, and profile lifecycle is trace, we don't stop the profiler - if (rootSpanCounter > 0) { - return; - } - // rootSpanCounter should never be negative, unless the user changed profile lifecycle while - // the profiler is running or close() is called. This is just a safety check. - if (rootSpanCounter < 0) { - rootSpanCounter = 0; - } - stop(false); - break; - case MANUAL: - stop(false); - break; + public void stopProfiler(final @NotNull ProfileLifecycle profileLifecycle) { + try (final @NotNull ISentryLifecycleToken ignored = lock.acquire()) { + switch (profileLifecycle) { + case TRACE: + rootSpanCounter--; + // If there are active spans, and profile lifecycle is trace, we don't stop the profiler + if (rootSpanCounter > 0) { + return; + } + // rootSpanCounter should never be negative, unless the user changed profile lifecycle + // while the profiler is running or close() is called. This is just a safety check. + if (rootSpanCounter < 0) { + rootSpanCounter = 0; + } + shouldStop = true; + break; + case MANUAL: + shouldStop = true; + break; + } } } - private synchronized void stop(final boolean restartProfiler) { - if (stopFuture != null) { - stopFuture.cancel(true); - } - // check if profiler was created and it's running - if (profiler == null || !isRunning) { - // When the profiler is stopped due to an error (e.g. offline or rate limited), reset the ids - profilerId = SentryId.EMPTY_ID; - chunkId = SentryId.EMPTY_ID; - return; - } - - // onTransactionStart() is only available since Lollipop_MR1 - // and SystemClock.elapsedRealtimeNanos() since Jelly Bean - if (buildInfoProvider.getSdkInfoVersion() < Build.VERSION_CODES.LOLLIPOP_MR1) { - return; - } + private void stop(final boolean restartProfiler) { + try (final @NotNull ISentryLifecycleToken ignored = lock.acquire()) { + if (stopFuture != null) { + stopFuture.cancel(true); + } + // check if profiler was created and it's running + if (profiler == null || !isRunning) { + // When the profiler is stopped due to an error (e.g. offline or rate limited), reset the + // ids + profilerId = SentryId.EMPTY_ID; + chunkId = SentryId.EMPTY_ID; + return; + } - List performanceCollectionData = null; - if (performanceCollector != null) { - performanceCollectionData = performanceCollector.stop(chunkId.toString()); - } + // onTransactionStart() is only available since Lollipop_MR1 + // and SystemClock.elapsedRealtimeNanos() since Jelly Bean + if (buildInfoProvider.getSdkInfoVersion() < Build.VERSION_CODES.LOLLIPOP_MR1) { + return; + } - final AndroidProfiler.ProfileEndData endData = - profiler.endAndCollect(false, performanceCollectionData); + List performanceCollectionData = null; + if (performanceCollector != null) { + performanceCollectionData = performanceCollector.stop(chunkId.toString()); + } - // check if profiler end successfully - if (endData == null) { - logger.log( - SentryLevel.ERROR, - "An error occurred while collecting a profile chunk, and it won't be sent."); - } else { - // The scopes can be null if the profiler is started before the SDK is initialized (app start - // profiling), meaning there's no scopes to send the chunks. In that case, we store the data - // in a list and send it when the next chunk is finished. - synchronized (payloadBuilders) { - payloadBuilders.add( - new ProfileChunk.Builder( - profilerId, - chunkId, - endData.measurementsMap, - endData.traceFile, - startProfileChunkTimestamp)); + final AndroidProfiler.ProfileEndData endData = + profiler.endAndCollect(false, performanceCollectionData); + + // check if profiler end successfully + if (endData == null) { + logger.log( + SentryLevel.ERROR, + "An error occurred while collecting a profile chunk, and it won't be sent."); + } else { + // The scopes can be null if the profiler is started before the SDK is initialized (app + // start profiling), meaning there's no scopes to send the chunks. In that case, we store + // the data in a list and send it when the next chunk is finished. + try (final @NotNull ISentryLifecycleToken ignored2 = payloadLock.acquire()) { + payloadBuilders.add( + new ProfileChunk.Builder( + profilerId, + chunkId, + endData.measurementsMap, + endData.traceFile, + startProfileChunkTimestamp)); + } } - } - isRunning = false; - // A chunk is finished. Next chunk will have a different id. - chunkId = SentryId.EMPTY_ID; + isRunning = false; + // A chunk is finished. Next chunk will have a different id. + chunkId = SentryId.EMPTY_ID; - if (scopes != null) { - sendChunks(scopes, scopes.getOptions()); - } + if (scopes != null) { + sendChunks(scopes, scopes.getOptions()); + } - if (restartProfiler) { - logger.log(SentryLevel.DEBUG, "Profile chunk finished. Starting a new one."); - start(); - } else { - // When the profiler is stopped manually, we have to reset its id - profilerId = SentryId.EMPTY_ID; - logger.log(SentryLevel.DEBUG, "Profile chunk finished."); + if (restartProfiler && !shouldStop) { + logger.log(SentryLevel.DEBUG, "Profile chunk finished. Starting a new one."); + start(); + } else { + // When the profiler is stopped manually, we have to reset its id + profilerId = SentryId.EMPTY_ID; + logger.log(SentryLevel.DEBUG, "Profile chunk finished."); + } } } - public synchronized void reevaluateSampling() { + public void reevaluateSampling() { shouldSample = true; } - public synchronized void close() { - rootSpanCounter = 0; - stop(false); - isClosed.set(true); + public void close() { + try (final @NotNull ISentryLifecycleToken ignored = lock.acquire()) { + rootSpanCounter = 0; + shouldStop = true; + stop(false); + isClosed.set(true); + } } @Override @@ -328,7 +347,7 @@ private void sendChunks(final @NotNull IScopes scopes, final @NotNull SentryOpti return; } final ArrayList payloads = new ArrayList<>(payloadBuilders.size()); - synchronized (payloadBuilders) { + try (final @NotNull ISentryLifecycleToken ignored = payloadLock.acquire()) { for (ProfileChunk.Builder builder : payloadBuilders) { payloads.add(builder.build(options)); } 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 daf7c84d156..4e1b45ebb02 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 @@ -10,7 +10,6 @@ import io.sentry.DataCategory import io.sentry.IConnectionStatusProvider import io.sentry.ILogger import io.sentry.IScopes -import io.sentry.ISentryExecutorService import io.sentry.MemoryCollectionData import io.sentry.PerformanceCollectionData import io.sentry.ProfileLifecycle @@ -39,7 +38,6 @@ import org.mockito.kotlin.times import org.mockito.kotlin.verify import org.mockito.kotlin.whenever import java.io.File -import java.util.concurrent.Callable import java.util.concurrent.Future import kotlin.test.AfterTest import kotlin.test.BeforeTest @@ -61,6 +59,7 @@ class AndroidContinuousProfilerTest { val buildInfo = mock { whenever(it.sdkInfoVersion).thenReturn(Build.VERSION_CODES.LOLLIPOP_MR1) } + val executor = DeferredExecutorService() val mockedSentry = mockStatic(Sentry::class.java) val mockLogger = mock() val mockTracesSampler = mock() @@ -84,6 +83,7 @@ class AndroidContinuousProfilerTest { } fun getSut(buildInfoProvider: BuildInfoProvider = buildInfo, optionConfig: ((options: SentryAndroidOptions) -> Unit) = {}): AndroidContinuousProfiler { + options.executorService = executor optionConfig(options) whenever(scopes.options).thenReturn(options) transaction1 = SentryTracer(TransactionContext("", ""), scopes) @@ -152,6 +152,20 @@ class AndroidContinuousProfilerTest { profiler.startProfiler(ProfileLifecycle.MANUAL, fixture.mockTracesSampler) assertTrue(profiler.isRunning) profiler.stopProfiler(ProfileLifecycle.MANUAL) + fixture.executor.runAll() + assertFalse(profiler.isRunning) + } + + @Test + fun `stopProfiler stops the profiler after chunk is finished`() { + val profiler = fixture.getSut() + profiler.startProfiler(ProfileLifecycle.MANUAL, fixture.mockTracesSampler) + assertTrue(profiler.isRunning) + // 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) + // We run the executor service to trigger the chunk finish, and the profiler shouldn't restart + fixture.executor.runAll() assertFalse(profiler.isRunning) } @@ -183,11 +197,13 @@ class AndroidContinuousProfilerTest { // rootSpanCounter is decremented when the profiler stops in trace mode, and keeps running until rootSpanCounter is 0 profiler.stopProfiler(ProfileLifecycle.TRACE) + fixture.executor.runAll() assertEquals(1, profiler.rootSpanCounter) assertTrue(profiler.isRunning) // only when rootSpanCounter is 0 the profiler stops profiler.stopProfiler(ProfileLifecycle.TRACE) + fixture.executor.runAll() assertEquals(0, profiler.rootSpanCounter) assertFalse(profiler.isRunning) } @@ -316,19 +332,6 @@ class AndroidContinuousProfilerTest { assertFalse(profiler.isRunning) } - @Test - fun `profiler never use background threads`() { - val mockExecutorService: ISentryExecutorService = mock() - val profiler = fixture.getSut { - it.executorService = mockExecutorService - } - whenever(mockExecutorService.submit(any>())).thenReturn(mock()) - profiler.startProfiler(ProfileLifecycle.MANUAL, fixture.mockTracesSampler) - verify(mockExecutorService, never()).submit(any()) - profiler.stopProfiler(ProfileLifecycle.MANUAL) - verify(mockExecutorService, never()).submit(any>()) - } - @Test fun `profiler does not throw if traces cannot be written to disk`() { val profiler = fixture.getSut { @@ -336,6 +339,7 @@ class AndroidContinuousProfilerTest { } profiler.startProfiler(ProfileLifecycle.MANUAL, fixture.mockTracesSampler) profiler.stopProfiler(ProfileLifecycle.MANUAL) + fixture.executor.runAll() // We assert that no trace files are written assertTrue( File(fixture.options.profilingTracesDirPath!!) @@ -363,6 +367,7 @@ class AndroidContinuousProfilerTest { profiler.startProfiler(ProfileLifecycle.MANUAL, fixture.mockTracesSampler) verify(performanceCollector, never()).stop(any()) profiler.stopProfiler(ProfileLifecycle.MANUAL) + fixture.executor.runAll() verify(performanceCollector).stop(any()) } @@ -374,6 +379,7 @@ class AndroidContinuousProfilerTest { profiler.startProfiler(ProfileLifecycle.MANUAL, fixture.mockTracesSampler) verify(fixture.frameMetricsCollector, never()).stopCollection(frameMetricsCollectorId) profiler.stopProfiler(ProfileLifecycle.MANUAL) + fixture.executor.runAll() verify(fixture.frameMetricsCollector).stopCollection(frameMetricsCollectorId) } @@ -393,46 +399,39 @@ class AndroidContinuousProfilerTest { val stopFuture = profiler.stopFuture assertNotNull(stopFuture) - assertTrue(stopFuture.isCancelled) + assertTrue(stopFuture.isCancelled || stopFuture.isDone) } @Test fun `profiler stops and restart for each chunk`() { - val executorService = DeferredExecutorService() - val profiler = fixture.getSut { - it.executorService = executorService - } + val profiler = fixture.getSut() profiler.startProfiler(ProfileLifecycle.MANUAL, fixture.mockTracesSampler) assertTrue(profiler.isRunning) - executorService.runAll() + fixture.executor.runAll() verify(fixture.mockLogger).log(eq(SentryLevel.DEBUG), eq("Profile chunk finished. Starting a new one.")) assertTrue(profiler.isRunning) - executorService.runAll() + fixture.executor.runAll() verify(fixture.mockLogger, times(2)).log(eq(SentryLevel.DEBUG), eq("Profile chunk finished. Starting a new one.")) assertTrue(profiler.isRunning) } @Test fun `profiler sends chunk on each restart`() { - val executorService = DeferredExecutorService() - val profiler = fixture.getSut { - it.executorService = executorService - } + val profiler = fixture.getSut() profiler.startProfiler(ProfileLifecycle.MANUAL, fixture.mockTracesSampler) assertTrue(profiler.isRunning) // We run the executor service to trigger the profiler restart (chunk finish) - executorService.runAll() + fixture.executor.runAll() verify(fixture.scopes, never()).captureProfileChunk(any()) // Now the executor is used to send the chunk - executorService.runAll() + fixture.executor.runAll() verify(fixture.scopes).captureProfileChunk(any()) } @Test fun `profiler sends chunk with measurements`() { - val executorService = DeferredExecutorService() val performanceCollector = mock() val collectionData = PerformanceCollectionData() @@ -441,13 +440,13 @@ class AndroidContinuousProfilerTest { whenever(performanceCollector.stop(any())).thenReturn(listOf(collectionData)) fixture.options.compositePerformanceCollector = performanceCollector - val profiler = fixture.getSut { - it.executorService = executorService - } + val profiler = fixture.getSut() profiler.startProfiler(ProfileLifecycle.MANUAL, fixture.mockTracesSampler) profiler.stopProfiler(ProfileLifecycle.MANUAL) - // We run the executor service to send the profile chunk - executorService.runAll() + // We run the executor service to stop the profiler + fixture.executor.runAll() + // Then we run it again to send the profile chunk + fixture.executor.runAll() verify(fixture.scopes).captureProfileChunk( check { assertContains(it.measurements, ProfileMeasurement.ID_CPU_USAGE) @@ -459,28 +458,21 @@ class AndroidContinuousProfilerTest { @Test fun `profiler sends another chunk on stop`() { - val executorService = DeferredExecutorService() - val profiler = fixture.getSut { - it.executorService = executorService - } + val profiler = fixture.getSut() profiler.startProfiler(ProfileLifecycle.MANUAL, fixture.mockTracesSampler) assertTrue(profiler.isRunning) // We run the executor service to trigger the profiler restart (chunk finish) - executorService.runAll() + fixture.executor.runAll() verify(fixture.scopes, never()).captureProfileChunk(any()) - // We stop the profiler, which should send an additional chunk profiler.stopProfiler(ProfileLifecycle.MANUAL) - // Now the executor is used to send the chunk - executorService.runAll() - verify(fixture.scopes, times(2)).captureProfileChunk(any()) + // We stop the profiler, which should send a chunk + fixture.executor.runAll() + verify(fixture.scopes).captureProfileChunk(any()) } @Test fun `profiler does not send chunks after close`() { - val executorService = DeferredExecutorService() - val profiler = fixture.getSut { - it.executorService = executorService - } + val profiler = fixture.getSut() profiler.startProfiler(ProfileLifecycle.MANUAL, fixture.mockTracesSampler) assertTrue(profiler.isRunning) @@ -488,16 +480,13 @@ class AndroidContinuousProfilerTest { profiler.close() // The executor used to send the chunk doesn't do anything - executorService.runAll() + fixture.executor.runAll() verify(fixture.scopes, never()).captureProfileChunk(any()) } @Test fun `profiler stops when rate limited`() { - val executorService = DeferredExecutorService() - val profiler = fixture.getSut { - it.executorService = executorService - } + val profiler = fixture.getSut() val rateLimiter = mock() whenever(rateLimiter.isActiveForCategory(DataCategory.ProfileChunk)).thenReturn(true) @@ -513,10 +502,7 @@ class AndroidContinuousProfilerTest { @Test fun `profiler does not start when rate limited`() { - val executorService = DeferredExecutorService() - val profiler = fixture.getSut { - it.executorService = executorService - } + val profiler = fixture.getSut() val rateLimiter = mock() whenever(rateLimiter.isActiveForCategory(DataCategory.ProfileChunk)).thenReturn(true) whenever(fixture.scopes.rateLimiter).thenReturn(rateLimiter) @@ -530,9 +516,7 @@ class AndroidContinuousProfilerTest { @Test fun `profiler does not start when offline`() { - val executorService = DeferredExecutorService() val profiler = fixture.getSut { - it.executorService = executorService it.connectionStatusProvider = mock { provider -> whenever(provider.connectionStatus).thenReturn(IConnectionStatusProvider.ConnectionStatus.DISCONNECTED) } diff --git a/sentry-test-support/src/main/kotlin/io/sentry/test/Mocks.kt b/sentry-test-support/src/main/kotlin/io/sentry/test/Mocks.kt index b30fe3464f8..d048b42d428 100644 --- a/sentry-test-support/src/main/kotlin/io/sentry/test/Mocks.kt +++ b/sentry-test-support/src/main/kotlin/io/sentry/test/Mocks.kt @@ -13,6 +13,7 @@ import org.mockito.kotlin.mock import org.mockito.kotlin.whenever import java.util.concurrent.Callable import java.util.concurrent.Future +import java.util.concurrent.FutureTask import java.util.concurrent.atomic.AtomicBoolean class ImmediateExecutorService : ISentryExecutorService { @@ -58,7 +59,7 @@ class DeferredExecutorService : ISentryExecutorService { synchronized(this) { runnables.add(runnable) } - return mock() + return FutureTask {} } override fun submit(callable: Callable): Future = mock() @@ -66,7 +67,7 @@ class DeferredExecutorService : ISentryExecutorService { synchronized(this) { scheduledRunnables.add(runnable) } - return mock() + return FutureTask {} } override fun close(timeoutMillis: Long) {} override fun isClosed(): Boolean = false diff --git a/sentry/src/main/java/io/sentry/SentryTracer.java b/sentry/src/main/java/io/sentry/SentryTracer.java index 8da554cd3e9..0496f407219 100644 --- a/sentry/src/main/java/io/sentry/SentryTracer.java +++ b/sentry/src/main/java/io/sentry/SentryTracer.java @@ -81,6 +81,8 @@ public SentryTracer( this.transactionNameSource = context.getTransactionNameSource(); this.transactionOptions = transactionOptions; + setDefaultSpanData(root); + final @NotNull SentryId continuousProfilerId = scopes.getOptions().getContinuousProfiler().getProfilerId(); if (!continuousProfilerId.equals(SentryId.EMPTY_ID) && Boolean.TRUE.equals(isSampled())) { @@ -519,14 +521,7 @@ private ISpan createChild( // } // }); // span.setDescription(description); - final @NotNull IThreadChecker threadChecker = scopes.getOptions().getThreadChecker(); - final SentryId profilerId = scopes.getOptions().getContinuousProfiler().getProfilerId(); - if (!profilerId.equals(SentryId.EMPTY_ID) && Boolean.TRUE.equals(span.isSampled())) { - span.setData(SpanDataConvention.PROFILER_ID, profilerId.toString()); - } - span.setData( - SpanDataConvention.THREAD_ID, String.valueOf(threadChecker.currentThreadSystemId())); - span.setData(SpanDataConvention.THREAD_NAME, threadChecker.getCurrentThreadName()); + setDefaultSpanData(span); this.children.add(span); if (compositePerformanceCollector != null) { compositePerformanceCollector.onSpanStarted(span); @@ -545,6 +540,19 @@ private ISpan createChild( } } + /** Sets the default data in the span, including profiler _id, thread id and thread name */ + private void setDefaultSpanData(final @NotNull ISpan span) { + final @NotNull IThreadChecker threadChecker = scopes.getOptions().getThreadChecker(); + final @NotNull SentryId profilerId = + scopes.getOptions().getContinuousProfiler().getProfilerId(); + if (!profilerId.equals(SentryId.EMPTY_ID) && Boolean.TRUE.equals(span.isSampled())) { + span.setData(SpanDataConvention.PROFILER_ID, profilerId.toString()); + } + span.setData( + SpanDataConvention.THREAD_ID, String.valueOf(threadChecker.currentThreadSystemId())); + span.setData(SpanDataConvention.THREAD_NAME, threadChecker.getCurrentThreadName()); + } + @Override public @NotNull ISpan startChild(final @NotNull String operation) { return this.startChild(operation, (String) null); diff --git a/sentry/src/test/java/io/sentry/SentryTracerTest.kt b/sentry/src/test/java/io/sentry/SentryTracerTest.kt index 0c36034c287..347d92d4732 100644 --- a/sentry/src/test/java/io/sentry/SentryTracerTest.kt +++ b/sentry/src/test/java/io/sentry/SentryTracerTest.kt @@ -86,6 +86,15 @@ class SentryTracerTest { assertEquals("new-origin", transaction.spanContext.origin) } + @Test + fun `root span has thread name and thread id in the data`() { + val tracer = fixture.getSut() + assertTrue(tracer.root.data.containsKey(SpanDataConvention.THREAD_NAME)) + assertTrue(tracer.root.data.containsKey(SpanDataConvention.THREAD_ID)) + assertTrue(tracer.data!!.containsKey(SpanDataConvention.THREAD_NAME)) + assertTrue(tracer.data!!.containsKey(SpanDataConvention.THREAD_ID)) + } + @Test fun `does not create child span if origin is ignored`() { val tracer = fixture.getSut({ From 2d1ee74e0353bdab7eff676635f2225bab36796c Mon Sep 17 00:00:00 2001 From: stefanosiano Date: Thu, 27 Mar 2025 11:37:12 +0100 Subject: [PATCH 6/9] updated changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 760b600d506..1932af56847 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,7 @@ ### Features +- Continuous Profiling - Add delayed stop ([#4293](https://github.com/getsentry/sentry-java/pull/4293)) - Increase http timeouts from 5s to 30s to have a better chance of events being delivered without retry ([#4276](https://github.com/getsentry/sentry-java/pull/4276)) ### Fixes From dacdab4fd00284101901cd51efccd331d41e4a86 Mon Sep 17 00:00:00 2001 From: stefanosiano Date: Tue, 1 Apr 2025 17:40:02 +0200 Subject: [PATCH 7/9] app going in the background now stops the continuous profiler --- sentry-android-core/api/sentry-android-core.api | 1 + .../android/core/AndroidContinuousProfiler.java | 9 +++++++++ .../sentry/android/core/LifecycleWatcher.java | 1 + .../core/AndroidContinuousProfilerTest.kt | 17 +++++++++++++++++ .../sentry/android/core/LifecycleWatcherTest.kt | 4 ++++ sentry/api/sentry.api | 2 ++ .../java/io/sentry/IContinuousProfiler.java | 3 +++ .../java/io/sentry/NoOpContinuousProfiler.java | 3 +++ .../io/sentry/NoOpContinuousProfilerTest.kt | 4 ++++ 9 files changed, 44 insertions(+) diff --git a/sentry-android-core/api/sentry-android-core.api b/sentry-android-core/api/sentry-android-core.api index d0d139ac8c3..92f4a7cebad 100644 --- a/sentry-android-core/api/sentry-android-core.api +++ b/sentry-android-core/api/sentry-android-core.api @@ -49,6 +49,7 @@ public class io/sentry/android/core/AndroidContinuousProfiler : io/sentry/IConti public fun onRateLimitChanged (Lio/sentry/transport/RateLimiter;)V public fun reevaluateSampling ()V public fun startProfiler (Lio/sentry/ProfileLifecycle;Lio/sentry/TracesSampler;)V + public fun stopAllProfiles ()V public fun stopProfiler (Lio/sentry/ProfileLifecycle;)V } 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 d5f5f6a3543..498edfaa352 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 @@ -322,6 +322,15 @@ public void reevaluateSampling() { shouldSample = true; } + @Override + public void stopAllProfiles() { + try (final @NotNull ISentryLifecycleToken ignored = lock.acquire()) { + rootSpanCounter = 0; + shouldStop = true; + } + } + + @Override public void close() { try (final @NotNull ISentryLifecycleToken ignored = lock.acquire()) { rootSpanCounter = 0; diff --git a/sentry-android-core/src/main/java/io/sentry/android/core/LifecycleWatcher.java b/sentry-android-core/src/main/java/io/sentry/android/core/LifecycleWatcher.java index d83ecdd6752..218955bb9d8 100644 --- a/sentry-android-core/src/main/java/io/sentry/android/core/LifecycleWatcher.java +++ b/sentry-android-core/src/main/java/io/sentry/android/core/LifecycleWatcher.java @@ -122,6 +122,7 @@ public void run() { scopes.endSession(); } scopes.getOptions().getReplayController().stop(); + scopes.getOptions().getContinuousProfiler().stopAllProfiles(); } }; 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 4e1b45ebb02..4128f222dc3 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 @@ -470,6 +470,23 @@ class AndroidContinuousProfilerTest { verify(fixture.scopes).captureProfileChunk(any()) } + @Test + fun `stopAllProfiles stops all profiles after chunk is finished`() { + val profiler = fixture.getSut() + profiler.startProfiler(ProfileLifecycle.MANUAL, fixture.mockTracesSampler) + profiler.startProfiler(ProfileLifecycle.TRACE, fixture.mockTracesSampler) + assertTrue(profiler.isRunning) + // We are scheduling the profiler to stop at the end of the chunk, so it should still be running + profiler.stopAllProfiles() + assertTrue(profiler.isRunning) + // However, stopAllProfiles already resets the rootSpanCounter + assertEquals(0, profiler.rootSpanCounter) + + // We run the executor service to trigger the chunk finish, and the profiler shouldn't restart + fixture.executor.runAll() + assertFalse(profiler.isRunning) + } + @Test fun `profiler does not send chunks after close`() { val profiler = fixture.getSut() diff --git a/sentry-android-core/src/test/java/io/sentry/android/core/LifecycleWatcherTest.kt b/sentry-android-core/src/test/java/io/sentry/android/core/LifecycleWatcherTest.kt index 5f088221b9d..120cd3fc2f5 100644 --- a/sentry-android-core/src/test/java/io/sentry/android/core/LifecycleWatcherTest.kt +++ b/sentry-android-core/src/test/java/io/sentry/android/core/LifecycleWatcherTest.kt @@ -3,6 +3,7 @@ package io.sentry.android.core import androidx.lifecycle.LifecycleOwner import io.sentry.Breadcrumb import io.sentry.DateUtils +import io.sentry.IContinuousProfiler import io.sentry.IScope import io.sentry.IScopes import io.sentry.ReplayController @@ -38,6 +39,7 @@ class LifecycleWatcherTest { val dateProvider = mock() val options = SentryOptions() val replayController = mock() + val continuousProfiler = mock() fun getSUT( sessionIntervalMillis: Long = 0L, @@ -52,6 +54,7 @@ class LifecycleWatcherTest { argumentCaptor.value.run(scope) } options.setReplayController(replayController) + options.setContinuousProfiler(continuousProfiler) whenever(scopes.options).thenReturn(options) return LifecycleWatcher( @@ -106,6 +109,7 @@ class LifecycleWatcherTest { watcher.onStop(fixture.ownerMock) verify(fixture.scopes, timeout(10000)).endSession() verify(fixture.replayController, timeout(10000)).stop() + verify(fixture.continuousProfiler, timeout(10000)).stopAllProfiles() } @Test diff --git a/sentry/api/sentry.api b/sentry/api/sentry.api index 8ea97a7430c..d0cf15de0a2 100644 --- a/sentry/api/sentry.api +++ b/sentry/api/sentry.api @@ -753,6 +753,7 @@ public abstract interface class io/sentry/IContinuousProfiler { public abstract fun isRunning ()Z public abstract fun reevaluateSampling ()V public abstract fun startProfiler (Lio/sentry/ProfileLifecycle;Lio/sentry/TracesSampler;)V + public abstract fun stopAllProfiles ()V public abstract fun stopProfiler (Lio/sentry/ProfileLifecycle;)V } @@ -1437,6 +1438,7 @@ public final class io/sentry/NoOpContinuousProfiler : io/sentry/IContinuousProfi public fun isRunning ()Z public fun reevaluateSampling ()V public fun startProfiler (Lio/sentry/ProfileLifecycle;Lio/sentry/TracesSampler;)V + public fun stopAllProfiles ()V public fun stopProfiler (Lio/sentry/ProfileLifecycle;)V } diff --git a/sentry/src/main/java/io/sentry/IContinuousProfiler.java b/sentry/src/main/java/io/sentry/IContinuousProfiler.java index f423401c5d6..33378bd21f0 100644 --- a/sentry/src/main/java/io/sentry/IContinuousProfiler.java +++ b/sentry/src/main/java/io/sentry/IContinuousProfiler.java @@ -19,6 +19,9 @@ void startProfiler( void reevaluateSampling(); + /** Stops all profiles currently running, regardless of the profileLifecycle that started them. */ + void stopAllProfiles(); + @NotNull SentryId getProfilerId(); } diff --git a/sentry/src/main/java/io/sentry/NoOpContinuousProfiler.java b/sentry/src/main/java/io/sentry/NoOpContinuousProfiler.java index 35bb0db5f05..833fe9957cc 100644 --- a/sentry/src/main/java/io/sentry/NoOpContinuousProfiler.java +++ b/sentry/src/main/java/io/sentry/NoOpContinuousProfiler.java @@ -26,6 +26,9 @@ public void startProfiler( final @NotNull ProfileLifecycle profileLifecycle, final @NotNull TracesSampler tracesSampler) {} + @Override + public void stopAllProfiles() {} + @Override public void close() {} diff --git a/sentry/src/test/java/io/sentry/NoOpContinuousProfilerTest.kt b/sentry/src/test/java/io/sentry/NoOpContinuousProfilerTest.kt index de2dc7e4c48..69bc8c55887 100644 --- a/sentry/src/test/java/io/sentry/NoOpContinuousProfilerTest.kt +++ b/sentry/src/test/java/io/sentry/NoOpContinuousProfilerTest.kt @@ -17,6 +17,10 @@ class NoOpContinuousProfilerTest { fun `stop does not throw`() = profiler.stopProfiler(mock()) + @Test + fun `stopAllProfiles does not throw`() = + profiler.stopAllProfiles() + @Test fun `isRunning returns false`() { assertFalse(profiler.isRunning) From cfdfcbac6e8591033e0b8a9defeffd7e37e4140f Mon Sep 17 00:00:00 2001 From: stefanosiano Date: Tue, 1 Apr 2025 17:43:46 +0200 Subject: [PATCH 8/9] updated changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1932af56847..b29dbc6bf63 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,7 @@ ### Features +- Continuous Profiling - stop when app goes in background ([#4311](https://github.com/getsentry/sentry-java/pull/4311)) - Continuous Profiling - Add delayed stop ([#4293](https://github.com/getsentry/sentry-java/pull/4293)) - Increase http timeouts from 5s to 30s to have a better chance of events being delivered without retry ([#4276](https://github.com/getsentry/sentry-java/pull/4276)) From 8283fc0911722217471271f8e8e9f1f27b59a5e4 Mon Sep 17 00:00:00 2001 From: stefanosiano Date: Mon, 7 Apr 2025 12:55:29 +0200 Subject: [PATCH 9/9] removed AndroidContinuousProfiler.stopAllProfiles added AndroidContinuousProfiler.close param isTerminating --- sentry-android-core/api/sentry-android-core.api | 3 +-- .../android/core/AndroidContinuousProfiler.java | 16 +++++----------- .../android/core/AndroidOptionsInitializer.java | 2 +- .../io/sentry/android/core/LifecycleWatcher.java | 2 +- .../android/core/SentryPerformanceProvider.java | 2 +- .../core/performance/AppStartMetrics.java | 4 ++-- .../core/AndroidContinuousProfilerTest.kt | 10 +++++----- .../core/AndroidOptionsInitializerTest.kt | 2 +- .../sentry/android/core/LifecycleWatcherTest.kt | 3 ++- .../core/performance/AppStartMetricsTest.kt | 5 +++-- sentry/api/sentry.api | 6 ++---- .../main/java/io/sentry/IContinuousProfiler.java | 11 ++++++----- .../java/io/sentry/NoOpContinuousProfiler.java | 5 +---- sentry/src/main/java/io/sentry/Scopes.java | 2 +- .../java/io/sentry/NoOpContinuousProfilerTest.kt | 6 +----- sentry/src/test/java/io/sentry/ScopesTest.kt | 2 +- 16 files changed, 34 insertions(+), 47 deletions(-) diff --git a/sentry-android-core/api/sentry-android-core.api b/sentry-android-core/api/sentry-android-core.api index f699cf39fb4..c2b313b8e83 100644 --- a/sentry-android-core/api/sentry-android-core.api +++ b/sentry-android-core/api/sentry-android-core.api @@ -42,14 +42,13 @@ 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 ()V + public fun close (Z)V public fun getProfilerId ()Lio/sentry/protocol/SentryId; public fun getRootSpanCounter ()I public fun isRunning ()Z public fun onRateLimitChanged (Lio/sentry/transport/RateLimiter;)V public fun reevaluateSampling ()V public fun startProfiler (Lio/sentry/ProfileLifecycle;Lio/sentry/TracesSampler;)V - public fun stopAllProfiles ()V public fun stopProfiler (Lio/sentry/ProfileLifecycle;)V } 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 6d37d84f3a2..f3e8088760d 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 @@ -323,20 +323,14 @@ public void reevaluateSampling() { } @Override - public void stopAllProfiles() { + public void close(final boolean isTerminating) { try (final @NotNull ISentryLifecycleToken ignored = lock.acquire()) { rootSpanCounter = 0; shouldStop = true; - } - } - - @Override - public void close() { - try (final @NotNull ISentryLifecycleToken ignored = lock.acquire()) { - rootSpanCounter = 0; - shouldStop = true; - stop(false); - isClosed.set(true); + if (isTerminating) { + stop(false); + isClosed.set(true); + } } } 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 8230157a753..657e6d369ae 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 @@ -268,7 +268,7 @@ private static void setupProfiler( // This is a safeguard, but it should never happen, as the app start profiler should be the // continuous one. if (appStartContinuousProfiler != null) { - appStartContinuousProfiler.close(); + appStartContinuousProfiler.close(true); } if (appStartTransactionProfiler != null) { options.setTransactionProfiler(appStartTransactionProfiler); diff --git a/sentry-android-core/src/main/java/io/sentry/android/core/LifecycleWatcher.java b/sentry-android-core/src/main/java/io/sentry/android/core/LifecycleWatcher.java index 218955bb9d8..89d78193207 100644 --- a/sentry-android-core/src/main/java/io/sentry/android/core/LifecycleWatcher.java +++ b/sentry-android-core/src/main/java/io/sentry/android/core/LifecycleWatcher.java @@ -122,7 +122,7 @@ public void run() { scopes.endSession(); } scopes.getOptions().getReplayController().stop(); - scopes.getOptions().getContinuousProfiler().stopAllProfiles(); + scopes.getOptions().getContinuousProfiler().close(false); } }; diff --git a/sentry-android-core/src/main/java/io/sentry/android/core/SentryPerformanceProvider.java b/sentry-android-core/src/main/java/io/sentry/android/core/SentryPerformanceProvider.java index 4b569cba6a8..3c162aab1ad 100644 --- a/sentry-android-core/src/main/java/io/sentry/android/core/SentryPerformanceProvider.java +++ b/sentry-android-core/src/main/java/io/sentry/android/core/SentryPerformanceProvider.java @@ -95,7 +95,7 @@ public void shutdown() { final @Nullable IContinuousProfiler appStartContinuousProfiler = AppStartMetrics.getInstance().getAppStartContinuousProfiler(); if (appStartContinuousProfiler != null) { - appStartContinuousProfiler.close(); + appStartContinuousProfiler.close(true); } } } diff --git a/sentry-android-core/src/main/java/io/sentry/android/core/performance/AppStartMetrics.java b/sentry-android-core/src/main/java/io/sentry/android/core/performance/AppStartMetrics.java index 0cc7cdca8e2..562c8949914 100644 --- a/sentry-android-core/src/main/java/io/sentry/android/core/performance/AppStartMetrics.java +++ b/sentry-android-core/src/main/java/io/sentry/android/core/performance/AppStartMetrics.java @@ -225,7 +225,7 @@ public void clear() { } appStartProfiler = null; if (appStartContinuousProfiler != null) { - appStartContinuousProfiler.close(); + appStartContinuousProfiler.close(true); } appStartContinuousProfiler = null; appStartSamplingDecision = null; @@ -333,7 +333,7 @@ private void checkCreateTimeOnMain() { appStartProfiler = null; } if (appStartContinuousProfiler != null && appStartContinuousProfiler.isRunning()) { - appStartContinuousProfiler.close(); + appStartContinuousProfiler.close(true); appStartContinuousProfiler = null; } } 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 4128f222dc3..db0964f15f4 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 @@ -389,7 +389,7 @@ class AndroidContinuousProfilerTest { profiler.startProfiler(ProfileLifecycle.MANUAL, fixture.mockTracesSampler) assertTrue(profiler.isRunning) - profiler.close() + profiler.close(true) assertFalse(profiler.isRunning) // The timeout scheduled job should be cleared @@ -471,15 +471,15 @@ class AndroidContinuousProfilerTest { } @Test - fun `stopAllProfiles stops all profiles after chunk is finished`() { + fun `close without terminating stops all profiles after chunk is finished`() { val profiler = fixture.getSut() profiler.startProfiler(ProfileLifecycle.MANUAL, fixture.mockTracesSampler) profiler.startProfiler(ProfileLifecycle.TRACE, fixture.mockTracesSampler) assertTrue(profiler.isRunning) // We are scheduling the profiler to stop at the end of the chunk, so it should still be running - profiler.stopAllProfiles() + profiler.close(false) assertTrue(profiler.isRunning) - // However, stopAllProfiles already resets the rootSpanCounter + // However, close() already resets the rootSpanCounter assertEquals(0, profiler.rootSpanCounter) // We run the executor service to trigger the chunk finish, and the profiler shouldn't restart @@ -494,7 +494,7 @@ class AndroidContinuousProfilerTest { assertTrue(profiler.isRunning) // We close the profiler, which should prevent sending additional chunks - profiler.close() + profiler.close(true) // The executor used to send the chunk doesn't do anything fixture.executor.runAll() 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 dfc88fa0e78..e5a2395feb3 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 @@ -446,7 +446,7 @@ class AndroidOptionsInitializerTest { assertEquals(fixture.sentryOptions.continuousProfiler, NoOpContinuousProfiler.getInstance()) // app start profiler is closed, because it will never be used - verify(appStartContinuousProfiler).close() + verify(appStartContinuousProfiler).close(eq(true)) // AppStartMetrics should be cleared assertNull(AppStartMetrics.getInstance().appStartProfiler) diff --git a/sentry-android-core/src/test/java/io/sentry/android/core/LifecycleWatcherTest.kt b/sentry-android-core/src/test/java/io/sentry/android/core/LifecycleWatcherTest.kt index 120cd3fc2f5..c1862b41f7b 100644 --- a/sentry-android-core/src/test/java/io/sentry/android/core/LifecycleWatcherTest.kt +++ b/sentry-android-core/src/test/java/io/sentry/android/core/LifecycleWatcherTest.kt @@ -16,6 +16,7 @@ import io.sentry.transport.ICurrentDateProvider import org.mockito.ArgumentCaptor import org.mockito.kotlin.any import org.mockito.kotlin.check +import org.mockito.kotlin.eq import org.mockito.kotlin.mock import org.mockito.kotlin.never import org.mockito.kotlin.timeout @@ -109,7 +110,7 @@ class LifecycleWatcherTest { watcher.onStop(fixture.ownerMock) verify(fixture.scopes, timeout(10000)).endSession() verify(fixture.replayController, timeout(10000)).stop() - verify(fixture.continuousProfiler, timeout(10000)).stopAllProfiles() + verify(fixture.continuousProfiler, timeout(10000)).close(eq(false)) } @Test diff --git a/sentry-android-core/src/test/java/io/sentry/android/core/performance/AppStartMetricsTest.kt b/sentry-android-core/src/test/java/io/sentry/android/core/performance/AppStartMetricsTest.kt index f8734a26f00..114ea06d2d2 100644 --- a/sentry-android-core/src/test/java/io/sentry/android/core/performance/AppStartMetricsTest.kt +++ b/sentry-android-core/src/test/java/io/sentry/android/core/performance/AppStartMetricsTest.kt @@ -16,6 +16,7 @@ import io.sentry.android.core.SentryAndroidOptions import io.sentry.android.core.SentryShadowProcess import org.junit.Before import org.junit.runner.RunWith +import org.mockito.kotlin.any import org.mockito.kotlin.eq import org.mockito.kotlin.mock import org.mockito.kotlin.never @@ -273,7 +274,7 @@ class AppStartMetricsTest { // Job on main thread checks if activity was launched Shadows.shadowOf(Looper.getMainLooper()).idle() - verify(profiler).close() + verify(profiler).close(eq(true)) } @Test @@ -301,7 +302,7 @@ class AppStartMetricsTest { // Job on main thread checks if activity was launched Shadows.shadowOf(Looper.getMainLooper()).idle() - verify(profiler, never()).close() + verify(profiler, never()).close(any()) } @Test diff --git a/sentry/api/sentry.api b/sentry/api/sentry.api index e85ea313204..06a7ff0e4fa 100644 --- a/sentry/api/sentry.api +++ b/sentry/api/sentry.api @@ -744,12 +744,11 @@ public abstract interface class io/sentry/IConnectionStatusProvider$IConnectionS } public abstract interface class io/sentry/IContinuousProfiler { - public abstract fun close ()V + public abstract fun close (Z)V public abstract fun getProfilerId ()Lio/sentry/protocol/SentryId; public abstract fun isRunning ()Z public abstract fun reevaluateSampling ()V public abstract fun startProfiler (Lio/sentry/ProfileLifecycle;Lio/sentry/TracesSampler;)V - public abstract fun stopAllProfiles ()V public abstract fun stopProfiler (Lio/sentry/ProfileLifecycle;)V } @@ -1437,13 +1436,12 @@ public final class io/sentry/NoOpConnectionStatusProvider : io/sentry/IConnectio } public final class io/sentry/NoOpContinuousProfiler : io/sentry/IContinuousProfiler { - public fun close ()V + public fun close (Z)V public static fun getInstance ()Lio/sentry/NoOpContinuousProfiler; public fun getProfilerId ()Lio/sentry/protocol/SentryId; public fun isRunning ()Z public fun reevaluateSampling ()V public fun startProfiler (Lio/sentry/ProfileLifecycle;Lio/sentry/TracesSampler;)V - public fun stopAllProfiles ()V public fun stopProfiler (Lio/sentry/ProfileLifecycle;)V } diff --git a/sentry/src/main/java/io/sentry/IContinuousProfiler.java b/sentry/src/main/java/io/sentry/IContinuousProfiler.java index 33378bd21f0..3abca9822aa 100644 --- a/sentry/src/main/java/io/sentry/IContinuousProfiler.java +++ b/sentry/src/main/java/io/sentry/IContinuousProfiler.java @@ -14,14 +14,15 @@ void startProfiler( void stopProfiler(final @NotNull ProfileLifecycle profileLifecycle); - /** Cancel the profiler and stops it. Used on SDK close. */ - void close(); + /** + * Cancel the profiler and stops it. + * + * @param isTerminating whether the profiler is terminating and won't be restarted or not. + */ + void close(final boolean isTerminating); void reevaluateSampling(); - /** Stops all profiles currently running, regardless of the profileLifecycle that started them. */ - void stopAllProfiles(); - @NotNull SentryId getProfilerId(); } diff --git a/sentry/src/main/java/io/sentry/NoOpContinuousProfiler.java b/sentry/src/main/java/io/sentry/NoOpContinuousProfiler.java index 833fe9957cc..893eb914ad9 100644 --- a/sentry/src/main/java/io/sentry/NoOpContinuousProfiler.java +++ b/sentry/src/main/java/io/sentry/NoOpContinuousProfiler.java @@ -27,10 +27,7 @@ public void startProfiler( final @NotNull TracesSampler tracesSampler) {} @Override - public void stopAllProfiles() {} - - @Override - public void close() {} + public void close(final boolean isTerminating) {} @Override public void reevaluateSampling() {} diff --git a/sentry/src/main/java/io/sentry/Scopes.java b/sentry/src/main/java/io/sentry/Scopes.java index b2c1bb1aaec..002043c3dfc 100644 --- a/sentry/src/main/java/io/sentry/Scopes.java +++ b/sentry/src/main/java/io/sentry/Scopes.java @@ -405,7 +405,7 @@ public void close(final boolean isRestarting) { configureScope(ScopeType.ISOLATION, scope -> scope.clear()); getOptions().getBackpressureMonitor().close(); getOptions().getTransactionProfiler().close(); - getOptions().getContinuousProfiler().close(); + getOptions().getContinuousProfiler().close(true); getOptions().getCompositePerformanceCollector().close(); final @NotNull ISentryExecutorService executorService = getOptions().getExecutorService(); if (isRestarting) { diff --git a/sentry/src/test/java/io/sentry/NoOpContinuousProfilerTest.kt b/sentry/src/test/java/io/sentry/NoOpContinuousProfilerTest.kt index 69bc8c55887..559190004b8 100644 --- a/sentry/src/test/java/io/sentry/NoOpContinuousProfilerTest.kt +++ b/sentry/src/test/java/io/sentry/NoOpContinuousProfilerTest.kt @@ -17,10 +17,6 @@ class NoOpContinuousProfilerTest { fun `stop does not throw`() = profiler.stopProfiler(mock()) - @Test - fun `stopAllProfiles does not throw`() = - profiler.stopAllProfiles() - @Test fun `isRunning returns false`() { assertFalse(profiler.isRunning) @@ -28,7 +24,7 @@ class NoOpContinuousProfilerTest { @Test fun `close does not throw`() = - profiler.close() + profiler.close(true) @Test fun `getProfilerId returns Empty SentryId`() { diff --git a/sentry/src/test/java/io/sentry/ScopesTest.kt b/sentry/src/test/java/io/sentry/ScopesTest.kt index d61ae59f60c..ebb84162240 100644 --- a/sentry/src/test/java/io/sentry/ScopesTest.kt +++ b/sentry/src/test/java/io/sentry/ScopesTest.kt @@ -1826,7 +1826,7 @@ class ScopesTest { verify(backpressureMonitorMock).close() verify(executor).close(any()) verify(profiler).close() - verify(continuousProfiler).close() + verify(continuousProfiler).close(eq(true)) verify(performanceCollector).close() }