From e627b4dd1ce133cef85feb2e75dfe2a678d4a6c0 Mon Sep 17 00:00:00 2001 From: Christopher Kolstad Date: Wed, 29 Nov 2023 14:50:22 +0100 Subject: [PATCH 1/9] feat: obey http status and backoff on 429, 50x --- metrics.go | 35 ++++++++++++++++++++++++++++++++++- repository.go | 42 +++++++++++++++++++++++++++++++++++++++--- spec_test.go | 1 + utils.go | 2 +- utils_test.go | 1 - 5 files changed, 75 insertions(+), 6 deletions(-) diff --git a/metrics.go b/metrics.go index 04a426a..a380f53 100644 --- a/metrics.go +++ b/metrics.go @@ -5,6 +5,7 @@ import ( "context" "encoding/json" "fmt" + "math" "net/http" "net/url" "sync" @@ -66,6 +67,9 @@ type metrics struct { closed chan struct{} ctx context.Context cancel func() + maxSkips float64 + errors float64 + skips float64 } func newMetrics(options metricsOptions, channels metricsChannels) *metrics { @@ -75,6 +79,8 @@ func newMetrics(options metricsOptions, channels metricsChannels) *metrics { started: time.Now(), close: make(chan struct{}), closed: make(chan struct{}), + maxSkips: 10, + errors: 0, } ctx, cancel := context.WithCancel(context.Background()) m.ctx = ctx @@ -111,7 +117,11 @@ func (m *metrics) sync() { for { select { case <-m.ticker.C: - m.sendMetrics() + if m.errors == 0 { + m.sendMetrics() + } else { + m.decrementSkip() + } case <-m.close: close(m.closed) return @@ -136,7 +146,24 @@ func (m *metrics) registerInstance() { m.registered <- payload } +func (m *metrics) backoff() { + m.errors = math.Min(m.maxSkips, m.errors+1) + m.skips = m.errors +} + +func (m *metrics) configurationError() { + m.errors = m.maxSkips + m.skips = m.errors +} +func (m *metrics) successfulPost() { + m.errors = math.Max(0, m.errors-1) + m.skips = m.errors +} + +func (m *metrics) decrementSkip() { + m.skips = math.Max(0, m.skips-1) +} func (m *metrics) sendMetrics() { m.bucketMu.Lock() bucket := m.resetBucket() @@ -161,6 +188,11 @@ func (m *metrics) sendMetrics() { if resp.StatusCode < http.StatusOK || resp.StatusCode > http.StatusMultipleChoices { m.warn(fmt.Errorf("%s return %d", u.String(), resp.StatusCode)) + if resp.StatusCode == http.StatusUnauthorized || resp.StatusCode == http.StatusForbidden || resp.StatusCode == http.StatusNotFound { + m.configurationError() + } else if resp.StatusCode == http.StatusTooManyRequests || resp.StatusCode > http.StatusInternalServerError { + m.backoff() + } // The post failed, re-add the metrics we attempted to send so // they are included in the next post. for name, tc := range bucket.Toggles { @@ -174,6 +206,7 @@ func (m *metrics) sendMetrics() { m.bucket.Start = bucket.Start m.bucketMu.Unlock() } else { + m.successfulPost() m.sent <- payload } } diff --git a/repository.go b/repository.go index 31e915f..e4e7edf 100644 --- a/repository.go +++ b/repository.go @@ -4,6 +4,7 @@ import ( "context" "encoding/json" "fmt" + "math" "net/http" "net/url" "sync" @@ -26,6 +27,9 @@ type repository struct { isReady bool refreshTicker *time.Ticker segments map[int][]api.Constraint + errors float64 + maxSkips float64 + skips float64 } func newRepository(options repositoryOptions, channels repositoryChannels) *repository { @@ -36,6 +40,9 @@ func newRepository(options repositoryOptions, channels repositoryChannels) *repo closed: make(chan struct{}), refreshTicker: time.NewTicker(options.refreshInterval), segments: map[int][]api.Constraint{}, + errors: 0, + maxSkips: 10, + skips: 0, } ctx, cancel := context.WithCancel(context.Background()) repo.ctx = ctx @@ -80,11 +87,33 @@ func (r *repository) sync() { close(r.closed) return case <-r.refreshTicker.C: - r.fetchAndReportError() + if r.skips == 0 { + r.fetchAndReportError() + } else { + r.decrementSkips() + } } } } +func (r *repository) backoff() { + r.errors = math.Min(r.maxSkips, r.errors+1) + r.skips = r.errors +} + +func (r *repository) successfulFetch() { + r.errors = math.Max(0, r.errors-1) + r.skips = r.errors +} + +func (r *repository) decrementSkips() { + r.skips = math.Max(0, r.skips-1) +} +func (r *repository) configurationError() { + r.errors = r.maxSkips + r.skips = r.errors +} + func (r *repository) fetch() error { u, _ := r.options.url.Parse(getFetchURLPath(r.options.projectName)) @@ -119,7 +148,7 @@ func (r *repository) fetch() error { if resp.StatusCode == http.StatusNotModified { return nil } - if err := statusIsOK(resp); err != nil { + if err := r.statusIsOK(resp); err != nil { return err } @@ -133,14 +162,21 @@ func (r *repository) fetch() error { r.etag = resp.Header.Get("Etag") r.segments = featureResp.SegmentsMap() r.options.storage.Reset(featureResp.FeatureMap(), true) + r.successfulFetch() r.Unlock() return nil } -func statusIsOK(resp *http.Response) error { +func (r *repository) statusIsOK(resp *http.Response) error { s := resp.StatusCode if 200 <= s && s < 300 { return nil + } else if s == 401 || s == 403 || s == 404 { + r.configurationError() + return fmt.Errorf("%s %s returned status code %d your SDK is most likely misconfigured, backing off to maximum (%f times our interval)", resp.Request.Method, resp.Request.URL, s, r.maxSkips) + } else if s == 429 || s >= 500 { + r.backoff() + return fmt.Errorf("%s %s returned status code %d, backing off (%f times our interval)", resp.Request.Method, resp.Request.URL, s, r.errors) } return fmt.Errorf("%s %s returned status code %d", resp.Request.Method, resp.Request.URL, s) diff --git a/spec_test.go b/spec_test.go index 3355c77..cfe55fb 100644 --- a/spec_test.go +++ b/spec_test.go @@ -1,3 +1,4 @@ +//go:build norace // +build norace package unleash diff --git a/utils.go b/utils.go index d79d3d5..7021925 100644 --- a/utils.go +++ b/utils.go @@ -66,7 +66,7 @@ func every(slice interface{}, condition func(interface{}) bool) bool { return false } - if (sliceValue.Len() == 0) { + if sliceValue.Len() == 0 { return false } diff --git a/utils_test.go b/utils_test.go index 204269b..58bca64 100644 --- a/utils_test.go +++ b/utils_test.go @@ -116,4 +116,3 @@ func TestContains(t *testing.T) { } }) } - From fad1a5c91602ee49eb9a61010b76dba85f7a3bb8 Mon Sep 17 00:00:00 2001 From: Christopher Kolstad Date: Wed, 29 Nov 2023 14:57:27 +0100 Subject: [PATCH 2/9] fix: remember to init skips as well for metrics --- metrics.go | 1 + 1 file changed, 1 insertion(+) diff --git a/metrics.go b/metrics.go index a380f53..ba78c0c 100644 --- a/metrics.go +++ b/metrics.go @@ -81,6 +81,7 @@ func newMetrics(options metricsOptions, channels metricsChannels) *metrics { closed: make(chan struct{}), maxSkips: 10, errors: 0, + skips: 0, } ctx, cancel := context.WithCancel(context.Background()) m.ctx = ctx From 6c9e719999efd5bbf9dd5da10190712711dd4a8b Mon Sep 17 00:00:00 2001 From: Christopher Kolstad Date: Wed, 29 Nov 2023 14:59:02 +0100 Subject: [PATCH 3/9] Use correct variable for checking if we're ready to send --- metrics.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/metrics.go b/metrics.go index ba78c0c..6694cb5 100644 --- a/metrics.go +++ b/metrics.go @@ -118,7 +118,7 @@ func (m *metrics) sync() { for { select { case <-m.ticker.C: - if m.errors == 0 { + if m.skips == 0 { m.sendMetrics() } else { m.decrementSkip() From 9e92d9442147b220143b7a362f402871137b3343 Mon Sep 17 00:00:00 2001 From: Christopher Kolstad Date: Wed, 29 Nov 2023 15:59:45 +0100 Subject: [PATCH 4/9] chore(test): added metrics test --- metrics.go | 4 +- metrics_test.go | 99 +++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 101 insertions(+), 2 deletions(-) diff --git a/metrics.go b/metrics.go index 6694cb5..94eb178 100644 --- a/metrics.go +++ b/metrics.go @@ -188,12 +188,12 @@ func (m *metrics) sendMetrics() { defer resp.Body.Close() if resp.StatusCode < http.StatusOK || resp.StatusCode > http.StatusMultipleChoices { - m.warn(fmt.Errorf("%s return %d", u.String(), resp.StatusCode)) if resp.StatusCode == http.StatusUnauthorized || resp.StatusCode == http.StatusForbidden || resp.StatusCode == http.StatusNotFound { m.configurationError() - } else if resp.StatusCode == http.StatusTooManyRequests || resp.StatusCode > http.StatusInternalServerError { + } else if resp.StatusCode == http.StatusTooManyRequests || resp.StatusCode >= http.StatusInternalServerError { m.backoff() } + m.warn(fmt.Errorf("%s return %d", u.String(), resp.StatusCode)) // The post failed, re-add the metrics we attempted to send so // they are included in the next post. for name, tc := range bucket.Toggles { diff --git a/metrics_test.go b/metrics_test.go index 689d6c5..0c1ccda 100644 --- a/metrics_test.go +++ b/metrics_test.go @@ -339,3 +339,102 @@ func TestMetrics_ShouldNotCountMetricsForParentToggles(t *testing.T) { assert.Nil(err, "client should not return an error") assert.True(gock.IsDone(), "there should be no more mocks") } + +func TestMetrics_ShouldBackoffOn500(t *testing.T) { + assert := assert.New(t) + defer gock.OffAll() + + gock.New(mockerServer). + Post("/client/register"). + Reply(200) + gock.New(mockerServer). + Post("/client/metrics"). + Persist(). + Reply(500) + gock.New(mockerServer). + Get("/client/features"). + Reply(200). + JSON(api.FeatureResponse{}) + mockListener := &MockedListener{} + mockListener.On("OnReady").Return() + mockListener.On("OnRegistered", mock.AnythingOfType("ClientData")).Return() + mockListener.On("OnCount", "foo", false).Return() + mockListener.On("OnCount", "bar", false).Return() + mockListener.On("OnCount", "baz", false).Return() + mockListener.On("OnWarning", mock.MatchedBy(func(e error) bool { + return strings.HasSuffix(e.Error(), "http://foo.com/client/metrics return 500") + })).Return() + mockListener.On("OnError", mock.Anything).Return() + + client, err := NewClient( + WithUrl(mockerServer), + WithMetricsInterval(50*time.Millisecond), + WithAppName(mockAppName), + WithInstanceId(mockInstanceId), + WithListener(mockListener), + ) + assert.Nil(err, "client should not return an error") + + client.WaitForReady() + client.IsEnabled("foo") + client.IsEnabled("bar") + client.IsEnabled("baz") + + time.Sleep(320 * time.Millisecond) + assert.Equal(float64(3), client.metrics.errors) + err = client.Close() + assert.Nil(err, "Client should close without a problem") + +} + +func TestMetrics_ErrorCountShouldDecreaseIfSuccessful(t *testing.T) { + assert := assert.New(t) + defer gock.OffAll() + + gock.New(mockerServer). + Post("/client/register"). + Reply(200) + gock.New(mockerServer). + Post("/client/metrics"). + Times(2). + Reply(500) + gock.New(mockerServer). + Get("/client/features"). + Reply(200). + JSON(api.FeatureResponse{}) + gock.New(mockerServer). + Post("/client/metrics"). + Persist(). + Reply(200) + mockListener := &MockedListener{} + mockListener.On("OnReady").Return() + mockListener.On("OnRegistered", mock.AnythingOfType("ClientData")).Return() + mockListener.On("OnCount", "foo", false).Return() + mockListener.On("OnCount", "bar", false).Return() + mockListener.On("OnCount", "baz", false).Return() + mockListener.On("OnWarning", mock.MatchedBy(func(e error) bool { + return strings.HasSuffix(e.Error(), "http://foo.com/client/metrics return 500") + })).Return() + mockListener.On("OnError", mock.Anything).Return() + mockListener.On("OnSent", mock.Anything).Return() + + client, err := NewClient( + WithUrl(mockerServer), + WithMetricsInterval(50*time.Millisecond), + WithAppName(mockAppName), + WithInstanceId(mockInstanceId), + WithListener(mockListener), + ) + assert.Nil(err, "client should not return an error") + + client.WaitForReady() + client.IsEnabled("foo") + client.IsEnabled("bar") + client.IsEnabled("baz") + time.Sleep(360 * time.Millisecond) + client.IsEnabled("foo") + time.Sleep(100 * time.Millisecond) + assert.Equal(float64(0), client.metrics.errors) + err = client.Close() + assert.Nil(err, "Client should close without a problem") +} From 4352b848bc6a24cb0c6da80e20b99aaf3620f831 Mon Sep 17 00:00:00 2001 From: Christopher Kolstad Date: Thu, 30 Nov 2023 11:00:14 +0100 Subject: [PATCH 5/9] chore: added backoff tests for repository --- repository_test.go | 60 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 60 insertions(+) diff --git a/repository_test.go b/repository_test.go index dcdbaeb..9593b52 100644 --- a/repository_test.go +++ b/repository_test.go @@ -3,6 +3,7 @@ package unleash import ( "bytes" "encoding/json" + "gopkg.in/h2non/gock.v1" "net/http" "net/http/httptest" "strings" @@ -127,3 +128,62 @@ func TestRepository_ParseAPIResponse(t *testing.T) { assert.Equal(2, len(response.Features)) assert.Equal(0, len(response.Segments)) } + +func TestRepository_backs_off_on_http_statuses(t *testing.T) { + a := assert.New(t) + testCases := []struct { + statusCode int + errorCount float64 + }{ + { 401, 10}, + { 403, 10}, + { 404, 10}, + { 429, 1}, + { 500, 1}, + { 502, 1}, + { 503, 1}, + } + defer gock.Off() + for _, tc := range testCases { + gock.New(mockerServer). + Get("/client/features"). + Reply(tc.statusCode) + client, err := NewClient( + WithUrl(mockerServer), + WithAppName(mockAppName), + WithDisableMetrics(true), + WithInstanceId(mockInstanceId), + WithRefreshInterval(time.Millisecond * 15), + ) + a.Nil(err) + time.Sleep(20 * time.Millisecond) + a.Equal(tc.errorCount, client.repository.errors) + err = client.Close() + a.Nil(err) + } +} + +func TestRepository_back_offs_are_gradually_reduced_on_success(t *testing.T) { + a := assert.New(t) + defer gock.Off() + gock.New(mockerServer). + Get("/client/features"). + Times(4). + Reply(429) + gock.New(mockerServer). + Get("/client/features"). + Reply(200). + BodyString(`{ "version": 2, "features": []}`) + client, err := NewClient( + WithUrl(mockerServer), + WithAppName(mockAppName), + WithDisableMetrics(true), + WithInstanceId(mockInstanceId), + WithRefreshInterval(time.Millisecond * 10), + ) + a.Nil(err) + client.WaitForReady() + a.Equal(float64(3), client.repository.errors) // 4 failures, and then one success, should reduce error count to 3 + err = client.Close() + a.Nil(err) +} \ No newline at end of file From b0ec1007ce75ef1cea2dbf0edb0ee38c166bcbe7 Mon Sep 17 00:00:00 2001 From: Christopher Kolstad Date: Thu, 30 Nov 2023 11:48:29 +0100 Subject: [PATCH 6/9] Closing the client before reading error count solves race --- metrics_test.go | 20 ++++---------------- repository_test.go | 6 +++--- 2 files changed, 7 insertions(+), 19 deletions(-) diff --git a/metrics_test.go b/metrics_test.go index 0c1ccda..82fca5d 100644 --- a/metrics_test.go +++ b/metrics_test.go @@ -328,13 +328,13 @@ func TestMetrics_ShouldNotCountMetricsForParentToggles(t *testing.T) { WithInstanceId(mockInstanceId), WithListener(mockListener), ) - + assert.Nil(err, "client should not return an error") client.WaitForReady() client.IsEnabled("child") assert.EqualValues(client.metrics.bucket.Toggles["child"].Yes, 1) assert.EqualValues(client.metrics.bucket.Toggles["parent"].Yes, 0) - client.Close() + err = client.Close() assert.Nil(err, "client should not return an error") assert.True(gock.IsDone(), "there should be no more mocks") @@ -381,8 +381,8 @@ func TestMetrics_ShouldBackoffOn500(t *testing.T) { client.IsEnabled("baz") time.Sleep(320 * time.Millisecond) - assert.Equal(float64(3), client.metrics.errors) err = client.Close() + assert.Equal(float64(3), client.metrics.errors) assert.Nil(err, "Client should close without a problem") } @@ -406,24 +406,12 @@ func TestMetrics_ErrorCountShouldDecreaseIfSuccessful(t *testing.T) { Post("/client/metrics"). Persist(). Reply(200) - mockListener := &MockedListener{} - mockListener.On("OnReady").Return() - mockListener.On("OnRegistered", mock.AnythingOfType("ClientData")).Return() - mockListener.On("OnCount", "foo", false).Return() - mockListener.On("OnCount", "bar", false).Return() - mockListener.On("OnCount", "baz", false).Return() - mockListener.On("OnWarning", mock.MatchedBy(func(e error) bool { - return strings.HasSuffix(e.Error(), "http://foo.com/client/metrics return 500") - })).Return() - mockListener.On("OnError", mock.Anything).Return() - mockListener.On("OnSent", mock.Anything).Return() client, err := NewClient( WithUrl(mockerServer), WithMetricsInterval(50*time.Millisecond), WithAppName(mockAppName), WithInstanceId(mockInstanceId), - WithListener(mockListener), ) assert.Nil(err, "client should not return an error") @@ -434,7 +422,7 @@ func TestMetrics_ErrorCountShouldDecreaseIfSuccessful(t *testing.T) { time.Sleep(360 * time.Millisecond) client.IsEnabled("foo") time.Sleep(100 * time.Millisecond) - assert.Equal(float64(0), client.metrics.errors) err = client.Close() + assert.Equal(float64(0), client.metrics.errors) assert.Nil(err, "Client should close without a problem") } diff --git a/repository_test.go b/repository_test.go index 9593b52..2854730 100644 --- a/repository_test.go +++ b/repository_test.go @@ -129,6 +129,7 @@ func TestRepository_ParseAPIResponse(t *testing.T) { assert.Equal(0, len(response.Segments)) } + func TestRepository_backs_off_on_http_statuses(t *testing.T) { a := assert.New(t) testCases := []struct { @@ -157,12 +158,11 @@ func TestRepository_backs_off_on_http_statuses(t *testing.T) { ) a.Nil(err) time.Sleep(20 * time.Millisecond) - a.Equal(tc.errorCount, client.repository.errors) err = client.Close() + a.Equal(tc.errorCount, client.repository.errors) a.Nil(err) } } - func TestRepository_back_offs_are_gradually_reduced_on_success(t *testing.T) { a := assert.New(t) defer gock.Off() @@ -183,7 +183,7 @@ func TestRepository_back_offs_are_gradually_reduced_on_success(t *testing.T) { ) a.Nil(err) client.WaitForReady() - a.Equal(float64(3), client.repository.errors) // 4 failures, and then one success, should reduce error count to 3 err = client.Close() + a.Equal(float64(3), client.repository.errors) // 4 failures, and then one success, should reduce error count to 3 a.Nil(err) } \ No newline at end of file From b56567568570b7712a008fc5c3aaafc4d4d0031f Mon Sep 17 00:00:00 2001 From: Christopher Kolstad Date: Thu, 30 Nov 2023 15:02:06 +0100 Subject: [PATCH 7/9] Extend timeout --- .github/workflows/build.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index d9f3be2..c833df6 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -28,7 +28,7 @@ jobs: - name: Run spec tests run: go test -v ./... -tags='norace' - name: Run all tests with race detection - timeout-minutes: 1 + timeout-minutes: 3 run: go test -race -covermode atomic -coverprofile=profile.cov -v ./... -tags='!norace' - name: Send coverage uses: shogo82148/actions-goveralls@v1 From eace93e9ef227a4f992f97c25a44798c1905af49 Mon Sep 17 00:00:00 2001 From: Christopher Kolstad Date: Thu, 30 Nov 2023 15:06:17 +0100 Subject: [PATCH 8/9] revert timeout setting --- .github/workflows/build.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index c833df6..d9f3be2 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -28,7 +28,7 @@ jobs: - name: Run spec tests run: go test -v ./... -tags='norace' - name: Run all tests with race detection - timeout-minutes: 3 + timeout-minutes: 1 run: go test -race -covermode atomic -coverprofile=profile.cov -v ./... -tags='!norace' - name: Send coverage uses: shogo82148/actions-goveralls@v1 From 7c8994db9c65ce93fdcbe29f64ae4329e20eeeda Mon Sep 17 00:00:00 2001 From: Christopher Kolstad Date: Fri, 15 Dec 2023 14:36:36 +0100 Subject: [PATCH 9/9] fix: use http helpers to name http status codes --- repository.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/repository.go b/repository.go index e4e7edf..7921f38 100644 --- a/repository.go +++ b/repository.go @@ -169,12 +169,12 @@ func (r *repository) fetch() error { func (r *repository) statusIsOK(resp *http.Response) error { s := resp.StatusCode - if 200 <= s && s < 300 { + if http.StatusOK <= s && s < http.StatusMultipleChoices { return nil - } else if s == 401 || s == 403 || s == 404 { + } else if s == http.StatusUnauthorized || s == http.StatusForbidden || s == http.StatusNotFound { r.configurationError() return fmt.Errorf("%s %s returned status code %d your SDK is most likely misconfigured, backing off to maximum (%f times our interval)", resp.Request.Method, resp.Request.URL, s, r.maxSkips) - } else if s == 429 || s >= 500 { + } else if s == http.StatusTooManyRequests || s >= http.StatusInternalServerError { r.backoff() return fmt.Errorf("%s %s returned status code %d, backing off (%f times our interval)", resp.Request.Method, resp.Request.URL, s, r.errors) }