From ddbc8c25f0f915773e6a8f861c2701598716e762 Mon Sep 17 00:00:00 2001 From: Cyril Tovena Date: Tue, 18 Feb 2020 15:42:18 -0500 Subject: [PATCH 1/5] Frontent does not return 500 if the user request cancellation. Signed-off-by: Cyril Tovena --- pkg/querier/frontend/frontend.go | 21 ++++++++++++++++----- pkg/querier/frontend/frontend_test.go | 19 +++++++++++++++++++ 2 files changed, 35 insertions(+), 5 deletions(-) diff --git a/pkg/querier/frontend/frontend.go b/pkg/querier/frontend/frontend.go index 9e53924a72a..c1ba2441811 100644 --- a/pkg/querier/frontend/frontend.go +++ b/pkg/querier/frontend/frontend.go @@ -38,8 +38,8 @@ var ( Help: "Number of queries in the queue.", }) - errTooManyRequest = httpgrpc.Errorf(http.StatusTooManyRequests, "too many outstanding requests") - errCanceled = httpgrpc.Errorf(http.StatusInternalServerError, "context cancelled") + errTooManyRequest = httpgrpc.Errorf(http.StatusTooManyRequests, "too many outstanding requests") + statusRequestCanceled = 499 ) // Config for a Frontend. @@ -151,14 +151,14 @@ func (f *Frontend) handle(w http.ResponseWriter, r *http.Request) { startTime := time.Now() resp, err := f.roundTripper.RoundTrip(r) - queryResponseTime := time.Now().Sub(startTime) + queryResponseTime := time.Since(startTime) if f.cfg.LogQueriesLongerThan > 0 && queryResponseTime > f.cfg.LogQueriesLongerThan { level.Info(f.log).Log("msg", "slow query", "org_id", userID, "url", fmt.Sprintf("http://%s", r.Host+r.RequestURI), "time_taken", queryResponseTime.String()) } if err != nil { - server.WriteError(w, err) + writeError(w, err) return } @@ -170,6 +170,17 @@ func (f *Frontend) handle(w http.ResponseWriter, r *http.Request) { io.Copy(w, resp.Body) } +func writeError(w http.ResponseWriter, err error) { + switch err { + case context.Canceled: + err = httpgrpc.Errorf(statusRequestCanceled, err.Error()) + case context.DeadlineExceeded: + err = httpgrpc.Errorf(http.StatusGatewayTimeout, err.Error()) + default: + } + server.WriteError(w, err) +} + // RoundTrip implement http.Transport. func (f *Frontend) RoundTrip(r *http.Request) (*http.Response, error) { req, err := server.HTTPRequest(r) @@ -230,7 +241,7 @@ func (f *Frontend) RoundTripGRPC(ctx context.Context, req *ProcessRequest) (*Pro select { case <-ctx.Done(): - return nil, errCanceled + return nil, ctx.Err() case resp := <-request.response: return resp, nil diff --git a/pkg/querier/frontend/frontend_test.go b/pkg/querier/frontend/frontend_test.go index b1d1ff65448..baf4bc5a2e9 100644 --- a/pkg/querier/frontend/frontend_test.go +++ b/pkg/querier/frontend/frontend_test.go @@ -2,10 +2,12 @@ package frontend import ( "context" + "errors" "fmt" "io/ioutil" "net" "net/http" + "net/http/httptest" "sync/atomic" "testing" "time" @@ -133,6 +135,23 @@ func TestFrontendCancel(t *testing.T) { testFrontend(t, handler, test) } +func TestFrontendCancelStatusCode(t *testing.T) { + for _, test := range []struct { + status int + err error + }{ + {http.StatusInternalServerError, errors.New("unknown")}, + {http.StatusGatewayTimeout, context.DeadlineExceeded}, + {statusRequestCanceled, context.Canceled}, + } { + t.Run(test.err.Error(), func(t *testing.T) { + w := httptest.NewRecorder() + writeError(w, test.err) + require.Equal(t, test.status, w.Result().StatusCode) + }) + } +} + func defaultOverrides(t *testing.T) *validation.Overrides { var limits validation.Limits flagext.DefaultValues(&limits) From 6048f0a082ece807e55f4f693bd6c52490b765ca Mon Sep 17 00:00:00 2001 From: Cyril Tovena Date: Tue, 18 Feb 2020 15:46:58 -0500 Subject: [PATCH 2/5] Improves global errors variable for better reusability. Signed-off-by: Cyril Tovena --- pkg/querier/frontend/frontend.go | 9 +++++---- pkg/querier/frontend/frontend_test.go | 2 +- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/pkg/querier/frontend/frontend.go b/pkg/querier/frontend/frontend.go index c1ba2441811..326ba6a616d 100644 --- a/pkg/querier/frontend/frontend.go +++ b/pkg/querier/frontend/frontend.go @@ -38,8 +38,9 @@ var ( Help: "Number of queries in the queue.", }) - errTooManyRequest = httpgrpc.Errorf(http.StatusTooManyRequests, "too many outstanding requests") - statusRequestCanceled = 499 + errTooManyRequest = httpgrpc.Errorf(http.StatusTooManyRequests, "too many outstanding requests") + errCanceled = httpgrpc.Errorf(499, context.Canceled.Error()) + errDeadlineExceeded = httpgrpc.Errorf(http.StatusGatewayTimeout, context.DeadlineExceeded.Error()) ) // Config for a Frontend. @@ -173,9 +174,9 @@ func (f *Frontend) handle(w http.ResponseWriter, r *http.Request) { func writeError(w http.ResponseWriter, err error) { switch err { case context.Canceled: - err = httpgrpc.Errorf(statusRequestCanceled, err.Error()) + err = errCanceled case context.DeadlineExceeded: - err = httpgrpc.Errorf(http.StatusGatewayTimeout, err.Error()) + err = errDeadlineExceeded default: } server.WriteError(w, err) diff --git a/pkg/querier/frontend/frontend_test.go b/pkg/querier/frontend/frontend_test.go index baf4bc5a2e9..d451af1740b 100644 --- a/pkg/querier/frontend/frontend_test.go +++ b/pkg/querier/frontend/frontend_test.go @@ -142,7 +142,7 @@ func TestFrontendCancelStatusCode(t *testing.T) { }{ {http.StatusInternalServerError, errors.New("unknown")}, {http.StatusGatewayTimeout, context.DeadlineExceeded}, - {statusRequestCanceled, context.Canceled}, + {499, context.Canceled}, } { t.Run(test.err.Error(), func(t *testing.T) { w := httptest.NewRecorder() From 8067089809386920d2b0c9999bf9963a03e00732 Mon Sep 17 00:00:00 2001 From: Cyril Tovena Date: Tue, 18 Feb 2020 16:00:07 -0500 Subject: [PATCH 3/5] Upate changelog Signed-off-by: Cyril Tovena --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9df155d04cc..72dcde753bf 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,7 @@ ## master / unreleased +* [CHANGE] The frontend http server will now send 502 in case of deadline exceeded and 499 if the user requested cancellation. #2148 * [CHANGE] Config file changed to remove top level `config_store` field in favor of a nested `configdb` field. #2125 * [CHANGE] Removed unnecessary `frontend.cache-split-interval` in favor of `querier.split-queries-by-interval` both to reduce configuration complexity and guarantee alignment of these two configs. Starting from now, `-querier.cache-results` may only be enabled in conjunction with `-querier.split-queries-by-interval` (previously the cache interval default was `24h` so if you want to preserve the same behaviour you should set `-querier.split-queries-by-interval=24h`). #2040 * [CHANGE] Removed remaining support for using denormalised tokens in the ring. If you're still running ingesters with denormalised tokens (Cortex 0.4 or earlier, with `-ingester.normalise-tokens=false`), such ingesters will now be completely invisible to distributors and need to be either switched to Cortex 0.6.0 or later, or be configured to use normalised tokens. #2034 From 20986946bf8eb28b777e1380d4eaab7497b9bdbc Mon Sep 17 00:00:00 2001 From: Cyril Tovena Date: Tue, 18 Feb 2020 16:06:24 -0500 Subject: [PATCH 4/5] Add one more tests and fixes PR number in changelog. Signed-off-by: Cyril Tovena --- CHANGELOG.md | 2 +- pkg/querier/frontend/frontend_test.go | 2 ++ 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 72dcde753bf..cb8f4af4698 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,7 +2,7 @@ ## master / unreleased -* [CHANGE] The frontend http server will now send 502 in case of deadline exceeded and 499 if the user requested cancellation. #2148 +* [CHANGE] The frontend http server will now send 502 in case of deadline exceeded and 499 if the user requested cancellation. #2156 * [CHANGE] Config file changed to remove top level `config_store` field in favor of a nested `configdb` field. #2125 * [CHANGE] Removed unnecessary `frontend.cache-split-interval` in favor of `querier.split-queries-by-interval` both to reduce configuration complexity and guarantee alignment of these two configs. Starting from now, `-querier.cache-results` may only be enabled in conjunction with `-querier.split-queries-by-interval` (previously the cache interval default was `24h` so if you want to preserve the same behaviour you should set `-querier.split-queries-by-interval=24h`). #2040 * [CHANGE] Removed remaining support for using denormalised tokens in the ring. If you're still running ingesters with denormalised tokens (Cortex 0.4 or earlier, with `-ingester.normalise-tokens=false`), such ingesters will now be completely invisible to distributors and need to be either switched to Cortex 0.6.0 or later, or be configured to use normalised tokens. #2034 diff --git a/pkg/querier/frontend/frontend_test.go b/pkg/querier/frontend/frontend_test.go index d451af1740b..161d6a6af7c 100644 --- a/pkg/querier/frontend/frontend_test.go +++ b/pkg/querier/frontend/frontend_test.go @@ -20,6 +20,7 @@ import ( "github.com/stretchr/testify/require" jaeger "github.com/uber/jaeger-client-go" "github.com/uber/jaeger-client-go/config" + "github.com/weaveworks/common/httpgrpc" httpgrpc_server "github.com/weaveworks/common/httpgrpc/server" "github.com/weaveworks/common/middleware" "github.com/weaveworks/common/user" @@ -143,6 +144,7 @@ func TestFrontendCancelStatusCode(t *testing.T) { {http.StatusInternalServerError, errors.New("unknown")}, {http.StatusGatewayTimeout, context.DeadlineExceeded}, {499, context.Canceled}, + {http.StatusBadRequest, httpgrpc.Errorf(http.StatusBadRequest, "")}, } { t.Run(test.err.Error(), func(t *testing.T) { w := httptest.NewRecorder() From 8c6877334fedb4fe555df6e1cbca807951aba1f1 Mon Sep 17 00:00:00 2001 From: Cyril Tovena Date: Wed, 19 Feb 2020 08:38:27 -0500 Subject: [PATCH 5/5] Introduce global variable for 499 status code. Signed-off-by: Cyril Tovena --- pkg/querier/frontend/frontend.go | 5 ++++- pkg/querier/frontend/frontend_test.go | 2 +- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/pkg/querier/frontend/frontend.go b/pkg/querier/frontend/frontend.go index 326ba6a616d..2f532daf063 100644 --- a/pkg/querier/frontend/frontend.go +++ b/pkg/querier/frontend/frontend.go @@ -38,8 +38,11 @@ var ( Help: "Number of queries in the queue.", }) + // StatusClientClosedRequest is the status code for when a client request cancellation of an http request + StatusClientClosedRequest = 499 + errTooManyRequest = httpgrpc.Errorf(http.StatusTooManyRequests, "too many outstanding requests") - errCanceled = httpgrpc.Errorf(499, context.Canceled.Error()) + errCanceled = httpgrpc.Errorf(StatusClientClosedRequest, context.Canceled.Error()) errDeadlineExceeded = httpgrpc.Errorf(http.StatusGatewayTimeout, context.DeadlineExceeded.Error()) ) diff --git a/pkg/querier/frontend/frontend_test.go b/pkg/querier/frontend/frontend_test.go index 161d6a6af7c..6e10756c195 100644 --- a/pkg/querier/frontend/frontend_test.go +++ b/pkg/querier/frontend/frontend_test.go @@ -143,7 +143,7 @@ func TestFrontendCancelStatusCode(t *testing.T) { }{ {http.StatusInternalServerError, errors.New("unknown")}, {http.StatusGatewayTimeout, context.DeadlineExceeded}, - {499, context.Canceled}, + {StatusClientClosedRequest, context.Canceled}, {http.StatusBadRequest, httpgrpc.Errorf(http.StatusBadRequest, "")}, } { t.Run(test.err.Error(), func(t *testing.T) {