From e5da2e95b62ac34d47a3e686b275c7a64702612e Mon Sep 17 00:00:00 2001 From: Mike Zorn Date: Mon, 7 Oct 2024 11:52:11 -0700 Subject: [PATCH] fix: Retry when there are 429s on flag metadata fetches (#446) Retry when there are 429s --- internal/dev_server/adapters/api.go | 68 +---------- .../dev_server/adapters/internal/api_util.go | 108 ++++++++++++++++++ .../api_util_test.go} | 45 +++++++- .../dev_server/adapters/internal/mocks.go | 66 +++++++++++ 4 files changed, 221 insertions(+), 66 deletions(-) create mode 100644 internal/dev_server/adapters/internal/api_util.go rename internal/dev_server/adapters/{api_test.go => internal/api_util_test.go} (71%) create mode 100644 internal/dev_server/adapters/internal/mocks.go diff --git a/internal/dev_server/adapters/api.go b/internal/dev_server/adapters/api.go index 541ddc18..6087f014 100644 --- a/internal/dev_server/adapters/api.go +++ b/internal/dev_server/adapters/api.go @@ -4,9 +4,8 @@ import ( "context" "fmt" "log" - "net/url" - "strconv" + "github.com/launchdarkly/ldcli/internal/dev_server/adapters/internal" "github.com/pkg/errors" ldapi "github.com/launchdarkly/api-client-go/v14" @@ -65,7 +64,8 @@ func (a apiClientApi) GetProjectEnvironments(ctx context.Context, projectKey str } func (a apiClientApi) getFlags(ctx context.Context, projectKey string, href *string) ([]ldapi.FeatureFlag, error) { - return getPaginatedItems(ctx, projectKey, href, func(ctx context.Context, projectKey string, limit, offset *int64) (*ldapi.FeatureFlags, error) { + return internal.GetPaginatedItems(ctx, projectKey, href, func(ctx context.Context, projectKey string, limit, offset *int64) (flags *ldapi.FeatureFlags, err error) { + // loop until we do not get rate limited query := a.apiClient.FeatureFlagsApi.GetFeatureFlags(ctx, projectKey).Limit(100) if limit != nil { @@ -75,10 +75,7 @@ func (a apiClientApi) getFlags(ctx context.Context, projectKey string, href *str if offset != nil { query = query.Offset(*offset) } - - flags, _, err := query. - Execute() - return flags, err + return internal.Retry429s(query.Execute) }) } @@ -105,60 +102,3 @@ func (a apiClientApi) getEnvironments(ctx context.Context, projectKey string, hr return envs.Items, nil } - -func getPaginatedItems[T any, R interface { - GetItems() []T - GetLinks() map[string]ldapi.Link -}](ctx context.Context, projectKey string, href *string, fetchFunc func(context.Context, string, *int64, *int64) (R, error)) ([]T, error) { - var result R - var err error - - if href == nil { - result, err = fetchFunc(ctx, projectKey, nil, nil) - if err != nil { - return nil, err - } - } else { - limit, offset, err := parseHref(*href) - if err != nil { - return nil, errors.Wrapf(err, "unable to parse href for next link: %s", *href) - } - result, err = fetchFunc(ctx, projectKey, &limit, &offset) - if err != nil { - return nil, err - } - } - - items := result.GetItems() - - if links := result.GetLinks(); links != nil { - if next, ok := links["next"]; ok && next.Href != nil { - newItems, err := getPaginatedItems(ctx, projectKey, next.Href, fetchFunc) - if err != nil { - return nil, err - } - items = append(items, newItems...) - } - } - - return items, nil -} - -func parseHref(href string) (limit, offset int64, err error) { - parsedUrl, err := url.Parse(href) - if err != nil { - return - } - l, err := strconv.Atoi(parsedUrl.Query().Get("limit")) - if err != nil { - return - } - o, err := strconv.Atoi(parsedUrl.Query().Get("offset")) - if err != nil { - return - } - - limit = int64(l) - offset = int64(o) - return -} diff --git a/internal/dev_server/adapters/internal/api_util.go b/internal/dev_server/adapters/internal/api_util.go new file mode 100644 index 00000000..8a2b3d33 --- /dev/null +++ b/internal/dev_server/adapters/internal/api_util.go @@ -0,0 +1,108 @@ +package internal + +import ( + "context" + "log" + "net/http" + "net/url" + "strconv" + "time" + + "github.com/launchdarkly/api-client-go/v14" + "github.com/pkg/errors" +) + +func GetPaginatedItems[T any, R interface { + GetItems() []T + GetLinks() map[string]ldapi.Link +}](ctx context.Context, projectKey string, href *string, fetchFunc func(context.Context, string, *int64, *int64) (R, error)) ([]T, error) { + var result R + var err error + + if href == nil { + result, err = fetchFunc(ctx, projectKey, nil, nil) + if err != nil { + return nil, err + } + } else { + limit, offset, err := parseHref(*href) + if err != nil { + return nil, errors.Wrapf(err, "unable to parse href for next link: %s", *href) + } + result, err = fetchFunc(ctx, projectKey, &limit, &offset) + if err != nil { + return nil, err + } + } + + items := result.GetItems() + + if links := result.GetLinks(); links != nil { + if next, ok := links["next"]; ok && next.Href != nil { + newItems, err := GetPaginatedItems(ctx, projectKey, next.Href, fetchFunc) + if err != nil { + return nil, err + } + items = append(items, newItems...) + } + } + + return items, nil +} + +func parseHref(href string) (limit, offset int64, err error) { + parsedUrl, err := url.Parse(href) + if err != nil { + return + } + l, err := strconv.Atoi(parsedUrl.Query().Get("limit")) + if err != nil { + return + } + o, err := strconv.Atoi(parsedUrl.Query().Get("offset")) + if err != nil { + return + } + + limit = int64(l) + offset = int64(o) + return +} + +//go:generate go run go.uber.org/mock/mockgen -destination mocks.go -package internal . MockableTime +type MockableTime interface { + Sleep(duration time.Duration) + Now() time.Time +} + +type realTime struct{} + +func (realTime) Sleep(duration time.Duration) { + time.Sleep(duration) +} + +func (realTime) Now() time.Time { + return time.Now() +} + +var timeImpl MockableTime = realTime{} + +func Retry429s[T any](requester func() (T, *http.Response, error)) (result T, err error) { + for { + var res *http.Response + result, res, err = requester() + if res.StatusCode == 429 { + resetUnixMillisString := res.Header.Get("X-Ratelimit-Reset") + resetUnixMillis, strconvErr := strconv.ParseInt(resetUnixMillisString, 10, 64) + if strconvErr != nil { + err = errors.Wrapf(err, `unable to retry rate limited request: X-RateLimit-Reset: "%s" was not parsable`, resetUnixMillisString) + return + } + sleep := resetUnixMillis - timeImpl.Now().UnixMilli() + log.Printf("Got 429 in API response. Retrying in %d milliseconds.", sleep) + timeImpl.Sleep(time.Duration(sleep) * time.Millisecond) + } else { + return + } + } +} diff --git a/internal/dev_server/adapters/api_test.go b/internal/dev_server/adapters/internal/api_util_test.go similarity index 71% rename from internal/dev_server/adapters/api_test.go rename to internal/dev_server/adapters/internal/api_util_test.go index c30eb19c..28d388d0 100644 --- a/internal/dev_server/adapters/api_test.go +++ b/internal/dev_server/adapters/internal/api_util_test.go @@ -1,11 +1,14 @@ -package adapters +package internal import ( "context" + "net/http" "testing" + "time" ldapi "github.com/launchdarkly/api-client-go/v14" "github.com/stretchr/testify/assert" + "go.uber.org/mock/gomock" ) type testItem struct { @@ -131,7 +134,7 @@ func TestGetPaginatedItems(t *testing.T) { return result, nil } - items, err := getPaginatedItems(ctx, projectKey, nil, fetchFunc) + items, err := GetPaginatedItems(ctx, projectKey, nil, fetchFunc) if tc.expectedError { assert.Error(t, err) @@ -143,6 +146,44 @@ func TestGetPaginatedItems(t *testing.T) { } } +func TestRetry429s(t *testing.T) { + t.Run("it should call exactly once if not a 429", func(t *testing.T) { + called := 0 + res, err := Retry429s(func() (string, *http.Response, error) { + called++ + return "lol", &http.Response{StatusCode: 200}, nil + }) + assert.Equal(t, "lol", res) + assert.NoError(t, err) + assert.Equal(t, 1, called) + }) + + t.Run("it should retry when a 429 is received", func(t *testing.T) { + ctrl := gomock.NewController(t) + timeMock := NewMockMockableTime(ctrl) + defer func() { ctrl.Finish() }() + timeImpl = timeMock + defer func() { timeImpl = realTime{} }() + timeMock.EXPECT().Now().Return(time.UnixMilli(0)) + timeMock.EXPECT().Sleep(time.Duration(1000) * time.Millisecond) + + called := 0 + res, err := Retry429s(func() (string, *http.Response, error) { + called++ + if called > 1 { + return "lol", &http.Response{StatusCode: 200}, nil + } else { + header := make(http.Header) + header.Set("X-Ratelimit-Reset", "1000") + return "", &http.Response{StatusCode: 429, Header: header}, nil + } + }) + assert.Equal(t, "lol", res) + assert.NoError(t, err) + assert.Equal(t, 2, called) + }) +} + func strPtr(s string) *string { return &s } diff --git a/internal/dev_server/adapters/internal/mocks.go b/internal/dev_server/adapters/internal/mocks.go new file mode 100644 index 00000000..59d33443 --- /dev/null +++ b/internal/dev_server/adapters/internal/mocks.go @@ -0,0 +1,66 @@ +// Code generated by MockGen. DO NOT EDIT. +// Source: github.com/launchdarkly/ldcli/internal/dev_server/adapters/internal (interfaces: MockableTime) +// +// Generated by this command: +// +// mockgen -destination mocks.go -package internal . MockableTime +// + +// Package internal is a generated GoMock package. +package internal + +import ( + reflect "reflect" + time "time" + + gomock "go.uber.org/mock/gomock" +) + +// MockMockableTime is a mock of MockableTime interface. +type MockMockableTime struct { + ctrl *gomock.Controller + recorder *MockMockableTimeMockRecorder +} + +// MockMockableTimeMockRecorder is the mock recorder for MockMockableTime. +type MockMockableTimeMockRecorder struct { + mock *MockMockableTime +} + +// NewMockMockableTime creates a new mock instance. +func NewMockMockableTime(ctrl *gomock.Controller) *MockMockableTime { + mock := &MockMockableTime{ctrl: ctrl} + mock.recorder = &MockMockableTimeMockRecorder{mock} + return mock +} + +// EXPECT returns an object that allows the caller to indicate expected use. +func (m *MockMockableTime) EXPECT() *MockMockableTimeMockRecorder { + return m.recorder +} + +// Now mocks base method. +func (m *MockMockableTime) Now() time.Time { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "Now") + ret0, _ := ret[0].(time.Time) + return ret0 +} + +// Now indicates an expected call of Now. +func (mr *MockMockableTimeMockRecorder) Now() *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Now", reflect.TypeOf((*MockMockableTime)(nil).Now)) +} + +// Sleep mocks base method. +func (m *MockMockableTime) Sleep(arg0 time.Duration) { + m.ctrl.T.Helper() + m.ctrl.Call(m, "Sleep", arg0) +} + +// Sleep indicates an expected call of Sleep. +func (mr *MockMockableTimeMockRecorder) Sleep(arg0 any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Sleep", reflect.TypeOf((*MockMockableTime)(nil).Sleep), arg0) +}