Expand OkHttp retry exception predicate (#7047)

Author: jack-berg

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;

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<Arguments> 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

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<IOException> 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<IOException> retryExceptionPredicate);