From e7bc6a870e2a67bb8bfedd20c431ce28a08c4bf5 Mon Sep 17 00:00:00 2001 From: xliu2 Date: Mon, 20 Apr 2020 10:14:34 +1000 Subject: [PATCH 1/4] make linter happy --- sfxclient/httpsink.go | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/sfxclient/httpsink.go b/sfxclient/httpsink.go index 777a28a..d066501 100644 --- a/sfxclient/httpsink.go +++ b/sfxclient/httpsink.go @@ -109,6 +109,7 @@ func (e *TooManyRequestError) Error() string { return fmt.Sprintf("too many %s requests, retry after %.3f seconds", e.ThrottleType, e.RetryAfter.Seconds()) } +// Unwrap returns the wrapped error. func (e *TooManyRequestError) Unwrap() error { return e.Err } @@ -498,15 +499,15 @@ func parseRetryAfterHeader(v string) (time.Duration, error) { // Retry-After: // https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Date if t, err := http.ParseTime(v); err == nil { - return t.Sub(time.Now()), nil + return time.Until(t), nil } // Retry-After: - if i, err := strconv.Atoi(v); err != nil { + i, err := strconv.Atoi(v) + if err != nil { return 0, err - } else { - return time.Duration(i) * time.Second, nil } + return time.Duration(i) * time.Second, nil } // NewHTTPSink creates a default NewHTTPSink using package level constants as From 311344461d0ac8234e2d9bdd1594cd2828c399f5 Mon Sep 17 00:00:00 2001 From: xliu2 Date: Mon, 20 Apr 2020 10:40:00 +1000 Subject: [PATCH 2/4] remove go 1.13 stuff to make go 1.12 build green --- sfxclient/httpsink.go | 7 +------ sfxclient/multitokensink.go | 10 ++++++---- 2 files changed, 7 insertions(+), 10 deletions(-) diff --git a/sfxclient/httpsink.go b/sfxclient/httpsink.go index d066501..e01929d 100644 --- a/sfxclient/httpsink.go +++ b/sfxclient/httpsink.go @@ -102,18 +102,13 @@ type TooManyRequestError struct { Err error } -func (e *TooManyRequestError) Error() string { +func (e TooManyRequestError) Error() string { if e.ThrottleType == "" { return fmt.Sprintf("too many requests, retry after %.3f seconds", e.RetryAfter.Seconds()) } return fmt.Sprintf("too many %s requests, retry after %.3f seconds", e.ThrottleType, e.RetryAfter.Seconds()) } -// Unwrap returns the wrapped error. -func (e *TooManyRequestError) Unwrap() error { - return e.Err -} - type responseValidator func(respBody []byte) error func (h *HTTPSink) handleResponse(resp *http.Response, respValidator responseValidator) (err error) { diff --git a/sfxclient/multitokensink.go b/sfxclient/multitokensink.go index b57d72c..e000ee9 100644 --- a/sfxclient/multitokensink.go +++ b/sfxclient/multitokensink.go @@ -1,7 +1,6 @@ package sfxclient import ( - "errors" "fmt" "hash" "hash/fnv" @@ -55,9 +54,12 @@ func getHTTPStatusCode(status *tokenStatus, err error) *tokenStatus { if err == nil { status.status = http.StatusOK } else { - var apiErr *SFXAPIError - if errors.As(err, &apiErr) { - status.status = apiErr.StatusCode + // refactor: use `errors.As` to simplify type assertion on error chain once we get rid of go 1.12 build. + if obj, ok := err.(*TooManyRequestError); ok { + err = obj.Err + } + if obj, ok := err.(*SFXAPIError); ok { + status.status = obj.StatusCode } } return status From d97913790b535a164362b28d2906e81496427844 Mon Sep 17 00:00:00 2001 From: xliu2 Date: Mon, 20 Apr 2020 12:06:55 +1000 Subject: [PATCH 3/4] add tests to make sure 100% coverage --- sfxclient/httpsink.go | 10 ++-------- sfxclient/httpsink_test.go | 28 ++++++++++++++++++++++++++++ sfxclient/multitokensink_test.go | 22 ++++++++++++++++++++++ 3 files changed, 52 insertions(+), 8 deletions(-) diff --git a/sfxclient/httpsink.go b/sfxclient/httpsink.go index e01929d..06614c9 100644 --- a/sfxclient/httpsink.go +++ b/sfxclient/httpsink.go @@ -103,10 +103,8 @@ type TooManyRequestError struct { } func (e TooManyRequestError) Error() string { - if e.ThrottleType == "" { - return fmt.Sprintf("too many requests, retry after %.3f seconds", e.RetryAfter.Seconds()) - } - return fmt.Sprintf("too many %s requests, retry after %.3f seconds", e.ThrottleType, e.RetryAfter.Seconds()) + return fmt.Sprintf("[%s] too many requests, retry after %.3f seconds", + e.ThrottleType, e.RetryAfter.Seconds()) } type responseValidator func(respBody []byte) error @@ -487,10 +485,6 @@ func sapmMarshal(v []*trace.Span) ([]byte, error) { } func parseRetryAfterHeader(v string) (time.Duration, error) { - if v == "" { - return 0, errors.New("empty header value") - } - // Retry-After: // https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Date if t, err := http.ParseTime(v); err == nil { diff --git a/sfxclient/httpsink_test.go b/sfxclient/httpsink_test.go index af231ae..feb6183 100644 --- a/sfxclient/httpsink_test.go +++ b/sfxclient/httpsink_test.go @@ -165,6 +165,7 @@ func TestHTTPDatapointSink(t *testing.T) { Convey("with a test endpoint", func() { retString := `"OK"` retCode := http.StatusOK + retHeaders := map[string]string{} var blockResponse chan struct{} var cancelCallback func() seenBodyPoints := &com_signalfx_metrics_protobuf.DataPointUploadMessage{} @@ -174,6 +175,9 @@ func TestHTTPDatapointSink(t *testing.T) { log.IfErr(log.Panic, err) log.IfErr(log.Panic, req.Body.Close()) log.IfErr(log.Panic, proto.Unmarshal(bodyBytes.Bytes(), seenBodyPoints)) + for k, v := range retHeaders { + rw.Header().Add(k, v) + } rw.WriteHeader(retCode) errors.PanicIfErrWrite(io.WriteString(rw, retString)) if blockResponse != nil { @@ -250,6 +254,30 @@ func TestHTTPDatapointSink(t *testing.T) { retCode = http.StatusNotAcceptable So(errors.Details(s.AddDatapoints(ctx, dps)), ShouldContainSubstring, "invalid status code") }) + Convey("HTTP 429 should be checked", func() { + retCode = http.StatusTooManyRequests + retHeaders = map[string]string{ + "Throttle-Type": "x", + } + Convey("retry after seconds", func() { + retHeaders["Retry-After"] = "1" + err, ok := s.AddDatapoints(ctx, dps).(*TooManyRequestError) + So(ok, ShouldBeTrue) + So(err.RetryAfter, ShouldEqual, time.Second) + So(err.Error(), ShouldContainSubstring, "[x] too many requests, retry after") + }) + Convey("retry until date", func() { + retHeaders["Retry-After"] = time.Now().Add(time.Second).Format(time.RFC850) + err, ok := s.AddDatapoints(ctx, dps).(*TooManyRequestError) + So(ok, ShouldBeTrue) + So(err.RetryAfter, ShouldBeLessThanOrEqualTo, time.Second) + So(err.Error(), ShouldContainSubstring, "[x] too many requests, retry after") + }) + Convey("error fallback", func() { + retHeaders["Retry-After"] = "" + So(errors.Details(s.AddDatapoints(ctx, dps)), ShouldContainSubstring, "invalid status code") + }) + }) Convey("return string should be checked", func() { retString = `"nope"` So(errors.Details(s.AddDatapoints(ctx, dps)), ShouldContainSubstring, "invalid response body") diff --git a/sfxclient/multitokensink_test.go b/sfxclient/multitokensink_test.go index b69d0aa..4c2b784 100644 --- a/sfxclient/multitokensink_test.go +++ b/sfxclient/multitokensink_test.go @@ -104,6 +104,28 @@ func AddSpansGetSuccess(ctx context.Context, evs []*trace.Span) (err error) { return } +func TestGetHTTPStatusCode(t *testing.T) { + ts := &tokenStatus{} + Convey("should handle different type of errors", t, func() { + So(getHTTPStatusCode(ts, nil).status, ShouldEqual, http.StatusOK) + + So(getHTTPStatusCode(ts, &SFXAPIError{ + StatusCode: http.StatusBadRequest, + ResponseBody: "", + }).status, ShouldEqual, http.StatusBadRequest) + + err := &TooManyRequestError{ + ThrottleType: "", + RetryAfter: 0, + Err: &SFXAPIError{ + StatusCode: http.StatusTooManyRequests, + ResponseBody: "", + }, + } + So(getHTTPStatusCode(ts, err).status, ShouldEqual, http.StatusTooManyRequests) + }) +} + func TestWorkerErrorHandlerDps(t *testing.T) { Convey("An AsyncMultiTokeSink Worker Dps", t, func() { Convey("should handle errors while emitting datapoints", func() { From 63a0847d73287e75f03d2eb66dab9ffc6948ed3d Mon Sep 17 00:00:00 2001 From: xliu2 Date: Mon, 20 Apr 2020 12:20:47 +1000 Subject: [PATCH 4/4] increase test timeout by 10s to avoid timeout --- .travis.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.travis.yml b/.travis.yml index c31dcb4..f113ead 100644 --- a/.travis.yml +++ b/.travis.yml @@ -18,7 +18,7 @@ script: - export GO111MODULE=on - go mod download - golangci-lint run - - go test -v -covermode atomic -race -timeout 60s -cpu 4 -parallel 8 -coverprofile ./coverage.out $(go list ./...) + - go test -v -covermode atomic -race -timeout 70s -cpu 4 -parallel 8 -coverprofile ./coverage.out $(go list ./...) - "! go tool cover -func coverage.out | grep -v 100.0%" after_script: