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
Expand Up @@ -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

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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());
Expand Down
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;
Expand All @@ -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,
Expand All @@ -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);
}

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

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

Expand All @@ -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.");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -63,6 +64,10 @@ class InternalSentrySdkTest {
override fun flush(timeoutMillis: Long) {
// no-op
}

override fun getRateLimiter(): RateLimiter? {
return null
}
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand All @@ -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
Expand All @@ -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
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Up @@ -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
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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");
Expand Down
21 changes: 17 additions & 4 deletions sentry/api/sentry.api
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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
}

Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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
}
Expand Down Expand Up @@ -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
}

Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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
}

Expand All @@ -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
}
Expand All @@ -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
}

Expand All @@ -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
}

Expand All @@ -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
}

Expand Down
Loading