From 08ffbfbbe067291e610bab9271ed541e6daaa1a0 Mon Sep 17 00:00:00 2001
From: Pip <pipt@users.noreply.github.com>
Date: Wed, 17 Jul 2024 16:21:39 +0100
Subject: [PATCH] Use the Retry-After header to wait after 429 response (#138)

Fixes https://github.com/incident-io/catalog-importer/issues/132

We no longer have our own rate limiting in the client, and instead
respect the `Retry-After` header on a 429 response.
---
 client/client.go                 | 51 +++++++-------------------------
 cmd/catalog-importer/cmd/sync.go |  2 +-
 2 files changed, 11 insertions(+), 42 deletions(-)

diff --git a/client/client.go b/client/client.go
index 87ffed4..9874f1e 100644
--- a/client/client.go
+++ b/client/client.go
@@ -12,38 +12,9 @@ import (
 	"github.com/go-kit/log/level"
 	"github.com/hashicorp/go-retryablehttp"
 	"github.com/pkg/errors"
-	"golang.org/x/time/rate"
 )
 
-const (
-	rpm = 600
-
-	// last retry will wait 64 seconds
-	maxRetries   = 7
-	minRetryWait = 1 * time.Second
-	maxRetryWait = 65 * time.Second
-)
-
-type RateLimitedClient struct {
-	client      *http.Client
-	rateLimiter *rate.Limiter
-}
-
-func (c *RateLimitedClient) Do(req *http.Request) (*http.Response, error) {
-	if c.rateLimiter == nil {
-		return nil, errors.New("rate limiter not set")
-	}
-	ctx := req.Context()
-	err := c.rateLimiter.Wait(ctx)
-	if err != nil {
-		return nil, err
-	}
-	resp, err := c.client.Do(req)
-	if err != nil {
-		return nil, err
-	}
-	return resp, nil
-}
+const maxRetries = 3
 
 func attentiveBackoff(min, max time.Duration, attemptNum int, resp *http.Response) time.Duration {
 	// Retry for rate limits and server errors.
@@ -51,9 +22,15 @@ func attentiveBackoff(min, max time.Duration, attemptNum int, resp *http.Respons
 		// Check for a 'Retry-After' header.
 		retryAfter := resp.Header.Get("Retry-After")
 		if retryAfter != "" {
-			if retryAfterSeconds, err := time.ParseDuration(retryAfter + "s"); err == nil {
-				return retryAfterSeconds
+			retryAfterDate, err := time.Parse(time.RFC1123, retryAfter)
+			if err != nil {
+				// If we can't parse the Retry-After, lets just wait for 10 seconds
+				return 10
 			}
+
+			timeToWait := time.Until(retryAfterDate)
+
+			return timeToWait
 		}
 
 	}
@@ -80,8 +57,6 @@ func New(ctx context.Context, apiKey, apiEndpoint, version string, logger kitlog
 	retryClient := retryablehttp.NewClient()
 	retryClient.Logger = &retryableHttpLogger{logger}
 	retryClient.RetryMax = maxRetries
-	retryClient.RetryWaitMin = minRetryWait
-	retryClient.RetryWaitMax = maxRetryWait
 	retryClient.Backoff = attentiveBackoff
 
 	base := retryClient.StandardClient()
@@ -105,14 +80,8 @@ func New(ctx context.Context, apiKey, apiEndpoint, version string, logger kitlog
 		return resp, err
 	})
 
-	// Adding a naive rate limiter to prevent us from using the API keys entire allowance
-	rlClient := &RateLimitedClient{
-		client:      base,
-		rateLimiter: rate.NewLimiter(rpm/60, 1),
-	}
-
 	clientOpts := append([]ClientOption{
-		WithHTTPClient(rlClient),
+		WithHTTPClient(base),
 		WithRequestEditorFn(bearerTokenProvider.Intercept),
 		// Add a user-agent so we can tell which version these requests came from.
 		WithRequestEditorFn(func(ctx context.Context, req *http.Request) error {
diff --git a/cmd/catalog-importer/cmd/sync.go b/cmd/catalog-importer/cmd/sync.go
index 84fc424..4a36aac 100644
--- a/cmd/catalog-importer/cmd/sync.go
+++ b/cmd/catalog-importer/cmd/sync.go
@@ -103,13 +103,13 @@ func (opt *SyncOptions) Run(ctx context.Context, logger kitlog.Logger, cfg *conf
 	if err != nil {
 		return err
 	}
-	OUT("✔ Connected to incident.io API (%s)", opt.APIEndpoint)
 
 	// Load existing catalog types
 	result, err := cl.CatalogV2ListTypesWithResponse(ctx)
 	if err != nil {
 		return errors.Wrap(err, "listing catalog types")
 	}
+	OUT("✔ Connected to incident.io API (%s)", opt.APIEndpoint)
 
 	existingCatalogTypes := []client.CatalogTypeV2{}
 	for _, catalogType := range result.JSON200.CatalogTypes {