Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't send cached envelopes when rate-limiting is active #2937

Merged
merged 13 commits into from
Sep 28, 2023
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -17,6 +17,7 @@ Breaking changes:
- 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))
- Do not try to send and drop cached envelopes when rate-limiting is active ([#2937](https://github.com/getsentry/sentry-java/pull/2937))

### Fixes

Original file line number Diff line number Diff line change
@@ -43,7 +43,8 @@ public final void register(final @NotNull IHub hub, final @NotNull SentryOptions
options.getEnvelopeReader(),
options.getSerializer(),
logger,
options.getFlushTimeoutMillis());
options.getFlushTimeoutMillis(),
options.getMaxQueueSize());

observer =
new EnvelopeFileObserver(path, outboxSender, logger, options.getFlushTimeoutMillis());
Original file line number Diff line number Diff line change
@@ -1,11 +1,13 @@
package io.sentry.android.core;

import io.sentry.DataCategory;
import io.sentry.IConnectionStatusProvider;
import io.sentry.IHub;
import io.sentry.Integration;
import io.sentry.SendCachedEnvelopeFireAndForgetIntegration;
import io.sentry.SentryLevel;
import io.sentry.SentryOptions;
import io.sentry.transport.RateLimiter;
import io.sentry.util.LazyEvaluator;
import io.sentry.util.Objects;
import java.io.Closeable;
@@ -28,6 +30,7 @@ final class SendCachedEnvelopeIntegration
private @Nullable IConnectionStatusProvider connectionStatusProvider;
private @Nullable IHub hub;
private @Nullable SentryAndroidOptions options;
private @Nullable SendCachedEnvelopeFireAndForgetIntegration.SendFireAndForget sender;

public SendCachedEnvelopeIntegration(
final @NotNull SendCachedEnvelopeFireAndForgetIntegration.SendFireAndForgetFactory factory,
@@ -54,6 +57,8 @@ public void register(@NotNull IHub hub, @NotNull SentryOptions options) {
connectionStatusProvider = options.getConnectionStatusProvider();
connectionStatusProvider.addConnectionStatusObserver(this);

sender = factory.create(hub, options);

sendCachedEnvelopes(hub, this.options);
}

@@ -71,6 +76,7 @@ public void onConnectionStatusChanged(IConnectionStatusProvider.ConnectionStatus
}
}

@SuppressWarnings({"NullAway"})
private synchronized void sendCachedEnvelopes(
final @NotNull IHub hub, final @NotNull SentryAndroidOptions options) {

@@ -81,8 +87,14 @@ private synchronized void sendCachedEnvelopes(
return;
}

final SendCachedEnvelopeFireAndForgetIntegration.SendFireAndForget sender =
factory.create(hub, options);
// in case there's rate limiting active, skip processing
final @Nullable RateLimiter rateLimiter = hub.getRateLimiter();
if (rateLimiter != null && rateLimiter.isActiveForCategory(DataCategory.All)) {
options
.getLogger()
.log(SentryLevel.INFO, "SendCachedEnvelopeIntegration, rate limiting active.");
return;
}

if (sender == null) {
options.getLogger().log(SentryLevel.ERROR, "SendCachedEnvelopeIntegration factory is null.");
Original file line number Diff line number Diff line change
@@ -8,7 +8,6 @@
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;
@@ -63,9 +62,14 @@ public AndroidConnectionStatusProvider(
return getConnectionType(context, logger, buildInfoProvider);
}

@RequiresApi(api = Build.VERSION_CODES.LOLLIPOP)
@SuppressLint("NewApi") // we have an if-check for that down below
@Override
public boolean addConnectionStatusObserver(@NotNull IConnectionStatusObserver observer) {
if (buildInfoProvider.getSdkInfoVersion() < Build.VERSION_CODES.LOLLIPOP) {
logger.log(SentryLevel.DEBUG, "addConnectionStatusObserver requires Android 5+.");
return false;
}

final ConnectivityManager.NetworkCallback callback =
new ConnectivityManager.NetworkCallback() {
@Override
Original file line number Diff line number Diff line change
@@ -23,6 +23,7 @@ import io.sentry.protocol.Mechanism
import io.sentry.protocol.SentryId
import io.sentry.protocol.User
import io.sentry.transport.ITransport
import io.sentry.transport.RateLimiter
import org.junit.runner.RunWith
import org.mockito.kotlin.mock
import org.mockito.kotlin.whenever
@@ -63,6 +64,10 @@ class InternalSentrySdkTest {
override fun flush(timeoutMillis: Long) {
// no-op
}

override fun getRateLimiter(): RateLimiter? {
return null
}
}
}
}
Original file line number Diff line number Diff line change
@@ -7,6 +7,8 @@ import io.sentry.ILogger
import io.sentry.SendCachedEnvelopeFireAndForgetIntegration.SendFireAndForget
import io.sentry.SendCachedEnvelopeFireAndForgetIntegration.SendFireAndForgetFactory
import io.sentry.SentryLevel.DEBUG
import io.sentry.test.ImmediateExecutorService
import io.sentry.transport.RateLimiter
import io.sentry.util.LazyEvaluator
import org.awaitility.kotlin.await
import org.mockito.kotlin.any
@@ -35,8 +37,12 @@ class SendCachedEnvelopeIntegrationTest {
hasStartupCrashMarker: Boolean = false,
hasSender: Boolean = true,
delaySend: Long = 0L,
taskFails: Boolean = false
taskFails: Boolean = false,
useImmediateExecutor: Boolean = false
): SendCachedEnvelopeIntegration {
if (useImmediateExecutor) {
options.executorService = ImmediateExecutorService()
}
options.cacheDirPath = cacheDirPath
options.setLogger(logger)
options.isDebug = true
@@ -144,7 +150,7 @@ class SendCachedEnvelopeIntegrationTest {
)

sut.register(fixture.hub, fixture.options)
verify(fixture.factory, never()).create(any(), any())
verify(fixture.sender, never()).send()
}

@Test
@@ -164,7 +170,7 @@ class SendCachedEnvelopeIntegrationTest {

@Test
fun `whenever network connection status changes, retries sending for relevant statuses`() {
val sut = fixture.getSut(hasStartupCrashMarker = false)
val sut = fixture.getSut(hasStartupCrashMarker = false, useImmediateExecutor = true)

val connectionStatusProvider = mock<IConnectionStatusProvider>()
fixture.options.connectionStatusProvider = connectionStatusProvider
@@ -174,22 +180,36 @@ class SendCachedEnvelopeIntegrationTest {
sut.register(fixture.hub, fixture.options)

// when there's no connection no factory create call should be done
verify(fixture.factory, never()).create(any(), any())
verify(fixture.sender, never()).send()

// but for any other status processing should be triggered
// CONNECTED
whenever(connectionStatusProvider.connectionStatus).thenReturn(ConnectionStatus.CONNECTED)
sut.onConnectionStatusChanged(ConnectionStatus.CONNECTED)
verify(fixture.factory).create(any(), any())
verify(fixture.sender).send()

// UNKNOWN
whenever(connectionStatusProvider.connectionStatus).thenReturn(ConnectionStatus.UNKNOWN)
sut.onConnectionStatusChanged(ConnectionStatus.UNKNOWN)
verify(fixture.factory, times(2)).create(any(), any())
verify(fixture.sender, times(2)).send()

// NO_PERMISSION
whenever(connectionStatusProvider.connectionStatus).thenReturn(ConnectionStatus.NO_PERMISSION)
sut.onConnectionStatusChanged(ConnectionStatus.NO_PERMISSION)
verify(fixture.factory, times(3)).create(any(), any())
verify(fixture.sender, times(3)).send()
}

@Test
fun `when rate limiter is active, does not send envelopes`() {
val sut = fixture.getSut(hasStartupCrashMarker = false)
val rateLimiter = mock<RateLimiter> {
whenever(mock.isActiveForCategory(any())).thenReturn(true)
}
whenever(fixture.hub.rateLimiter).thenReturn(rateLimiter)

sut.register(fixture.hub, fixture.options)

// no factory call should be done if there's rate limiting active
verify(fixture.sender, never()).send()
}
}
Original file line number Diff line number Diff line change
@@ -21,6 +21,7 @@ import io.sentry.TraceContext
import io.sentry.UserFeedback
import io.sentry.protocol.SentryId
import io.sentry.protocol.SentryTransaction
import io.sentry.transport.RateLimiter
import org.junit.runner.RunWith
import org.mockito.kotlin.mock
import org.robolectric.annotation.Config
@@ -162,5 +163,9 @@ class SessionTrackingIntegrationTest {
): SentryId {
TODO("Not yet implemented")
}

override fun getRateLimiter(): RateLimiter? {
TODO("Not yet implemented")
}
}
}
Original file line number Diff line number Diff line change
@@ -2,6 +2,7 @@ public final class io/sentry/transport/apache/ApacheHttpClientTransport : io/sen
public fun <init> (Lio/sentry/SentryOptions;Lio/sentry/RequestDetails;Lorg/apache/hc/client5/http/impl/async/CloseableHttpAsyncClient;Lio/sentry/transport/RateLimiter;)V
public fun close ()V
public fun flush (J)V
public fun getRateLimiter ()Lio/sentry/transport/RateLimiter;
public fun send (Lio/sentry/SentryEnvelope;Lio/sentry/Hint;)V
}

Original file line number Diff line number Diff line change
@@ -189,6 +189,11 @@ public void flush(long timeoutMillis) {
}
}

