Repository: prometheus/prometheus (language: go)

Issue:
Respect Retry-After header on non-429 requests.
### Proposal

The `retry_on_http_429` field on queue_config allows Prometheus to retry requests when the server responds with the matching status code. On the other hand, for `5xx` error codes, Prometheus will retry using the min_backoff/max_backoff parameters only, ignoring any Retry-After headers.

In cases that the remote endpoint responds with an informed header value, it will not be respected and an erroneous queue_configs can lead to overloading it.

Given that 5xx are understood to be retryable and can happen in different circumstances, I think it would be a good idea to allow Prometheus to respect the Retry-After headers on 5xx responses as a _minimum_ time of waiting and start the min_backoff approach _after_ this time has elapsed.

What do you think?


Discussion / hints:

Fix patch:
diff --git a/storage/remote/client.go b/storage/remote/client.go
index d493e414f47..fbb68049832 100644
--- a/storage/remote/client.go
+++ b/storage/remote/client.go
@@ -236,10 +236,8 @@ func (c *Client) Store(ctx context.Context, req []byte, attempt int) error {
 		}
 		err = fmt.Errorf("server returned HTTP status %s: %s", httpResp.Status, line)
 	}
-	if httpResp.StatusCode/100 == 5 {
-		return RecoverableError{err, defaultBackoff}
-	}
-	if c.retryOnRateLimit && httpResp.StatusCode == http.StatusTooManyRequests {
+	if httpResp.StatusCode/100 == 5 ||
+		(c.retryOnRateLimit && httpResp.StatusCode == http.StatusTooManyRequests) {
 		return RecoverableError{err, retryAfterDuration(httpResp.Header.Get("Retry-After"))}
 	}
 	return err


Test patch:
diff --git a/storage/remote/client_test.go b/storage/remote/client_test.go
index a42a52e9a75..9217e1c7e22 100644
--- a/storage/remote/client_test.go
+++ b/storage/remote/client_test.go
@@ -85,12 +85,22 @@ func TestStoreHTTPErrorHandling(t *testing.T) {
 }
 
 func TestClientRetryAfter(t *testing.T) {
-	server := httptest.NewServer(
-		http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-			http.Error(w, longErrMessage, http.StatusTooManyRequests)
-		}),
-	)
-	defer server.Close()
+	setupServer := func(statusCode int) *httptest.Server {
+		return httptest.NewServer(
+			http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+				w.Header().Set("Retry-After", "5")
+				http.Error(w, longErrMessage, statusCode)
+			}),
+		)
+	}
+
+	getClientConfig := func(serverURL *url.URL, retryOnRateLimit bool) *ClientConfig {
+		return &ClientConfig{
+			URL:              &config_util.URL{URL: serverURL},
+			Timeout:          model.Duration(time.Second),
+			RetryOnRateLimit: retryOnRateLimit,
+		}
+	}
 
 	getClient := func(conf *ClientConfig) WriteClient {
 		hash, err := toHash(conf)
@@ -100,30 +110,36 @@ func TestClientRetryAfter(t *testing.T) {
 		return c
 	}
 
-	serverURL, err := url.Parse(server.URL)
-	require.NoError(t, err)
-
-	conf := &ClientConfig{
-		URL:              &config_util.URL{URL: serverURL},
-		Timeout:          model.Duration(time.Second),
-		RetryOnRateLimit: false,
+	testCases := []struct {
+		name                string
+		statusCode          int
+		retryOnRateLimit    bool
+		expectedRecoverable bool
+		expectedRetryAfter  model.Duration
+	}{
+		{"TooManyRequests - No Retry", http.StatusTooManyRequests, false, false, 0},
+		{"TooManyRequests - With Retry", http.StatusTooManyRequests, true, true, 5 * model.Duration(time.Second)},
+		{"InternalServerError", http.StatusInternalServerError, false, true, 5 * model.Duration(time.Second)}, // HTTP 5xx errors do not depend on retryOnRateLimit.
 	}
 
-	var recErr RecoverableError
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			server := setupServer(tc.statusCode)
+			defer server.Close()
 
-	c := getClient(conf)
-	err = c.Store(context.Background(), []byte{}, 0)
-	require.False(t, errors.As(err, &recErr), "Recoverable error not expected.")
+			serverURL, err := url.Parse(server.URL)
+			require.NoError(t, err)
 
-	conf = &ClientConfig{
-		URL:              &config_util.URL{URL: serverURL},
-		Timeout:          model.Duration(time.Second),
-		RetryOnRateLimit: true,
-	}
+			c := getClient(getClientConfig(serverURL, tc.retryOnRateLimit))
 
-	c = getClient(conf)
-	err = c.Store(context.Background(), []byte{}, 0)
-	require.True(t, errors.As(err, &recErr), "Recoverable error was expected.")
+			var recErr RecoverableError
+			err = c.Store(context.Background(), []byte{}, 0)
+			require.Equal(t, tc.expectedRecoverable, errors.As(err, &recErr), "Mismatch in expected recoverable error status.")
+			if tc.expectedRecoverable {
+				require.Equal(t, tc.expectedRetryAfter, err.(RecoverableError).retryAfter)
+			}
+		})
+	}
 }
 
 func TestRetryAfterDuration(t *testing.T) {