From cf75a26014b97ca1dccf3099a30e2f2d0de271f4 Mon Sep 17 00:00:00 2001 From: Vilius Pranckaitis Date: Mon, 30 Nov 2020 13:22:36 +0200 Subject: [PATCH] [query] Fix query metrics assigning invalid status codes for errors --- .../api/experimental/annotated/handler.go | 34 +++++++++++-------- .../api/v1/handler/prometheus/native/read.go | 4 +-- .../handler/prometheus/native/read_common.go | 9 +++++ .../api/v1/handler/prometheus/remote/read.go | 14 ++++++-- .../api/v1/handler/prometheus/remote/write.go | 25 +++++++++----- src/x/net/http/errors.go | 6 ++++ 6 files changed, 65 insertions(+), 27 deletions(-) diff --git a/src/query/api/experimental/annotated/handler.go b/src/query/api/experimental/annotated/handler.go index 4243b91fd6..0625606f90 100644 --- a/src/query/api/experimental/annotated/handler.go +++ b/src/query/api/experimental/annotated/handler.go @@ -72,16 +72,18 @@ func NewHandler( func (h *Handler) ServeHTTP(w http.ResponseWriter, r *http.Request) { if r.Body == nil { - h.metrics.writeErrorsClient.Inc(1) - xhttp.WriteError(w, errEmptyBody) + err := errEmptyBody + h.metrics.incError(err) + xhttp.WriteError(w, err) return } defer r.Body.Close() req, err := parseRequest(r.Body) if err != nil { - h.metrics.writeErrorsClient.Inc(1) - xhttp.WriteError(w, xhttp.NewError(err, http.StatusBadRequest)) + resultError := xhttp.NewError(err, http.StatusBadRequest) + h.metrics.incError(resultError) + xhttp.WriteError(w, resultError) return } @@ -97,20 +99,16 @@ func (h *Handler) ServeHTTP(w http.ResponseWriter, r *http.Request) { break } - var ( - status = http.StatusBadRequest - counter = h.metrics.writeErrorsClient - ) + status := http.StatusBadRequest if foundInternalErr { status = http.StatusInternalServerError - counter = h.metrics.writeErrorsServer } - counter.Inc(1) - err = fmt.Errorf( - "unable to write metric batch, encountered %d errors: %v", len(batchErr.Errors()), batchErr.Error(), - ) - xhttp.WriteError(w, xhttp.NewError(err, status)) + err = fmt.Errorf("unable to write metric batch, encountered %d errors: %w", + len(batchErr.Errors()), batchErr) + responseError := xhttp.NewError(err, status) + h.metrics.incError(responseError) + xhttp.WriteError(w, responseError) return } @@ -150,3 +148,11 @@ func newHandlerMetrics(s tally.Scope) handlerMetrics { writeErrorsClient: s.SubScope("write").Tagged(map[string]string{"code": "4XX"}).Counter("errors"), } } + +func (m *handlerMetrics) incError(err error) { + if xhttp.IsClientError(err) { + m.writeErrorsClient.Inc(1) + } else { + m.writeErrorsServer.Inc(1) + } +} diff --git a/src/query/api/v1/handler/prometheus/native/read.go b/src/query/api/v1/handler/prometheus/native/read.go index 255f6dd69c..d1aa35437e 100644 --- a/src/query/api/v1/handler/prometheus/native/read.go +++ b/src/query/api/v1/handler/prometheus/native/read.go @@ -117,7 +117,7 @@ func (h *promReadHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) { parsedOptions, rErr := ParseRequest(ctx, r, h.instant, h.opts) if rErr != nil { - h.promReadMetrics.fetchErrorsClient.Inc(1) + h.promReadMetrics.incError(rErr) logger.Error("could not parse request", zap.Error(rErr)) xhttp.WriteError(w, rErr) return @@ -143,7 +143,7 @@ func (h *promReadHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) { logger.Error("range query error", zap.Error(err), zap.Any("parsedOptions", parsedOptions)) - h.promReadMetrics.fetchErrorsServer.Inc(1) + h.promReadMetrics.incError(err) xhttp.WriteError(w, err) return diff --git a/src/query/api/v1/handler/prometheus/native/read_common.go b/src/query/api/v1/handler/prometheus/native/read_common.go index 5420693b7b..206b1ccce0 100644 --- a/src/query/api/v1/handler/prometheus/native/read_common.go +++ b/src/query/api/v1/handler/prometheus/native/read_common.go @@ -35,6 +35,7 @@ import ( "github.com/m3db/m3/src/query/storage" "github.com/m3db/m3/src/query/ts" xerrors "github.com/m3db/m3/src/x/errors" + xhttp "github.com/m3db/m3/src/x/net/http" xopentracing "github.com/m3db/m3/src/x/opentracing" opentracinglog "github.com/opentracing/opentracing-go/log" @@ -59,6 +60,14 @@ func newPromReadMetrics(scope tally.Scope) promReadMetrics { } } +func (m *promReadMetrics) incError(err error) { + if xhttp.IsClientError(err) { + m.fetchErrorsClient.Inc(1) + } else { + m.fetchErrorsServer.Inc(1) + } +} + // ReadResponse is the response that gets returned to the user type ReadResponse struct { Results []ts.Series `json:"results,omitempty"` diff --git a/src/query/api/v1/handler/prometheus/remote/read.go b/src/query/api/v1/handler/prometheus/remote/read.go index 83f32b8f27..7bdaf99aaf 100644 --- a/src/query/api/v1/handler/prometheus/remote/read.go +++ b/src/query/api/v1/handler/prometheus/remote/read.go @@ -101,6 +101,14 @@ func newPromReadMetrics(scope tally.Scope) promReadMetrics { } } +func (m *promReadMetrics) incError(err error) { + if xhttp.IsClientError(err) { + m.fetchErrorsClient.Inc(1) + } else { + m.fetchErrorsServer.Inc(1) + } +} + func (h *promReadHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) { timer := h.promReadMetrics.fetchTimerSuccess.Start() defer timer.Stop() @@ -109,7 +117,7 @@ func (h *promReadHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) { logger := logging.WithContext(ctx, h.opts.InstrumentOpts()) req, fetchOpts, rErr := ParseRequest(ctx, r, h.opts) if rErr != nil { - h.promReadMetrics.fetchErrorsClient.Inc(1) + h.promReadMetrics.incError(rErr) logger.Error("remote read query parse error", zap.Error(rErr), zap.Any("req", req), @@ -121,7 +129,7 @@ func (h *promReadHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) { cancelWatcher := handler.NewResponseWriterCanceller(w, h.opts.InstrumentOpts()) readResult, err := Read(ctx, cancelWatcher, req, fetchOpts, h.opts) if err != nil { - h.promReadMetrics.fetchErrorsServer.Inc(1) + h.promReadMetrics.incError(err) logger.Error("remote read query error", zap.Error(err), zap.Any("req", req), @@ -183,7 +191,7 @@ func (h *promReadHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) { } if err != nil { - h.promReadMetrics.fetchErrorsServer.Inc(1) + h.promReadMetrics.incError(err) } else { h.promReadMetrics.fetchSuccess.Inc(1) } diff --git a/src/query/api/v1/handler/prometheus/remote/write.go b/src/query/api/v1/handler/prometheus/remote/write.go index 063670a304..e5a7ff2736 100644 --- a/src/query/api/v1/handler/prometheus/remote/write.go +++ b/src/query/api/v1/handler/prometheus/remote/write.go @@ -196,6 +196,14 @@ type promWriteMetrics struct { forwardLatency tally.Histogram } +func (m *promWriteMetrics) incError(err error) { + if xhttp.IsClientError(err) { + m.writeErrorsClient.Inc(1) + } else { + m.writeErrorsServer.Inc(1) + } +} + func newPromWriteMetrics(scope tally.Scope) (promWriteMetrics, error) { upTo1sBuckets, err := tally.LinearDurationBuckets(0, 100*time.Millisecond, 10) if err != nil { @@ -263,7 +271,7 @@ func (h *PromWriteHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) { checkedReq, err := h.checkedParseRequest(r) if err != nil { - h.metrics.writeErrorsClient.Inc(1) + h.metrics.incError(err) xhttp.WriteError(w, err) return } @@ -359,10 +367,8 @@ func (h *PromWriteHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) { switch { case numBadRequest == len(errs): status = http.StatusBadRequest - h.metrics.writeErrorsClient.Inc(1) default: status = http.StatusInternalServerError - h.metrics.writeErrorsServer.Inc(1) } logger := logging.WithContext(r.Context(), h.instrumentOpts) @@ -374,9 +380,9 @@ func (h *PromWriteHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) { zap.String("lastRegularError", lastRegularErr), zap.String("lastBadRequestErr", lastBadRequestErr)) - var resultErr string + var resultErrMessage string if lastRegularErr != "" { - resultErr = fmt.Sprintf("retryable_errors: count=%d, last=%s", + resultErrMessage = fmt.Sprintf("retryable_errors: count=%d, last=%s", numRegular, lastRegularErr) } if lastBadRequestErr != "" { @@ -384,10 +390,13 @@ func (h *PromWriteHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) { if lastRegularErr != "" { sep = ", " } - resultErr = fmt.Sprintf("%s%sbad_request_errors: count=%d, last=%s", - resultErr, sep, numBadRequest, lastBadRequestErr) + resultErrMessage = fmt.Sprintf("%s%sbad_request_errors: count=%d, last=%s", + resultErrMessage, sep, numBadRequest, lastBadRequestErr) } - xhttp.WriteError(w, xhttp.NewError(errors.New(resultErr), status)) + + resultError := xhttp.NewError(errors.New(resultErrMessage), status) + h.metrics.incError(resultError) + xhttp.WriteError(w, resultError) return } diff --git a/src/x/net/http/errors.go b/src/x/net/http/errors.go index a0fbcde08d..331e23e4d3 100644 --- a/src/x/net/http/errors.go +++ b/src/x/net/http/errors.go @@ -116,3 +116,9 @@ func getStatusCode(err error) int { } return http.StatusInternalServerError } + +// IsClientError returns true if this error would result in 4xx status code +func IsClientError(err error) bool { + code := getStatusCode(err) + return code >= 400 && code < 500 +}