@Override
public @NotNull RateLimiter getRateLimiter() {
return rateLimiter;
}

@Override
public void close() throws IOException {
options.getLogger().log(DEBUG, "Shutting down");
21 changes: 17 additions & 4 deletions sentry/api/sentry.api
Original file line number Diff line number Diff line change
@@ -239,7 +239,7 @@ public final class io/sentry/EnvelopeReader : io/sentry/IEnvelopeReader {
}

public final class io/sentry/EnvelopeSender : io/sentry/IEnvelopeSender {
public fun <init> (Lio/sentry/IHub;Lio/sentry/ISerializer;Lio/sentry/ILogger;JLio/sentry/cache/IEnvelopeCache;)V
public fun <init> (Lio/sentry/IHub;Lio/sentry/ISerializer;Lio/sentry/ILogger;JI)V
public synthetic fun processDirectory (Ljava/io/File;)V
public fun processEnvelopeFile (Ljava/lang/String;Lio/sentry/Hint;)V
}
@@ -373,6 +373,7 @@ public final class io/sentry/Hub : io/sentry/IHub {
public fun getBaggage ()Lio/sentry/BaggageHeader;
public fun getLastEventId ()Lio/sentry/protocol/SentryId;
public fun getOptions ()Lio/sentry/SentryOptions;
public fun getRateLimiter ()Lio/sentry/transport/RateLimiter;
public fun getSpan ()Lio/sentry/ISpan;
public fun getTraceparent ()Lio/sentry/SentryTraceHeader;
public fun getTransaction ()Lio/sentry/ITransaction;
@@ -422,6 +423,7 @@ public final class io/sentry/HubAdapter : io/sentry/IHub {
public static fun getInstance ()Lio/sentry/HubAdapter;
public fun getLastEventId ()Lio/sentry/protocol/SentryId;
public fun getOptions ()Lio/sentry/SentryOptions;
public fun getRateLimiter ()Lio/sentry/transport/RateLimiter;
public fun getSpan ()Lio/sentry/ISpan;
public fun getTraceparent ()Lio/sentry/SentryTraceHeader;
public fun getTransaction ()Lio/sentry/ITransaction;
@@ -515,6 +517,7 @@ public abstract interface class io/sentry/IHub {
public abstract fun getBaggage ()Lio/sentry/BaggageHeader;
public abstract fun getLastEventId ()Lio/sentry/protocol/SentryId;
public abstract fun getOptions ()Lio/sentry/SentryOptions;
public abstract fun getRateLimiter ()Lio/sentry/transport/RateLimiter;
public abstract fun getSpan ()Lio/sentry/ISpan;
public abstract fun getTraceparent ()Lio/sentry/SentryTraceHeader;
public abstract fun getTransaction ()Lio/sentry/ITransaction;
@@ -608,6 +611,7 @@ public abstract interface class io/sentry/ISentryClient {
public abstract fun captureUserFeedback (Lio/sentry/UserFeedback;)V
public abstract fun close ()V
public abstract fun flush (J)V
public abstract fun getRateLimiter ()Lio/sentry/transport/RateLimiter;
public abstract fun isEnabled ()Z
}

@@ -910,6 +914,7 @@ public final class io/sentry/NoOpHub : io/sentry/IHub {
public static fun getInstance ()Lio/sentry/NoOpHub;
public fun getLastEventId ()Lio/sentry/protocol/SentryId;
public fun getOptions ()Lio/sentry/SentryOptions;
public fun getRateLimiter ()Lio/sentry/transport/RateLimiter;
public fun getSpan ()Lio/sentry/ISpan;
public fun getTraceparent ()Lio/sentry/SentryTraceHeader;
public fun getTransaction ()Lio/sentry/ITransaction;
@@ -1070,7 +1075,7 @@ public final class io/sentry/OptionsContainer {
}

public final class io/sentry/OutboxSender : io/sentry/IEnvelopeSender {
public fun <init> (Lio/sentry/IHub;Lio/sentry/IEnvelopeReader;Lio/sentry/ISerializer;Lio/sentry/ILogger;J)V
public fun <init> (Lio/sentry/IHub;Lio/sentry/IEnvelopeReader;Lio/sentry/ISerializer;Lio/sentry/ILogger;JI)V
public synthetic fun processDirectory (Ljava/io/File;)V
public fun processEnvelopeFile (Ljava/lang/String;Lio/sentry/Hint;)V
}
@@ -1516,6 +1521,7 @@ public final class io/sentry/SentryClient : io/sentry/ISentryClient {
public fun captureUserFeedback (Lio/sentry/UserFeedback;)V
public fun close ()V
public fun flush (J)V
public fun getRateLimiter ()Lio/sentry/transport/RateLimiter;
public fun isEnabled ()Z
}

@@ -2472,7 +2478,6 @@ public final class io/sentry/TypeCheckHint {
public static final field OKHTTP_RESPONSE Ljava/lang/String;
public static final field OPEN_FEIGN_REQUEST Ljava/lang/String;
public static final field OPEN_FEIGN_RESPONSE Ljava/lang/String;
public static final field SENTRY_CACHED_ENVELOPE_FILE_PATH Ljava/lang/String;
public static final field SENTRY_DART_SDK_NAME Ljava/lang/String;
public static final field SENTRY_DOTNET_SDK_NAME Ljava/lang/String;
public static final field SENTRY_EVENT_DROP_REASON Ljava/lang/String;
@@ -2761,6 +2766,10 @@ public abstract interface class io/sentry/hints/DiskFlushNotification {
public abstract fun markFlushed ()V
}

public abstract interface class io/sentry/hints/Enqueable {
public abstract fun markEnqueued ()V
}

public final class io/sentry/hints/EventDropReason : java/lang/Enum {
public static final field MULTITHREADED_DEDUPLICATION Lio/sentry/hints/EventDropReason;
public static fun valueOf (Ljava/lang/String;)Lio/sentry/hints/EventDropReason;
@@ -4138,6 +4147,7 @@ public final class io/sentry/transport/AsyncHttpTransport : io/sentry/transport/
public fun <init> (Lio/sentry/transport/QueuedThreadPoolExecutor;Lio/sentry/SentryOptions;Lio/sentry/transport/RateLimiter;Lio/sentry/transport/ITransportGate;Lio/sentry/transport/HttpConnection;)V
public fun close ()V
public fun flush (J)V
public fun getRateLimiter ()Lio/sentry/transport/RateLimiter;
public fun send (Lio/sentry/SentryEnvelope;Lio/sentry/Hint;)V
}

@@ -4152,6 +4162,7 @@ public abstract interface class io/sentry/transport/ICurrentDateProvider {

public abstract interface class io/sentry/transport/ITransport : java/io/Closeable {
public abstract fun flush (J)V
public abstract fun getRateLimiter ()Lio/sentry/transport/RateLimiter;
public fun send (Lio/sentry/SentryEnvelope;)V
public abstract fun send (Lio/sentry/SentryEnvelope;Lio/sentry/Hint;)V
}
@@ -4173,6 +4184,7 @@ public final class io/sentry/transport/NoOpTransport : io/sentry/transport/ITran
public fun close ()V
public fun flush (J)V
public static fun getInstance ()Lio/sentry/transport/NoOpTransport;
public fun getRateLimiter ()Lio/sentry/transport/RateLimiter;
public fun send (Lio/sentry/SentryEnvelope;Lio/sentry/Hint;)V
}

@@ -4185,7 +4197,7 @@ public final class io/sentry/transport/RateLimiter {
public fun <init> (Lio/sentry/SentryOptions;)V
public fun <init> (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 isActiveForCategory (Lio/sentry/DataCategory;)Z
public fun updateRetryAfterLimits (Ljava/lang/String;Ljava/lang/String;I)V
}

@@ -4203,6 +4215,7 @@ public final class io/sentry/transport/StdoutTransport : io/sentry/transport/ITr
public fun <init> (Lio/sentry/ISerializer;)V
public fun close ()V
public fun flush (J)V
public fun getRateLimiter ()Lio/sentry/transport/RateLimiter;
public fun send (Lio/sentry/SentryEnvelope;Lio/sentry/Hint;)V
}

Loading
Oops, something went wrong.