Frontend: do not return 500 if the user request cancellation. (#2156)

* Frontent does not return 500 if the user request cancellation.

Signed-off-by: Cyril Tovena <[email protected]>

Author: cyriltovena

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. #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

pkg/querier/frontend/frontend.go

@@ -38,8 +38,12 @@ var (
 		Help:      "Number of queries in the queue.",
 	})
 
-	errTooManyRequest = httpgrpc.Errorf(http.StatusTooManyRequests, "too many outstanding requests")
-	errCanceled       = httpgrpc.Errorf(http.StatusInternalServerError, "context cancelled")
+	// 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(StatusClientClosedRequest, context.Canceled.Error())
+	errDeadlineExceeded = httpgrpc.Errorf(http.StatusGatewayTimeout, context.DeadlineExceeded.Error())
 )
 
 // Config for a Frontend.
@@ -151,14 +155,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 +174,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 = errCanceled
+	case context.DeadlineExceeded:
+		err = errDeadlineExceeded
+	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 +245,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

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"
@@ -18,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"
@@ -133,6 +136,24 @@ 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},
+		{StatusClientClosedRequest, context.Canceled},
+		{http.StatusBadRequest, httpgrpc.Errorf(http.StatusBadRequest, "")},
+	} {
+		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)