From 46abe8cbf23e0562e00415f0b327b3875dde50b0 Mon Sep 17 00:00:00 2001 From: Tom Wilkie Date: Wed, 31 May 2017 13:46:08 +0100 Subject: [PATCH 1/2] Remote write: read first line of response and include it in the error. --- storage/remote/client.go | 8 +++++++- storage/remote/client_test.go | 8 ++++---- 2 files changed, 11 insertions(+), 5 deletions(-) diff --git a/storage/remote/client.go b/storage/remote/client.go index 483037291..a98d5876f 100644 --- a/storage/remote/client.go +++ b/storage/remote/client.go @@ -14,6 +14,7 @@ package remote import ( + "bufio" "bytes" "fmt" "io/ioutil" @@ -117,7 +118,12 @@ func (c *Client) Store(samples model.Samples) error { defer httpResp.Body.Close() if httpResp.StatusCode/100 != 2 { - err = fmt.Errorf("server returned HTTP status %s", httpResp.Status) + scanner := bufio.NewScanner(httpResp.Body) + line := "" + if scanner.Scan() { + line = scanner.Text() + } + err = fmt.Errorf("server returned HTTP status %s: %s", httpResp.Status, line) } if httpResp.StatusCode/100 == 5 { return recoverableError{err} diff --git a/storage/remote/client_test.go b/storage/remote/client_test.go index 6f3645b39..9f0980b35 100644 --- a/storage/remote/client_test.go +++ b/storage/remote/client_test.go @@ -37,15 +37,15 @@ func TestStoreHTTPErrorHandling(t *testing.T) { }, { code: 300, - err: fmt.Errorf("server returned HTTP status 300 Multiple Choices"), + err: fmt.Errorf("server returned HTTP status 300 Multiple Choices: test error"), }, { code: 404, - err: fmt.Errorf("server returned HTTP status 404 Not Found"), + err: fmt.Errorf("server returned HTTP status 404 Not Found: test error"), }, { code: 500, - err: recoverableError{fmt.Errorf("server returned HTTP status 500 Internal Server Error")}, + err: recoverableError{fmt.Errorf("server returned HTTP status 500 Internal Server Error: test error")}, }, } @@ -68,7 +68,7 @@ func TestStoreHTTPErrorHandling(t *testing.T) { err = c.Store(nil) if !reflect.DeepEqual(err, test.err) { - t.Fatalf("%d. Unexpected error; want %v, got %v", i, test.err, err) + t.Errorf("%d. Unexpected error; want %v, got %v", i, test.err, err) } server.Close() From 24a113bb0943ceb8b9db15e6ff0b25f0e5d57066 Mon Sep 17 00:00:00 2001 From: Tom Wilkie Date: Thu, 1 Jun 2017 11:21:48 +0100 Subject: [PATCH 2/2] Review feedback: limit number of bytes read under error. --- storage/remote/client.go | 5 ++++- storage/remote/client_test.go | 11 +++++++---- 2 files changed, 11 insertions(+), 5 deletions(-) diff --git a/storage/remote/client.go b/storage/remote/client.go index a98d5876f..fc60cfba6 100644 --- a/storage/remote/client.go +++ b/storage/remote/client.go @@ -17,6 +17,7 @@ import ( "bufio" "bytes" "fmt" + "io" "io/ioutil" "net/http" "time" @@ -32,6 +33,8 @@ import ( "github.com/prometheus/prometheus/util/httputil" ) +const maxErrMsgLen = 256 + // Client allows reading and writing from/to a remote HTTP endpoint. type Client struct { index int // Used to differentiate metrics. @@ -118,7 +121,7 @@ func (c *Client) Store(samples model.Samples) error { defer httpResp.Body.Close() if httpResp.StatusCode/100 != 2 { - scanner := bufio.NewScanner(httpResp.Body) + scanner := bufio.NewScanner(io.LimitReader(httpResp.Body, maxErrMsgLen)) line := "" if scanner.Scan() { line = scanner.Text() diff --git a/storage/remote/client_test.go b/storage/remote/client_test.go index 9f0980b35..32a2ae320 100644 --- a/storage/remote/client_test.go +++ b/storage/remote/client_test.go @@ -19,6 +19,7 @@ import ( "net/http/httptest" "net/url" "reflect" + "strings" "testing" "time" @@ -26,6 +27,8 @@ import ( "github.com/prometheus/prometheus/config" ) +var longErrMessage = strings.Repeat("error message", maxErrMsgLen) + func TestStoreHTTPErrorHandling(t *testing.T) { tests := []struct { code int @@ -37,22 +40,22 @@ func TestStoreHTTPErrorHandling(t *testing.T) { }, { code: 300, - err: fmt.Errorf("server returned HTTP status 300 Multiple Choices: test error"), + err: fmt.Errorf("server returned HTTP status 300 Multiple Choices: " + longErrMessage[:maxErrMsgLen]), }, { code: 404, - err: fmt.Errorf("server returned HTTP status 404 Not Found: test error"), + err: fmt.Errorf("server returned HTTP status 404 Not Found: " + longErrMessage[:maxErrMsgLen]), }, { code: 500, - err: recoverableError{fmt.Errorf("server returned HTTP status 500 Internal Server Error: test error")}, + err: recoverableError{fmt.Errorf("server returned HTTP status 500 Internal Server Error: " + longErrMessage[:maxErrMsgLen])}, }, } for i, test := range tests { server := httptest.NewServer( http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - http.Error(w, "test error", test.code) + http.Error(w, longErrMessage, test.code) }), )