From b527259e23febe99ac2cba076249e9777d113024 Mon Sep 17 00:00:00 2001 From: Brett Hoerner Date: Fri, 23 Feb 2018 13:08:55 -0600 Subject: [PATCH] Changed buffer code to only retry in the case of HTTP 429 or connectivity issues. --- CHANGES | 3 + docs/config.rst | 2 +- .../io/sentry/DefaultSentryClientFactory.java | 4 +- .../sentry/connection/BufferedConnection.java | 19 ++++- .../connection/ConnectionException.java | 20 +++--- .../io/sentry/connection/HttpConnection.java | 16 +++-- .../connection/TooManyRequestsException.java | 16 ++--- .../connection/AbstractConnectionTest.java | 2 +- .../connection/BufferedConnectionTest.java | 72 +++++++++++++++---- 9 files changed, 111 insertions(+), 43 deletions(-) diff --git a/CHANGES b/CHANGES index 71f3b2f2d90..6915374b3be 100644 --- a/CHANGES +++ b/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 ------------- diff --git a/docs/config.rst b/docs/config.rst index 6adb99dd1c5..b2f8d71418e 100644 --- a/docs/config.rst +++ b/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``: :: diff --git a/sentry/src/main/java/io/sentry/DefaultSentryClientFactory.java b/sentry/src/main/java/io/sentry/DefaultSentryClientFactory.java index e38ec68bf55..f5a13834b70 100644 --- a/sentry/src/main/java/io/sentry/DefaultSentryClientFactory.java +++ b/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. */ diff --git a/sentry/src/main/java/io/sentry/connection/BufferedConnection.java b/sentry/src/main/java/io/sentry/connection/BufferedConnection.java index 2102bb05f63..d904c36b0fe 100644 --- a/sentry/src/main/java/io/sentry/connection/BufferedConnection.java +++ b/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); diff --git a/sentry/src/main/java/io/sentry/connection/ConnectionException.java b/sentry/src/main/java/io/sentry/connection/ConnectionException.java index d56fd865ce4..4191c4c2b91 100644 --- a/sentry/src/main/java/io/sentry/connection/ConnectionException.java +++ b/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 } diff --git a/sentry/src/main/java/io/sentry/connection/HttpConnection.java b/sentry/src/main/java/io/sentry/connection/HttpConnection.java index baacffceca0..4aaccfaf1d8 100644 --- a/sentry/src/main/java/io/sentry/connection/HttpConnection.java +++ b/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(); } diff --git a/sentry/src/main/java/io/sentry/connection/TooManyRequestsException.java b/sentry/src/main/java/io/sentry/connection/TooManyRequestsException.java index 562341a1cf6..27b0759d871 100644 --- a/sentry/src/main/java/io/sentry/connection/TooManyRequestsException.java +++ b/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 diff --git a/sentry/src/test/java/io/sentry/connection/AbstractConnectionTest.java b/sentry/src/test/java/io/sentry/connection/AbstractConnectionTest.java index 9387c267957..1d5ac9b8983 100644 --- a/sentry/src/test/java/io/sentry/connection/AbstractConnectionTest.java +++ b/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 { diff --git a/sentry/src/test/java/io/sentry/connection/BufferedConnectionTest.java b/sentry/src/test/java/io/sentry/connection/BufferedConnectionTest.java index bc404501eca..46d8a6fa08f 100644 --- a/sentry/src/test/java/io/sentry/connection/BufferedConnectionTest.java +++ b/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() { @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)); + } + }