From d7e2ffc57d245a7cf01565b1e452cf6a997404e4 Mon Sep 17 00:00:00 2001 From: Markus Hintersteiner Date: Tue, 8 Oct 2024 16:04:48 +0200 Subject: [PATCH 1/8] Fix don't register any full drawn listeners if feature is not enabled --- .../core/ActivityLifecycleIntegration.java | 2 +- .../core/ActivityLifecycleIntegrationTest.kt | 22 +++++++++++++++++-- sentry/api/sentry.api | 1 + .../main/java/io/sentry/SentryOptions.java | 9 +++++++- 4 files changed, 30 insertions(+), 4 deletions(-) diff --git a/sentry-android-core/src/main/java/io/sentry/android/core/ActivityLifecycleIntegration.java b/sentry-android-core/src/main/java/io/sentry/android/core/ActivityLifecycleIntegration.java index 7556afb235..6e7a22ac05 100644 --- a/sentry-android-core/src/main/java/io/sentry/android/core/ActivityLifecycleIntegration.java +++ b/sentry-android-core/src/main/java/io/sentry/android/core/ActivityLifecycleIntegration.java @@ -382,7 +382,7 @@ public synchronized void onActivityCreated( firstActivityCreated = true; - if (fullyDisplayedReporter != null) { + if (performanceEnabled && ttfdSpan != null && fullyDisplayedReporter != null) { fullyDisplayedReporter.registerFullyDrawnListener(() -> onFullFrameDrawn(ttfdSpan)); } } diff --git a/sentry-android-core/src/test/java/io/sentry/android/core/ActivityLifecycleIntegrationTest.kt b/sentry-android-core/src/test/java/io/sentry/android/core/ActivityLifecycleIntegrationTest.kt index b355075ff1..addeb94819 100644 --- a/sentry-android-core/src/test/java/io/sentry/android/core/ActivityLifecycleIntegrationTest.kt +++ b/sentry-android-core/src/test/java/io/sentry/android/core/ActivityLifecycleIntegrationTest.kt @@ -78,7 +78,6 @@ class ActivityLifecycleIntegrationTest { } val bundle = mock() val activityFramesTracker = mock() - val fullyDisplayedReporter = FullyDisplayedReporter.getInstance() val transactionFinishedCallback = mock() lateinit var shadowActivityManager: ShadowActivityManager @@ -619,11 +618,30 @@ class ActivityLifecycleIntegrationTest { sut.onActivityCreated(activity, mock()) val ttfdSpan = sut.ttfdSpanMap[activity] sut.ttidSpanMap.values.first().finish() - fixture.fullyDisplayedReporter.reportFullyDrawn() + fixture.options.fullyDisplayedReporter.reportFullyDrawn() assertTrue(ttfdSpan!!.isFinished) assertNotEquals(SpanStatus.CANCELLED, ttfdSpan.status) } + @Test + fun `if ttfd is disabled, no listener is registered for FullyDisplayedReporter`() { + val ttfdReporter = mock() + + val sut = fixture.getSut() + fixture.options.apply { + tracesSampleRate = 1.0 + isEnableTimeToFullDisplayTracing = false + fullyDisplayedReporter = ttfdReporter + } + + sut.register(fixture.hub, fixture.options) + + val activity = mock() + sut.onActivityCreated(activity, mock()) + + verify(ttfdReporter, never()).registerFullyDrawnListener(any()) + } + @Test fun `App start is Cold when savedInstanceState is null`() { val sut = fixture.getSut() diff --git a/sentry/api/sentry.api b/sentry/api/sentry.api index 530d8241f5..786f5ebd5c 100644 --- a/sentry/api/sentry.api +++ b/sentry/api/sentry.api @@ -2523,6 +2523,7 @@ public class io/sentry/SentryOptions { public fun setEnvironment (Ljava/lang/String;)V public fun setExecutorService (Lio/sentry/ISentryExecutorService;)V public fun setFlushTimeoutMillis (J)V + public fun setFullyDisplayedReporter (Lio/sentry/FullyDisplayedReporter;)V public fun setGestureTargetLocators (Ljava/util/List;)V public fun setIdleTimeout (Ljava/lang/Long;)V public fun setIgnoredCheckIns (Ljava/util/List;)V diff --git a/sentry/src/main/java/io/sentry/SentryOptions.java b/sentry/src/main/java/io/sentry/SentryOptions.java index 3ff84c48de..404bbee40b 100644 --- a/sentry/src/main/java/io/sentry/SentryOptions.java +++ b/sentry/src/main/java/io/sentry/SentryOptions.java @@ -428,7 +428,7 @@ public class SentryOptions { private boolean enableTimeToFullDisplayTracing = false; /** Screen fully displayed reporter, used for time-to-full-display spans. */ - private final @NotNull FullyDisplayedReporter fullyDisplayedReporter = + private @NotNull FullyDisplayedReporter fullyDisplayedReporter = FullyDisplayedReporter.getInstance(); private @NotNull IConnectionStatusProvider connectionStatusProvider = @@ -2097,6 +2097,13 @@ public void setEnableTimeToFullDisplayTracing(final boolean enableTimeToFullDisp return fullyDisplayedReporter; } + @ApiStatus.Internal + @TestOnly + public void setFullyDisplayedReporter( + final @NotNull FullyDisplayedReporter fullyDisplayedReporter) { + this.fullyDisplayedReporter = fullyDisplayedReporter; + } + /** * Whether OPTIONS requests should be traced. * From fd2bc62041be800df5b04288a8b26a11533643f5 Mon Sep 17 00:00:00 2001 From: Markus Hintersteiner Date: Tue, 8 Oct 2024 16:05:14 +0200 Subject: [PATCH 2/8] Offload app components breadcrumbs to background thread --- .../AppComponentsBreadcrumbsIntegration.java | 66 ++++++++++++------- ...AppComponentsBreadcrumbsIntegrationTest.kt | 9 ++- 2 files changed, 48 insertions(+), 27 deletions(-) diff --git a/sentry-android-core/src/main/java/io/sentry/android/core/AppComponentsBreadcrumbsIntegration.java b/sentry-android-core/src/main/java/io/sentry/android/core/AppComponentsBreadcrumbsIntegration.java index 97c7d06ce7..84267a2812 100644 --- a/sentry-android-core/src/main/java/io/sentry/android/core/AppComponentsBreadcrumbsIntegration.java +++ b/sentry-android-core/src/main/java/io/sentry/android/core/AppComponentsBreadcrumbsIntegration.java @@ -85,41 +85,20 @@ public void close() throws IOException { @SuppressWarnings("deprecation") @Override public void onConfigurationChanged(@NotNull Configuration newConfig) { - if (hub != null) { - final Device.DeviceOrientation deviceOrientation = - DeviceOrientations.getOrientation(context.getResources().getConfiguration().orientation); - - String orientation; - if (deviceOrientation != null) { - orientation = deviceOrientation.name().toLowerCase(Locale.ROOT); - } else { - orientation = "undefined"; - } - - final Breadcrumb breadcrumb = new Breadcrumb(); - breadcrumb.setType("navigation"); - breadcrumb.setCategory("device.orientation"); - breadcrumb.setData("position", orientation); - breadcrumb.setLevel(SentryLevel.INFO); - - final Hint hint = new Hint(); - hint.set(ANDROID_CONFIGURATION, newConfig); - - hub.addBreadcrumb(breadcrumb, hint); - } + executeInBackground(() -> captureConfigurationChangedBreadcrumb(newConfig)); } @Override public void onLowMemory() { - createLowMemoryBreadcrumb(null); + executeInBackground(() -> captureLowMemoryBreadcrumb(null)); } @Override public void onTrimMemory(final int level) { - createLowMemoryBreadcrumb(level); + executeInBackground(() -> captureLowMemoryBreadcrumb(level)); } - private void createLowMemoryBreadcrumb(final @Nullable Integer level) { + private void captureLowMemoryBreadcrumb(final @Nullable Integer level) { if (hub != null) { final Breadcrumb breadcrumb = new Breadcrumb(); if (level != null) { @@ -147,4 +126,41 @@ private void createLowMemoryBreadcrumb(final @Nullable Integer level) { hub.addBreadcrumb(breadcrumb); } } + + private void captureConfigurationChangedBreadcrumb(final @NotNull Configuration newConfig) { + if (hub != null) { + final Device.DeviceOrientation deviceOrientation = + DeviceOrientations.getOrientation(context.getResources().getConfiguration().orientation); + + String orientation; + if (deviceOrientation != null) { + orientation = deviceOrientation.name().toLowerCase(Locale.ROOT); + } else { + orientation = "undefined"; + } + + final Breadcrumb breadcrumb = new Breadcrumb(); + breadcrumb.setType("navigation"); + breadcrumb.setCategory("device.orientation"); + breadcrumb.setData("position", orientation); + breadcrumb.setLevel(SentryLevel.INFO); + + final Hint hint = new Hint(); + hint.set(ANDROID_CONFIGURATION, newConfig); + + hub.addBreadcrumb(breadcrumb, hint); + } + } + + private void executeInBackground(final @NotNull Runnable runnable) { + if (options != null) { + try { + options.getExecutorService().submit(runnable); + } catch (Throwable t) { + options + .getLogger() + .log(SentryLevel.ERROR, t, "Failed to submit app components breadcrumb task"); + } + } + } } diff --git a/sentry-android-core/src/test/java/io/sentry/android/core/AppComponentsBreadcrumbsIntegrationTest.kt b/sentry-android-core/src/test/java/io/sentry/android/core/AppComponentsBreadcrumbsIntegrationTest.kt index 15a6d690e5..75152767a5 100644 --- a/sentry-android-core/src/test/java/io/sentry/android/core/AppComponentsBreadcrumbsIntegrationTest.kt +++ b/sentry-android-core/src/test/java/io/sentry/android/core/AppComponentsBreadcrumbsIntegrationTest.kt @@ -7,6 +7,7 @@ import androidx.test.ext.junit.runners.AndroidJUnit4 import io.sentry.Breadcrumb import io.sentry.IHub import io.sentry.SentryLevel +import io.sentry.test.ImmediateExecutorService import org.junit.runner.RunWith import org.mockito.kotlin.any import org.mockito.kotlin.anyOrNull @@ -88,7 +89,9 @@ class AppComponentsBreadcrumbsIntegrationTest { @Test fun `When low memory event, a breadcrumb with type, category and level should be set`() { val sut = fixture.getSut() - val options = SentryAndroidOptions() + val options = SentryAndroidOptions().apply { + executorService = ImmediateExecutorService() + } val hub = mock() sut.register(hub, options) sut.onLowMemory() @@ -104,7 +107,9 @@ class AppComponentsBreadcrumbsIntegrationTest { @Test fun `When trim memory event with level, a breadcrumb with type, category and level should be set`() { val sut = fixture.getSut() - val options = SentryAndroidOptions() + val options = SentryAndroidOptions().apply { + executorService = ImmediateExecutorService() + } val hub = mock() sut.register(hub, options) sut.onTrimMemory(ComponentCallbacks2.TRIM_MEMORY_BACKGROUND) From 7561fe45bd88dc28af09a3a28dd7f990aaa3de70 Mon Sep 17 00:00:00 2001 From: Markus Hintersteiner Date: Tue, 8 Oct 2024 16:10:02 +0200 Subject: [PATCH 3/8] Offload system events breadcrumbs to background thread --- .../SystemEventsBreadcrumbsIntegration.java | 39 +++++++++++++------ 1 file changed, 27 insertions(+), 12 deletions(-) diff --git a/sentry-android-core/src/main/java/io/sentry/android/core/SystemEventsBreadcrumbsIntegration.java b/sentry-android-core/src/main/java/io/sentry/android/core/SystemEventsBreadcrumbsIntegration.java index f196b7ca90..5d5068c64f 100644 --- a/sentry-android-core/src/main/java/io/sentry/android/core/SystemEventsBreadcrumbsIntegration.java +++ b/sentry-android-core/src/main/java/io/sentry/android/core/SystemEventsBreadcrumbsIntegration.java @@ -211,7 +211,7 @@ static final class SystemEventsBroadcastReceiver extends BroadcastReceiver { private static final long DEBOUNCE_WAIT_TIME_MS = 60 * 1000; private final @NotNull IHub hub; private final @NotNull SentryAndroidOptions options; - private final @NotNull Debouncer debouncer = + private final @NotNull Debouncer batteryChangedDebouncer = new Debouncer(AndroidCurrentDateProvider.getInstance(), DEBOUNCE_WAIT_TIME_MS, 0); SystemEventsBroadcastReceiver( @@ -221,19 +221,38 @@ static final class SystemEventsBroadcastReceiver extends BroadcastReceiver { } @Override - public void onReceive(Context context, Intent intent) { - final boolean shouldDebounce = debouncer.checkForDebounce(); - final String action = intent.getAction(); + public void onReceive(final Context context, final @NotNull Intent intent) { + final @Nullable String action = intent.getAction(); final boolean isBatteryChanged = ACTION_BATTERY_CHANGED.equals(action); - if (isBatteryChanged && shouldDebounce) { - // aligning with iOS which only captures battery status changes every minute at maximum + + // aligning with iOS which only captures battery status changes every minute at maximum + if (isBatteryChanged && batteryChangedDebouncer.checkForDebounce()) { return; } + try { + options + .getExecutorService() + .submit( + () -> { + final Breadcrumb breadcrumb = createBreadcrumb(intent, action, isBatteryChanged); + final Hint hint = new Hint(); + hint.set(ANDROID_INTENT, intent); + hub.addBreadcrumb(breadcrumb, hint); + }); + } catch (Throwable t) { + options + .getLogger() + .log(SentryLevel.ERROR, t, "Failed to submit system event breadcrumb action."); + } + } + + private @NotNull Breadcrumb createBreadcrumb( + final @NotNull Intent intent, final @Nullable String action, boolean isBatteryChanged) { final Breadcrumb breadcrumb = new Breadcrumb(); breadcrumb.setType("system"); breadcrumb.setCategory("device.event"); - String shortAction = StringUtils.getStringAfterDot(action); + final String shortAction = StringUtils.getStringAfterDot(action); if (shortAction != null) { breadcrumb.setData("action", shortAction); } @@ -273,11 +292,7 @@ public void onReceive(Context context, Intent intent) { } } breadcrumb.setLevel(SentryLevel.INFO); - - final Hint hint = new Hint(); - hint.set(ANDROID_INTENT, intent); - - hub.addBreadcrumb(breadcrumb, hint); + return breadcrumb; } } } From 0870f0908d851149c8552fa459c76bb08ac3fac9 Mon Sep 17 00:00:00 2001 From: Markus Hintersteiner Date: Tue, 8 Oct 2024 16:20:55 +0200 Subject: [PATCH 4/8] Update Changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4e6bc7c535..050920b471 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -20,6 +20,7 @@ - If you're using code obfuscation, adjust your proguard-rules accordingly, so your custom view class name is not minified - Fix ensure Application Context is used even when SDK is initialized via Activity Context ([#3669](https://github.com/getsentry/sentry-java/pull/3669)) - Fix potential ANRs due to `Calendar.getInstance` usage in Breadcrumbs constructor ([#3736](https://github.com/getsentry/sentry-java/pull/3736)) +- Fix potential ANRs due to default integrations ([#3778](https://github.com/getsentry/sentry-java/pull/3778)) *Breaking changes*: From 4ffe90320bb593eaf096c7c5ed52ccdc361d5b13 Mon Sep 17 00:00:00 2001 From: Markus Hintersteiner Date: Wed, 9 Oct 2024 10:17:44 +0200 Subject: [PATCH 5/8] Fix flaky tests --- ...AppComponentsBreadcrumbsIntegrationTest.kt | 25 ++++++++++++++----- 1 file changed, 19 insertions(+), 6 deletions(-) diff --git a/sentry-android-core/src/test/java/io/sentry/android/core/AppComponentsBreadcrumbsIntegrationTest.kt b/sentry-android-core/src/test/java/io/sentry/android/core/AppComponentsBreadcrumbsIntegrationTest.kt index 75152767a5..8f45c23802 100644 --- a/sentry-android-core/src/test/java/io/sentry/android/core/AppComponentsBreadcrumbsIntegrationTest.kt +++ b/sentry-android-core/src/test/java/io/sentry/android/core/AppComponentsBreadcrumbsIntegrationTest.kt @@ -37,7 +37,9 @@ class AppComponentsBreadcrumbsIntegrationTest { @Test fun `When app components breadcrumb is enabled, it registers callback`() { val sut = fixture.getSut() - val options = SentryAndroidOptions() + val options = SentryAndroidOptions().apply { + executorService = ImmediateExecutorService() + } val hub = mock() sut.register(hub, options) verify(fixture.context).registerComponentCallbacks(any()) @@ -46,7 +48,9 @@ class AppComponentsBreadcrumbsIntegrationTest { @Test fun `When app components breadcrumb is enabled, but ComponentCallbacks is not ready, do not throw`() { val sut = fixture.getSut() - val options = SentryAndroidOptions() + val options = SentryAndroidOptions().apply { + executorService = ImmediateExecutorService() + } val hub = mock() sut.register(hub, options) whenever(fixture.context.registerComponentCallbacks(any())).thenThrow(NullPointerException()) @@ -59,6 +63,7 @@ class AppComponentsBreadcrumbsIntegrationTest { val sut = fixture.getSut() val options = SentryAndroidOptions().apply { isEnableAppComponentBreadcrumbs = false + executorService = ImmediateExecutorService() } val hub = mock() sut.register(hub, options) @@ -68,7 +73,9 @@ class AppComponentsBreadcrumbsIntegrationTest { @Test fun `When AppComponentsBreadcrumbsIntegrationTest is closed, it should unregister the callback`() { val sut = fixture.getSut() - val options = SentryAndroidOptions() + val options = SentryAndroidOptions().apply { + executorService = ImmediateExecutorService() + } val hub = mock() sut.register(hub, options) sut.close() @@ -78,7 +85,9 @@ class AppComponentsBreadcrumbsIntegrationTest { @Test fun `When app components breadcrumb is closed, but ComponentCallbacks is not ready, do not throw`() { val sut = fixture.getSut() - val options = SentryAndroidOptions() + val options = SentryAndroidOptions().apply { + executorService = ImmediateExecutorService() + } val hub = mock() whenever(fixture.context.registerComponentCallbacks(any())).thenThrow(NullPointerException()) whenever(fixture.context.unregisterComponentCallbacks(any())).thenThrow(NullPointerException()) @@ -125,7 +134,9 @@ class AppComponentsBreadcrumbsIntegrationTest { @Test fun `When trim memory event with level not so high, do not add a breadcrumb`() { val sut = fixture.getSut() - val options = SentryAndroidOptions() + val options = SentryAndroidOptions().apply { + executorService = ImmediateExecutorService() + } val hub = mock() sut.register(hub, options) sut.onTrimMemory(ComponentCallbacks2.TRIM_MEMORY_RUNNING_CRITICAL) @@ -135,7 +146,9 @@ class AppComponentsBreadcrumbsIntegrationTest { @Test fun `When device orientation event, a breadcrumb with type, category and level should be set`() { val sut = AppComponentsBreadcrumbsIntegration(ApplicationProvider.getApplicationContext()) - val options = SentryAndroidOptions() + val options = SentryAndroidOptions().apply { + executorService = ImmediateExecutorService() + } val hub = mock() sut.register(hub, options) sut.onConfigurationChanged(mock()) From 41b11a9e648255e10502ef6c3c519a3e763e9a74 Mon Sep 17 00:00:00 2001 From: Markus Hintersteiner Date: Wed, 9 Oct 2024 11:03:31 +0200 Subject: [PATCH 6/8] Remove session breadcrumbs --- .../main/java/io/sentry/android/core/LifecycleWatcher.java | 7 ------- .../main/java/io/sentry/android/core/SentryAndroid.java | 1 - 2 files changed, 8 deletions(-) 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 81e77a75fb..c16747515f 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 @@ -90,7 +90,6 @@ private void startSession() { if (lastUpdatedSession == 0L || (lastUpdatedSession + sessionIntervalMillis) <= currentTimeMillis) { if (enableSessionTracking) { - addSessionBreadcrumb("start"); hub.startSession(); } hub.getOptions().getReplayController().start(); @@ -125,7 +124,6 @@ private void scheduleEndSession() { @Override public void run() { if (enableSessionTracking) { - addSessionBreadcrumb("end"); hub.endSession(); } hub.getOptions().getReplayController().stop(); @@ -157,11 +155,6 @@ private void addAppBreadcrumb(final @NotNull String state) { } } - private void addSessionBreadcrumb(final @NotNull String state) { - final Breadcrumb breadcrumb = BreadcrumbFactory.forSession(state); - hub.addBreadcrumb(breadcrumb); - } - @TestOnly @Nullable TimerTask getTimerTask() { diff --git a/sentry-android-core/src/main/java/io/sentry/android/core/SentryAndroid.java b/sentry-android-core/src/main/java/io/sentry/android/core/SentryAndroid.java index 0c1a74edb0..b083309a3f 100644 --- a/sentry-android-core/src/main/java/io/sentry/android/core/SentryAndroid.java +++ b/sentry-android-core/src/main/java/io/sentry/android/core/SentryAndroid.java @@ -173,7 +173,6 @@ public static synchronized void init( } }); if (!sessionStarted.get()) { - hub.addBreadcrumb(BreadcrumbFactory.forSession("session.start")); hub.startSession(); } } From 40cbfea12a0eced8214dd082c040af309ef03d20 Mon Sep 17 00:00:00 2001 From: Markus Hintersteiner Date: Wed, 9 Oct 2024 12:14:26 +0200 Subject: [PATCH 7/8] Fix tests --- .../sentry/android/core/LifecycleWatcher.java | 1 - .../core/NetworkBreadcrumbsIntegration.java | 73 +++++++++++++------ .../io/sentry/android/core/SentryAndroid.java | 1 - .../android/core/LifecycleWatcherTest.kt | 43 ----------- .../core/NetworkBreadcrumbsIntegrationTest.kt | 51 +++++++++++-- 5 files changed, 95 insertions(+), 74 deletions(-) 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 c16747515f..23072265eb 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 @@ -6,7 +6,6 @@ import io.sentry.IHub; import io.sentry.SentryLevel; import io.sentry.Session; -import io.sentry.android.core.internal.util.BreadcrumbFactory; import io.sentry.transport.CurrentDateProvider; import io.sentry.transport.ICurrentDateProvider; import java.util.Timer; diff --git a/sentry-android-core/src/main/java/io/sentry/android/core/NetworkBreadcrumbsIntegration.java b/sentry-android-core/src/main/java/io/sentry/android/core/NetworkBreadcrumbsIntegration.java index e30dfb681c..bf4f9ccd52 100644 --- a/sentry-android-core/src/main/java/io/sentry/android/core/NetworkBreadcrumbsIntegration.java +++ b/sentry-android-core/src/main/java/io/sentry/android/core/NetworkBreadcrumbsIntegration.java @@ -31,12 +31,12 @@ public final class NetworkBreadcrumbsIntegration implements Integration, Closeable { private final @NotNull Context context; - private final @NotNull BuildInfoProvider buildInfoProvider; - private final @NotNull ILogger logger; + private volatile boolean isClosed; + private @Nullable SentryOptions options; - @TestOnly @Nullable NetworkBreadcrumbsNetworkCallback networkCallback; + @TestOnly @Nullable volatile NetworkBreadcrumbsNetworkCallback networkCallback; public NetworkBreadcrumbsIntegration( final @NotNull Context context, @@ -63,40 +63,71 @@ public void register(final @NotNull IHub hub, final @NotNull SentryOptions optio "NetworkBreadcrumbsIntegration enabled: %s", androidOptions.isEnableNetworkEventBreadcrumbs()); + this.options = options; + if (androidOptions.isEnableNetworkEventBreadcrumbs()) { // The specific error is logged in the ConnectivityChecker method - if (buildInfoProvider.getSdkInfoVersion() < Build.VERSION_CODES.LOLLIPOP) { - networkCallback = null; - logger.log(SentryLevel.DEBUG, "NetworkBreadcrumbsIntegration requires Android 5+"); + if (buildInfoProvider.getSdkInfoVersion() < Build.VERSION_CODES.N) { + logger.log(SentryLevel.DEBUG, "NetworkCallbacks need Android N+."); return; } networkCallback = new NetworkBreadcrumbsNetworkCallback(hub, buildInfoProvider, options.getDateProvider()); - final boolean registered = - AndroidConnectionStatusProvider.registerNetworkCallback( - context, logger, buildInfoProvider, networkCallback); - // The specific error is logged in the ConnectivityChecker method - if (!registered) { - networkCallback = null; - logger.log(SentryLevel.DEBUG, "NetworkBreadcrumbsIntegration not installed."); - return; + try { + options + .getExecutorService() + .submit( + new Runnable() { + @Override + public void run() { + // in case integration is closed before the task is executed, simply return + final @Nullable NetworkBreadcrumbsNetworkCallback callback = networkCallback; + if (isClosed || callback == null) { + networkCallback = null; + return; + } + + final boolean registered = + AndroidConnectionStatusProvider.registerNetworkCallback( + context, logger, buildInfoProvider, callback); + if (registered) { + logger.log(SentryLevel.DEBUG, "NetworkBreadcrumbsIntegration installed."); + addIntegrationToSdkVersion(getClass()); + } else { + logger.log(SentryLevel.DEBUG, "NetworkBreadcrumbsIntegration not installed."); + // The specific error is logged by AndroidConnectionStatusProvider + } + } + }); + } catch (Throwable t) { + logger.log(SentryLevel.ERROR, "Error submitting NetworkBreadcrumbsIntegration task.", t); } - logger.log(SentryLevel.DEBUG, "NetworkBreadcrumbsIntegration installed."); - addIntegrationToSdkVersion(getClass()); } } @Override public void close() throws IOException { - if (networkCallback != null) { - AndroidConnectionStatusProvider.unregisterNetworkCallback( - context, logger, buildInfoProvider, networkCallback); - logger.log(SentryLevel.DEBUG, "NetworkBreadcrumbsIntegration remove."); + isClosed = true; + + try { + Objects.requireNonNull(options, "Options is required") + .getExecutorService() + .submit( + () -> { + final @Nullable NetworkBreadcrumbsNetworkCallback callback = networkCallback; + if (callback != null) { + AndroidConnectionStatusProvider.unregisterNetworkCallback( + context, logger, buildInfoProvider, callback); + logger.log(SentryLevel.DEBUG, "NetworkBreadcrumbsIntegration removed."); + } + networkCallback = null; + }); + } catch (Throwable t) { + logger.log(SentryLevel.ERROR, "Error submitting NetworkBreadcrumbsIntegration task.", t); } - networkCallback = null; } @SuppressLint("ObsoleteSdkInt") diff --git a/sentry-android-core/src/main/java/io/sentry/android/core/SentryAndroid.java b/sentry-android-core/src/main/java/io/sentry/android/core/SentryAndroid.java index b083309a3f..e6e677334c 100644 --- a/sentry-android-core/src/main/java/io/sentry/android/core/SentryAndroid.java +++ b/sentry-android-core/src/main/java/io/sentry/android/core/SentryAndroid.java @@ -13,7 +13,6 @@ import io.sentry.SentryLevel; import io.sentry.SentryOptions; import io.sentry.Session; -import io.sentry.android.core.internal.util.BreadcrumbFactory; import io.sentry.android.core.performance.AppStartMetrics; import io.sentry.android.core.performance.TimeSpan; import io.sentry.android.fragment.FragmentLifecycleIntegration; 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 388bfbe274..1bc88961da 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 @@ -137,49 +137,6 @@ class LifecycleWatcherTest { verify(fixture.hub, never()).endSession() } - @Test - fun `When session tracking is enabled, add breadcrumb on start`() { - val watcher = fixture.getSUT(enableAppLifecycleBreadcrumbs = false) - watcher.onStart(fixture.ownerMock) - verify(fixture.hub).addBreadcrumb( - check { - assertEquals("app.lifecycle", it.category) - assertEquals("session", it.type) - assertEquals(SentryLevel.INFO, it.level) - // cant assert data, its not a public API - } - ) - } - - @Test - fun `When session tracking is enabled, add breadcrumb on stop`() { - val watcher = fixture.getSUT(enableAppLifecycleBreadcrumbs = false) - watcher.onStop(fixture.ownerMock) - verify(fixture.hub, timeout(10000)).endSession() - verify(fixture.hub).addBreadcrumb( - check { - assertEquals("app.lifecycle", it.category) - assertEquals("session", it.type) - assertEquals(SentryLevel.INFO, it.level) - // cant assert data, its not a public API - } - ) - } - - @Test - fun `When session tracking is disabled, do not add breadcrumb on start`() { - val watcher = fixture.getSUT(enableAutoSessionTracking = false, enableAppLifecycleBreadcrumbs = false) - watcher.onStart(fixture.ownerMock) - verify(fixture.hub, never()).addBreadcrumb(any()) - } - - @Test - fun `When session tracking is disabled, do not add breadcrumb on stop`() { - val watcher = fixture.getSUT(enableAutoSessionTracking = false, enableAppLifecycleBreadcrumbs = false) - watcher.onStop(fixture.ownerMock) - verify(fixture.hub, never()).addBreadcrumb(any()) - } - @Test fun `When app lifecycle breadcrumbs is enabled, add breadcrumb on start`() { val watcher = fixture.getSUT(enableAutoSessionTracking = false) diff --git a/sentry-android-core/src/test/java/io/sentry/android/core/NetworkBreadcrumbsIntegrationTest.kt b/sentry-android-core/src/test/java/io/sentry/android/core/NetworkBreadcrumbsIntegrationTest.kt index 146f229fdf..c664d98699 100644 --- a/sentry-android-core/src/test/java/io/sentry/android/core/NetworkBreadcrumbsIntegrationTest.kt +++ b/sentry-android-core/src/test/java/io/sentry/android/core/NetworkBreadcrumbsIntegrationTest.kt @@ -9,12 +9,15 @@ import android.os.Build import io.sentry.Breadcrumb import io.sentry.DateUtils import io.sentry.IHub +import io.sentry.ISentryExecutorService import io.sentry.SentryDateProvider import io.sentry.SentryLevel import io.sentry.SentryNanotimeDate import io.sentry.TypeCheckHint import io.sentry.android.core.NetworkBreadcrumbsIntegration.NetworkBreadcrumbConnectionDetail import io.sentry.android.core.NetworkBreadcrumbsIntegration.NetworkBreadcrumbsNetworkCallback +import io.sentry.test.DeferredExecutorService +import io.sentry.test.ImmediateExecutorService import org.mockito.kotlin.KInOrder import org.mockito.kotlin.any import org.mockito.kotlin.anyOrNull @@ -47,14 +50,22 @@ class NetworkBreadcrumbsIntegrationTest { init { whenever(mockBuildInfoProvider.sdkInfoVersion).thenReturn(Build.VERSION_CODES.N) - whenever(context.getSystemService(eq(Context.CONNECTIVITY_SERVICE))).thenReturn(connectivityManager) + whenever(context.getSystemService(eq(Context.CONNECTIVITY_SERVICE))).thenReturn( + connectivityManager + ) } - fun getSut(enableNetworkEventBreadcrumbs: Boolean = true, buildInfo: BuildInfoProvider = mockBuildInfoProvider): NetworkBreadcrumbsIntegration { + fun getSut( + enableNetworkEventBreadcrumbs: Boolean = true, + buildInfo: BuildInfoProvider = mockBuildInfoProvider, + executor: ISentryExecutorService = ImmediateExecutorService() + ): NetworkBreadcrumbsIntegration { options = SentryAndroidOptions().apply { + executorService = executor isEnableNetworkEventBreadcrumbs = enableNetworkEventBreadcrumbs dateProvider = SentryDateProvider { - val nowNanos = TimeUnit.MILLISECONDS.toNanos(nowMs ?: System.currentTimeMillis()) + val nowNanos = + TimeUnit.MILLISECONDS.toNanos(nowMs ?: System.currentTimeMillis()) SentryNanotimeDate(DateUtils.nanosToDate(nowNanos), nowNanos) } } @@ -117,7 +128,10 @@ class NetworkBreadcrumbsIntegrationTest { sut.register(fixture.hub, fixture.options) sut.close() - verify(fixture.connectivityManager, never()).unregisterNetworkCallback(any()) + verify( + fixture.connectivityManager, + never() + ).unregisterNetworkCallback(any()) assertNull(sut.networkCallback) } @@ -482,11 +496,27 @@ class NetworkBreadcrumbsIntegrationTest { } } + @Test + fun `If integration is opened and closed immediately it still properly unregisters`() { + val executor = DeferredExecutorService() + val sut = fixture.getSut(executor = executor) + + sut.register(fixture.hub, fixture.options) + sut.close() + + executor.runAll() + + assertNull(sut.networkCallback) + verify(fixture.connectivityManager, never()).registerDefaultNetworkCallback(any()) + verify(fixture.connectivityManager, never()).unregisterNetworkCallback(any()) + } + private fun KInOrder.verifyBreadcrumbInOrder(check: (detail: NetworkBreadcrumbConnectionDetail) -> Unit) { verify(fixture.hub, times(1)).addBreadcrumb( any(), check { - val connectionDetail = it[TypeCheckHint.ANDROID_NETWORK_CAPABILITIES] as NetworkBreadcrumbConnectionDetail + val connectionDetail = + it[TypeCheckHint.ANDROID_NETWORK_CAPABILITIES] as NetworkBreadcrumbConnectionDetail check(connectionDetail) } ) @@ -496,7 +526,8 @@ class NetworkBreadcrumbsIntegrationTest { verify(fixture.hub).addBreadcrumb( any(), check { - val connectionDetail = it[TypeCheckHint.ANDROID_NETWORK_CAPABILITIES] as NetworkBreadcrumbConnectionDetail + val connectionDetail = + it[TypeCheckHint.ANDROID_NETWORK_CAPABILITIES] as NetworkBreadcrumbConnectionDetail check(connectionDetail) } ) @@ -516,9 +547,13 @@ class NetworkBreadcrumbsIntegrationTest { whenever(capabilities.linkUpstreamBandwidthKbps).thenReturn(upstreamBandwidthKbps) whenever(capabilities.signalStrength).thenReturn(signalStrength) whenever(capabilities.hasTransport(NetworkCapabilities.TRANSPORT_VPN)).thenReturn(isVpn) - whenever(capabilities.hasTransport(NetworkCapabilities.TRANSPORT_ETHERNET)).thenReturn(isEthernet) + whenever(capabilities.hasTransport(NetworkCapabilities.TRANSPORT_ETHERNET)).thenReturn( + isEthernet + ) whenever(capabilities.hasTransport(NetworkCapabilities.TRANSPORT_WIFI)).thenReturn(isWifi) - whenever(capabilities.hasTransport(NetworkCapabilities.TRANSPORT_CELLULAR)).thenReturn(isCellular) + whenever(capabilities.hasTransport(NetworkCapabilities.TRANSPORT_CELLULAR)).thenReturn( + isCellular + ) return capabilities } From e524d077db5e07584c3dd676f18ad3ed0543737b Mon Sep 17 00:00:00 2001 From: Markus Hintersteiner Date: Wed, 9 Oct 2024 14:40:40 +0200 Subject: [PATCH 8/8] Address PR feedback --- .../AppComponentsBreadcrumbsIntegration.java | 18 +++++--- .../core/NetworkBreadcrumbsIntegration.java | 46 ++++++++++--------- .../SystemEventsBreadcrumbsIntegration.java | 11 +++-- 3 files changed, 44 insertions(+), 31 deletions(-) diff --git a/sentry-android-core/src/main/java/io/sentry/android/core/AppComponentsBreadcrumbsIntegration.java b/sentry-android-core/src/main/java/io/sentry/android/core/AppComponentsBreadcrumbsIntegration.java index 84267a2812..f2d9f6a2a7 100644 --- a/sentry-android-core/src/main/java/io/sentry/android/core/AppComponentsBreadcrumbsIntegration.java +++ b/sentry-android-core/src/main/java/io/sentry/android/core/AppComponentsBreadcrumbsIntegration.java @@ -85,22 +85,25 @@ public void close() throws IOException { @SuppressWarnings("deprecation") @Override public void onConfigurationChanged(@NotNull Configuration newConfig) { - executeInBackground(() -> captureConfigurationChangedBreadcrumb(newConfig)); + final long now = System.currentTimeMillis(); + executeInBackground(() -> captureConfigurationChangedBreadcrumb(now, newConfig)); } @Override public void onLowMemory() { - executeInBackground(() -> captureLowMemoryBreadcrumb(null)); + final long now = System.currentTimeMillis(); + executeInBackground(() -> captureLowMemoryBreadcrumb(now, null)); } @Override public void onTrimMemory(final int level) { - executeInBackground(() -> captureLowMemoryBreadcrumb(level)); + final long now = System.currentTimeMillis(); + executeInBackground(() -> captureLowMemoryBreadcrumb(now, level)); } - private void captureLowMemoryBreadcrumb(final @Nullable Integer level) { + private void captureLowMemoryBreadcrumb(final long timeMs, final @Nullable Integer level) { if (hub != null) { - final Breadcrumb breadcrumb = new Breadcrumb(); + final Breadcrumb breadcrumb = new Breadcrumb(timeMs); if (level != null) { // only add breadcrumb if TRIM_MEMORY_BACKGROUND, TRIM_MEMORY_MODERATE or // TRIM_MEMORY_COMPLETE. @@ -127,7 +130,8 @@ private void captureLowMemoryBreadcrumb(final @Nullable Integer level) { } } - private void captureConfigurationChangedBreadcrumb(final @NotNull Configuration newConfig) { + private void captureConfigurationChangedBreadcrumb( + final long timeMs, final @NotNull Configuration newConfig) { if (hub != null) { final Device.DeviceOrientation deviceOrientation = DeviceOrientations.getOrientation(context.getResources().getConfiguration().orientation); @@ -139,7 +143,7 @@ private void captureConfigurationChangedBreadcrumb(final @NotNull Configuration orientation = "undefined"; } - final Breadcrumb breadcrumb = new Breadcrumb(); + final Breadcrumb breadcrumb = new Breadcrumb(timeMs); breadcrumb.setType("navigation"); breadcrumb.setCategory("device.orientation"); breadcrumb.setData("position", orientation); diff --git a/sentry-android-core/src/main/java/io/sentry/android/core/NetworkBreadcrumbsIntegration.java b/sentry-android-core/src/main/java/io/sentry/android/core/NetworkBreadcrumbsIntegration.java index bf4f9ccd52..fa5724e8c5 100644 --- a/sentry-android-core/src/main/java/io/sentry/android/core/NetworkBreadcrumbsIntegration.java +++ b/sentry-android-core/src/main/java/io/sentry/android/core/NetworkBreadcrumbsIntegration.java @@ -33,6 +33,7 @@ public final class NetworkBreadcrumbsIntegration implements Integration, Closeab private final @NotNull Context context; private final @NotNull BuildInfoProvider buildInfoProvider; private final @NotNull ILogger logger; + private final @NotNull Object lock = new Object(); private volatile boolean isClosed; private @Nullable SentryOptions options; @@ -73,9 +74,6 @@ public void register(final @NotNull IHub hub, final @NotNull SentryOptions optio return; } - networkCallback = - new NetworkBreadcrumbsNetworkCallback(hub, buildInfoProvider, options.getDateProvider()); - try { options .getExecutorService() @@ -84,21 +82,26 @@ public void register(final @NotNull IHub hub, final @NotNull SentryOptions optio @Override public void run() { // in case integration is closed before the task is executed, simply return - final @Nullable NetworkBreadcrumbsNetworkCallback callback = networkCallback; - if (isClosed || callback == null) { - networkCallback = null; + if (isClosed) { return; } - final boolean registered = - AndroidConnectionStatusProvider.registerNetworkCallback( - context, logger, buildInfoProvider, callback); - if (registered) { - logger.log(SentryLevel.DEBUG, "NetworkBreadcrumbsIntegration installed."); - addIntegrationToSdkVersion(getClass()); - } else { - logger.log(SentryLevel.DEBUG, "NetworkBreadcrumbsIntegration not installed."); - // The specific error is logged by AndroidConnectionStatusProvider + synchronized (lock) { + networkCallback = + new NetworkBreadcrumbsNetworkCallback( + hub, buildInfoProvider, options.getDateProvider()); + + final boolean registered = + AndroidConnectionStatusProvider.registerNetworkCallback( + context, logger, buildInfoProvider, networkCallback); + if (registered) { + logger.log(SentryLevel.DEBUG, "NetworkBreadcrumbsIntegration installed."); + addIntegrationToSdkVersion(getClass()); + } else { + logger.log( + SentryLevel.DEBUG, "NetworkBreadcrumbsIntegration not installed."); + // The specific error is logged by AndroidConnectionStatusProvider + } } } }); @@ -117,13 +120,14 @@ public void close() throws IOException { .getExecutorService() .submit( () -> { - final @Nullable NetworkBreadcrumbsNetworkCallback callback = networkCallback; - if (callback != null) { - AndroidConnectionStatusProvider.unregisterNetworkCallback( - context, logger, buildInfoProvider, callback); - logger.log(SentryLevel.DEBUG, "NetworkBreadcrumbsIntegration removed."); + synchronized (lock) { + if (networkCallback != null) { + AndroidConnectionStatusProvider.unregisterNetworkCallback( + context, logger, buildInfoProvider, networkCallback); + logger.log(SentryLevel.DEBUG, "NetworkBreadcrumbsIntegration removed."); + } + networkCallback = null; } - networkCallback = null; }); } catch (Throwable t) { logger.log(SentryLevel.ERROR, "Error submitting NetworkBreadcrumbsIntegration task.", t); diff --git a/sentry-android-core/src/main/java/io/sentry/android/core/SystemEventsBreadcrumbsIntegration.java b/sentry-android-core/src/main/java/io/sentry/android/core/SystemEventsBreadcrumbsIntegration.java index 5d5068c64f..76422fdf6e 100644 --- a/sentry-android-core/src/main/java/io/sentry/android/core/SystemEventsBreadcrumbsIntegration.java +++ b/sentry-android-core/src/main/java/io/sentry/android/core/SystemEventsBreadcrumbsIntegration.java @@ -230,12 +230,14 @@ public void onReceive(final Context context, final @NotNull Intent intent) { return; } + final long now = System.currentTimeMillis(); try { options .getExecutorService() .submit( () -> { - final Breadcrumb breadcrumb = createBreadcrumb(intent, action, isBatteryChanged); + final Breadcrumb breadcrumb = + createBreadcrumb(now, intent, action, isBatteryChanged); final Hint hint = new Hint(); hint.set(ANDROID_INTENT, intent); hub.addBreadcrumb(breadcrumb, hint); @@ -248,8 +250,11 @@ public void onReceive(final Context context, final @NotNull Intent intent) { } private @NotNull Breadcrumb createBreadcrumb( - final @NotNull Intent intent, final @Nullable String action, boolean isBatteryChanged) { - final Breadcrumb breadcrumb = new Breadcrumb(); + final long timeMs, + final @NotNull Intent intent, + final @Nullable String action, + boolean isBatteryChanged) { + final Breadcrumb breadcrumb = new Breadcrumb(timeMs); breadcrumb.setType("system"); breadcrumb.setCategory("device.event"); final String shortAction = StringUtils.getStringAfterDot(action);