diff --git a/exporters/sender/okhttp/src/main/java/io/opentelemetry/exporter/sender/okhttp/internal/RetryInterceptor.java b/exporters/sender/okhttp/src/main/java/io/opentelemetry/exporter/sender/okhttp/internal/RetryInterceptor.java index ed17849e44c..9f6cc4729f5 100644 --- a/exporters/sender/okhttp/src/main/java/io/opentelemetry/exporter/sender/okhttp/internal/RetryInterceptor.java +++ b/exporters/sender/okhttp/src/main/java/io/opentelemetry/exporter/sender/okhttp/internal/RetryInterceptor.java @@ -11,7 +11,7 @@ import java.io.IOException; import java.net.ConnectException; import java.net.SocketTimeoutException; -import java.util.Locale; +import java.net.UnknownHostException; import java.util.StringJoiner; import java.util.concurrent.ThreadLocalRandom; import java.util.concurrent.TimeUnit; @@ -154,14 +154,15 @@ boolean shouldRetryOnException(IOException e) { // Visible for testing static boolean isRetryableException(IOException e) { + // Known retryable SocketTimeoutException messages: null, "connect timed out", "timeout" + // Known retryable ConnectTimeout messages: "Failed to connect to + // localhost/[0:0:0:0:0:0:0:1]:62611" + // Known retryable UnknownHostException messages: "xxxxxx.com" if (e instanceof SocketTimeoutException) { - String message = e.getMessage(); - // Connect timeouts can produce SocketTimeoutExceptions with no message, or with "connect - // timed out" - return message == null || message.toLowerCase(Locale.ROOT).contains("connect timed out"); + return true; } else if (e instanceof ConnectException) { - // Exceptions resemble: java.net.ConnectException: Failed to connect to - // localhost/[0:0:0:0:0:0:0:1]:62611 + return true; + } else if (e instanceof UnknownHostException) { return true; } return false; diff --git a/exporters/sender/okhttp/src/test/java/io/opentelemetry/exporter/sender/okhttp/internal/RetryInterceptorTest.java b/exporters/sender/okhttp/src/test/java/io/opentelemetry/exporter/sender/okhttp/internal/RetryInterceptorTest.java index 81e01d52b82..fa7604fb99d 100644 --- a/exporters/sender/okhttp/src/test/java/io/opentelemetry/exporter/sender/okhttp/internal/RetryInterceptorTest.java +++ b/exporters/sender/okhttp/src/test/java/io/opentelemetry/exporter/sender/okhttp/internal/RetryInterceptorTest.java @@ -27,11 +27,13 @@ import java.net.HttpRetryException; import java.net.ServerSocket; import java.net.SocketTimeoutException; +import java.net.UnknownHostException; import java.time.Duration; import java.util.concurrent.TimeUnit; import java.util.function.Predicate; import java.util.logging.Level; import java.util.logging.Logger; +import java.util.stream.Stream; import okhttp3.OkHttpClient; import okhttp3.Request; import okhttp3.Response; @@ -40,6 +42,8 @@ import org.junit.jupiter.api.extension.ExtendWith; import org.junit.jupiter.api.extension.RegisterExtension; import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.Arguments; +import org.junit.jupiter.params.provider.MethodSource; import org.junit.jupiter.params.provider.ValueSource; import org.mockito.Mock; import org.mockito.junit.jupiter.MockitoExtension; @@ -217,26 +221,30 @@ private OkHttpClient connectTimeoutClient() { .build(); } - @Test - void isRetryableException() { - // Should retry on connection timeouts, where error message is "Connect timed out" or "connect - // timed out" - assertThat(retrier.shouldRetryOnException(new SocketTimeoutException("Connect timed out"))) - .isTrue(); - assertThat(retrier.shouldRetryOnException(new SocketTimeoutException("connect timed out"))) - .isTrue(); - // Shouldn't retry on read timeouts, where error message is "Read timed out" - assertThat(retrier.shouldRetryOnException(new SocketTimeoutException("Read timed out"))) - .isFalse(); - // Shouldn't retry on write timeouts or other IOException - assertThat(retrier.shouldRetryOnException(new SocketTimeoutException("timeout"))).isFalse(); - assertThat(retrier.shouldRetryOnException(new SocketTimeoutException())).isTrue(); - assertThat(retrier.shouldRetryOnException(new IOException("error"))).isFalse(); - - // Testing configured predicate - assertThat(retrier.shouldRetryOnException(new HttpRetryException("error", 400))).isFalse(); - assertThat(retrier.shouldRetryOnException(new HttpRetryException("timeout retry", 400))) - .isTrue(); + @ParameterizedTest + @MethodSource("isRetryableExceptionArgs") + void isRetryableException(IOException exception, boolean expectedRetryResult) { + assertThat(retrier.shouldRetryOnException(exception)).isEqualTo(expectedRetryResult); + } + + private static Stream isRetryableExceptionArgs() { + return Stream.of( + // Should retry on SocketTimeoutExceptions + Arguments.of(new SocketTimeoutException("Connect timed out"), true), + Arguments.of(new SocketTimeoutException("connect timed out"), true), + Arguments.of(new SocketTimeoutException("timeout"), true), + Arguments.of(new SocketTimeoutException("Read timed out"), true), + Arguments.of(new SocketTimeoutException(), true), + // Should retry on UnknownHostExceptions + Arguments.of(new UnknownHostException("host"), true), + // Should retry on ConnectException + Arguments.of( + new ConnectException("Failed to connect to localhost/[0:0:0:0:0:0:0:1]:62611"), true), + // Shouldn't retry other IOException + Arguments.of(new IOException("error"), false), + // Testing configured predicate + Arguments.of(new HttpRetryException("error", 400), false), + Arguments.of(new HttpRetryException("timeout retry", 400), true)); } @Test diff --git a/sdk/common/src/main/java/io/opentelemetry/sdk/common/export/RetryPolicy.java b/sdk/common/src/main/java/io/opentelemetry/sdk/common/export/RetryPolicy.java index 311317bce4c..edda64d6c44 100644 --- a/sdk/common/src/main/java/io/opentelemetry/sdk/common/export/RetryPolicy.java +++ b/sdk/common/src/main/java/io/opentelemetry/sdk/common/export/RetryPolicy.java @@ -70,8 +70,8 @@ public static RetryPolicyBuilder builder() { public abstract double getBackoffMultiplier(); /** - * Returns the predicate used to determine if thrown exception is retryableor {@code null} if no - * predicate was set. + * Returns the predicate used to determine if an attempt which failed exceptionally should be + * retried, or {@code null} if the exporter specific default predicate should be used. */ @Nullable public abstract Predicate getRetryExceptionPredicate(); @@ -106,7 +106,10 @@ public abstract static class RetryPolicyBuilder { */ public abstract RetryPolicyBuilder setBackoffMultiplier(double backoffMultiplier); - /** Set the predicate to determine if retry should happen based on exception. */ + /** + * Set the predicate used to determine if an attempt which failed exceptionally should be + * retried. By default, an exporter specific default predicate should be used. + */ public abstract RetryPolicyBuilder setRetryExceptionPredicate( Predicate retryExceptionPredicate);