Changed buffer code to only retry in the case of HTTP 429 or connectivity issues.

CHANGES

@@ -6,6 +6,9 @@ Version 1.6.9
 - Don't log a full stacktrace when an HTTP 429 (Too Many Requests) is returned.
 - Fix parsing of Retry-After header which apparently changed to a floating point number.
 - Pass MDC context down to AsyncConnection worker threads.
+- Changed `buffer.size` default from 50 to 10.
+- When buffering is enabled, only retry sending events when the network is down or the project
+  is being throttled (HTTP 429).
 
 Version 1.6.8
 -------------

docs/config.rst

@@ -315,7 +315,7 @@ Sentry always requires write permission on the buffer directory itself.
 
     buffer.dir=sentry-events
 
-The maximum number of events that will be stored on disk defaults to 50,
+The maximum number of events that will be stored on disk defaults to 10,
 but can also be configured with the option buffer.size``:
 
 ::

sentry/src/main/java/io/sentry/DefaultSentryClientFactory.java

@@ -71,9 +71,9 @@ public class DefaultSentryClientFactory extends SentryClientFactory {
      */
     public static final String BUFFER_SIZE_OPTION = "buffer.size";
     /**
-     * Default number of events to cache offline when network is down.
+     * Default number of events to cache offline when network is down or project is throttled.
      */
-    public static final int BUFFER_SIZE_DEFAULT = 50;
+    public static final int BUFFER_SIZE_DEFAULT = 10;
     /**
      * Option for how long to wait between attempts to flush the disk buffer, in milliseconds.
      */

sentry/src/main/java/io/sentry/connection/BufferedConnection.java

@@ -8,6 +8,7 @@
 import org.slf4j.LoggerFactory;
 
 import java.io.IOException;
+import java.io.NotSerializableException;
 import java.util.Iterator;
 import java.util.List;
 import java.util.concurrent.Executors;
@@ -100,7 +101,23 @@ public BufferedConnection(Connection actualConnection, Buffer buffer, long flush
 
     @Override
     public void send(Event event) {
-        actualConnection.send(event);
+        try {
+            actualConnection.send(event);
+        } catch (ConnectionException e) {
+            boolean notSerializable = e.getCause() instanceof NotSerializableException;
+
+            Integer responseCode = e.getResponseCode();
+            if (notSerializable || (responseCode != null && responseCode != HttpConnection.HTTP_TOO_MANY_REQUESTS)) {
+                // don't retry events (discard from the buffer) if:
+                // 1. they aren't serializable
+                // 2. the connection is up (valid response code was returned) and it's not an HTTP 429
+                buffer.discard(event);
+            }
+
+            // throw regardless
+            throw e;
+        }
+
 
         // success, remove the event from the buffer
         buffer.discard(event);

sentry/src/main/java/io/sentry/connection/ConnectionException.java

@@ -11,30 +11,32 @@ public class ConnectionException extends RuntimeException {
      */
     private Long recommendedLockdownTime = null;
 
+    /**
+     * HTTP response status code, if available.
+     */
+    private Integer responseCode = null;
+
     //CHECKSTYLE.OFF: JavadocMethod
     public ConnectionException() {
 
     }
 
-    public ConnectionException(String message) {
-        super(message);
-    }
-
     public ConnectionException(String message, Throwable cause) {
         super(message, cause);
     }
 
-    public ConnectionException(String message, Throwable cause, Long recommendedLockdownTime) {
+    public ConnectionException(String message, Throwable cause, Long recommendedLockdownTime, Integer responseCode) {
         super(message, cause);
         this.recommendedLockdownTime = recommendedLockdownTime;
-    }
-
-    public ConnectionException(Throwable cause) {
-        super(cause);
+        this.responseCode = responseCode;
     }
 
     public Long getRecommendedLockdownTime() {
         return recommendedLockdownTime;
     }
+
+    public Integer getResponseCode() {
+        return responseCode;
+    }
     //CHECKSTYLE.ON: JavadocMethod
 }

sentry/src/main/java/io/sentry/connection/HttpConnection.java

@@ -24,6 +24,10 @@
  * It is possible to enable the "naive mode" to allow a connection over SSL using a certificate with a wildcard.
  */
 public class HttpConnection extends AbstractConnection {
+    /**
+     * HTTP code `429 Too Many Requests`, which is not included in HttpURLConnection.
+     */
+    public static final int HTTP_TOO_MANY_REQUESTS = 429;
     private static final Charset UTF_8 = Charset.forName("UTF-8");
     private static final Logger logger = LoggerFactory.getLogger(HttpConnection.class);
     /**
@@ -34,10 +38,6 @@ public class HttpConnection extends AbstractConnection {
      * HTTP Header for the authentication to Sentry.
      */
     private static final String SENTRY_AUTH = "X-Sentry-Auth";
-    /**
-     * HTTP code `429 Too Many Requests`, which is not included in HttpURLConnection.
-     */
-    private static final int HTTP_TOO_MANY_REQUESTS = 429;
     /**
      * Default timeout of an HTTP connection to Sentry.
      */
@@ -174,8 +174,9 @@ protected void doSend(Event event) throws ConnectionException {
                 // CHECKSTYLE.ON: EmptyCatchBlock
             }
 
+            Integer responseCode = null;
             try {
-                int responseCode = connection.getResponseCode();
+                responseCode = connection.getResponseCode();
                 if (responseCode == HttpURLConnection.HTTP_FORBIDDEN) {
                     logger.debug("Event '" + event.getId() + "' was rejected by the Sentry server due to a filter.");
                     return;
@@ -185,7 +186,8 @@ protected void doSend(Event event) throws ConnectionException {
                     avoid logging this is an error.
                     */
                     throw new TooManyRequestsException(
-                            "Too many requests to Sentry: https://docs.sentry.io/learn/quotas/", e, retryAfterMs);
+                            "Too many requests to Sentry: https://docs.sentry.io/learn/quotas/",
+                            e, retryAfterMs, responseCode);
                 }
             } catch (IOException responseCodeException) {
                 // pass
@@ -200,7 +202,7 @@ protected void doSend(Event event) throws ConnectionException {
                 errorMessage = "An exception occurred while submitting the event to the Sentry server.";
             }
 
-            throw new ConnectionException(errorMessage, e, retryAfterMs);
+            throw new ConnectionException(errorMessage, e, retryAfterMs, responseCode);
         } finally {
             connection.disconnect();
         }

sentry/src/main/java/io/sentry/connection/TooManyRequestsException.java

@@ -6,16 +6,12 @@
 public class TooManyRequestsException extends ConnectionException {
 
     //CHECKSTYLE.OFF: JavadocMethod
-    public TooManyRequestsException(String message) {
-        super(message);
-    }
-
-    public TooManyRequestsException(String message, Throwable cause) {
-        super(message, cause);
-    }
-
-    public TooManyRequestsException(String message, Throwable cause, Long recommendedLockdownTime) {
-        super(message, cause, recommendedLockdownTime);
+    public TooManyRequestsException(
+            String message,
+            Throwable cause,
+            Long recommendedLockdownTime,
+            Integer responseCode) {
+        super(message, cause, recommendedLockdownTime, responseCode);
     }
     //CHECKSTYLE.ON: JavadocMethod
 

sentry/src/test/java/io/sentry/connection/AbstractConnectionTest.java

@@ -202,7 +202,7 @@ public void testRecommendedLockdownRespected(@Injectable final Event mockEvent)
         final long recommendedLockdownWaitTime = 12345L;
         new NonStrictExpectations() {{
             abstractConnection.doSend((Event) any);
-            result = new ConnectionException("Message", null, recommendedLockdownWaitTime);
+            result = new ConnectionException("Message", null, recommendedLockdownWaitTime, HttpConnection.HTTP_TOO_MANY_REQUESTS);
         }};
 
         try {

sentry/src/test/java/io/sentry/connection/BufferedConnectionTest.java

@@ -12,10 +12,9 @@
 import org.testng.collections.Sets;
 
 import java.io.IOException;
-import java.util.Date;
-import java.util.Iterator;
-import java.util.List;
-import java.util.Set;
+import java.io.NotSerializableException;
+import java.net.HttpURLConnection;
+import java.util.*;
 import java.util.concurrent.Callable;
 import java.util.concurrent.TimeUnit;
 
@@ -34,22 +33,22 @@ public class BufferedConnectionTest extends BaseTest {
     private Buffer mockBuffer;
     private Connection mockConnection;
     private Connection bufferedConnection;
-    private volatile boolean connectionUp;
+    private ConnectionException connectionException;
 
     @BeforeMethod
     public void setup() {
         bufferedEvents = Sets.newHashSet();
         sentEvents = Lists.newArrayList();
-        connectionUp = true;
+        connectionException = null;
 
         mockConnection = new AbstractConnection("public", "private") {
             @Override
             protected void doSend(Event event) throws ConnectionException {
-                if (connectionUp) {
-                    sentEvents.add(event);
-                } else {
-                    throw new ConnectionException("Connection is down.");
+                if (connectionException != null) {
+                    throw connectionException;
                 }
+
+                sentEvents.add(event);
             }
 
             @Override
@@ -99,7 +98,7 @@ public void test() throws Exception {
         setField(mockConnection, "lockdownManager", lockdownManager);
 
         Event event = new EventBuilder().build();
-        connectionUp = false;
+        connectionException = new ConnectionException();
         try {
             bufferedConnection.send(event);
         } catch (Exception e) {
@@ -120,7 +119,7 @@ public void test() throws Exception {
         // End the lockdown
         fixedClock.tick(LockdownManager.DEFAULT_MAX_LOCKDOWN_TIME, TimeUnit.MILLISECONDS);
 
-        connectionUp = true;
+        connectionException = null;
         waitUntilTrue(1000, new Callable<Boolean>() {
             @Override
             public Boolean call() throws Exception {
@@ -131,4 +130,53 @@ public Boolean call() throws Exception {
         assertThat(sentEvents.contains(event), is(true));
         assertThat(sentEvents.contains(event2), is(true));
     }
+
+    @Test
+    public void testNotSerializableNotBuffered() throws Exception {
+        Event event = new EventBuilder().build();
+        connectionException = new ConnectionException("NonSerializable", new NotSerializableException());
+        try {
+            bufferedConnection.send(event);
+        } catch (Exception e) {
+
+        }
+        assertThat(bufferedEvents.size(), equalTo(0));
+    }
+
+    @Test
+    public void test500NotBuffered() throws Exception {
+        Event event = new EventBuilder().build();
+        connectionException = new ConnectionException("500", new IOException(), null, HttpURLConnection.HTTP_INTERNAL_ERROR);
+        try {
+            bufferedConnection.send(event);
+        } catch (Exception e) {
+
+        }
+        assertThat(bufferedEvents.size(), equalTo(0));
+    }
+
+    @Test
+    public void test429IsBuffered() throws Exception {
+        Event event = new EventBuilder().build();
+        connectionException = new ConnectionException("429", new IOException(), null, HttpConnection.HTTP_TOO_MANY_REQUESTS);
+        try {
+            bufferedConnection.send(event);
+        } catch (Exception e) {
+
+        }
+        assertThat(bufferedEvents.size(), equalTo(1));
+    }
+
+    @Test
+    public void testNoResponseCodeIsBuffered() throws Exception {
+        Event event = new EventBuilder().build();
+        connectionException = new ConnectionException("NoResponseCode", new IOException(), null, null);
+        try {
+            bufferedConnection.send(event);
+        } catch (Exception e) {
+
+        }
+        assertThat(bufferedEvents.size(), equalTo(1));
+    }
+
 }