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 ebc10beb0c..8ea97a7430 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 62892e3ed4..353e3eb1ec 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 6903d9b1bb..b999ddca41 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 5456b27e8a..3e303be2d2 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 e4e8e173ca..a3130fc1b3 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 792816b4ed..760b600d50 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 353e3eb1ec..9a76d118a9 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 f9c6df4e52..48df7ff090 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 0c66e1adfd..d5f5f6a354 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 daf7c84d15..4e1b45ebb0 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 b30fe3464f..d048b42d42 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 8da554cd3e..0496f40721 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 0c36034c28..347d92d473 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 760b600d50..1932af5684 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 d0d139ac8c..92f4a7ceba 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 d5f5f6a354..498edfaa35 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 d83ecdd675..218955bb9d 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 4e1b45ebb0..4128f222dc 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 5f088221b9..120cd3fc2f 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 8ea97a7430..d0cf15de0a 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 f423401c5d..33378bd21f 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 35bb0db5f0..833fe9957c 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 de2dc7e4c4..69bc8c5588 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 1932af5684..b29dbc6bf6 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 f699cf39fb..c2b313b8e8 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 6d37d84f3a..f3e8088760 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 8230157a75..657e6d369a 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 218955bb9d..89d7819320 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 4b569cba6a..3c162aab1a 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 0cc7cdca8e..562c894991 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 4128f222dc..db0964f15f 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 dfc88fa0e7..e5a2395feb 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 120cd3fc2f..c1862b41f7 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 f8734a26f0..114ea06d2d 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 e85ea31320..06a7ff0e4f 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 33378bd21f..3abca9822a 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 833fe9957c..893eb914ad 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 b2c1bb1aae..002043c3df 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 69bc8c5588..559190004b 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 d61ae59f60..ebb8416224 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() }