From 0b8c9c98862bd1060bb04fd53924997ee8a3bdce Mon Sep 17 00:00:00 2001 From: Markus Hintersteiner Date: Thu, 31 Aug 2023 16:59:27 +0200 Subject: [PATCH 1/5] Send cached envelopes only if there's a network connection --- .../core/AndroidOptionsInitializer.java | 9 +- .../android/core/AndroidTransportGate.java | 17 ++-- .../sentry/android/core/DeviceInfoUtil.java | 8 +- .../core/NetworkBreadcrumbsIntegration.java | 8 +- ...a => AndroidConnectionStatusProvider.java} | 95 +++++++++++++++---- ...=> AndroidConnectionStatusProviderTest.kt} | 75 ++++++++------- .../android/core/AndroidTransportGateTest.kt | 12 +-- sentry/api/sentry.api | 37 +++++++- .../io/sentry/IConnectionStatusProvider.java | 30 ++++++ .../sentry/NoOpConnectionStatusProvider.java | 28 ++++++ ...achedEnvelopeFireAndForgetIntegration.java | 60 ++++++++++-- .../main/java/io/sentry/SentryOptions.java | 13 +++ .../java/io/sentry/transport/RateLimiter.java | 5 + 13 files changed, 305 insertions(+), 92 deletions(-) rename sentry-android-core/src/main/java/io/sentry/android/core/internal/util/{ConnectivityChecker.java => AndroidConnectionStatusProvider.java} (76%) rename sentry-android-core/src/test/java/io/sentry/android/core/{ConnectivityCheckerTest.kt => AndroidConnectionStatusProviderTest.kt} (72%) create mode 100644 sentry/src/main/java/io/sentry/IConnectionStatusProvider.java create mode 100644 sentry/src/main/java/io/sentry/NoOpConnectionStatusProvider.java diff --git a/sentry-android-core/src/main/java/io/sentry/android/core/AndroidOptionsInitializer.java b/sentry-android-core/src/main/java/io/sentry/android/core/AndroidOptionsInitializer.java index 60c3160fe8..e014d1ac40 100644 --- a/sentry-android-core/src/main/java/io/sentry/android/core/AndroidOptionsInitializer.java +++ b/sentry-android-core/src/main/java/io/sentry/android/core/AndroidOptionsInitializer.java @@ -8,6 +8,7 @@ import io.sentry.DeduplicateMultithreadedEventProcessor; import io.sentry.DefaultTransactionPerformanceCollector; import io.sentry.ILogger; +import io.sentry.NoOpConnectionStatusProvider; import io.sentry.SendFireAndForgetEnvelopeSender; import io.sentry.SendFireAndForgetOutboxSender; import io.sentry.SentryLevel; @@ -15,6 +16,7 @@ import io.sentry.android.core.internal.debugmeta.AssetsDebugMetaLoader; import io.sentry.android.core.internal.gestures.AndroidViewGestureTargetLocator; import io.sentry.android.core.internal.modules.AssetsModulesLoader; +import io.sentry.android.core.internal.util.AndroidConnectionStatusProvider; import io.sentry.android.core.internal.util.AndroidMainThreadChecker; import io.sentry.android.core.internal.util.SentryFrameMetricsCollector; import io.sentry.android.fragment.FragmentLifecycleIntegration; @@ -130,6 +132,11 @@ static void initializeIntegrationsAndProcessors( options.setEnvelopeDiskCache(new AndroidEnvelopeCache(options)); } + if (options.getConnectionStatusProvider() instanceof NoOpConnectionStatusProvider) { + options.setConnectionStatusProvider( + new AndroidConnectionStatusProvider(context, options.getLogger(), buildInfoProvider)); + } + options.addEventProcessor(new DeduplicateMultithreadedEventProcessor(options)); options.addEventProcessor( new DefaultAndroidEventProcessor(context, buildInfoProvider, options)); @@ -137,7 +144,7 @@ static void initializeIntegrationsAndProcessors( options.addEventProcessor(new ScreenshotEventProcessor(options, buildInfoProvider)); options.addEventProcessor(new ViewHierarchyEventProcessor(options)); options.addEventProcessor(new AnrV2EventProcessor(context, options, buildInfoProvider)); - options.setTransportGate(new AndroidTransportGate(context, options.getLogger())); + options.setTransportGate(new AndroidTransportGate(options)); final SentryFrameMetricsCollector frameMetricsCollector = new SentryFrameMetricsCollector(context, options, buildInfoProvider); options.setTransactionProfiler( diff --git a/sentry-android-core/src/main/java/io/sentry/android/core/AndroidTransportGate.java b/sentry-android-core/src/main/java/io/sentry/android/core/AndroidTransportGate.java index fd9215970a..fa138a802f 100644 --- a/sentry-android-core/src/main/java/io/sentry/android/core/AndroidTransportGate.java +++ b/sentry-android-core/src/main/java/io/sentry/android/core/AndroidTransportGate.java @@ -1,29 +1,26 @@ package io.sentry.android.core; -import android.content.Context; -import io.sentry.ILogger; -import io.sentry.android.core.internal.util.ConnectivityChecker; +import io.sentry.IConnectionStatusProvider; +import io.sentry.SentryOptions; import io.sentry.transport.ITransportGate; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.TestOnly; final class AndroidTransportGate implements ITransportGate { - private final Context context; - private final @NotNull ILogger logger; + private final @NotNull SentryOptions options; - AndroidTransportGate(final @NotNull Context context, final @NotNull ILogger logger) { - this.context = context; - this.logger = logger; + AndroidTransportGate(final @NotNull SentryOptions options) { + this.options = options; } @Override public boolean isConnected() { - return isConnected(ConnectivityChecker.getConnectionStatus(context, logger)); + return isConnected(options.getConnectionStatusProvider().getConnectionStatus()); } @TestOnly - boolean isConnected(final @NotNull ConnectivityChecker.Status status) { + boolean isConnected(final @NotNull IConnectionStatusProvider.ConnectionStatus status) { // let's assume its connected if there's no permission or something as we can't really know // whether is online or not. switch (status) { diff --git a/sentry-android-core/src/main/java/io/sentry/android/core/DeviceInfoUtil.java b/sentry-android-core/src/main/java/io/sentry/android/core/DeviceInfoUtil.java index f9fcbf972d..9ad18e26d8 100644 --- a/sentry-android-core/src/main/java/io/sentry/android/core/DeviceInfoUtil.java +++ b/sentry-android-core/src/main/java/io/sentry/android/core/DeviceInfoUtil.java @@ -16,7 +16,6 @@ import android.util.DisplayMetrics; import io.sentry.DateUtils; import io.sentry.SentryLevel; -import io.sentry.android.core.internal.util.ConnectivityChecker; import io.sentry.android.core.internal.util.CpuInfoUtils; import io.sentry.android.core.internal.util.DeviceOrientations; import io.sentry.android.core.internal.util.RootChecker; @@ -180,8 +179,8 @@ private void setDeviceIO(final @NotNull Device device, final boolean includeDyna } Boolean connected; - switch (ConnectivityChecker.getConnectionStatus(context, options.getLogger())) { - case NOT_CONNECTED: + switch (options.getConnectionStatusProvider().getConnectionStatus()) { + case DISCONNECTED: connected = false; break; case CONNECTED: @@ -223,8 +222,7 @@ private void setDeviceIO(final @NotNull Device device, final boolean includeDyna if (device.getConnectionType() == null) { // wifi, ethernet or cellular, null if none - device.setConnectionType( - ConnectivityChecker.getConnectionType(context, options.getLogger(), buildInfoProvider)); + device.setConnectionType(options.getConnectionStatusProvider().getConnectionType()); } } 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 317cb60467..771d3d9d29 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 @@ -16,7 +16,7 @@ import io.sentry.SentryLevel; import io.sentry.SentryOptions; import io.sentry.TypeCheckHint; -import io.sentry.android.core.internal.util.ConnectivityChecker; +import io.sentry.android.core.internal.util.AndroidConnectionStatusProvider; import io.sentry.util.Objects; import java.io.Closeable; import java.io.IOException; @@ -69,7 +69,7 @@ public void register(final @NotNull IHub hub, final @NotNull SentryOptions optio networkCallback = new NetworkBreadcrumbsNetworkCallback(hub, buildInfoProvider); final boolean registered = - ConnectivityChecker.registerNetworkCallback( + AndroidConnectionStatusProvider.registerNetworkCallback( context, logger, buildInfoProvider, networkCallback); // The specific error is logged in the ConnectivityChecker method @@ -86,7 +86,7 @@ public void register(final @NotNull IHub hub, final @NotNull SentryOptions optio @Override public void close() throws IOException { if (networkCallback != null) { - ConnectivityChecker.unregisterNetworkCallback( + AndroidConnectionStatusProvider.unregisterNetworkCallback( context, logger, buildInfoProvider, networkCallback); logger.log(SentryLevel.DEBUG, "NetworkBreadcrumbsIntegration remove."); } @@ -208,7 +208,7 @@ static class NetworkBreadcrumbConnectionDetail { this.signalStrength = strength > -100 ? strength : 0; this.isVpn = networkCapabilities.hasTransport(NetworkCapabilities.TRANSPORT_VPN); String connectionType = - ConnectivityChecker.getConnectionType(networkCapabilities, buildInfoProvider); + AndroidConnectionStatusProvider.getConnectionType(networkCapabilities, buildInfoProvider); this.type = connectionType != null ? connectionType : ""; } diff --git a/sentry-android-core/src/main/java/io/sentry/android/core/internal/util/ConnectivityChecker.java b/sentry-android-core/src/main/java/io/sentry/android/core/internal/util/AndroidConnectionStatusProvider.java similarity index 76% rename from sentry-android-core/src/main/java/io/sentry/android/core/internal/util/ConnectivityChecker.java rename to sentry-android-core/src/main/java/io/sentry/android/core/internal/util/AndroidConnectionStatusProvider.java index 113ad55120..9acf470dfe 100644 --- a/sentry-android-core/src/main/java/io/sentry/android/core/internal/util/ConnectivityChecker.java +++ b/sentry-android-core/src/main/java/io/sentry/android/core/internal/util/AndroidConnectionStatusProvider.java @@ -7,9 +7,14 @@ import android.net.Network; import android.net.NetworkCapabilities; import android.os.Build; +import androidx.annotation.NonNull; +import androidx.annotation.RequiresApi; +import io.sentry.IConnectionStatusProvider; import io.sentry.ILogger; import io.sentry.SentryLevel; import io.sentry.android.core.BuildInfoProvider; +import java.util.HashMap; +import java.util.Map; import org.jetbrains.annotations.ApiStatus; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; @@ -20,27 +25,29 @@ * details */ @ApiStatus.Internal -public final class ConnectivityChecker { +public final class AndroidConnectionStatusProvider implements IConnectionStatusProvider { - public enum Status { - CONNECTED, - NOT_CONNECTED, - NO_PERMISSION, - UNKNOWN - } + private final @NotNull Context context; + private final @NotNull ILogger logger; + private final @NotNull BuildInfoProvider buildInfoProvider; + private final @NotNull Map + registeredCallbacks; - private ConnectivityChecker() {} + public AndroidConnectionStatusProvider( + @NotNull Context context, + @NotNull ILogger logger, + @NotNull BuildInfoProvider buildInfoProvider) { + this.context = context; + this.logger = logger; + this.buildInfoProvider = buildInfoProvider; + this.registeredCallbacks = new HashMap<>(); + } - /** - * Return the Connection status - * - * @return the ConnectionStatus - */ - public static @NotNull ConnectivityChecker.Status getConnectionStatus( - final @NotNull Context context, final @NotNull ILogger logger) { + @Override + public @NotNull ConnectionStatus getConnectionStatus() { final ConnectivityManager connectivityManager = getConnectivityManager(context, logger); if (connectivityManager == null) { - return Status.UNKNOWN; + return ConnectionStatus.UNKNOWN; } return getConnectionStatus(context, connectivityManager, logger); // getActiveNetworkInfo might return null if VPN doesn't specify its @@ -50,6 +57,50 @@ private ConnectivityChecker() {} // connectivityManager.registerDefaultNetworkCallback(...) } + @Override + public @Nullable String getConnectionType() { + return getConnectionType(context, logger, buildInfoProvider); + } + + @RequiresApi(api = Build.VERSION_CODES.LOLLIPOP) + @Override + public void addConnectionStatusObserver(@NotNull IConnectionStatusObserver observer) { + final ConnectivityManager.NetworkCallback callback = + new ConnectivityManager.NetworkCallback() { + @Override + public void onAvailable(@NonNull Network network) { + observer.onConnectionStatusChanged(getConnectionStatus()); + } + + @Override + public void onLosing(@NonNull Network network, int maxMsToLive) { + observer.onConnectionStatusChanged(getConnectionStatus()); + } + + @Override + public void onLost(@NonNull Network network) { + observer.onConnectionStatusChanged(getConnectionStatus()); + } + + @Override + public void onUnavailable() { + observer.onConnectionStatusChanged(getConnectionStatus()); + } + }; + + registeredCallbacks.put(observer, callback); + registerNetworkCallback(context, logger, buildInfoProvider, callback); + } + + @Override + public void removeConnectionStatusObserver(@NotNull IConnectionStatusObserver observer) { + final @Nullable ConnectivityManager.NetworkCallback callback = + registeredCallbacks.remove(observer); + if (callback != null) { + unregisterNetworkCallback(context, logger, buildInfoProvider, callback); + } + } + /** * Return the Connection status * @@ -59,25 +110,27 @@ private ConnectivityChecker() {} * @return true if connected or no permission to check, false otherwise */ @SuppressWarnings({"deprecation", "MissingPermission"}) - private static @NotNull ConnectivityChecker.Status getConnectionStatus( + private static @NotNull IConnectionStatusProvider.ConnectionStatus getConnectionStatus( final @NotNull Context context, final @NotNull ConnectivityManager connectivityManager, final @NotNull ILogger logger) { if (!Permissions.hasPermission(context, Manifest.permission.ACCESS_NETWORK_STATE)) { logger.log(SentryLevel.INFO, "No permission (ACCESS_NETWORK_STATE) to check network status."); - return Status.NO_PERMISSION; + return ConnectionStatus.NO_PERMISSION; } try { final android.net.NetworkInfo activeNetworkInfo = connectivityManager.getActiveNetworkInfo(); if (activeNetworkInfo == null) { logger.log(SentryLevel.INFO, "NetworkInfo is null, there's no active network."); - return Status.NOT_CONNECTED; + return ConnectionStatus.DISCONNECTED; } - return activeNetworkInfo.isConnected() ? Status.CONNECTED : Status.NOT_CONNECTED; + return activeNetworkInfo.isConnected() + ? ConnectionStatus.CONNECTED + : ConnectionStatus.DISCONNECTED; } catch (Throwable t) { logger.log(SentryLevel.ERROR, "Could not retrieve Connection Status", t); - return Status.UNKNOWN; + return ConnectionStatus.UNKNOWN; } } diff --git a/sentry-android-core/src/test/java/io/sentry/android/core/ConnectivityCheckerTest.kt b/sentry-android-core/src/test/java/io/sentry/android/core/AndroidConnectionStatusProviderTest.kt similarity index 72% rename from sentry-android-core/src/test/java/io/sentry/android/core/ConnectivityCheckerTest.kt rename to sentry-android-core/src/test/java/io/sentry/android/core/AndroidConnectionStatusProviderTest.kt index 9542adce1c..4e9d80ba50 100644 --- a/sentry-android-core/src/test/java/io/sentry/android/core/ConnectivityCheckerTest.kt +++ b/sentry-android-core/src/test/java/io/sentry/android/core/AndroidConnectionStatusProviderTest.kt @@ -14,7 +14,8 @@ import android.net.NetworkCapabilities.TRANSPORT_ETHERNET import android.net.NetworkCapabilities.TRANSPORT_WIFI import android.net.NetworkInfo import android.os.Build -import io.sentry.android.core.internal.util.ConnectivityChecker +import io.sentry.IConnectionStatusProvider +import io.sentry.android.core.internal.util.AndroidConnectionStatusProvider import org.mockito.kotlin.any import org.mockito.kotlin.eq import org.mockito.kotlin.mock @@ -28,8 +29,9 @@ import kotlin.test.assertFalse import kotlin.test.assertNull import kotlin.test.assertTrue -class ConnectivityCheckerTest { +class AndroidConnectionStatusProviderTest { + private lateinit var connectionStatusProvider: IConnectionStatusProvider private lateinit var contextMock: Context private lateinit var connectivityManager: ConnectivityManager private lateinit var networkInfo: NetworkInfo @@ -54,24 +56,26 @@ class ConnectivityCheckerTest { networkCapabilities = mock() whenever(connectivityManager.getNetworkCapabilities(any())).thenReturn(networkCapabilities) + + connectionStatusProvider = AndroidConnectionStatusProvider(contextMock, mock(), buildInfo) } @Test fun `When network is active and connected with permission, return CONNECTED for isConnected`() { whenever(networkInfo.isConnected).thenReturn(true) assertEquals( - ConnectivityChecker.Status.CONNECTED, - ConnectivityChecker.getConnectionStatus(contextMock, mock()) + IConnectionStatusProvider.ConnectionStatus.CONNECTED, + connectionStatusProvider.connectionStatus ) } @Test - fun `When network is active but not connected with permission, return NOT_CONNECTED for isConnected`() { + fun `When network is active but not connected with permission, return DISCONNECTED for isConnected`() { whenever(networkInfo.isConnected).thenReturn(false) assertEquals( - ConnectivityChecker.Status.NOT_CONNECTED, - ConnectivityChecker.getConnectionStatus(contextMock, mock()) + IConnectionStatusProvider.ConnectionStatus.DISCONNECTED, + connectionStatusProvider.connectionStatus ) } @@ -80,30 +84,31 @@ class ConnectivityCheckerTest { whenever(contextMock.checkPermission(any(), any(), any())).thenReturn(PERMISSION_DENIED) assertEquals( - ConnectivityChecker.Status.NO_PERMISSION, - ConnectivityChecker.getConnectionStatus(contextMock, mock()) + IConnectionStatusProvider.ConnectionStatus.NO_PERMISSION, + connectionStatusProvider.connectionStatus ) } @Test - fun `When network is not active, return NOT_CONNECTED for isConnected`() { + fun `When network is not active, return DISCONNECTED for isConnected`() { assertEquals( - ConnectivityChecker.Status.NOT_CONNECTED, - ConnectivityChecker.getConnectionStatus(contextMock, mock()) + IConnectionStatusProvider.ConnectionStatus.DISCONNECTED, + connectionStatusProvider.connectionStatus ) } @Test fun `When ConnectivityManager is not available, return UNKNOWN for isConnected`() { + whenever(contextMock.getSystemService(any())).thenReturn(null) assertEquals( - ConnectivityChecker.Status.UNKNOWN, - ConnectivityChecker.getConnectionStatus(mock(), mock()) + IConnectionStatusProvider.ConnectionStatus.UNKNOWN, + connectionStatusProvider.connectionStatus ) } @Test fun `When ConnectivityManager is not available, return null for getConnectionType`() { - assertNull(ConnectivityChecker.getConnectionType(mock(), mock(), buildInfo)) + assertNull(AndroidConnectionStatusProvider.getConnectionType(mock(), mock(), buildInfo)) } @Test @@ -111,33 +116,33 @@ class ConnectivityCheckerTest { val buildInfo = mock() whenever(buildInfo.sdkInfoVersion).thenReturn(Build.VERSION_CODES.KITKAT) - assertNull(ConnectivityChecker.getConnectionType(mock(), mock(), buildInfo)) + assertNull(AndroidConnectionStatusProvider.getConnectionType(mock(), mock(), buildInfo)) } @Test fun `When there's no permission, return null for getConnectionType`() { whenever(contextMock.checkPermission(any(), any(), any())).thenReturn(PERMISSION_DENIED) - assertNull(ConnectivityChecker.getConnectionType(contextMock, mock(), buildInfo)) + assertNull(AndroidConnectionStatusProvider.getConnectionType(contextMock, mock(), buildInfo)) } @Test fun `When network is not active, return null for getConnectionType`() { whenever(contextMock.getSystemService(any())).thenReturn(connectivityManager) - assertNull(ConnectivityChecker.getConnectionType(contextMock, mock(), buildInfo)) + assertNull(AndroidConnectionStatusProvider.getConnectionType(contextMock, mock(), buildInfo)) } @Test fun `When network capabilities are not available, return null for getConnectionType`() { - assertNull(ConnectivityChecker.getConnectionType(contextMock, mock(), buildInfo)) + assertNull(AndroidConnectionStatusProvider.getConnectionType(contextMock, mock(), buildInfo)) } @Test fun `When network capabilities has TRANSPORT_WIFI, return wifi`() { whenever(networkCapabilities.hasTransport(eq(TRANSPORT_WIFI))).thenReturn(true) - assertEquals("wifi", ConnectivityChecker.getConnectionType(contextMock, mock(), buildInfo)) + assertEquals("wifi", AndroidConnectionStatusProvider.getConnectionType(contextMock, mock(), buildInfo)) } @Test @@ -145,7 +150,7 @@ class ConnectivityCheckerTest { whenever(buildInfo.sdkInfoVersion).thenReturn(Build.VERSION_CODES.KITKAT) whenever(networkInfo.type).thenReturn(TYPE_WIFI) - assertEquals("wifi", ConnectivityChecker.getConnectionType(contextMock, mock(), buildInfo)) + assertEquals("wifi", AndroidConnectionStatusProvider.getConnectionType(contextMock, mock(), buildInfo)) } @Test @@ -154,7 +159,7 @@ class ConnectivityCheckerTest { assertEquals( "ethernet", - ConnectivityChecker.getConnectionType(contextMock, mock(), buildInfo) + AndroidConnectionStatusProvider.getConnectionType(contextMock, mock(), buildInfo) ) } @@ -165,7 +170,7 @@ class ConnectivityCheckerTest { assertEquals( "ethernet", - ConnectivityChecker.getConnectionType(contextMock, mock(), buildInfo) + AndroidConnectionStatusProvider.getConnectionType(contextMock, mock(), buildInfo) ) } @@ -175,7 +180,7 @@ class ConnectivityCheckerTest { assertEquals( "cellular", - ConnectivityChecker.getConnectionType(contextMock, mock(), buildInfo) + AndroidConnectionStatusProvider.getConnectionType(contextMock, mock(), buildInfo) ) } @@ -186,7 +191,7 @@ class ConnectivityCheckerTest { assertEquals( "cellular", - ConnectivityChecker.getConnectionType(contextMock, mock(), buildInfo) + AndroidConnectionStatusProvider.getConnectionType(contextMock, mock(), buildInfo) ) } @@ -197,7 +202,7 @@ class ConnectivityCheckerTest { whenever(contextMock.getSystemService(any())).thenReturn(connectivityManager) whenever(contextMock.checkPermission(any(), any(), any())).thenReturn(PERMISSION_DENIED) val registered = - ConnectivityChecker.registerNetworkCallback(contextMock, mock(), buildInfo, mock()) + AndroidConnectionStatusProvider.registerNetworkCallback(contextMock, mock(), buildInfo, mock()) assertFalse(registered) verify(connectivityManager, never()).registerDefaultNetworkCallback(any()) @@ -207,7 +212,7 @@ class ConnectivityCheckerTest { fun `When sdkInfoVersion is not min N, do not register any NetworkCallback`() { whenever(contextMock.getSystemService(any())).thenReturn(connectivityManager) val registered = - ConnectivityChecker.registerNetworkCallback(contextMock, mock(), buildInfo, mock()) + AndroidConnectionStatusProvider.registerNetworkCallback(contextMock, mock(), buildInfo, mock()) assertFalse(registered) verify(connectivityManager, never()).registerDefaultNetworkCallback(any()) @@ -219,7 +224,7 @@ class ConnectivityCheckerTest { whenever(buildInfo.sdkInfoVersion).thenReturn(Build.VERSION_CODES.N) whenever(contextMock.getSystemService(any())).thenReturn(connectivityManager) val registered = - ConnectivityChecker.registerNetworkCallback(contextMock, mock(), buildInfo, mock()) + AndroidConnectionStatusProvider.registerNetworkCallback(contextMock, mock(), buildInfo, mock()) assertTrue(registered) verify(connectivityManager).registerDefaultNetworkCallback(any()) @@ -230,7 +235,7 @@ class ConnectivityCheckerTest { val buildInfo = mock() whenever(buildInfo.sdkInfoVersion).thenReturn(Build.VERSION_CODES.KITKAT) whenever(contextMock.getSystemService(any())).thenReturn(connectivityManager) - ConnectivityChecker.unregisterNetworkCallback(contextMock, mock(), buildInfo, mock()) + AndroidConnectionStatusProvider.unregisterNetworkCallback(contextMock, mock(), buildInfo, mock()) verify(connectivityManager, never()).unregisterNetworkCallback(any()) } @@ -238,7 +243,7 @@ class ConnectivityCheckerTest { @Test fun `unregisterNetworkCallback calls connectivityManager unregisterDefaultNetworkCallback`() { whenever(contextMock.getSystemService(any())).thenReturn(connectivityManager) - ConnectivityChecker.unregisterNetworkCallback(contextMock, mock(), buildInfo, mock()) + AndroidConnectionStatusProvider.unregisterNetworkCallback(contextMock, mock(), buildInfo, mock()) verify(connectivityManager).unregisterNetworkCallback(any()) } @@ -248,7 +253,7 @@ class ConnectivityCheckerTest { whenever(buildInfo.sdkInfoVersion).thenReturn(Build.VERSION_CODES.S) whenever(connectivityManager.activeNetwork).thenThrow(SecurityException("Android OS Bug")) - assertNull(ConnectivityChecker.getConnectionType(contextMock, mock(), buildInfo)) + assertNull(AndroidConnectionStatusProvider.getConnectionType(contextMock, mock(), buildInfo)) } @Test @@ -256,8 +261,8 @@ class ConnectivityCheckerTest { whenever(buildInfo.sdkInfoVersion).thenReturn(Build.VERSION_CODES.KITKAT) whenever(connectivityManager.activeNetworkInfo).thenThrow(SecurityException("Android OS Bug")) - assertNull(ConnectivityChecker.getConnectionType(contextMock, mock(), buildInfo)) - assertEquals(ConnectivityChecker.Status.UNKNOWN, ConnectivityChecker.getConnectionStatus(contextMock, mock())) + assertNull(AndroidConnectionStatusProvider.getConnectionType(contextMock, mock(), buildInfo)) + assertEquals(IConnectionStatusProvider.ConnectionStatus.UNKNOWN, connectionStatusProvider.connectionStatus) } @Test @@ -266,7 +271,7 @@ class ConnectivityCheckerTest { SecurityException("Android OS Bug") ) assertFalse( - ConnectivityChecker.registerNetworkCallback( + AndroidConnectionStatusProvider.registerNetworkCallback( contextMock, mock(), buildInfo, @@ -283,7 +288,7 @@ class ConnectivityCheckerTest { var failed = false try { - ConnectivityChecker.unregisterNetworkCallback(contextMock, mock(), buildInfo, mock()) + AndroidConnectionStatusProvider.unregisterNetworkCallback(contextMock, mock(), buildInfo, mock()) } catch (t: Throwable) { failed = true } diff --git a/sentry-android-core/src/test/java/io/sentry/android/core/AndroidTransportGateTest.kt b/sentry-android-core/src/test/java/io/sentry/android/core/AndroidTransportGateTest.kt index 1b1ee68318..6d248db6e8 100644 --- a/sentry-android-core/src/test/java/io/sentry/android/core/AndroidTransportGateTest.kt +++ b/sentry-android-core/src/test/java/io/sentry/android/core/AndroidTransportGateTest.kt @@ -1,6 +1,6 @@ package io.sentry.android.core -import io.sentry.android.core.internal.util.ConnectivityChecker +import io.sentry.IConnectionStatusProvider import org.mockito.kotlin.mock import kotlin.test.Test import kotlin.test.assertFalse @@ -11,7 +11,7 @@ class AndroidTransportGateTest { private class Fixture { fun getSut(): AndroidTransportGate { - return AndroidTransportGate(mock(), mock()) + return AndroidTransportGate(mock()) } } private val fixture = Fixture() @@ -23,21 +23,21 @@ class AndroidTransportGateTest { @Test fun `isConnected returns true if connection was not found`() { - assertTrue(fixture.getSut().isConnected(ConnectivityChecker.Status.UNKNOWN)) + assertTrue(fixture.getSut().isConnected(IConnectionStatusProvider.ConnectionStatus.UNKNOWN)) } @Test fun `isConnected returns true if connection is connected`() { - assertTrue(fixture.getSut().isConnected(ConnectivityChecker.Status.CONNECTED)) + assertTrue(fixture.getSut().isConnected(IConnectionStatusProvider.ConnectionStatus.CONNECTED)) } @Test fun `isConnected returns false if connection is not connected`() { - assertFalse(fixture.getSut().isConnected(ConnectivityChecker.Status.NOT_CONNECTED)) + assertFalse(fixture.getSut().isConnected(IConnectionStatusProvider.ConnectionStatus.DISCONNECTED)) } @Test fun `isConnected returns false if no permission`() { - assertTrue(fixture.getSut().isConnected(ConnectivityChecker.Status.NO_PERMISSION)) + assertTrue(fixture.getSut().isConnected(IConnectionStatusProvider.ConnectionStatus.NO_PERMISSION)) } } diff --git a/sentry/api/sentry.api b/sentry/api/sentry.api index cdb0800c35..3a88083ce7 100644 --- a/sentry/api/sentry.api +++ b/sentry/api/sentry.api @@ -440,6 +440,26 @@ public abstract interface class io/sentry/ICollector { public abstract fun setup ()V } +public abstract interface class io/sentry/IConnectionStatusProvider { + public abstract fun addConnectionStatusObserver (Lio/sentry/IConnectionStatusProvider$IConnectionStatusObserver;)V + public abstract fun getConnectionStatus ()Lio/sentry/IConnectionStatusProvider$ConnectionStatus; + public abstract fun getConnectionType ()Ljava/lang/String; + public abstract fun removeConnectionStatusObserver (Lio/sentry/IConnectionStatusProvider$IConnectionStatusObserver;)V +} + +public final class io/sentry/IConnectionStatusProvider$ConnectionStatus : java/lang/Enum { + public static final field CONNECTED Lio/sentry/IConnectionStatusProvider$ConnectionStatus; + public static final field DISCONNECTED Lio/sentry/IConnectionStatusProvider$ConnectionStatus; + public static final field NO_PERMISSION Lio/sentry/IConnectionStatusProvider$ConnectionStatus; + public static final field UNKNOWN Lio/sentry/IConnectionStatusProvider$ConnectionStatus; + public static fun valueOf (Ljava/lang/String;)Lio/sentry/IConnectionStatusProvider$ConnectionStatus; + public static fun values ()[Lio/sentry/IConnectionStatusProvider$ConnectionStatus; +} + +public abstract interface class io/sentry/IConnectionStatusProvider$IConnectionStatusObserver { + public abstract fun onConnectionStatusChanged (Lio/sentry/IConnectionStatusProvider$ConnectionStatus;)V +} + public abstract interface class io/sentry/IEnvelopeReader { public abstract fun read (Ljava/io/InputStream;)Lio/sentry/SentryEnvelope; } @@ -841,6 +861,14 @@ public final class io/sentry/MemoryCollectionData { public fun getUsedNativeMemory ()J } +public final class io/sentry/NoOpConnectionStatusProvider : io/sentry/IConnectionStatusProvider { + public fun ()V + public fun addConnectionStatusObserver (Lio/sentry/IConnectionStatusProvider$IConnectionStatusObserver;)V + public fun getConnectionStatus ()Lio/sentry/IConnectionStatusProvider$ConnectionStatus; + public fun getConnectionType ()Ljava/lang/String; + public fun removeConnectionStatusObserver (Lio/sentry/IConnectionStatusProvider$IConnectionStatusObserver;)V +} + public final class io/sentry/NoOpEnvelopeReader : io/sentry/IEnvelopeReader { public static fun getInstance ()Lio/sentry/NoOpEnvelopeReader; public fun read (Ljava/io/InputStream;)Lio/sentry/SentryEnvelope; @@ -1266,9 +1294,11 @@ public abstract interface class io/sentry/ScopeCallback { public abstract fun run (Lio/sentry/Scope;)V } -public final class io/sentry/SendCachedEnvelopeFireAndForgetIntegration : io/sentry/Integration { +public final class io/sentry/SendCachedEnvelopeFireAndForgetIntegration : io/sentry/IConnectionStatusProvider$IConnectionStatusObserver, io/sentry/Integration, java/io/Closeable { public fun (Lio/sentry/SendCachedEnvelopeFireAndForgetIntegration$SendFireAndForgetFactory;)V - public final fun register (Lio/sentry/IHub;Lio/sentry/SentryOptions;)V + public fun close ()V + public fun onConnectionStatusChanged (Lio/sentry/IConnectionStatusProvider$ConnectionStatus;)V + public fun register (Lio/sentry/IHub;Lio/sentry/SentryOptions;)V } public abstract interface class io/sentry/SendCachedEnvelopeFireAndForgetIntegration$SendFireAndForget { @@ -1748,6 +1778,7 @@ public class io/sentry/SentryOptions { public fun getCacheDirPath ()Ljava/lang/String; public fun getClientReportRecorder ()Lio/sentry/clientreport/IClientReportRecorder; public fun getCollectors ()Ljava/util/List; + public fun getConnectionStatusProvider ()Lio/sentry/IConnectionStatusProvider; public fun getConnectionTimeoutMillis ()I public fun getContextTags ()Ljava/util/List; public fun getDateProvider ()Lio/sentry/SentryDateProvider; @@ -1839,6 +1870,7 @@ public class io/sentry/SentryOptions { public fun setBeforeSend (Lio/sentry/SentryOptions$BeforeSendCallback;)V public fun setBeforeSendTransaction (Lio/sentry/SentryOptions$BeforeSendTransactionCallback;)V public fun setCacheDirPath (Ljava/lang/String;)V + public fun setConnectionStatusProvider (Lio/sentry/IConnectionStatusProvider;)V public fun setConnectionTimeoutMillis (I)V public fun setDateProvider (Lio/sentry/SentryDateProvider;)V public fun setDebug (Z)V @@ -4108,6 +4140,7 @@ public final class io/sentry/transport/RateLimiter { public fun (Lio/sentry/SentryOptions;)V public fun (Lio/sentry/transport/ICurrentDateProvider;Lio/sentry/SentryOptions;)V public fun filter (Lio/sentry/SentryEnvelope;Lio/sentry/Hint;)Lio/sentry/SentryEnvelope; + public fun getRateLimitedUntilFor (Ljava/lang/String;)Ljava/util/Date; public fun updateRetryAfterLimits (Ljava/lang/String;Ljava/lang/String;I)V } diff --git a/sentry/src/main/java/io/sentry/IConnectionStatusProvider.java b/sentry/src/main/java/io/sentry/IConnectionStatusProvider.java new file mode 100644 index 0000000000..bbcd43c465 --- /dev/null +++ b/sentry/src/main/java/io/sentry/IConnectionStatusProvider.java @@ -0,0 +1,30 @@ +package io.sentry; + +import org.jetbrains.annotations.ApiStatus; +import org.jetbrains.annotations.NotNull; +import org.jetbrains.annotations.Nullable; + +@ApiStatus.Internal +public interface IConnectionStatusProvider { + + enum ConnectionStatus { + UNKNOWN, + CONNECTED, + DISCONNECTED, + NO_PERMISSION + } + + interface IConnectionStatusObserver { + void onConnectionStatusChanged(ConnectionStatus status); + } + + @NotNull + ConnectionStatus getConnectionStatus(); + + @Nullable + String getConnectionType(); + + void addConnectionStatusObserver(@NotNull final IConnectionStatusObserver observer); + + void removeConnectionStatusObserver(@NotNull final IConnectionStatusObserver observer); +} diff --git a/sentry/src/main/java/io/sentry/NoOpConnectionStatusProvider.java b/sentry/src/main/java/io/sentry/NoOpConnectionStatusProvider.java new file mode 100644 index 0000000000..8ff33b5a8d --- /dev/null +++ b/sentry/src/main/java/io/sentry/NoOpConnectionStatusProvider.java @@ -0,0 +1,28 @@ +package io.sentry; + +import org.jetbrains.annotations.ApiStatus; +import org.jetbrains.annotations.NotNull; +import org.jetbrains.annotations.Nullable; + +@ApiStatus.Internal +public final class NoOpConnectionStatusProvider implements IConnectionStatusProvider { + @Override + public @NotNull ConnectionStatus getConnectionStatus() { + return ConnectionStatus.UNKNOWN; + } + + @Override + public @Nullable String getConnectionType() { + return null; + } + + @Override + public void addConnectionStatusObserver(@NotNull IConnectionStatusObserver observer) { + // no-op + } + + @Override + public void removeConnectionStatusObserver(@NotNull IConnectionStatusObserver observer) { + // no-op + } +} diff --git a/sentry/src/main/java/io/sentry/SendCachedEnvelopeFireAndForgetIntegration.java b/sentry/src/main/java/io/sentry/SendCachedEnvelopeFireAndForgetIntegration.java index 01f9b96018..e96a983f27 100644 --- a/sentry/src/main/java/io/sentry/SendCachedEnvelopeFireAndForgetIntegration.java +++ b/sentry/src/main/java/io/sentry/SendCachedEnvelopeFireAndForgetIntegration.java @@ -1,15 +1,21 @@ package io.sentry; import io.sentry.util.Objects; +import java.io.Closeable; import java.io.File; +import java.io.IOException; import java.util.concurrent.RejectedExecutionException; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; /** Sends cached events over when your App. is starting. */ -public final class SendCachedEnvelopeFireAndForgetIntegration implements Integration { +public final class SendCachedEnvelopeFireAndForgetIntegration + implements Integration, IConnectionStatusProvider.IConnectionStatusObserver, Closeable { private final @NotNull SendFireAndForgetFactory factory; + private @Nullable IConnectionStatusProvider connectionStatusProvider; + private @Nullable IHub hub; + private @Nullable SentryOptions options; public interface SendFireAndForget { void send(); @@ -52,20 +58,62 @@ public SendCachedEnvelopeFireAndForgetIntegration( this.factory = Objects.requireNonNull(factory, "SendFireAndForgetFactory is required"); } - @SuppressWarnings("FutureReturnValueIgnored") @Override - public final void register(final @NotNull IHub hub, final @NotNull SentryOptions options) { + public void register(final @NotNull IHub hub, final @NotNull SentryOptions options) { Objects.requireNonNull(hub, "Hub is required"); Objects.requireNonNull(options, "SentryOptions is required"); + this.hub = hub; + this.options = options; + final String cachedDir = options.getCacheDirPath(); if (!factory.hasValidPath(cachedDir, options.getLogger())) { options.getLogger().log(SentryLevel.ERROR, "No cache dir path is defined in options."); return; } - final SendFireAndForget sender = factory.create(hub, options); + options + .getLogger() + .log(SentryLevel.DEBUG, "SendCachedEventFireAndForgetIntegration installed."); + addIntegrationToSdkVersion(); + + connectionStatusProvider = options.getConnectionStatusProvider(); + connectionStatusProvider.addConnectionStatusObserver(this); + + run(hub, options); + } + @Override + public void close() throws IOException { + if (connectionStatusProvider != null) { + connectionStatusProvider.removeConnectionStatusObserver(this); + } + } + + @Override + public void onConnectionStatusChanged(IConnectionStatusProvider.ConnectionStatus status) { + if (hub != null && options != null) { + run(hub, options); + } + } + + @SuppressWarnings("FutureReturnValueIgnored") + private synchronized void run(@NotNull IHub hub, @NotNull SentryOptions options) { + + // assume we're connected unless overruled by the provider + @NotNull + IConnectionStatusProvider.ConnectionStatus connectionStatus = + IConnectionStatusProvider.ConnectionStatus.CONNECTED; + if (connectionStatusProvider != null) { + connectionStatus = connectionStatusProvider.getConnectionStatus(); + } + + // skip run only if we're certainly disconnected + if (connectionStatus == IConnectionStatusProvider.ConnectionStatus.DISCONNECTED) { + return; + } + + final SendFireAndForget sender = factory.create(hub, options); if (sender == null) { options.getLogger().log(SentryLevel.ERROR, "SendFireAndForget factory is null."); return; @@ -85,10 +133,6 @@ public final void register(final @NotNull IHub hub, final @NotNull SentryOptions } }); - options - .getLogger() - .log(SentryLevel.DEBUG, "SendCachedEventFireAndForgetIntegration installed."); - addIntegrationToSdkVersion(); } catch (RejectedExecutionException e) { options .getLogger() diff --git a/sentry/src/main/java/io/sentry/SentryOptions.java b/sentry/src/main/java/io/sentry/SentryOptions.java index 7599d07027..588ded8d95 100644 --- a/sentry/src/main/java/io/sentry/SentryOptions.java +++ b/sentry/src/main/java/io/sentry/SentryOptions.java @@ -425,6 +425,9 @@ public class SentryOptions { private final @NotNull FullyDisplayedReporter fullyDisplayedReporter = FullyDisplayedReporter.getInstance(); + private @NotNull IConnectionStatusProvider connectionStatusProvider = + new NoOpConnectionStatusProvider(); + /** * Adds an event processor * @@ -2088,6 +2091,16 @@ public void addCollector(final @NotNull ICollector collector) { return collectors; } + @NotNull + public IConnectionStatusProvider getConnectionStatusProvider() { + return connectionStatusProvider; + } + + public void setConnectionStatusProvider( + final @NotNull IConnectionStatusProvider connectionStatusProvider) { + this.connectionStatusProvider = connectionStatusProvider; + } + /** The BeforeSend callback */ public interface BeforeSendCallback { diff --git a/sentry/src/main/java/io/sentry/transport/RateLimiter.java b/sentry/src/main/java/io/sentry/transport/RateLimiter.java index 0ad8dbc55a..0c818707a6 100644 --- a/sentry/src/main/java/io/sentry/transport/RateLimiter.java +++ b/sentry/src/main/java/io/sentry/transport/RateLimiter.java @@ -87,6 +87,11 @@ public RateLimiter(final @NotNull SentryOptions options) { return envelope; } + @Nullable + public Date getRateLimitedUntilFor(final @NotNull String itemType) { + return null; + } + /** * It marks the hint when sending has failed, so it's not necessary to wait the timeout * From f73ff154d8f749f00fc418ededdc71f865b009dd Mon Sep 17 00:00:00 2001 From: Markus Hintersteiner Date: Fri, 1 Sep 2023 10:44:39 +0200 Subject: [PATCH 2/5] Add more tests and docs --- .../util/AndroidConnectionStatusProvider.java | 12 ++++- .../AndroidConnectionStatusProviderTest.kt | 51 ++++++++++++++++++- sentry/api/sentry.api | 4 +- .../io/sentry/IConnectionStatusProvider.java | 31 ++++++++++- .../sentry/NoOpConnectionStatusProvider.java | 4 +- .../NoOpConnectionStatusProviderTest.kt | 33 ++++++++++++ .../test/java/io/sentry/SentryOptionsTest.kt | 28 ++++++++++ 7 files changed, 154 insertions(+), 9 deletions(-) create mode 100644 sentry/src/test/java/io/sentry/NoOpConnectionStatusProviderTest.kt diff --git a/sentry-android-core/src/main/java/io/sentry/android/core/internal/util/AndroidConnectionStatusProvider.java b/sentry-android-core/src/main/java/io/sentry/android/core/internal/util/AndroidConnectionStatusProvider.java index 9acf470dfe..e593987ba8 100644 --- a/sentry-android-core/src/main/java/io/sentry/android/core/internal/util/AndroidConnectionStatusProvider.java +++ b/sentry-android-core/src/main/java/io/sentry/android/core/internal/util/AndroidConnectionStatusProvider.java @@ -18,6 +18,7 @@ import org.jetbrains.annotations.ApiStatus; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; +import org.jetbrains.annotations.TestOnly; /** * Note: ConnectivityManager sometimes throws SecurityExceptions on Android 11. Hence all relevant @@ -64,7 +65,7 @@ public AndroidConnectionStatusProvider( @RequiresApi(api = Build.VERSION_CODES.LOLLIPOP) @Override - public void addConnectionStatusObserver(@NotNull IConnectionStatusObserver observer) { + public boolean addConnectionStatusObserver(@NotNull IConnectionStatusObserver observer) { final ConnectivityManager.NetworkCallback callback = new ConnectivityManager.NetworkCallback() { @Override @@ -89,7 +90,7 @@ public void onUnavailable() { }; registeredCallbacks.put(observer, callback); - registerNetworkCallback(context, logger, buildInfoProvider, callback); + return registerNetworkCallback(context, logger, buildInfoProvider, callback); } @Override @@ -326,4 +327,11 @@ public static void unregisterNetworkCallback( logger.log(SentryLevel.ERROR, "unregisterNetworkCallback failed", t); } } + + @TestOnly + @NotNull + public Map + getRegisteredCallbacks() { + return registeredCallbacks; + } } diff --git a/sentry-android-core/src/test/java/io/sentry/android/core/AndroidConnectionStatusProviderTest.kt b/sentry-android-core/src/test/java/io/sentry/android/core/AndroidConnectionStatusProviderTest.kt index 4e9d80ba50..359fee49cc 100644 --- a/sentry-android-core/src/test/java/io/sentry/android/core/AndroidConnectionStatusProviderTest.kt +++ b/sentry-android-core/src/test/java/io/sentry/android/core/AndroidConnectionStatusProviderTest.kt @@ -20,6 +20,7 @@ import org.mockito.kotlin.any import org.mockito.kotlin.eq import org.mockito.kotlin.mock import org.mockito.kotlin.never +import org.mockito.kotlin.times import org.mockito.kotlin.verify import org.mockito.kotlin.whenever import kotlin.test.BeforeTest @@ -31,7 +32,7 @@ import kotlin.test.assertTrue class AndroidConnectionStatusProviderTest { - private lateinit var connectionStatusProvider: IConnectionStatusProvider + private lateinit var connectionStatusProvider: AndroidConnectionStatusProvider private lateinit var contextMock: Context private lateinit var connectivityManager: ConnectivityManager private lateinit var networkInfo: NetworkInfo @@ -294,4 +295,52 @@ class AndroidConnectionStatusProviderTest { } assertFalse(failed) } + + @Test + fun `connectionStatus returns NO_PERMISSIONS when context does not hold the permission`() { + whenever(contextMock.checkPermission(any(), any(), any())).thenReturn(PERMISSION_DENIED) + assertEquals(IConnectionStatusProvider.ConnectionStatus.NO_PERMISSION, connectionStatusProvider.connectionStatus) + } + + @Test + fun `connectionStatus returns ethernet when underlying mechanism provides ethernet`() { + whenever(networkCapabilities.hasTransport(eq(TRANSPORT_ETHERNET))).thenReturn(true) + assertEquals( + "ethernet", + connectionStatusProvider.connectionType + ) + } + + @Test + fun `adding and removing an observer works correctly`() { + whenever(buildInfo.sdkInfoVersion).thenReturn(Build.VERSION_CODES.N) + + val observer = IConnectionStatusProvider.IConnectionStatusObserver { } + val addResult = connectionStatusProvider.addConnectionStatusObserver(observer) + assertTrue(addResult) + + connectionStatusProvider.removeConnectionStatusObserver(observer) + assertTrue(connectionStatusProvider.registeredCallbacks.isEmpty()) + } + + @Test + fun `underlying callbacks correctly trigger update`() { + whenever(buildInfo.sdkInfoVersion).thenReturn(Build.VERSION_CODES.N) + + var callback: NetworkCallback? = null + whenever(connectivityManager.registerDefaultNetworkCallback(any())).then { invocation -> + callback = invocation.getArgument(0, NetworkCallback::class.java) + Unit + } + val observer = mock() + connectionStatusProvider.addConnectionStatusObserver(observer) + callback!!.onAvailable(mock()) + callback!!.onUnavailable() + callback!!.onLosing(mock(), 0) + callback!!.onLost(mock()) + callback!!.onUnavailable() + connectionStatusProvider.removeConnectionStatusObserver(observer) + + verify(observer, times(5)).onConnectionStatusChanged(any()) + } } diff --git a/sentry/api/sentry.api b/sentry/api/sentry.api index 3a88083ce7..e073f229e2 100644 --- a/sentry/api/sentry.api +++ b/sentry/api/sentry.api @@ -441,7 +441,7 @@ public abstract interface class io/sentry/ICollector { } public abstract interface class io/sentry/IConnectionStatusProvider { - public abstract fun addConnectionStatusObserver (Lio/sentry/IConnectionStatusProvider$IConnectionStatusObserver;)V + public abstract fun addConnectionStatusObserver (Lio/sentry/IConnectionStatusProvider$IConnectionStatusObserver;)Z public abstract fun getConnectionStatus ()Lio/sentry/IConnectionStatusProvider$ConnectionStatus; public abstract fun getConnectionType ()Ljava/lang/String; public abstract fun removeConnectionStatusObserver (Lio/sentry/IConnectionStatusProvider$IConnectionStatusObserver;)V @@ -863,7 +863,7 @@ public final class io/sentry/MemoryCollectionData { public final class io/sentry/NoOpConnectionStatusProvider : io/sentry/IConnectionStatusProvider { public fun ()V - public fun addConnectionStatusObserver (Lio/sentry/IConnectionStatusProvider$IConnectionStatusObserver;)V + public fun addConnectionStatusObserver (Lio/sentry/IConnectionStatusProvider$IConnectionStatusObserver;)Z public fun getConnectionStatus ()Lio/sentry/IConnectionStatusProvider$ConnectionStatus; public fun getConnectionType ()Ljava/lang/String; public fun removeConnectionStatusObserver (Lio/sentry/IConnectionStatusProvider$IConnectionStatusObserver;)V diff --git a/sentry/src/main/java/io/sentry/IConnectionStatusProvider.java b/sentry/src/main/java/io/sentry/IConnectionStatusProvider.java index bbcd43c465..86d1a0d59a 100644 --- a/sentry/src/main/java/io/sentry/IConnectionStatusProvider.java +++ b/sentry/src/main/java/io/sentry/IConnectionStatusProvider.java @@ -15,16 +15,43 @@ enum ConnectionStatus { } interface IConnectionStatusObserver { + /** + * Invoked whenever the connection status changed. + * + * @param status the new connection status + */ void onConnectionStatusChanged(ConnectionStatus status); } + /** + * Gets the connection status. + * + * @return the current connection status + */ @NotNull ConnectionStatus getConnectionStatus(); + /** + * Gets the connection type. + * + * @return the current connection type. E.g. "ethernet", "wifi" or "cellular" + */ @Nullable String getConnectionType(); - void addConnectionStatusObserver(@NotNull final IConnectionStatusObserver observer); - + /** + * Adds an observer for listening to connection status changes. + * + * @param observer the observer to register + * @return true if the observer was sucessfully registered + */ + boolean addConnectionStatusObserver(@NotNull final IConnectionStatusObserver observer); + + /** + * Removes an observer. + * + * @param observer a previously added observer via {@link + * #addConnectionStatusObserver(IConnectionStatusObserver)} + */ void removeConnectionStatusObserver(@NotNull final IConnectionStatusObserver observer); } diff --git a/sentry/src/main/java/io/sentry/NoOpConnectionStatusProvider.java b/sentry/src/main/java/io/sentry/NoOpConnectionStatusProvider.java index 8ff33b5a8d..a1d66c9115 100644 --- a/sentry/src/main/java/io/sentry/NoOpConnectionStatusProvider.java +++ b/sentry/src/main/java/io/sentry/NoOpConnectionStatusProvider.java @@ -17,8 +17,8 @@ public final class NoOpConnectionStatusProvider implements IConnectionStatusProv } @Override - public void addConnectionStatusObserver(@NotNull IConnectionStatusObserver observer) { - // no-op + public boolean addConnectionStatusObserver(@NotNull IConnectionStatusObserver observer) { + return false; } @Override diff --git a/sentry/src/test/java/io/sentry/NoOpConnectionStatusProviderTest.kt b/sentry/src/test/java/io/sentry/NoOpConnectionStatusProviderTest.kt new file mode 100644 index 0000000000..0ccc911dcf --- /dev/null +++ b/sentry/src/test/java/io/sentry/NoOpConnectionStatusProviderTest.kt @@ -0,0 +1,33 @@ +package io.sentry + +import org.mockito.kotlin.mock +import kotlin.test.Test +import kotlin.test.assertEquals +import kotlin.test.assertFalse +import kotlin.test.assertNull + +class NoOpConnectionStatusProviderTest { + + private val provider = NoOpConnectionStatusProvider() + + @Test + fun `provider returns unknown status`() { + assertEquals(IConnectionStatusProvider.ConnectionStatus.UNKNOWN, provider.connectionStatus) + } + + @Test + fun `connection type returns null`() { + assertNull(provider.connectionType) + } + + @Test + fun `adding a listener is a no-op and returns false`() { + val result = provider.addConnectionStatusObserver(mock()) + assertFalse(result) + } + + @Test + fun `removing a listener is a no-op`() { + provider.addConnectionStatusObserver(mock()) + } +} diff --git a/sentry/src/test/java/io/sentry/SentryOptionsTest.kt b/sentry/src/test/java/io/sentry/SentryOptionsTest.kt index bf0703988c..8eaaeb75f4 100644 --- a/sentry/src/test/java/io/sentry/SentryOptionsTest.kt +++ b/sentry/src/test/java/io/sentry/SentryOptionsTest.kt @@ -478,4 +478,32 @@ class SentryOptionsTest { fun `when options are initialized, FullyDrawnReporter is set`() { assertEquals(FullyDisplayedReporter.getInstance(), SentryOptions().fullyDisplayedReporter) } + + @Test + fun `when options is initialized, connectionStatusProvider is not null and default to noop`() { + assertNotNull(SentryOptions().connectionStatusProvider) + assertTrue(SentryOptions().connectionStatusProvider is NoOpConnectionStatusProvider) + } + + @Test + fun `when connectionStatusProvider is set, its returned as well`() { + val options = SentryOptions() + val customProvider = object : IConnectionStatusProvider { + override fun getConnectionStatus(): IConnectionStatusProvider.ConnectionStatus { + return IConnectionStatusProvider.ConnectionStatus.UNKNOWN + } + + override fun getConnectionType(): String? = null + + override fun addConnectionStatusObserver(observer: IConnectionStatusProvider.IConnectionStatusObserver) { + // no-op + } + + override fun removeConnectionStatusObserver(observer: IConnectionStatusProvider.IConnectionStatusObserver) { + // no-op + } + } + options.connectionStatusProvider = customProvider + assertEquals(customProvider, options.connectionStatusProvider) + } } From eb9837551d79d617df77a209f3c480a893f521d6 Mon Sep 17 00:00:00 2001 From: Markus Hintersteiner Date: Fri, 1 Sep 2023 10:50:17 +0200 Subject: [PATCH 3/5] Fix bad naming --- .../sentry/SendCachedEnvelopeFireAndForgetIntegration.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/sentry/src/main/java/io/sentry/SendCachedEnvelopeFireAndForgetIntegration.java b/sentry/src/main/java/io/sentry/SendCachedEnvelopeFireAndForgetIntegration.java index e96a983f27..81be5bffca 100644 --- a/sentry/src/main/java/io/sentry/SendCachedEnvelopeFireAndForgetIntegration.java +++ b/sentry/src/main/java/io/sentry/SendCachedEnvelopeFireAndForgetIntegration.java @@ -80,7 +80,7 @@ public void register(final @NotNull IHub hub, final @NotNull SentryOptions optio connectionStatusProvider = options.getConnectionStatusProvider(); connectionStatusProvider.addConnectionStatusObserver(this); - run(hub, options); + sendCachedEnvelopes(hub, options); } @Override @@ -93,12 +93,12 @@ public void close() throws IOException { @Override public void onConnectionStatusChanged(IConnectionStatusProvider.ConnectionStatus status) { if (hub != null && options != null) { - run(hub, options); + sendCachedEnvelopes(hub, options); } } @SuppressWarnings("FutureReturnValueIgnored") - private synchronized void run(@NotNull IHub hub, @NotNull SentryOptions options) { + private synchronized void sendCachedEnvelopes(@NotNull IHub hub, @NotNull SentryOptions options) { // assume we're connected unless overruled by the provider @NotNull From cfb4bbd467b48f35c8c86246ede97eb99c09a30d Mon Sep 17 00:00:00 2001 From: Markus Hintersteiner Date: Mon, 4 Sep 2023 09:35:45 +0200 Subject: [PATCH 4/5] Add connection observer to SendCachedEnvelopeIntegration --- .../core/SendCachedEnvelopeIntegration.java | 68 ++++++++++++++----- 1 file changed, 51 insertions(+), 17 deletions(-) diff --git a/sentry-android-core/src/main/java/io/sentry/android/core/SendCachedEnvelopeIntegration.java b/sentry-android-core/src/main/java/io/sentry/android/core/SendCachedEnvelopeIntegration.java index e0d08325b4..4c503e08f3 100644 --- a/sentry-android-core/src/main/java/io/sentry/android/core/SendCachedEnvelopeIntegration.java +++ b/sentry-android-core/src/main/java/io/sentry/android/core/SendCachedEnvelopeIntegration.java @@ -1,5 +1,6 @@ package io.sentry.android.core; +import io.sentry.IConnectionStatusProvider; import io.sentry.IHub; import io.sentry.Integration; import io.sentry.SendCachedEnvelopeFireAndForgetIntegration; @@ -7,17 +8,26 @@ import io.sentry.SentryOptions; import io.sentry.util.LazyEvaluator; import io.sentry.util.Objects; +import java.io.Closeable; +import java.io.IOException; import java.util.concurrent.Future; import java.util.concurrent.RejectedExecutionException; import java.util.concurrent.TimeUnit; import java.util.concurrent.TimeoutException; +import java.util.concurrent.atomic.AtomicBoolean; import org.jetbrains.annotations.NotNull; +import org.jetbrains.annotations.Nullable; -final class SendCachedEnvelopeIntegration implements Integration { +final class SendCachedEnvelopeIntegration + implements Integration, IConnectionStatusProvider.IConnectionStatusObserver, Closeable { private final @NotNull SendCachedEnvelopeFireAndForgetIntegration.SendFireAndForgetFactory factory; private final @NotNull LazyEvaluator startupCrashMarkerEvaluator; + private final AtomicBoolean startupCrashHandled = new AtomicBoolean(false); + private @Nullable IConnectionStatusProvider connectionStatusProvider; + private @Nullable IHub hub; + private @Nullable SentryAndroidOptions options; public SendCachedEnvelopeIntegration( final @NotNull SendCachedEnvelopeFireAndForgetIntegration.SendFireAndForgetFactory factory, @@ -29,7 +39,8 @@ public SendCachedEnvelopeIntegration( @Override public void register(@NotNull IHub hub, @NotNull SentryOptions options) { Objects.requireNonNull(hub, "Hub is required"); - final SentryAndroidOptions androidOptions = + this.hub = hub; + this.options = Objects.requireNonNull( (options instanceof SentryAndroidOptions) ? (SentryAndroidOptions) options : null, "SentryAndroidOptions is required"); @@ -40,51 +51,74 @@ public void register(@NotNull IHub hub, @NotNull SentryOptions options) { return; } + connectionStatusProvider = options.getConnectionStatusProvider(); + connectionStatusProvider.addConnectionStatusObserver(this); + + sendCachedEnvelopes(hub, this.options); + } + + @Override + public void close() throws IOException { + if (connectionStatusProvider != null) { + connectionStatusProvider.removeConnectionStatusObserver(this); + } + } + + @Override + public void onConnectionStatusChanged(IConnectionStatusProvider.ConnectionStatus status) { + if (hub != null && options != null) { + sendCachedEnvelopes(hub, options); + } + } + + private synchronized void sendCachedEnvelopes( + final @NotNull IHub hub, final @NotNull SentryAndroidOptions options) { + final SendCachedEnvelopeFireAndForgetIntegration.SendFireAndForget sender = - factory.create(hub, androidOptions); + factory.create(hub, options); if (sender == null) { - androidOptions.getLogger().log(SentryLevel.ERROR, "SendFireAndForget factory is null."); + options.getLogger().log(SentryLevel.ERROR, "SendFireAndForget factory is null."); return; } - try { - Future future = - androidOptions + final Future future = + options .getExecutorService() .submit( () -> { try { sender.send(); } catch (Throwable e) { - androidOptions + options .getLogger() .log(SentryLevel.ERROR, "Failed trying to send cached events.", e); } }); - if (startupCrashMarkerEvaluator.getValue()) { - androidOptions - .getLogger() - .log(SentryLevel.DEBUG, "Startup Crash marker exists, blocking flush."); + // startupCrashMarkerEvaluator remains true on subsequent runs, let's ensure we only block on + // the very first execution (=app start) + if (startupCrashMarkerEvaluator.getValue() + && startupCrashHandled.compareAndSet(false, true)) { + options.getLogger().log(SentryLevel.DEBUG, "Startup Crash marker exists, blocking flush."); try { - future.get(androidOptions.getStartupCrashFlushTimeoutMillis(), TimeUnit.MILLISECONDS); + future.get(options.getStartupCrashFlushTimeoutMillis(), TimeUnit.MILLISECONDS); } catch (TimeoutException e) { - androidOptions + options .getLogger() .log(SentryLevel.DEBUG, "Synchronous send timed out, continuing in the background."); } } - androidOptions.getLogger().log(SentryLevel.DEBUG, "SendCachedEnvelopeIntegration installed."); + options.getLogger().log(SentryLevel.DEBUG, "SendCachedEnvelopeIntegration installed."); } catch (RejectedExecutionException e) { - androidOptions + options .getLogger() .log( SentryLevel.ERROR, "Failed to call the executor. Cached events will not be sent. Did you call Sentry.close()?", e); } catch (Throwable e) { - androidOptions + options .getLogger() .log(SentryLevel.ERROR, "Failed to call the executor. Cached events will not be sent", e); } From c8f099aae5695c3f0e53f1bb105e4305927c34a3 Mon Sep 17 00:00:00 2001 From: Markus Hintersteiner Date: Mon, 4 Sep 2023 09:40:13 +0200 Subject: [PATCH 5/5] Update Changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 638653a579..b67950f6b8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,7 @@ Breaking changes: - Automatic user interaction tracking: every click now starts a new automatic transaction ([#2891](https://github.com/getsentry/sentry-java/pull/2891)) - Previously performing a click on the same UI widget twice would keep the existing transaction running, the new behavior now better aligns with other SDKs - Android only: If global hub mode is enabled, Sentry.getSpan() returns the root span instead of the latest span ([#2855](https://github.com/getsentry/sentry-java/pull/2855)) +- Android only: Observe network state to upload any unsent envelopes ([#2910](https://github.com/getsentry/sentry-java/pull/2910)) ### Fixes