From c333c48ad9df02f74d91bc6009f39893d3eca5f4 Mon Sep 17 00:00:00 2001 From: stefanosiano Date: Fri, 14 Oct 2022 11:03:59 +0200 Subject: [PATCH 1/4] AndroidTransactionProfiler now directly sends profile envelopes Dropped old timed out profiles handling. Now they are directly sent when they time out AndroidTransactionProfiler does not return the ProfileTraceData anymore Added ui test for timed out profiles --- .../core/AndroidTransactionProfiler.java | 91 +++++--------- .../core/AndroidTransactionProfilerTest.kt | 115 +++++++++--------- .../android/core/SentryAndroidOptionsTest.kt | 3 +- .../io/sentry/uitest/android/EnvelopeTests.kt | 31 ++++- sentry/api/sentry.api | 4 +- .../java/io/sentry/ITransactionProfiler.java | 4 +- .../io/sentry/NoOpTransactionProfiler.java | 5 +- .../src/main/java/io/sentry/SentryTracer.java | 7 +- sentry/src/test/java/io/sentry/HubTest.kt | 4 +- .../io/sentry/NoOpTransactionProfilerTest.kt | 5 +- 10 files changed, 127 insertions(+), 142 deletions(-) diff --git a/sentry-android-core/src/main/java/io/sentry/android/core/AndroidTransactionProfiler.java b/sentry-android-core/src/main/java/io/sentry/android/core/AndroidTransactionProfiler.java index 2ab39cb4a7..b5af030213 100644 --- a/sentry-android-core/src/main/java/io/sentry/android/core/AndroidTransactionProfiler.java +++ b/sentry-android-core/src/main/java/io/sentry/android/core/AndroidTransactionProfiler.java @@ -11,13 +11,13 @@ import android.os.Debug; import android.os.Process; import android.os.SystemClock; +import io.sentry.HubAdapter; import io.sentry.ITransaction; import io.sentry.ITransactionProfiler; import io.sentry.ProfilingTraceData; import io.sentry.ProfilingTransactionData; import io.sentry.SentryLevel; import io.sentry.android.core.internal.util.CpuInfoUtils; -import io.sentry.util.CollectionUtils; import io.sentry.util.Objects; import java.io.File; import java.util.ArrayList; @@ -48,7 +48,6 @@ final class AndroidTransactionProfiler implements ITransactionProfiler { private @Nullable File traceFile = null; private @Nullable File traceFilesDir = null; private @Nullable Future scheduledFinish = null; - private volatile @Nullable ProfilingTraceData timedOutProfilingData = null; private final @NotNull Context context; private final @NotNull SentryAndroidOptions options; private final @NotNull BuildInfoProvider buildInfoProvider; @@ -137,9 +136,7 @@ public synchronized void onTransactionStart(@NotNull ITransaction transaction) { scheduledFinish = options .getExecutorService() - .schedule( - () -> timedOutProfilingData = onTransactionFinish(transaction, true), - PROFILING_TIMEOUT_MILLIS); + .schedule(() -> onTransactionFinish(transaction, true), PROFILING_TIMEOUT_MILLIS); transactionStartNanos = SystemClock.elapsedRealtimeNanos(); profileStartCpuMillis = Process.getElapsedCpuTime(); @@ -166,45 +163,20 @@ public synchronized void onTransactionStart(@NotNull ITransaction transaction) { } @Override - public synchronized @Nullable ProfilingTraceData onTransactionFinish( - @NotNull ITransaction transaction) { - return onTransactionFinish(transaction, false); + public synchronized void onTransactionFinish(@NotNull ITransaction transaction) { + onTransactionFinish(transaction, false); } @SuppressLint("NewApi") - private synchronized @Nullable ProfilingTraceData onTransactionFinish( + private synchronized void onTransactionFinish( @NotNull ITransaction transaction, boolean isTimeout) { // onTransactionStart() is only available since Lollipop // and SystemClock.elapsedRealtimeNanos() since Jelly Bean - if (buildInfoProvider.getSdkInfoVersion() < Build.VERSION_CODES.LOLLIPOP) return null; - - final ProfilingTraceData profilingData = timedOutProfilingData; + if (buildInfoProvider.getSdkInfoVersion() < Build.VERSION_CODES.LOLLIPOP) return; - // Transaction finished, but it's not in the current profile + // Transaction finished, but it's not in the current profile. We can skip it if (!transactionMap.containsKey(transaction.getEventId().toString())) { - // We check if we cached a profiling data due to a timeout with this profile in it - // If so, we return it back, otherwise we would simply lose it - if (profilingData != null) { - // Don't use method reference. This can cause issues on Android - List ids = - CollectionUtils.map(profilingData.getTransactions(), (data) -> data.getId()); - if (ids.contains(transaction.getEventId().toString())) { - timedOutProfilingData = null; - return profilingData; - } else { - // Another transaction is finishing before the timed out one - options - .getLogger() - .log( - SentryLevel.INFO, - "A timed out profiling data exists, but the finishing transaction %s (%s) is not part of it", - transaction.getName(), - transaction.getSpanContext().getTraceId().toString()); - return null; - } - } - // A transaction is finishing, but it's not profiled. We can skip it options .getLogger() .log( @@ -212,7 +184,7 @@ public synchronized void onTransactionStart(@NotNull ITransaction transaction) { "Transaction %s (%s) finished, but was not currently being profiled. Skipping", transaction.getName(), transaction.getSpanContext().getTraceId().toString()); - return null; + return; } if (transactionsCounter > 0) { @@ -239,7 +211,7 @@ public synchronized void onTransactionStart(@NotNull ITransaction transaction) { Process.getElapsedCpuTime(), profileStartCpuMillis); } - return null; + return; } Debug.stopMethodTracing(); @@ -259,7 +231,7 @@ public synchronized void onTransactionStart(@NotNull ITransaction transaction) { if (traceFile == null) { options.getLogger().log(SentryLevel.ERROR, "Trace file does not exists"); - return null; + return; } String versionName = ""; @@ -287,26 +259,29 @@ public synchronized void onTransactionStart(@NotNull ITransaction transaction) { // cpu max frequencies are read with a lambda because reading files is involved, so it will be // done in the background when the trace file is read - return new ProfilingTraceData( - traceFile, - transactionList, - transaction, - Long.toString(transactionDurationNanos), - buildInfoProvider.getSdkInfoVersion(), - abis != null && abis.length > 0 ? abis[0] : "", - () -> CpuInfoUtils.getInstance().readMaxFrequencies(), - buildInfoProvider.getManufacturer(), - buildInfoProvider.getModel(), - buildInfoProvider.getVersionRelease(), - buildInfoProvider.isEmulator(), - totalMem, - options.getProguardUuid(), - versionName, - versionCode, - options.getEnvironment(), - isTimeout - ? ProfilingTraceData.TRUNCATION_REASON_TIMEOUT - : ProfilingTraceData.TRUNCATION_REASON_NORMAL); + ProfilingTraceData profilingTraceData = + new ProfilingTraceData( + traceFile, + transactionList, + transaction, + Long.toString(transactionDurationNanos), + buildInfoProvider.getSdkInfoVersion(), + abis != null && abis.length > 0 ? abis[0] : "", + () -> CpuInfoUtils.getInstance().readMaxFrequencies(), + buildInfoProvider.getManufacturer(), + buildInfoProvider.getModel(), + buildInfoProvider.getVersionRelease(), + buildInfoProvider.isEmulator(), + totalMem, + options.getProguardUuid(), + versionName, + versionCode, + options.getEnvironment(), + isTimeout + ? ProfilingTraceData.TRUNCATION_REASON_TIMEOUT + : ProfilingTraceData.TRUNCATION_REASON_NORMAL); + + HubAdapter.getInstance().captureProfile(profilingTraceData); } /** diff --git a/sentry-android-core/src/test/java/io/sentry/android/core/AndroidTransactionProfilerTest.kt b/sentry-android-core/src/test/java/io/sentry/android/core/AndroidTransactionProfilerTest.kt index c6e72c0527..9dc75b1afa 100644 --- a/sentry-android-core/src/test/java/io/sentry/android/core/AndroidTransactionProfilerTest.kt +++ b/sentry-android-core/src/test/java/io/sentry/android/core/AndroidTransactionProfilerTest.kt @@ -6,6 +6,7 @@ import androidx.test.core.app.ApplicationProvider import androidx.test.ext.junit.runners.AndroidJUnit4 import com.nhaarman.mockitokotlin2.any import com.nhaarman.mockitokotlin2.argumentCaptor +import com.nhaarman.mockitokotlin2.check import com.nhaarman.mockitokotlin2.mock import com.nhaarman.mockitokotlin2.never import com.nhaarman.mockitokotlin2.spy @@ -16,6 +17,7 @@ import io.sentry.IHub import io.sentry.ILogger import io.sentry.ISentryExecutorService import io.sentry.ProfilingTraceData +import io.sentry.Sentry import io.sentry.SentryLevel import io.sentry.SentryTracer import io.sentry.TransactionContext @@ -28,7 +30,6 @@ import kotlin.test.Test import kotlin.test.assertEquals import kotlin.test.assertFailsWith import kotlin.test.assertNotNull -import kotlin.test.assertNull import kotlin.test.assertTrue @RunWith(AndroidJUnit4::class) @@ -63,6 +64,7 @@ class AndroidTransactionProfilerTest { transaction1 = SentryTracer(TransactionContext("", ""), hub) transaction2 = SentryTracer(TransactionContext("", ""), hub) transaction3 = SentryTracer(TransactionContext("", ""), hub) + Sentry.setCurrentHub(hub) return AndroidTransactionProfiler(context, options, buildInfoProvider) } } @@ -100,8 +102,13 @@ class AndroidTransactionProfilerTest { fun `profiler profiles current transaction`() { val profiler = fixture.getSut(context) profiler.onTransactionStart(fixture.transaction1) - val traceData = profiler.onTransactionFinish(fixture.transaction1) - assertEquals(fixture.transaction1.eventId.toString(), traceData!!.transactionId) + profiler.onTransactionFinish(fixture.transaction1) + + verify(fixture.hub).captureProfile( + check { + assertEquals(it.transactionId, fixture.transaction1.eventId.toString()) + } + ) } @Test @@ -111,8 +118,8 @@ class AndroidTransactionProfilerTest { } val profiler = fixture.getSut(context, buildInfo) profiler.onTransactionStart(fixture.transaction1) - val traceData = profiler.onTransactionFinish(fixture.transaction1) - assertNull(traceData) + profiler.onTransactionFinish(fixture.transaction1) + verify(fixture.hub, never()).captureProfile(any()) } @Test @@ -122,8 +129,8 @@ class AndroidTransactionProfilerTest { } val profiler = fixture.getSut(context) profiler.onTransactionStart(fixture.transaction1) - val traceData = profiler.onTransactionFinish(fixture.transaction1) - assertNull(traceData) + profiler.onTransactionFinish(fixture.transaction1) + verify(fixture.hub, never()).captureProfile(any()) } @Test @@ -195,8 +202,8 @@ class AndroidTransactionProfilerTest { } val profiler = fixture.getSut(context) profiler.onTransactionStart(fixture.transaction1) - val traceData = profiler.onTransactionFinish(fixture.transaction1) - assertNull(traceData) + profiler.onTransactionFinish(fixture.transaction1) + verify(fixture.hub, never()).captureProfile(any()) } @Test @@ -206,8 +213,8 @@ class AndroidTransactionProfilerTest { } val profiler = fixture.getSut(context) profiler.onTransactionStart(fixture.transaction1) - val traceData = profiler.onTransactionFinish(fixture.transaction1) - assertNull(traceData) + profiler.onTransactionFinish(fixture.transaction1) + verify(fixture.hub, never()).captureProfile(any()) } @Test @@ -217,8 +224,8 @@ class AndroidTransactionProfilerTest { } val profiler = fixture.getSut(context) profiler.onTransactionStart(fixture.transaction1) - val traceData = profiler.onTransactionFinish(fixture.transaction1) - assertNull(traceData) + profiler.onTransactionFinish(fixture.transaction1) + verify(fixture.hub, never()).captureProfile(any()) } @Test @@ -235,34 +242,8 @@ class AndroidTransactionProfilerTest { @Test fun `onTransactionFinish works only if previously started`() { val profiler = fixture.getSut(context) - val traceData = profiler.onTransactionFinish(fixture.transaction1) - assertNull(traceData) - } - - @Test - fun `onTransactionFinish returns timedOutData to the timed out transaction once, even after other transactions`() { - val profiler = fixture.getSut(context) - - val executorService = mock() - val captor = argumentCaptor() - whenever(executorService.schedule(captor.capture(), any())).thenReturn(null) - whenever(fixture.options.executorService).thenReturn(executorService) - // Start and finish first transaction profiling - profiler.onTransactionStart(fixture.transaction1) - - // Set timed out data by calling the timeout scheduled job - captor.firstValue.run() - - // Start and finish second transaction. Since profiler returned data, it means no other profiling is running - profiler.onTransactionStart(fixture.transaction2) - assertEquals(fixture.transaction2.eventId.toString(), profiler.onTransactionFinish(fixture.transaction2)!!.transactionId) - - // First transaction finishes: timed out data is returned - val traceData = profiler.onTransactionFinish(fixture.transaction1) - assertEquals(traceData!!.transactionId, fixture.transaction1.eventId.toString()) - - // If first transaction is finished again, nothing is returned - assertNull(profiler.onTransactionFinish(fixture.transaction1)) + profiler.onTransactionFinish(fixture.transaction1) + verify(fixture.hub, never()).captureProfile(any()) } @Test @@ -281,9 +262,13 @@ class AndroidTransactionProfilerTest { captor.firstValue.run() // First transaction finishes: timed out data is returned - val traceData = profiler.onTransactionFinish(fixture.transaction1) - assertEquals(traceData!!.transactionId, fixture.transaction1.eventId.toString()) - assertEquals(ProfilingTraceData.TRUNCATION_REASON_TIMEOUT, traceData.truncationReason) + profiler.onTransactionFinish(fixture.transaction1) + verify(fixture.hub).captureProfile( + check { + assertEquals(it.transactionId, fixture.transaction1.eventId.toString()) + assertEquals(ProfilingTraceData.TRUNCATION_REASON_TIMEOUT, it.truncationReason) + } + ) } @Test @@ -292,11 +277,15 @@ class AndroidTransactionProfilerTest { profiler.onTransactionStart(fixture.transaction1) profiler.onTransactionStart(fixture.transaction2) - var traceData = profiler.onTransactionFinish(fixture.transaction2) - assertNull(traceData) + profiler.onTransactionFinish(fixture.transaction2) + verify(fixture.hub, never()).captureProfile(any()) - traceData = profiler.onTransactionFinish(fixture.transaction1) - assertEquals(fixture.transaction1.eventId.toString(), traceData!!.transactionId) + profiler.onTransactionFinish(fixture.transaction1) + verify(fixture.hub).captureProfile( + check { + assertEquals(it.transactionId, fixture.transaction1.eventId.toString()) + } + ) } @Test @@ -305,20 +294,26 @@ class AndroidTransactionProfilerTest { profiler.onTransactionStart(fixture.transaction1) profiler.onTransactionStart(fixture.transaction2) - var traceData = profiler.onTransactionFinish(fixture.transaction1) - assertNull(traceData) + profiler.onTransactionFinish(fixture.transaction1) + verify(fixture.hub, never()).captureProfile(any()) profiler.onTransactionStart(fixture.transaction3) - traceData = profiler.onTransactionFinish(fixture.transaction3) - assertNull(traceData) - traceData = profiler.onTransactionFinish(fixture.transaction2) - assertEquals(fixture.transaction2.eventId.toString(), traceData!!.transactionId) - val expectedTransactions = listOf( - fixture.transaction1.eventId.toString(), - fixture.transaction3.eventId.toString(), - fixture.transaction2.eventId.toString() + profiler.onTransactionFinish(fixture.transaction3) + verify(fixture.hub, never()).captureProfile(any()) + + profiler.onTransactionFinish(fixture.transaction2) + verify(fixture.hub).captureProfile( + check { + val expectedTransactions = listOf( + fixture.transaction1.eventId.toString(), + fixture.transaction3.eventId.toString(), + fixture.transaction2.eventId.toString() + ) + assertEquals(it.transactionId, fixture.transaction2.eventId.toString()) + + assertTrue(it.transactions.map { it.id }.containsAll(expectedTransactions)) + assertTrue(expectedTransactions.containsAll(it.transactions.map { it.id })) + } ) - assertTrue(traceData.transactions.map { it.id }.containsAll(expectedTransactions)) - assertTrue(expectedTransactions.containsAll(traceData.transactions.map { it.id })) } } diff --git a/sentry-android-core/src/test/java/io/sentry/android/core/SentryAndroidOptionsTest.kt b/sentry-android-core/src/test/java/io/sentry/android/core/SentryAndroidOptionsTest.kt index 7c44407ce0..520f9be700 100644 --- a/sentry-android-core/src/test/java/io/sentry/android/core/SentryAndroidOptionsTest.kt +++ b/sentry-android-core/src/test/java/io/sentry/android/core/SentryAndroidOptionsTest.kt @@ -3,7 +3,6 @@ package io.sentry.android.core import io.sentry.ITransaction import io.sentry.ITransactionProfiler import io.sentry.NoOpTransactionProfiler -import io.sentry.ProfilingTraceData import io.sentry.protocol.DebugImage import kotlin.test.Test import kotlin.test.assertEquals @@ -110,6 +109,6 @@ class SentryAndroidOptionsTest { private class CustomTransactionProfiler : ITransactionProfiler { override fun onTransactionStart(transaction: ITransaction) {} - override fun onTransactionFinish(transaction: ITransaction): ProfilingTraceData? = null + override fun onTransactionFinish(transaction: ITransaction) {} } } diff --git a/sentry-android-integration-tests/sentry-uitest-android/src/androidTest/java/io/sentry/uitest/android/EnvelopeTests.kt b/sentry-android-integration-tests/sentry-uitest-android/src/androidTest/java/io/sentry/uitest/android/EnvelopeTests.kt index d345cbe387..692178b18f 100644 --- a/sentry-android-integration-tests/sentry-uitest-android/src/androidTest/java/io/sentry/uitest/android/EnvelopeTests.kt +++ b/sentry-android-integration-tests/sentry-uitest-android/src/androidTest/java/io/sentry/uitest/android/EnvelopeTests.kt @@ -3,6 +3,7 @@ package io.sentry.uitest.android import androidx.lifecycle.Lifecycle import androidx.test.core.app.launchActivity import androidx.test.espresso.Espresso +import androidx.test.espresso.IdlingPolicies import androidx.test.espresso.IdlingRegistry import androidx.test.espresso.action.ViewActions import androidx.test.espresso.matcher.ViewMatchers @@ -14,6 +15,7 @@ import io.sentry.SentryOptions import io.sentry.protocol.SentryTransaction import org.junit.runner.RunWith import java.io.File +import java.util.concurrent.TimeUnit import kotlin.test.Test import kotlin.test.assertEquals import kotlin.test.assertFails @@ -180,9 +182,34 @@ class EnvelopeTests : BaseUiTest() { // The profile failed to be sent. Trying to read the envelope from the data transmitted throws an exception assertFails { assertEnvelope {} } assertEnvelope { - it.assertItem() - // Since the profile is empty, it is discarded and not sent to Sentry + val transactionItem: SentryTransaction = it.assertItem() it.assertNoOtherItems() + assertEquals("e2etests", transactionItem.transaction) + } + assertNoOtherEnvelopes() + assertNoOtherRequests() + } + } + + @Test + fun sendTimedOutProfile() { + // We increase the IdlingResources timeout to exceed the profiling timeout + IdlingPolicies.setIdlingResourceTimeout(1, TimeUnit.MINUTES) + initSentry(true) { options: SentryOptions -> + options.tracesSampleRate = 1.0 + options.profilesSampleRate = 1.0 + } + relayIdlingResource.increment() + Sentry.startTransaction("e2etests", "testTimeout") + // We don't call transaction.finish() and let the timeout do its job + + relay.assert { + // The profile failed to be sent. Trying to read the envelope from the data transmitted throws an exception + assertEnvelope { + val profilingTraceData: ProfilingTraceData = it.assertItem() + it.assertNoOtherItems() + assertEquals("e2etests", profilingTraceData.transactionName) + assertEquals(ProfilingTraceData.TRUNCATION_REASON_TIMEOUT, profilingTraceData.truncationReason) } assertNoOtherEnvelopes() assertNoOtherRequests() diff --git a/sentry/api/sentry.api b/sentry/api/sentry.api index 6fdb89ff61..91e42e80a7 100644 --- a/sentry/api/sentry.api +++ b/sentry/api/sentry.api @@ -519,7 +519,7 @@ public abstract interface class io/sentry/ITransaction : io/sentry/ISpan { } public abstract interface class io/sentry/ITransactionProfiler { - public abstract fun onTransactionFinish (Lio/sentry/ITransaction;)Lio/sentry/ProfilingTraceData; + public abstract fun onTransactionFinish (Lio/sentry/ITransaction;)V public abstract fun onTransactionStart (Lio/sentry/ITransaction;)V } @@ -784,7 +784,7 @@ public final class io/sentry/NoOpTransaction : io/sentry/ITransaction { public final class io/sentry/NoOpTransactionProfiler : io/sentry/ITransactionProfiler { public static fun getInstance ()Lio/sentry/NoOpTransactionProfiler; - public fun onTransactionFinish (Lio/sentry/ITransaction;)Lio/sentry/ProfilingTraceData; + public fun onTransactionFinish (Lio/sentry/ITransaction;)V public fun onTransactionStart (Lio/sentry/ITransaction;)V } diff --git a/sentry/src/main/java/io/sentry/ITransactionProfiler.java b/sentry/src/main/java/io/sentry/ITransactionProfiler.java index 6e6e9cf582..6bfc435b36 100644 --- a/sentry/src/main/java/io/sentry/ITransactionProfiler.java +++ b/sentry/src/main/java/io/sentry/ITransactionProfiler.java @@ -2,13 +2,11 @@ import org.jetbrains.annotations.ApiStatus; import org.jetbrains.annotations.NotNull; -import org.jetbrains.annotations.Nullable; /** Used for performing operations when a transaction is started or ended. */ @ApiStatus.Internal public interface ITransactionProfiler { void onTransactionStart(@NotNull ITransaction transaction); - @Nullable - ProfilingTraceData onTransactionFinish(@NotNull ITransaction transaction); + void onTransactionFinish(@NotNull ITransaction transaction); } diff --git a/sentry/src/main/java/io/sentry/NoOpTransactionProfiler.java b/sentry/src/main/java/io/sentry/NoOpTransactionProfiler.java index 2580ded221..d4fd1ff34c 100644 --- a/sentry/src/main/java/io/sentry/NoOpTransactionProfiler.java +++ b/sentry/src/main/java/io/sentry/NoOpTransactionProfiler.java @@ -1,7 +1,6 @@ package io.sentry; import org.jetbrains.annotations.NotNull; -import org.jetbrains.annotations.Nullable; public final class NoOpTransactionProfiler implements ITransactionProfiler { @@ -17,7 +16,5 @@ public static NoOpTransactionProfiler getInstance() { public void onTransactionStart(@NotNull ITransaction transaction) {} @Override - public @Nullable ProfilingTraceData onTransactionFinish(@NotNull ITransaction transaction) { - return null; - } + public void onTransactionFinish(@NotNull ITransaction transaction) {} } diff --git a/sentry/src/main/java/io/sentry/SentryTracer.java b/sentry/src/main/java/io/sentry/SentryTracer.java index a98c336368..6d5c069992 100644 --- a/sentry/src/main/java/io/sentry/SentryTracer.java +++ b/sentry/src/main/java/io/sentry/SentryTracer.java @@ -303,9 +303,8 @@ public void finish() { public void finish(@Nullable SpanStatus status) { this.finishStatus = FinishStatus.finishing(status); if (!root.isFinished() && (!waitForChildren || hasAllChildrenFinished())) { - ProfilingTraceData profilingTraceData = null; if (Boolean.TRUE.equals(isSampled()) && Boolean.TRUE.equals(isProfileSampled())) { - profilingTraceData = hub.getOptions().getTransactionProfiler().onTransactionFinish(this); + hub.getOptions().getTransactionProfiler().onTransactionFinish(this); } // try to get the high precision timestamp from the root span @@ -366,10 +365,6 @@ public void finish(@Nullable SpanStatus status) { } transaction.getMeasurements().putAll(measurements); - - if (profilingTraceData != null) { - hub.captureProfile(profilingTraceData); - } hub.captureTransaction(transaction, traceContext(), null); } } diff --git a/sentry/src/test/java/io/sentry/HubTest.kt b/sentry/src/test/java/io/sentry/HubTest.kt index a35d293d21..e41cf05aba 100644 --- a/sentry/src/test/java/io/sentry/HubTest.kt +++ b/sentry/src/test/java/io/sentry/HubTest.kt @@ -1302,8 +1302,8 @@ class HubTest { @Test fun `when startTransaction and profiling is enabled, transaction is profiled only if sampled`() { val mockTransactionProfiler = mock() - whenever(mockTransactionProfiler.onTransactionFinish(any())).thenReturn(mock()) val mockClient = mock() + whenever(mockTransactionProfiler.onTransactionFinish(any())).thenAnswer { mockClient.captureProfile(mock()) } val hub = generateHub { it.setTransactionProfiler(mockTransactionProfiler) } @@ -1324,8 +1324,8 @@ class HubTest { @Test fun `when startTransaction and is sampled but profiling is disabled, transaction is not profiled`() { val mockTransactionProfiler = mock() - whenever(mockTransactionProfiler.onTransactionFinish(any())).thenReturn(mock()) val mockClient = mock() + whenever(mockTransactionProfiler.onTransactionFinish(any())).thenAnswer { mockClient.captureProfile(mock()) } val hub = generateHub { it.profilesSampleRate = 0.0 it.setTransactionProfiler(mockTransactionProfiler) diff --git a/sentry/src/test/java/io/sentry/NoOpTransactionProfilerTest.kt b/sentry/src/test/java/io/sentry/NoOpTransactionProfilerTest.kt index cec88256c4..aaf30190ed 100644 --- a/sentry/src/test/java/io/sentry/NoOpTransactionProfilerTest.kt +++ b/sentry/src/test/java/io/sentry/NoOpTransactionProfilerTest.kt @@ -1,7 +1,6 @@ package io.sentry import kotlin.test.Test -import kotlin.test.assertNull class NoOpTransactionProfilerTest { private var profiler = NoOpTransactionProfiler.getInstance() @@ -11,6 +10,6 @@ class NoOpTransactionProfilerTest { profiler.onTransactionStart(NoOpTransaction.getInstance()) @Test - fun `onTransactionFinish returns null`() = - assertNull(profiler.onTransactionFinish(NoOpTransaction.getInstance())) + fun `onTransactionFinish does not throw`() = + profiler.onTransactionFinish(NoOpTransaction.getInstance()) } From 0ebc5ae64336602df203149ab1e50a6b2a3d2f9a Mon Sep 17 00:00:00 2001 From: stefanosiano Date: Fri, 14 Oct 2022 13:18:55 +0200 Subject: [PATCH 2/4] Fixed ui tests not clearing unasserted requests after running test --- .../androidTest/java/io/sentry/uitest/android/EnvelopeTests.kt | 1 - .../java/io/sentry/uitest/android/mockservers/MockRelay.kt | 2 ++ 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/sentry-android-integration-tests/sentry-uitest-android/src/androidTest/java/io/sentry/uitest/android/EnvelopeTests.kt b/sentry-android-integration-tests/sentry-uitest-android/src/androidTest/java/io/sentry/uitest/android/EnvelopeTests.kt index 692178b18f..b4bfd10d4f 100644 --- a/sentry-android-integration-tests/sentry-uitest-android/src/androidTest/java/io/sentry/uitest/android/EnvelopeTests.kt +++ b/sentry-android-integration-tests/sentry-uitest-android/src/androidTest/java/io/sentry/uitest/android/EnvelopeTests.kt @@ -204,7 +204,6 @@ class EnvelopeTests : BaseUiTest() { // We don't call transaction.finish() and let the timeout do its job relay.assert { - // The profile failed to be sent. Trying to read the envelope from the data transmitted throws an exception assertEnvelope { val profilingTraceData: ProfilingTraceData = it.assertItem() it.assertNoOtherItems() diff --git a/sentry-android-integration-tests/sentry-uitest-android/src/androidTest/java/io/sentry/uitest/android/mockservers/MockRelay.kt b/sentry-android-integration-tests/sentry-uitest-android/src/androidTest/java/io/sentry/uitest/android/mockservers/MockRelay.kt index ae28eb8e57..906450e3aa 100644 --- a/sentry-android-integration-tests/sentry-uitest-android/src/androidTest/java/io/sentry/uitest/android/mockservers/MockRelay.kt +++ b/sentry-android-integration-tests/sentry-uitest-android/src/androidTest/java/io/sentry/uitest/android/mockservers/MockRelay.kt @@ -67,6 +67,8 @@ class MockRelay( /** Shutdown the mock relay server and clear everything. */ fun shutdown() { responses.clear() + unassertedEnvelopes.clear() + unassertedRequests.clear() relay.shutdown() } From 103e4a9dc89676c7432c867f0c8da0f8b3ee228a Mon Sep 17 00:00:00 2001 From: stefanosiano Date: Fri, 14 Oct 2022 17:53:06 +0200 Subject: [PATCH 3/4] Fixed dogfood ui test sending data after some time to mock relay --- .../java/io/sentry/uitest/android/EnvelopeTests.kt | 4 +++- .../java/io/sentry/uitest/android/mockservers/MockRelay.kt | 2 -- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/sentry-android-integration-tests/sentry-uitest-android/src/androidTest/java/io/sentry/uitest/android/EnvelopeTests.kt b/sentry-android-integration-tests/sentry-uitest-android/src/androidTest/java/io/sentry/uitest/android/EnvelopeTests.kt index b4bfd10d4f..b39f421fc0 100644 --- a/sentry-android-integration-tests/sentry-uitest-android/src/androidTest/java/io/sentry/uitest/android/EnvelopeTests.kt +++ b/sentry-android-integration-tests/sentry-uitest-android/src/androidTest/java/io/sentry/uitest/android/EnvelopeTests.kt @@ -192,7 +192,7 @@ class EnvelopeTests : BaseUiTest() { } @Test - fun sendTimedOutProfile() { + fun checkTimedOutProfile() { // We increase the IdlingResources timeout to exceed the profiling timeout IdlingPolicies.setIdlingResourceTimeout(1, TimeUnit.MINUTES) initSentry(true) { options: SentryOptions -> @@ -231,6 +231,8 @@ class EnvelopeTests : BaseUiTest() { benchmarkScenario.moveToState(Lifecycle.State.DESTROYED) transaction.finish() IdlingRegistry.getInstance().unregister(ProfilingSampleActivity.scrollingIdlingResource) + // Let this test send all data, so that it doesn't interfere with other tests + Thread.sleep(1000) } private fun swipeList(times: Int) { diff --git a/sentry-android-integration-tests/sentry-uitest-android/src/androidTest/java/io/sentry/uitest/android/mockservers/MockRelay.kt b/sentry-android-integration-tests/sentry-uitest-android/src/androidTest/java/io/sentry/uitest/android/mockservers/MockRelay.kt index 906450e3aa..ae28eb8e57 100644 --- a/sentry-android-integration-tests/sentry-uitest-android/src/androidTest/java/io/sentry/uitest/android/mockservers/MockRelay.kt +++ b/sentry-android-integration-tests/sentry-uitest-android/src/androidTest/java/io/sentry/uitest/android/mockservers/MockRelay.kt @@ -67,8 +67,6 @@ class MockRelay( /** Shutdown the mock relay server and clear everything. */ fun shutdown() { responses.clear() - unassertedEnvelopes.clear() - unassertedRequests.clear() relay.shutdown() } From e858a65ec27ea9711d01e876bc01774d73083edd Mon Sep 17 00:00:00 2001 From: stefanosiano Date: Mon, 17 Oct 2022 20:31:14 +0200 Subject: [PATCH 4/4] added hub parameter to AndroidTransactionProfiler ctor removed captureProfile methods profiles are sent creating an envelope and using captureEnvelope added SentryEnvelope.from(... ProfilingTraceData ...) --- CHANGELOG.md | 1 + .../core/AndroidTransactionProfiler.java | 28 ++++++++- .../core/AndroidTransactionProfilerTest.kt | 58 +++++++++++-------- sentry/api/sentry.api | 7 +-- sentry/src/main/java/io/sentry/Hub.java | 29 ---------- .../src/main/java/io/sentry/HubAdapter.java | 21 ++++--- sentry/src/main/java/io/sentry/IHub.java | 10 ---- .../main/java/io/sentry/ISentryClient.java | 10 ---- sentry/src/main/java/io/sentry/NoOpHub.java | 7 +-- .../main/java/io/sentry/NoOpSentryClient.java | 5 -- .../src/main/java/io/sentry/SentryClient.java | 40 +++++-------- .../main/java/io/sentry/SentryEnvelope.java | 16 +++++ sentry/src/test/java/io/sentry/HubTest.kt | 28 ++------- .../test/java/io/sentry/SentryClientTest.kt | 35 +++++++---- 14 files changed, 139 insertions(+), 156 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8892a5827a..3b64ffadad 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,7 @@ ### Features +- Profile envelopes are sent directly from profiler ([#2298](https://github.com/getsentry/sentry-java/pull/2298)) - Add captureProfile method to hub and client ([#2290](https://github.com/getsentry/sentry-java/pull/2290)) ## 6.5.0 diff --git a/sentry-android-core/src/main/java/io/sentry/android/core/AndroidTransactionProfiler.java b/sentry-android-core/src/main/java/io/sentry/android/core/AndroidTransactionProfiler.java index b5af030213..6b3dc74baa 100644 --- a/sentry-android-core/src/main/java/io/sentry/android/core/AndroidTransactionProfiler.java +++ b/sentry-android-core/src/main/java/io/sentry/android/core/AndroidTransactionProfiler.java @@ -12,12 +12,15 @@ import android.os.Process; import android.os.SystemClock; import io.sentry.HubAdapter; +import io.sentry.IHub; import io.sentry.ITransaction; import io.sentry.ITransactionProfiler; import io.sentry.ProfilingTraceData; import io.sentry.ProfilingTransactionData; +import io.sentry.SentryEnvelope; import io.sentry.SentryLevel; import io.sentry.android.core.internal.util.CpuInfoUtils; +import io.sentry.exception.SentryEnvelopeException; import io.sentry.util.Objects; import java.io.File; import java.util.ArrayList; @@ -50,6 +53,7 @@ final class AndroidTransactionProfiler implements ITransactionProfiler { private @Nullable Future scheduledFinish = null; private final @NotNull Context context; private final @NotNull SentryAndroidOptions options; + private final @NotNull IHub hub; private final @NotNull BuildInfoProvider buildInfoProvider; private final @Nullable PackageInfo packageInfo; private long transactionStartNanos = 0; @@ -62,8 +66,17 @@ public AndroidTransactionProfiler( final @NotNull Context context, final @NotNull SentryAndroidOptions sentryAndroidOptions, final @NotNull BuildInfoProvider buildInfoProvider) { + this(context, sentryAndroidOptions, buildInfoProvider, HubAdapter.getInstance()); + } + + public AndroidTransactionProfiler( + final @NotNull Context context, + final @NotNull SentryAndroidOptions sentryAndroidOptions, + final @NotNull BuildInfoProvider buildInfoProvider, + final @NotNull IHub hub) { this.context = Objects.requireNonNull(context, "The application context is required"); this.options = Objects.requireNonNull(sentryAndroidOptions, "SentryAndroidOptions is required"); + this.hub = Objects.requireNonNull(hub, "Hub is required"); this.buildInfoProvider = Objects.requireNonNull(buildInfoProvider, "The BuildInfoProvider is required."); this.packageInfo = ContextUtils.getPackageInfo(context, options.getLogger(), buildInfoProvider); @@ -281,7 +294,20 @@ private synchronized void onTransactionFinish( ? ProfilingTraceData.TRUNCATION_REASON_TIMEOUT : ProfilingTraceData.TRUNCATION_REASON_NORMAL); - HubAdapter.getInstance().captureProfile(profilingTraceData); + SentryEnvelope envelope; + try { + envelope = + SentryEnvelope.from( + options.getSerializer(), + profilingTraceData, + options.getMaxTraceFileSize(), + options.getSdkVersion()); + } catch (SentryEnvelopeException e) { + options.getLogger().log(SentryLevel.ERROR, "Failed to capture profile.", e); + return; + } + + hub.captureEnvelope(envelope); } /** diff --git a/sentry-android-core/src/test/java/io/sentry/android/core/AndroidTransactionProfilerTest.kt b/sentry-android-core/src/test/java/io/sentry/android/core/AndroidTransactionProfilerTest.kt index 9dc75b1afa..faf1c1bd49 100644 --- a/sentry-android-core/src/test/java/io/sentry/android/core/AndroidTransactionProfilerTest.kt +++ b/sentry-android-core/src/test/java/io/sentry/android/core/AndroidTransactionProfilerTest.kt @@ -17,10 +17,10 @@ import io.sentry.IHub import io.sentry.ILogger import io.sentry.ISentryExecutorService import io.sentry.ProfilingTraceData -import io.sentry.Sentry import io.sentry.SentryLevel import io.sentry.SentryTracer import io.sentry.TransactionContext +import io.sentry.assertEnvelopeItem import io.sentry.test.getCtor import org.junit.runner.RunWith import java.io.File @@ -64,8 +64,7 @@ class AndroidTransactionProfilerTest { transaction1 = SentryTracer(TransactionContext("", ""), hub) transaction2 = SentryTracer(TransactionContext("", ""), hub) transaction3 = SentryTracer(TransactionContext("", ""), hub) - Sentry.setCurrentHub(hub) - return AndroidTransactionProfiler(context, options, buildInfoProvider) + return AndroidTransactionProfiler(context, options, buildInfoProvider, hub) } } @@ -104,9 +103,12 @@ class AndroidTransactionProfilerTest { profiler.onTransactionStart(fixture.transaction1) profiler.onTransactionFinish(fixture.transaction1) - verify(fixture.hub).captureProfile( + verify(fixture.hub).captureEnvelope( check { - assertEquals(it.transactionId, fixture.transaction1.eventId.toString()) + assertEquals(1, it.items.count()) + assertEnvelopeItem(it.items.toList()) { _, item -> + assertEquals(item.transactionId, fixture.transaction1.eventId.toString()) + } } ) } @@ -119,7 +121,7 @@ class AndroidTransactionProfilerTest { val profiler = fixture.getSut(context, buildInfo) profiler.onTransactionStart(fixture.transaction1) profiler.onTransactionFinish(fixture.transaction1) - verify(fixture.hub, never()).captureProfile(any()) + verify(fixture.hub, never()).captureEnvelope(any()) } @Test @@ -130,7 +132,7 @@ class AndroidTransactionProfilerTest { val profiler = fixture.getSut(context) profiler.onTransactionStart(fixture.transaction1) profiler.onTransactionFinish(fixture.transaction1) - verify(fixture.hub, never()).captureProfile(any()) + verify(fixture.hub, never()).captureEnvelope(any()) } @Test @@ -203,7 +205,7 @@ class AndroidTransactionProfilerTest { val profiler = fixture.getSut(context) profiler.onTransactionStart(fixture.transaction1) profiler.onTransactionFinish(fixture.transaction1) - verify(fixture.hub, never()).captureProfile(any()) + verify(fixture.hub, never()).captureEnvelope(any()) } @Test @@ -214,7 +216,7 @@ class AndroidTransactionProfilerTest { val profiler = fixture.getSut(context) profiler.onTransactionStart(fixture.transaction1) profiler.onTransactionFinish(fixture.transaction1) - verify(fixture.hub, never()).captureProfile(any()) + verify(fixture.hub, never()).captureEnvelope(any()) } @Test @@ -225,7 +227,7 @@ class AndroidTransactionProfilerTest { val profiler = fixture.getSut(context) profiler.onTransactionStart(fixture.transaction1) profiler.onTransactionFinish(fixture.transaction1) - verify(fixture.hub, never()).captureProfile(any()) + verify(fixture.hub, never()).captureEnvelope(any()) } @Test @@ -243,7 +245,7 @@ class AndroidTransactionProfilerTest { fun `onTransactionFinish works only if previously started`() { val profiler = fixture.getSut(context) profiler.onTransactionFinish(fixture.transaction1) - verify(fixture.hub, never()).captureProfile(any()) + verify(fixture.hub, never()).captureEnvelope(any()) } @Test @@ -263,10 +265,13 @@ class AndroidTransactionProfilerTest { // First transaction finishes: timed out data is returned profiler.onTransactionFinish(fixture.transaction1) - verify(fixture.hub).captureProfile( + verify(fixture.hub).captureEnvelope( check { - assertEquals(it.transactionId, fixture.transaction1.eventId.toString()) - assertEquals(ProfilingTraceData.TRUNCATION_REASON_TIMEOUT, it.truncationReason) + assertEquals(1, it.items.count()) + assertEnvelopeItem(it.items.toList()) { _, item -> + assertEquals(item.transactionId, fixture.transaction1.eventId.toString()) + assertEquals(ProfilingTraceData.TRUNCATION_REASON_TIMEOUT, item.truncationReason) + } } ) } @@ -278,12 +283,15 @@ class AndroidTransactionProfilerTest { profiler.onTransactionStart(fixture.transaction2) profiler.onTransactionFinish(fixture.transaction2) - verify(fixture.hub, never()).captureProfile(any()) + verify(fixture.hub, never()).captureEnvelope(any()) profiler.onTransactionFinish(fixture.transaction1) - verify(fixture.hub).captureProfile( + verify(fixture.hub).captureEnvelope( check { - assertEquals(it.transactionId, fixture.transaction1.eventId.toString()) + assertEquals(1, it.items.count()) + assertEnvelopeItem(it.items.toList()) { _, item -> + assertEquals(item.transactionId, fixture.transaction1.eventId.toString()) + } } ) } @@ -295,24 +303,28 @@ class AndroidTransactionProfilerTest { profiler.onTransactionStart(fixture.transaction2) profiler.onTransactionFinish(fixture.transaction1) - verify(fixture.hub, never()).captureProfile(any()) + verify(fixture.hub, never()).captureEnvelope(any()) profiler.onTransactionStart(fixture.transaction3) profiler.onTransactionFinish(fixture.transaction3) - verify(fixture.hub, never()).captureProfile(any()) + verify(fixture.hub, never()).captureEnvelope(any()) profiler.onTransactionFinish(fixture.transaction2) - verify(fixture.hub).captureProfile( + verify(fixture.hub).captureEnvelope( check { val expectedTransactions = listOf( fixture.transaction1.eventId.toString(), fixture.transaction3.eventId.toString(), fixture.transaction2.eventId.toString() ) - assertEquals(it.transactionId, fixture.transaction2.eventId.toString()) - assertTrue(it.transactions.map { it.id }.containsAll(expectedTransactions)) - assertTrue(expectedTransactions.containsAll(it.transactions.map { it.id })) + assertEquals(1, it.items.count()) + assertEnvelopeItem(it.items.toList()) { _, item -> + assertEquals(item.transactionId, fixture.transaction2.eventId.toString()) + + assertTrue(item.transactions.map { it.id }.containsAll(expectedTransactions)) + assertTrue(expectedTransactions.containsAll(item.transactions.map { it.id })) + } } ) } diff --git a/sentry/api/sentry.api b/sentry/api/sentry.api index 91e42e80a7..f9f303dffc 100644 --- a/sentry/api/sentry.api +++ b/sentry/api/sentry.api @@ -273,7 +273,6 @@ public final class io/sentry/Hub : io/sentry/IHub { public fun captureException (Ljava/lang/Throwable;Lio/sentry/Hint;Lio/sentry/ScopeCallback;)Lio/sentry/protocol/SentryId; public fun captureMessage (Ljava/lang/String;Lio/sentry/SentryLevel;)Lio/sentry/protocol/SentryId; public fun captureMessage (Ljava/lang/String;Lio/sentry/SentryLevel;Lio/sentry/ScopeCallback;)Lio/sentry/protocol/SentryId; - public fun captureProfile (Lio/sentry/ProfilingTraceData;)Lio/sentry/protocol/SentryId; public fun captureTransaction (Lio/sentry/protocol/SentryTransaction;Lio/sentry/TraceContext;Lio/sentry/Hint;)Lio/sentry/protocol/SentryId; public fun captureUserFeedback (Lio/sentry/UserFeedback;)V public fun clearBreadcrumbs ()V @@ -316,7 +315,6 @@ public final class io/sentry/HubAdapter : io/sentry/IHub { public fun captureException (Ljava/lang/Throwable;Lio/sentry/Hint;Lio/sentry/ScopeCallback;)Lio/sentry/protocol/SentryId; public fun captureMessage (Ljava/lang/String;Lio/sentry/SentryLevel;)Lio/sentry/protocol/SentryId; public fun captureMessage (Ljava/lang/String;Lio/sentry/SentryLevel;Lio/sentry/ScopeCallback;)Lio/sentry/protocol/SentryId; - public fun captureProfile (Lio/sentry/ProfilingTraceData;)Lio/sentry/protocol/SentryId; public fun captureTransaction (Lio/sentry/protocol/SentryTransaction;Lio/sentry/TraceContext;Lio/sentry/Hint;)Lio/sentry/protocol/SentryId; public fun captureTransaction (Lio/sentry/protocol/SentryTransaction;Lio/sentry/TraceContext;Lio/sentry/Hint;Lio/sentry/ProfilingTraceData;)Lio/sentry/protocol/SentryId; public fun captureUserFeedback (Lio/sentry/UserFeedback;)V @@ -380,7 +378,6 @@ public abstract interface class io/sentry/IHub { public fun captureMessage (Ljava/lang/String;Lio/sentry/ScopeCallback;)Lio/sentry/protocol/SentryId; public abstract fun captureMessage (Ljava/lang/String;Lio/sentry/SentryLevel;)Lio/sentry/protocol/SentryId; public abstract fun captureMessage (Ljava/lang/String;Lio/sentry/SentryLevel;Lio/sentry/ScopeCallback;)Lio/sentry/protocol/SentryId; - public abstract fun captureProfile (Lio/sentry/ProfilingTraceData;)Lio/sentry/protocol/SentryId; public fun captureTransaction (Lio/sentry/protocol/SentryTransaction;Lio/sentry/Hint;)Lio/sentry/protocol/SentryId; public fun captureTransaction (Lio/sentry/protocol/SentryTransaction;Lio/sentry/TraceContext;)Lio/sentry/protocol/SentryId; public abstract fun captureTransaction (Lio/sentry/protocol/SentryTransaction;Lio/sentry/TraceContext;Lio/sentry/Hint;)Lio/sentry/protocol/SentryId; @@ -450,7 +447,6 @@ public abstract interface class io/sentry/ISentryClient { public fun captureException (Ljava/lang/Throwable;Lio/sentry/Scope;Lio/sentry/Hint;)Lio/sentry/protocol/SentryId; public fun captureMessage (Ljava/lang/String;Lio/sentry/SentryLevel;)Lio/sentry/protocol/SentryId; public fun captureMessage (Ljava/lang/String;Lio/sentry/SentryLevel;Lio/sentry/Scope;)Lio/sentry/protocol/SentryId; - public abstract fun captureProfile (Lio/sentry/ProfilingTraceData;)Lio/sentry/protocol/SentryId; public fun captureSession (Lio/sentry/Session;)V public abstract fun captureSession (Lio/sentry/Session;Lio/sentry/Hint;)V public fun captureTransaction (Lio/sentry/protocol/SentryTransaction;)Lio/sentry/protocol/SentryId; @@ -671,7 +667,6 @@ public final class io/sentry/NoOpHub : io/sentry/IHub { public fun captureException (Ljava/lang/Throwable;Lio/sentry/Hint;Lio/sentry/ScopeCallback;)Lio/sentry/protocol/SentryId; public fun captureMessage (Ljava/lang/String;Lio/sentry/SentryLevel;)Lio/sentry/protocol/SentryId; public fun captureMessage (Ljava/lang/String;Lio/sentry/SentryLevel;Lio/sentry/ScopeCallback;)Lio/sentry/protocol/SentryId; - public fun captureProfile (Lio/sentry/ProfilingTraceData;)Lio/sentry/protocol/SentryId; public fun captureTransaction (Lio/sentry/protocol/SentryTransaction;Lio/sentry/TraceContext;Lio/sentry/Hint;)Lio/sentry/protocol/SentryId; public fun captureTransaction (Lio/sentry/protocol/SentryTransaction;Lio/sentry/TraceContext;Lio/sentry/Hint;Lio/sentry/ProfilingTraceData;)Lio/sentry/protocol/SentryId; public fun captureUserFeedback (Lio/sentry/UserFeedback;)V @@ -1161,7 +1156,6 @@ public final class io/sentry/SentryBaseEvent$Serializer { public final class io/sentry/SentryClient : io/sentry/ISentryClient { public fun captureEnvelope (Lio/sentry/SentryEnvelope;Lio/sentry/Hint;)Lio/sentry/protocol/SentryId; public fun captureEvent (Lio/sentry/SentryEvent;Lio/sentry/Scope;Lio/sentry/Hint;)Lio/sentry/protocol/SentryId; - public fun captureProfile (Lio/sentry/ProfilingTraceData;)Lio/sentry/protocol/SentryId; public fun captureSession (Lio/sentry/Session;Lio/sentry/Hint;)V public fun captureTransaction (Lio/sentry/protocol/SentryTransaction;Lio/sentry/TraceContext;Lio/sentry/Scope;Lio/sentry/Hint;)Lio/sentry/protocol/SentryId; public fun captureTransaction (Lio/sentry/protocol/SentryTransaction;Lio/sentry/TraceContext;Lio/sentry/Scope;Lio/sentry/Hint;Lio/sentry/ProfilingTraceData;)Lio/sentry/protocol/SentryId; @@ -1182,6 +1176,7 @@ public final class io/sentry/SentryEnvelope { public fun (Lio/sentry/SentryEnvelopeHeader;Ljava/lang/Iterable;)V public fun (Lio/sentry/protocol/SentryId;Lio/sentry/protocol/SdkVersion;Lio/sentry/SentryEnvelopeItem;)V public fun (Lio/sentry/protocol/SentryId;Lio/sentry/protocol/SdkVersion;Ljava/lang/Iterable;)V + public static fun from (Lio/sentry/ISerializer;Lio/sentry/ProfilingTraceData;JLio/sentry/protocol/SdkVersion;)Lio/sentry/SentryEnvelope; public static fun from (Lio/sentry/ISerializer;Lio/sentry/SentryBaseEvent;Lio/sentry/protocol/SdkVersion;)Lio/sentry/SentryEnvelope; public static fun from (Lio/sentry/ISerializer;Lio/sentry/Session;Lio/sentry/protocol/SdkVersion;)Lio/sentry/SentryEnvelope; public fun getHeader ()Lio/sentry/SentryEnvelopeHeader; diff --git a/sentry/src/main/java/io/sentry/Hub.java b/sentry/src/main/java/io/sentry/Hub.java index 192554d153..c573b45c12 100644 --- a/sentry/src/main/java/io/sentry/Hub.java +++ b/sentry/src/main/java/io/sentry/Hub.java @@ -600,35 +600,6 @@ public void flush(long timeoutMillis) { return new Hub(this.options, new Stack(this.stack)); } - @ApiStatus.Internal - @Override - public @NotNull SentryId captureProfile(final @NotNull ProfilingTraceData profilingTraceData) { - Objects.requireNonNull(profilingTraceData, "Profiling trace data is required"); - - SentryId sentryId = SentryId.EMPTY_ID; - if (!isEnabled()) { - options - .getLogger() - .log( - SentryLevel.WARNING, - "Instance is disabled and this 'captureProfile' call is a no-op."); - } else { - StackItem item = null; - try { - item = stack.peek(); - sentryId = item.getClient().captureProfile(profilingTraceData); - } catch (Throwable e) { - options - .getLogger() - .log( - SentryLevel.ERROR, - "Error while capturing profile with id: " + profilingTraceData.getProfileId(), - e); - } - } - return sentryId; - } - @ApiStatus.Internal @Override public @NotNull SentryId captureTransaction( diff --git a/sentry/src/main/java/io/sentry/HubAdapter.java b/sentry/src/main/java/io/sentry/HubAdapter.java index 1c18189d28..746d9c0b71 100644 --- a/sentry/src/main/java/io/sentry/HubAdapter.java +++ b/sentry/src/main/java/io/sentry/HubAdapter.java @@ -1,5 +1,6 @@ package io.sentry; +import io.sentry.exception.SentryEnvelopeException; import io.sentry.protocol.SentryId; import io.sentry.protocol.SentryTransaction; import io.sentry.protocol.User; @@ -172,14 +173,9 @@ public void flush(long timeoutMillis) { return Sentry.getCurrentHub().clone(); } - @Override - public @NotNull SentryId captureProfile(final @NotNull ProfilingTraceData profilingTraceData) { - return Sentry.getCurrentHub().captureProfile(profilingTraceData); - } - /** * @deprecated please use {{@link Hub#captureTransaction(SentryTransaction, TraceContext, Hint)}} - * and {{@link Hub#captureProfile(ProfilingTraceData)}} instead. + * and {{@link Hub#captureEnvelope(SentryEnvelope)}} instead. */ @Deprecated public @NotNull SentryId captureTransaction( @@ -188,7 +184,18 @@ public void flush(long timeoutMillis) { @Nullable Hint hint, @Nullable ProfilingTraceData profilingTraceData) { if (profilingTraceData != null) { - captureProfile(profilingTraceData); + SentryEnvelope envelope; + try { + envelope = + SentryEnvelope.from( + getOptions().getSerializer(), + profilingTraceData, + getOptions().getMaxTraceFileSize(), + getOptions().getSdkVersion()); + captureEnvelope(envelope); + } catch (SentryEnvelopeException e) { + getOptions().getLogger().log(SentryLevel.ERROR, "Failed to capture profile.", e); + } } return captureTransaction(transaction, traceContext, hint); } diff --git a/sentry/src/main/java/io/sentry/IHub.java b/sentry/src/main/java/io/sentry/IHub.java index fd221982aa..3b0779826c 100644 --- a/sentry/src/main/java/io/sentry/IHub.java +++ b/sentry/src/main/java/io/sentry/IHub.java @@ -341,16 +341,6 @@ default void addBreadcrumb(@NotNull String message, @NotNull String category) { @NotNull IHub clone(); - /** - * Captures a profile and enqueues it for sending to Sentry server. - * - * @param profilingTraceData the profiling trace data - * @return profile's id - */ - @ApiStatus.Internal - @NotNull - SentryId captureProfile(final @NotNull ProfilingTraceData profilingTraceData); - /** * Captures the transaction and enqueues it for sending to Sentry server. * diff --git a/sentry/src/main/java/io/sentry/ISentryClient.java b/sentry/src/main/java/io/sentry/ISentryClient.java index e6029c6299..bac9a7b3c1 100644 --- a/sentry/src/main/java/io/sentry/ISentryClient.java +++ b/sentry/src/main/java/io/sentry/ISentryClient.java @@ -192,16 +192,6 @@ default void captureSession(@NotNull Session session) { return captureEnvelope(envelope, null); } - /** - * Captures a profile. - * - * @param profilingTraceData The profiling trace data captured - * @return The Id (SentryId object) of the profile - */ - @NotNull - @ApiStatus.Internal - SentryId captureProfile(@NotNull ProfilingTraceData profilingTraceData); - /** * Captures a transaction. * diff --git a/sentry/src/main/java/io/sentry/NoOpHub.java b/sentry/src/main/java/io/sentry/NoOpHub.java index b7b8b7cccc..d94a1236ac 100644 --- a/sentry/src/main/java/io/sentry/NoOpHub.java +++ b/sentry/src/main/java/io/sentry/NoOpHub.java @@ -46,11 +46,6 @@ public boolean isEnabled() { return SentryId.EMPTY_ID; } - @Override - public @NotNull SentryId captureProfile(@NotNull ProfilingTraceData profilingTraceData) { - return SentryId.EMPTY_ID; - } - @Override public @NotNull SentryId captureEnvelope(@NotNull SentryEnvelope envelope, @Nullable Hint hint) { return SentryId.EMPTY_ID; @@ -139,7 +134,7 @@ public void flush(long timeoutMillis) {} /** * @deprecated please use {{@link Hub#captureTransaction(SentryTransaction, TraceContext, Hint)}} - * and {{@link Hub#captureProfile(ProfilingTraceData)}} instead. + * and {{@link Hub#captureEnvelope(SentryEnvelope)}} instead. */ @Deprecated @SuppressWarnings("InlineMeSuggester") diff --git a/sentry/src/main/java/io/sentry/NoOpSentryClient.java b/sentry/src/main/java/io/sentry/NoOpSentryClient.java index c06c2855f2..88bf38d017 100644 --- a/sentry/src/main/java/io/sentry/NoOpSentryClient.java +++ b/sentry/src/main/java/io/sentry/NoOpSentryClient.java @@ -43,11 +43,6 @@ public SentryId captureEnvelope(@NotNull SentryEnvelope envelope, @Nullable Hint return SentryId.EMPTY_ID; } - @Override - public @NotNull SentryId captureProfile(@NotNull ProfilingTraceData profilingTraceData) { - return SentryId.EMPTY_ID; - } - @Override public @NotNull SentryId captureTransaction( @NotNull SentryTransaction transaction, diff --git a/sentry/src/main/java/io/sentry/SentryClient.java b/sentry/src/main/java/io/sentry/SentryClient.java index 9bbe41acbc..c0d0506188 100644 --- a/sentry/src/main/java/io/sentry/SentryClient.java +++ b/sentry/src/main/java/io/sentry/SentryClient.java @@ -496,34 +496,9 @@ public void captureSession(final @NotNull Session session, final @Nullable Hint } } - @Override - public @NotNull SentryId captureProfile(final @NotNull ProfilingTraceData profilingTraceData) { - Objects.requireNonNull(profilingTraceData, "Profiling trace data is required."); - - SentryId sentryId = new SentryId(profilingTraceData.getProfileId()); - - options.getLogger().log(SentryLevel.DEBUG, "Capturing profile: %s", sentryId); - - try { - final SentryEnvelope envelope = buildEnvelope(null, null, null, null, profilingTraceData); - - if (envelope != null) { - transport.send(envelope); - } else { - sentryId = SentryId.EMPTY_ID; - } - } catch (IOException | SentryEnvelopeException e) { - options.getLogger().log(SentryLevel.WARNING, e, "Capturing profile %s failed.", sentryId); - // if there was an error capturing the profile, we return an emptyId - sentryId = SentryId.EMPTY_ID; - } - - return sentryId; - } - /** * @deprecated please use {{@link ISentryClient#captureTransaction(SentryTransaction, - * TraceContext, Scope, Hint)}} and {{@link ISentryClient#captureProfile(ProfilingTraceData)}} + * TraceContext, Scope, Hint)}} and {{@link ISentryClient#captureEnvelope(SentryEnvelope)}} * instead. */ @Deprecated @@ -534,7 +509,18 @@ public void captureSession(final @NotNull Session session, final @Nullable Hint @Nullable Hint hint, final @Nullable ProfilingTraceData profilingTraceData) { if (profilingTraceData != null) { - captureProfile(profilingTraceData); + SentryEnvelope envelope; + try { + envelope = + SentryEnvelope.from( + options.getSerializer(), + profilingTraceData, + options.getMaxTraceFileSize(), + options.getSdkVersion()); + captureEnvelope(envelope); + } catch (SentryEnvelopeException e) { + options.getLogger().log(SentryLevel.ERROR, "Failed to capture profile.", e); + } } return captureTransaction(transaction, traceContext, scope, hint); } diff --git a/sentry/src/main/java/io/sentry/SentryEnvelope.java b/sentry/src/main/java/io/sentry/SentryEnvelope.java index 5a11ee54d1..5c3f141baf 100644 --- a/sentry/src/main/java/io/sentry/SentryEnvelope.java +++ b/sentry/src/main/java/io/sentry/SentryEnvelope.java @@ -1,5 +1,6 @@ package io.sentry; +import io.sentry.exception.SentryEnvelopeException; import io.sentry.protocol.SdkVersion; import io.sentry.protocol.SentryId; import io.sentry.util.Objects; @@ -76,4 +77,19 @@ public SentryEnvelope( return new SentryEnvelope( event.getEventId(), sdkVersion, SentryEnvelopeItem.fromEvent(serializer, event)); } + + public static @NotNull SentryEnvelope from( + final @NotNull ISerializer serializer, + final @NotNull ProfilingTraceData profilingTraceData, + final long maxTraceFileSize, + final @Nullable SdkVersion sdkVersion) + throws SentryEnvelopeException { + Objects.requireNonNull(serializer, "Serializer is required."); + Objects.requireNonNull(profilingTraceData, "Profiling trace data is required."); + + return new SentryEnvelope( + new SentryId(profilingTraceData.getProfileId()), + sdkVersion, + SentryEnvelopeItem.fromProfilingTrace(profilingTraceData, maxTraceFileSize, serializer)); + } } diff --git a/sentry/src/test/java/io/sentry/HubTest.kt b/sentry/src/test/java/io/sentry/HubTest.kt index 54440d714d..4bc6b1c446 100644 --- a/sentry/src/test/java/io/sentry/HubTest.kt +++ b/sentry/src/test/java/io/sentry/HubTest.kt @@ -1363,29 +1363,13 @@ class HubTest { } //endregion - //region captureProfile tests - @Test - fun `when captureProfile is called on disabled client, do nothing`() { - val options = SentryOptions() - options.cacheDirPath = file.absolutePath - options.dsn = "https://key@sentry.io/proj" - options.setSerializer(mock()) - val sut = Hub(options) - val mockClient = mock() - sut.bindClient(mockClient) - sut.close() - - val sentryTracer = SentryTracer(TransactionContext("name", "op"), sut) - sentryTracer.finish() - sut.captureProfile(ProfilingTraceData(profilingTraceFile, sentryTracer)) - verify(mockClient, never()).captureProfile(any()) - } + //region profiling tests @Test fun `when startTransaction and profiling is enabled, transaction is profiled only if sampled`() { val mockTransactionProfiler = mock() val mockClient = mock() - whenever(mockTransactionProfiler.onTransactionFinish(any())).thenAnswer { mockClient.captureProfile(mock()) } + whenever(mockTransactionProfiler.onTransactionFinish(any())).thenAnswer { mockClient.captureEnvelope(mock()) } val hub = generateHub { it.setTransactionProfiler(mockTransactionProfiler) } @@ -1394,20 +1378,20 @@ class HubTest { val contexts = TransactionContext("name", "op", TracesSamplingDecision(false, null, true, null)) val transaction = hub.startTransaction(contexts) transaction.finish() - verify(mockClient, never()).captureProfile(any()) + verify(mockClient, never()).captureEnvelope(any()) // Transaction is sampled, so it should be profiled val sampledContexts = TransactionContext("name", "op", TracesSamplingDecision(true, null, true, null)) val sampledTransaction = hub.startTransaction(sampledContexts) sampledTransaction.finish() - verify(mockClient).captureProfile(any()) + verify(mockClient).captureEnvelope(any()) } @Test fun `when startTransaction and is sampled but profiling is disabled, transaction is not profiled`() { val mockTransactionProfiler = mock() val mockClient = mock() - whenever(mockTransactionProfiler.onTransactionFinish(any())).thenAnswer { mockClient.captureProfile(mock()) } + whenever(mockTransactionProfiler.onTransactionFinish(any())).thenAnswer { mockClient.captureEnvelope(mock()) } val hub = generateHub { it.profilesSampleRate = 0.0 it.setTransactionProfiler(mockTransactionProfiler) @@ -1416,7 +1400,7 @@ class HubTest { val contexts = TransactionContext("name", "op") val transaction = hub.startTransaction(contexts) transaction.finish() - verify(mockClient, never()).captureProfile(any()) + verify(mockClient, never()).captureEnvelope(any()) } //endregion diff --git a/sentry/src/test/java/io/sentry/SentryClientTest.kt b/sentry/src/test/java/io/sentry/SentryClientTest.kt index 48db17f085..3f0f282e0e 100644 --- a/sentry/src/test/java/io/sentry/SentryClientTest.kt +++ b/sentry/src/test/java/io/sentry/SentryClientTest.kt @@ -1043,34 +1043,48 @@ class SentryClientTest { } @Test - fun `when captureProfile`() { + fun `when captureEnvelope with ProfilingTraceData`() { val client = fixture.getSut() - client.captureProfile(fixture.profilingTraceData) + val options = fixture.sentryOptions + val envelope = SentryEnvelope.from(options.serializer, fixture.profilingTraceData, options.maxTraceFileSize, options.sdkVersion) + client.captureEnvelope(envelope) verifyProfilingTraceInEnvelope(SentryId(fixture.profilingTraceData.profileId)) } @Test - fun `captureTransaction with profile calls captureProfile and captureTransaction`() { + fun `captureTransaction with profile calls captureEnvelope and captureTransaction`() { val transaction = SentryTransaction(fixture.sentryTracer) val client = spy(fixture.getSut()) client.captureTransaction(transaction, null, null, null, fixture.profilingTraceData) verify(client).captureTransaction(transaction, null, null, null) - verify(client).captureProfile(fixture.profilingTraceData) + verify(client).captureEnvelope( + check { + assertEquals(1, it.items.count()) + assertEnvelopeItem(it.items.toList()) { _, item -> + assertEquals(item.profileId, fixture.profilingTraceData.profileId) + } + }, + anyOrNull() + ) } @Test - fun `when captureProfile with empty trace file, profile is not sent`() { + fun `when capture profile with empty trace file, profile is not sent`() { val client = fixture.getSut() - client.captureProfile(fixture.profilingTraceData) + val options = fixture.sentryOptions + val envelope = SentryEnvelope.from(options.serializer, fixture.profilingTraceData, options.maxTraceFileSize, options.sdkVersion) + client.captureEnvelope(envelope) fixture.profilingTraceFile.writeText("") assertFails { verifyProfilingTraceInEnvelope(SentryId(fixture.profilingTraceData.profileId)) } } @Test - fun `when captureProfile with non existing profiling trace file, profile is not sent`() { + fun `when capture profile with non existing profiling trace file, profile is not sent`() { val client = fixture.getSut() - client.captureProfile(fixture.profilingNonExistingTraceData) - assertFails { verifyProfilingTraceInEnvelope(SentryId(fixture.profilingTraceData.profileId)) } + val options = fixture.sentryOptions + val envelope = SentryEnvelope.from(options.serializer, fixture.profilingNonExistingTraceData, options.maxTraceFileSize, options.sdkVersion) + client.captureEnvelope(envelope) + assertFails { verifyProfilingTraceInEnvelope(SentryId(fixture.profilingNonExistingTraceData.profileId)) } } @Test @@ -2046,7 +2060,8 @@ class SentryClientTest { item.header.type == SentryItemType.Profile } assertNotNull(profilingTraceItem?.data) - } + }, + anyOrNull() ) }