From 90f902c3ab312eeb9de2fc7a8b9f6852e2c2bcec Mon Sep 17 00:00:00 2001 From: Marcin Owsiany Date: Fri, 12 Oct 2018 09:13:12 +0200 Subject: [PATCH 1/4] Add a way to return the body of a 5xx response. Fixes: #479. Signed-off-by: Marcin Owsiany --- api/prometheus/v1/api.go | 10 ++++---- api/prometheus/v1/api_test.go | 44 +++++++++++++++++++++++++++++++---- 2 files changed, 45 insertions(+), 9 deletions(-) diff --git a/api/prometheus/v1/api.go b/api/prometheus/v1/api.go index 6a19fac12..702a78e94 100644 --- a/api/prometheus/v1/api.go +++ b/api/prometheus/v1/api.go @@ -69,8 +69,9 @@ const ( // Error is an error returned by the API. type Error struct { - Type ErrorType - Msg string + Type ErrorType + Msg string + Detail string } func (e *Error) Error() string { @@ -470,8 +471,9 @@ func (c apiClient) Do(ctx context.Context, req *http.Request) (*http.Response, [ if code/100 != 2 && !apiError(code) { return resp, body, &Error{ - Type: ErrBadResponse, - Msg: fmt.Sprintf("bad response code %d", resp.StatusCode), + Type: ErrBadResponse, + Msg: fmt.Sprintf("bad response code %d", resp.StatusCode), + Detail: string(body), } } diff --git a/api/prometheus/v1/api_test.go b/api/prometheus/v1/api_test.go index 1f7fda7f9..58ae38746 100644 --- a/api/prometheus/v1/api_test.go +++ b/api/prometheus/v1/api_test.go @@ -30,15 +30,17 @@ import ( ) type apiTest struct { - do func() (interface{}, error) - inErr error - inRes interface{} + do func() (interface{}, error) + inErr error + inCode int + inRes interface{} reqPath string reqParam url.Values reqMethod string res interface{} err error + errCheck func(error) error } type apiTestClient struct { @@ -75,7 +77,9 @@ func (c *apiTestClient) Do(ctx context.Context, req *http.Request) (*http.Respon } resp := &http.Response{} - if test.inErr != nil { + if test.inCode != 0 { + resp.StatusCode = test.inCode + } else if test.inErr != nil { resp.StatusCode = statusAPIError } else { resp.StatusCode = http.StatusOK @@ -194,6 +198,30 @@ func TestAPIs(t *testing.T) { }, err: fmt.Errorf("some error"), }, + { + do: doQuery("2", testTime), + inRes: "some body", + inCode: 500, + inErr: &Error{ + Type: ErrBadResponse, + Msg: "bad response code: 500", + Detail: "a body", + }, + + reqMethod: "GET", + reqPath: "/api/v1/query", + reqParam: url.Values{ + "query": []string{"2"}, + "time": []string{testTime.Format(time.RFC3339Nano)}, + }, + errCheck: func(err error) error { + apiErr := err.(*Error) + if apiErr.Detail != "a body" { + return fmt.Errorf("%q should be %q", apiErr.Detail, "a body") + } + return nil + }, + }, { do: doQueryRange("2", Range{ @@ -503,7 +531,13 @@ func TestAPIs(t *testing.T) { res, err := test.do() - if test.err != nil { + if test.errCheck != nil { + err = test.errCheck(err) + if err != nil { + t.Errorf("returned error is wrong: %s", err) + } + continue + } else if test.err != nil { if err == nil { t.Errorf("expected error %q but got none", test.err) continue From ed1974a90b9232360eea0558ffd617a83097b6c5 Mon Sep 17 00:00:00 2001 From: Marcin Owsiany Date: Wed, 24 Oct 2018 09:23:41 +0200 Subject: [PATCH 2/4] Address review comments. Signed-off-by: Marcin Owsiany --- api/prometheus/v1/api_test.go | 43 +++++++++++++++-------------------- 1 file changed, 18 insertions(+), 25 deletions(-) diff --git a/api/prometheus/v1/api_test.go b/api/prometheus/v1/api_test.go index 58ae38746..f0e0da268 100644 --- a/api/prometheus/v1/api_test.go +++ b/api/prometheus/v1/api_test.go @@ -18,6 +18,7 @@ package v1 import ( "context" "encoding/json" + "errors" "fmt" "net/http" "net/url" @@ -30,17 +31,16 @@ import ( ) type apiTest struct { - do func() (interface{}, error) - inErr error - inCode int - inRes interface{} + do func() (interface{}, error) + inErr error + inStatusCode int + inRes interface{} reqPath string reqParam url.Values reqMethod string res interface{} err error - errCheck func(error) error } type apiTestClient struct { @@ -77,8 +77,8 @@ func (c *apiTestClient) Do(ctx context.Context, req *http.Request) (*http.Respon } resp := &http.Response{} - if test.inCode != 0 { - resp.StatusCode = test.inCode + if test.inStatusCode != 0 { + resp.StatusCode = test.inStatusCode } else if test.inErr != nil { resp.StatusCode = statusAPIError } else { @@ -199,13 +199,13 @@ func TestAPIs(t *testing.T) { err: fmt.Errorf("some error"), }, { - do: doQuery("2", testTime), - inRes: "some body", - inCode: 500, + do: doQuery("2", testTime), + inRes: "some body", + inStatusCode: 500, inErr: &Error{ Type: ErrBadResponse, Msg: "bad response code: 500", - Detail: "a body", + Detail: "some body", }, reqMethod: "GET", @@ -214,13 +214,7 @@ func TestAPIs(t *testing.T) { "query": []string{"2"}, "time": []string{testTime.Format(time.RFC3339Nano)}, }, - errCheck: func(err error) error { - apiErr := err.(*Error) - if apiErr.Detail != "a body" { - return fmt.Errorf("%q should be %q", apiErr.Detail, "a body") - } - return nil - }, + err: errors.New("bad_response: bad response code: 500"), }, { @@ -531,13 +525,7 @@ func TestAPIs(t *testing.T) { res, err := test.do() - if test.errCheck != nil { - err = test.errCheck(err) - if err != nil { - t.Errorf("returned error is wrong: %s", err) - } - continue - } else if test.err != nil { + if test.err != nil { if err == nil { t.Errorf("expected error %q but got none", test.err) continue @@ -545,6 +533,11 @@ func TestAPIs(t *testing.T) { if err.Error() != test.err.Error() { t.Errorf("unexpected error: want %s, got %s", test.err, err) } + if apiErr, ok := err.(*Error); ok { + if apiErr.Detail != test.inRes { + t.Errorf("%q should be %q", apiErr.Detail, test.inRes) + } + } continue } if err != nil { From 3899585ffaeb76bbbb9ff4bfffe224f261778bb5 Mon Sep 17 00:00:00 2001 From: Marcin Owsiany Date: Thu, 25 Oct 2018 16:31:07 +0200 Subject: [PATCH 3/4] Introduced ErrServer, ErrClient and improved table tests. Without t.Run() it is very hard to figure out which test in a table has failed. Signed-off-by: Marcin Owsiany --- api/prometheus/v1/api.go | 17 +++- api/prometheus/v1/api_test.go | 179 +++++++++++++++++++++------------- 2 files changed, 124 insertions(+), 72 deletions(-) diff --git a/api/prometheus/v1/api.go b/api/prometheus/v1/api.go index 702a78e94..255f3ba13 100644 --- a/api/prometheus/v1/api.go +++ b/api/prometheus/v1/api.go @@ -60,6 +60,8 @@ const ( ErrCanceled = "canceled" ErrExec = "execution" ErrBadResponse = "bad_response" + ErrServer = "server_error" + ErrClient = "client_error" // Possible values for HealthStatus. HealthGood HealthStatus = "up" @@ -461,6 +463,16 @@ func apiError(code int) bool { return code == statusAPIError || code == http.StatusBadRequest } +func errorTypeAndMsgFor(resp *http.Response) (ErrorType, string) { + switch resp.StatusCode / 100 { + case 4: + return ErrClient, fmt.Sprintf("client error: %d", resp.StatusCode) + case 5: + return ErrServer, fmt.Sprintf("server error: %d", resp.StatusCode) + } + return ErrBadResponse, fmt.Sprintf("bad response code %d", resp.StatusCode) +} + func (c apiClient) Do(ctx context.Context, req *http.Request) (*http.Response, []byte, error) { resp, body, err := c.Client.Do(ctx, req) if err != nil { @@ -470,9 +482,10 @@ func (c apiClient) Do(ctx context.Context, req *http.Request) (*http.Response, [ code := resp.StatusCode if code/100 != 2 && !apiError(code) { + errorType, errorMsg := errorTypeAndMsgFor(resp) return resp, body, &Error{ - Type: ErrBadResponse, - Msg: fmt.Sprintf("bad response code %d", resp.StatusCode), + Type: errorType, + Msg: errorMsg, Detail: string(body), } } diff --git a/api/prometheus/v1/api_test.go b/api/prometheus/v1/api_test.go index f0e0da268..90fff0154 100644 --- a/api/prometheus/v1/api_test.go +++ b/api/prometheus/v1/api_test.go @@ -203,8 +203,8 @@ func TestAPIs(t *testing.T) { inRes: "some body", inStatusCode: 500, inErr: &Error{ - Type: ErrBadResponse, - Msg: "bad response code: 500", + Type: ErrServer, + Msg: "server error: 500", Detail: "some body", }, @@ -214,7 +214,25 @@ func TestAPIs(t *testing.T) { "query": []string{"2"}, "time": []string{testTime.Format(time.RFC3339Nano)}, }, - err: errors.New("bad_response: bad response code: 500"), + err: errors.New("server_error: server error: 500"), + }, + { + do: doQuery("2", testTime), + inRes: "some body", + inStatusCode: 404, + inErr: &Error{ + Type: ErrClient, + Msg: "client error: 404", + Detail: "some body", + }, + + reqMethod: "GET", + reqPath: "/api/v1/query", + reqParam: url.Values{ + "query": []string{"2"}, + "time": []string{testTime.Format(time.RFC3339Nano)}, + }, + err: errors.New("client_error: client error: 404"), }, { @@ -520,34 +538,36 @@ func TestAPIs(t *testing.T) { var tests []apiTest tests = append(tests, queryTests...) - for _, test := range tests { - client.curTest = test + for i, test := range tests { + t.Run(fmt.Sprintf("%d", i), func(t *testing.T) { + client.curTest = test - res, err := test.do() + res, err := test.do() - if test.err != nil { - if err == nil { - t.Errorf("expected error %q but got none", test.err) - continue - } - if err.Error() != test.err.Error() { - t.Errorf("unexpected error: want %s, got %s", test.err, err) - } - if apiErr, ok := err.(*Error); ok { - if apiErr.Detail != test.inRes { - t.Errorf("%q should be %q", apiErr.Detail, test.inRes) + if test.err != nil { + if err == nil { + t.Errorf("expected error %q but got none", test.err) + return + } + if err.Error() != test.err.Error() { + t.Errorf("unexpected error: want %s, got %s", test.err, err) } + if apiErr, ok := err.(*Error); ok { + if apiErr.Detail != test.inRes { + t.Errorf("%q should be %q", apiErr.Detail, test.inRes) + } + } + return + } + if err != nil { + t.Errorf("unexpected error: %s", err) + return } - continue - } - if err != nil { - t.Errorf("unexpected error: %s", err) - continue - } - if !reflect.DeepEqual(res, test.res) { - t.Errorf("unexpected result: want %v, got %v", test.res, res) - } + if !reflect.DeepEqual(res, test.res) { + t.Errorf("unexpected result: want %v, got %v", test.res, res) + } + }) } } @@ -559,10 +579,10 @@ type testClient struct { } type apiClientTest struct { - code int - response interface{} - expected string - err *Error + code int + response interface{} + expectedBody string + expectedErr *Error } func (c *testClient) URL(ep string, args map[string]string) *url.URL { @@ -602,98 +622,108 @@ func (c *testClient) Do(ctx context.Context, req *http.Request) (*http.Response, func TestAPIClientDo(t *testing.T) { tests := []apiClientTest{ { + code: statusAPIError, response: &apiResponse{ Status: "error", Data: json.RawMessage(`null`), ErrorType: ErrBadData, Error: "failed", }, - err: &Error{ + expectedErr: &Error{ Type: ErrBadData, Msg: "failed", }, - code: statusAPIError, - expected: `null`, + expectedBody: `null`, }, { + code: statusAPIError, response: &apiResponse{ Status: "error", Data: json.RawMessage(`"test"`), ErrorType: ErrTimeout, Error: "timed out", }, - err: &Error{ + expectedErr: &Error{ Type: ErrTimeout, Msg: "timed out", }, - code: statusAPIError, - expected: `test`, + expectedBody: `test`, }, { - response: "bad json", - err: &Error{ - Type: ErrBadResponse, - Msg: "bad response code 500", + code: http.StatusInternalServerError, + response: "500 error details", + expectedErr: &Error{ + Type: ErrServer, + Msg: "server error: 500", + Detail: "500 error details", + }, + }, + { + code: http.StatusNotFound, + response: "404 error details", + expectedErr: &Error{ + Type: ErrClient, + Msg: "client error: 404", + Detail: "404 error details", }, - code: http.StatusInternalServerError, }, { + code: http.StatusBadRequest, response: &apiResponse{ Status: "error", Data: json.RawMessage(`null`), ErrorType: ErrBadData, Error: "end timestamp must not be before start time", }, - err: &Error{ + expectedErr: &Error{ Type: ErrBadData, Msg: "end timestamp must not be before start time", }, - code: http.StatusBadRequest, }, { + code: statusAPIError, response: "bad json", - err: &Error{ + expectedErr: &Error{ Type: ErrBadResponse, Msg: "invalid character 'b' looking for beginning of value", }, - code: statusAPIError, }, { + code: statusAPIError, response: &apiResponse{ Status: "success", Data: json.RawMessage(`"test"`), }, - err: &Error{ + expectedErr: &Error{ Type: ErrBadResponse, Msg: "inconsistent body for response code", }, - code: statusAPIError, }, { + code: statusAPIError, response: &apiResponse{ Status: "success", Data: json.RawMessage(`"test"`), ErrorType: ErrTimeout, Error: "timed out", }, - err: &Error{ + expectedErr: &Error{ Type: ErrBadResponse, Msg: "inconsistent body for response code", }, - code: statusAPIError, }, { + code: http.StatusOK, response: &apiResponse{ Status: "error", Data: json.RawMessage(`"test"`), ErrorType: ErrTimeout, Error: "timed out", }, - err: &Error{ + expectedErr: &Error{ Type: ErrBadResponse, Msg: "inconsistent body for response code", }, - code: http.StatusOK, }, } @@ -704,30 +734,39 @@ func TestAPIClientDo(t *testing.T) { } client := &apiClient{tc} - for _, test := range tests { + for i, test := range tests { + t.Run(fmt.Sprintf("%d", i), func(t *testing.T) { - tc.ch <- test + tc.ch <- test - _, body, err := client.Do(context.Background(), tc.req) + _, body, err := client.Do(context.Background(), tc.req) - if test.err != nil { - if err == nil { - t.Errorf("expected error %q but got none", test.err) - continue + if test.expectedErr != nil { + if err == nil { + t.Errorf("expected error %q but got none", test.expectedErr) + return + } + if test.expectedErr.Error() != err.Error() { + t.Errorf("unexpected error: want %q, got %q", test.expectedErr, err) + } + if test.expectedErr.Detail != "" { + apiERr := err.(*Error) + if apiERr.Detail != test.expectedErr.Detail { + t.Errorf("unexpected error details: want %q, got %q", test.expectedErr.Detail, apiERr.Detail) + } + } + return } - if test.err.Error() != err.Error() { - t.Errorf("unexpected error: want %q, got %q", test.err, err) + if err != nil { + t.Errorf("unexpeceted error %s", err) + return } - continue - } - if err != nil { - t.Errorf("unexpeceted error %s", err) - continue - } - want, got := test.expected, string(body) - if want != got { - t.Errorf("unexpected body: want %q, got %q", want, got) - } + want, got := test.expectedBody, string(body) + if want != got { + t.Errorf("unexpected body: want %q, got %q", want, got) + } + }) + } } From 173ef45ce4c3e037d4c7b56cf1a60e8fe968e902 Mon Sep 17 00:00:00 2001 From: Marcin Owsiany Date: Thu, 25 Oct 2018 19:26:06 +0200 Subject: [PATCH 4/4] More review comments addressed. Signed-off-by: Marcin Owsiany --- api/prometheus/v1/api_test.go | 18 +++++++----------- 1 file changed, 7 insertions(+), 11 deletions(-) diff --git a/api/prometheus/v1/api_test.go b/api/prometheus/v1/api_test.go index 90fff0154..8492a5c38 100644 --- a/api/prometheus/v1/api_test.go +++ b/api/prometheus/v1/api_test.go @@ -546,8 +546,7 @@ func TestAPIs(t *testing.T) { if test.err != nil { if err == nil { - t.Errorf("expected error %q but got none", test.err) - return + t.Fatalf("expected error %q but got none", test.err) } if err.Error() != test.err.Error() { t.Errorf("unexpected error: want %s, got %s", test.err, err) @@ -560,8 +559,7 @@ func TestAPIs(t *testing.T) { return } if err != nil { - t.Errorf("unexpected error: %s", err) - return + t.Fatalf("unexpected error: %s", err) } if !reflect.DeepEqual(res, test.res) { @@ -743,23 +741,21 @@ func TestAPIClientDo(t *testing.T) { if test.expectedErr != nil { if err == nil { - t.Errorf("expected error %q but got none", test.expectedErr) - return + t.Fatalf("expected error %q but got none", test.expectedErr) } if test.expectedErr.Error() != err.Error() { t.Errorf("unexpected error: want %q, got %q", test.expectedErr, err) } if test.expectedErr.Detail != "" { - apiERr := err.(*Error) - if apiERr.Detail != test.expectedErr.Detail { - t.Errorf("unexpected error details: want %q, got %q", test.expectedErr.Detail, apiERr.Detail) + apiErr := err.(*Error) + if apiErr.Detail != test.expectedErr.Detail { + t.Errorf("unexpected error details: want %q, got %q", test.expectedErr.Detail, apiErr.Detail) } } return } if err != nil { - t.Errorf("unexpeceted error %s", err) - return + t.Fatalf("unexpeceted error %s", err) } want, got := test.expectedBody, string(body)