From 48f14f25b77184045006c7eab73948890585696e Mon Sep 17 00:00:00 2001 From: Michail Safronov Date: Tue, 23 Jan 2024 11:59:13 +0500 Subject: [PATCH] fix(find): return detailed error instead of 500 --- cmd/carbonapi/http/find_handlers.go | 26 ++--- cmd/mockbackend/e2etesting.go | 92 +++++++++++++----- cmd/mockbackend/find.go | 53 +++++++---- .../testcases/find_error/find_error.yaml | 94 +++++++++++++++++++ .../testcases/render_error_all/carbonapi.yaml | 2 +- zipper/broadcast/broadcast_group.go | 42 ++++----- zipper/helper/requests.go | 26 +++++ zipper/zipper.go | 4 +- 8 files changed, 252 insertions(+), 87 deletions(-) create mode 100644 cmd/mockbackend/testcases/find_error/find_error.yaml diff --git a/cmd/carbonapi/http/find_handlers.go b/cmd/carbonapi/http/find_handlers.go index e4141bf4c..81af0fed7 100644 --- a/cmd/carbonapi/http/find_handlers.go +++ b/cmd/carbonapi/http/find_handlers.go @@ -214,9 +214,7 @@ func findHandler(w http.ResponseWriter, r *http.Request) { }() if !ok || !format.ValidFindFormat() { - http.Error(w, "unsupported format: "+formatRaw, http.StatusBadRequest) - accessLogDetails.HTTPCode = http.StatusBadRequest - accessLogDetails.Reason = "unsupported format: " + formatRaw + setError(w, &accessLogDetails, "unsupported format: "+formatRaw, http.StatusBadRequest, uid.String()) logAsError = true return } @@ -238,17 +236,15 @@ func findHandler(w http.ResponseWriter, r *http.Request) { if format == protoV3Format { body, err := io.ReadAll(r.Body) if err != nil { - accessLogDetails.HTTPCode = http.StatusBadRequest - accessLogDetails.Reason = "failed to parse message body: " + err.Error() - http.Error(w, "bad request (failed to parse format): "+err.Error(), http.StatusBadRequest) + setError(w, &accessLogDetails, "failed to parse message body: "+err.Error(), http.StatusBadRequest, uid.String()) + logAsError = true return } err = pv3Request.Unmarshal(body) if err != nil { - accessLogDetails.HTTPCode = http.StatusBadRequest - accessLogDetails.Reason = "failed to parse message body: " + err.Error() - http.Error(w, "bad request (failed to parse format): "+err.Error(), http.StatusBadRequest) + setError(w, &accessLogDetails, "failed to parse message body: "+err.Error(), http.StatusBadRequest, uid.String()) + logAsError = true return } } else { @@ -258,9 +254,7 @@ func findHandler(w http.ResponseWriter, r *http.Request) { } if len(pv3Request.Metrics) == 0 { - http.Error(w, "missing parameter `query`", http.StatusBadRequest) - accessLogDetails.HTTPCode = http.StatusBadRequest - accessLogDetails.Reason = "missing parameter `query`" + setError(w, &accessLogDetails, "missing parameter `query`", http.StatusBadRequest, uid.String()) logAsError = true return } @@ -283,9 +277,7 @@ func findHandler(w http.ResponseWriter, r *http.Request) { if returnCode < 300 { multiGlobs = &pbv3.MultiGlobResponse{Metrics: []pbv3.GlobResponse{}} } else { - http.Error(w, http.StatusText(returnCode), returnCode) - accessLogDetails.HTTPCode = int32(returnCode) - accessLogDetails.Reason = err.Error() + setError(w, &accessLogDetails, http.StatusText(returnCode), returnCode, uid.String()) // We don't want to log this as an error if it's something normal // Normal is everything that is >= 500. So if config.Config.NotFoundStatusCode is 500 - this will be // logged as error @@ -365,9 +357,7 @@ func findHandler(w http.ResponseWriter, r *http.Request) { } if err != nil { - http.Error(w, http.StatusText(http.StatusInternalServerError), http.StatusInternalServerError) - accessLogDetails.HTTPCode = http.StatusInternalServerError - accessLogDetails.Reason = err.Error() + setError(w, &accessLogDetails, err.Error(), http.StatusInternalServerError, uid.String()) logAsError = true return } diff --git a/cmd/mockbackend/e2etesting.go b/cmd/mockbackend/e2etesting.go index d57312804..4baa82205 100644 --- a/cmd/mockbackend/e2etesting.go +++ b/cmd/mockbackend/e2etesting.go @@ -12,6 +12,7 @@ import ( "net/http" "net/url" "os" + "reflect" "strconv" "strings" "sync" @@ -48,11 +49,21 @@ type ExpectedResponse struct { } type ExpectedResult struct { - SHA256 []string `yaml:"sha256"` - Metrics []CarbonAPIResponse + SHA256 []string `yaml:"sha256"` + Metrics []RenderResponse + MetricsFind []MetricsFindResponse `json:"metricsFind" yaml:"metricsFind"` } -type CarbonAPIResponse struct { +type MetricsFindResponse struct { + AllowChildren int `json:"allowChildren" yaml:"allowChildren"` + Expandable int `json:"expandable" yaml:"expandable"` + Leaf int `json:"leaf" yaml:"leaf"` + Id string `json:"id" yaml:"id"` + Text string `json:"text" yaml:"text"` + Context map[string]string `json:"context" yaml:"context"` +} + +type RenderResponse struct { Target string `json:"target" yaml:"target"` Datapoints []Datapoint `json:"datapoints" yaml:"datapoints"` Tags map[string]string `json:"tags" yaml:"tags"` @@ -121,7 +132,7 @@ func (d *Datapoint) UnmarshalYAML(unmarshal func(interface{}) error) error { return nil } -func isMetricsEqual(m1, m2 CarbonAPIResponse) error { +func isRenderEqual(m1, m2 RenderResponse) error { if m1.Target != m2.Target { return fmt.Errorf("target mismatch, got '%v', expected '%v'", m1.Target, m2.Target) } @@ -249,30 +260,65 @@ func doTest(logger *zap.Logger, t *Query) []error { return failures } case "application/json": - res := make([]CarbonAPIResponse, 0, 1) - err := json.Unmarshal(b, &res) - if err != nil { - err = merry2.Prepend(err, "failed to parse response") - failures = append(failures, err) - return failures - } + if strings.HasPrefix(t.URL, "/metrics/find") { + res := make([]MetricsFindResponse, 0, 1) + err := json.Unmarshal(b, &res) + if err != nil { + err = merry2.Prepend(err, "failed to parse response") + failures = append(failures, err) + return failures + } - if len(t.ExpectedResponse.ExpectedResults) == 0 { - return failures - } + if len(t.ExpectedResponse.ExpectedResults) == 0 { + if len(res) > 0 { + failures = append(failures, merry2.Errorf("unexpected amount of results, got %v, expected 0", len(res))) + } + return failures + } - if len(res) != len(t.ExpectedResponse.ExpectedResults[0].Metrics) { - failures = append(failures, merry2.Errorf("unexpected amount of results, got %v, expected %v", - len(res), - len(t.ExpectedResponse.ExpectedResults[0].Metrics))) - return failures - } + if len(res) != len(t.ExpectedResponse.ExpectedResults[0].MetricsFind) { + failures = append(failures, merry2.Errorf("unexpected amount of results, got %v, expected %v", + len(res), + len(t.ExpectedResponse.ExpectedResults[0].MetricsFind))) + return failures + } - for i := range res { - err := isMetricsEqual(res[i], t.ExpectedResponse.ExpectedResults[0].Metrics[i]) + for i := range res { + if !reflect.DeepEqual(res[i], t.ExpectedResponse.ExpectedResults[0].MetricsFind[i]) { + err = fmt.Errorf("metrics find[%d] are not equal, got=`%+v`, expected=`%+v`", i, res[i], t.ExpectedResponse.ExpectedResults[0].MetricsFind[i]) + failures = append(failures, err) + } + } + } else { + // render + res := make([]RenderResponse, 0, 1) + err := json.Unmarshal(b, &res) if err != nil { - err = merry2.Prependf(err, "metrics are not equal, got=`%+v`, expected=`%+v`", res[i], t.ExpectedResponse.ExpectedResults[0].Metrics[i]) + err = merry2.Prepend(err, "failed to parse response") failures = append(failures, err) + return failures + } + + if len(t.ExpectedResponse.ExpectedResults) == 0 { + if len(res) > 0 { + failures = append(failures, merry2.Errorf("unexpected amount of results, got %v, expected 0", len(res))) + } + return failures + } + + if len(res) != len(t.ExpectedResponse.ExpectedResults[0].Metrics) { + failures = append(failures, merry2.Errorf("unexpected amount of results, got %v, expected %v", + len(res), + len(t.ExpectedResponse.ExpectedResults[0].Metrics))) + return failures + } + + for i := range res { + err := isRenderEqual(res[i], t.ExpectedResponse.ExpectedResults[0].Metrics[i]) + if err != nil { + err = merry2.Prependf(err, "metrics are not equal, got=`%+v`, expected=`%+v`", res[i], t.ExpectedResponse.ExpectedResults[0].Metrics[i]) + failures = append(failures, err) + } } } diff --git a/cmd/mockbackend/find.go b/cmd/mockbackend/find.go index 2c0c14592..e31e78eb5 100644 --- a/cmd/mockbackend/find.go +++ b/cmd/mockbackend/find.go @@ -48,6 +48,13 @@ func (cfg *listener) findHandler(wr http.ResponseWriter, req *http.Request) { query := req.Form["query"] + if len(query) == 0 { + logger.Error("Bad request (no query)") + http.Error(wr, "Bad request (no query)", + http.StatusBadRequest) + return + } + if format == protoV3Format { body, err := io.ReadAll(req.Body) if err != nil { @@ -56,6 +63,7 @@ func (cfg *listener) findHandler(wr http.ResponseWriter, req *http.Request) { ) http.Error(wr, "Bad request (unsupported format)", http.StatusBadRequest) + return } var pv3Request carbonapi_v3_pb.MultiGlobRequest @@ -72,23 +80,7 @@ func (cfg *listener) findHandler(wr http.ResponseWriter, req *http.Request) { Metrics: []carbonapi_v3_pb.GlobResponse{}, } - if query[0] != "*" { - for m := range cfg.Listener.Expressions { - globMatches := []carbonapi_v3_pb.GlobMatch{} - - for _, metric := range cfg.Expressions[m].Data { - globMatches = append(globMatches, carbonapi_v3_pb.GlobMatch{ - Path: metric.MetricName, - IsLeaf: true, - }) - } - multiGlobs.Metrics = append(multiGlobs.Metrics, - carbonapi_v3_pb.GlobResponse{ - Name: cfg.Expressions[m].PathExpression, - Matches: globMatches, - }) - } - } else { + if query[0] == "*" { returnMap := make(map[string]struct{}) for m := range cfg.Listener.Expressions { response := cfg.Expressions[m] @@ -115,6 +107,33 @@ func (cfg *listener) findHandler(wr http.ResponseWriter, req *http.Request) { Name: "*", Matches: globMatches, }) + } else { + for _, m := range query { + globMatches := []carbonapi_v3_pb.GlobMatch{} + if response, ok := cfg.Expressions[m]; ok { + if response.ReplyDelayMS > 0 { + delay := time.Duration(response.ReplyDelayMS) * time.Millisecond + time.Sleep(delay) + } + if response.Code != 0 && response.Code != http.StatusOK { + // return first error + http.Error(wr, http.StatusText(response.Code), response.Code) + return + } + + for _, metric := range cfg.Expressions[m].Data { + globMatches = append(globMatches, carbonapi_v3_pb.GlobMatch{ + Path: metric.MetricName, + IsLeaf: true, + }) + } + multiGlobs.Metrics = append(multiGlobs.Metrics, + carbonapi_v3_pb.GlobResponse{ + Name: cfg.Expressions[m].PathExpression, + Matches: globMatches, + }) + } + } } if cfg.Listener.ShuffleResults { diff --git a/cmd/mockbackend/testcases/find_error/find_error.yaml b/cmd/mockbackend/testcases/find_error/find_error.yaml new file mode 100644 index 000000000..af092e903 --- /dev/null +++ b/cmd/mockbackend/testcases/find_error/find_error.yaml @@ -0,0 +1,94 @@ +version: "v1" +test: + apps: + - name: "carbonapi" + binary: "./carbonapi" + args: + - "-config" + - "./cmd/mockbackend/testcases/render_error/carbonapi.yaml" + queries: + - endpoint: "http://127.0.0.1:8081" + type: "GET" + URL: "/metrics/find?query=a&format=json" + expectedResponse: + httpCode: 200 + contentType: "application/json" + expectedResults: + - metricsFind: + - allowChildren: 0 + expandable: 0 + leaf: 1 + id: "a" + text: "a" + context: {} + + # empty + - endpoint: "http://127.0.0.1:8081" + type: "GET" + URL: "/render/?target=b&format=json" + expectedResponse: + httpCode: 200 + contentType: "application/json" + + - endpoint: "http://127.0.0.1:8081" + type: "GET" + URL: "/metrics/find?query=a&query=b&format=json" + expectedResponse: + httpCode: 200 + contentType: "application/json" + expectedResults: + - metricsFind: + - allowChildren: 0 + expandable: 0 + leaf: 1 + id: "a" + text: "a" + context: {} + + # timeout + - endpoint: "http://127.0.0.1:8081" + type: "GET" + URL: "/metrics/find?query=c&format=json" + expectedResponse: + httpCode: 503 + contentType: "text/plain; charset=utf-8" + + # 503 + - endpoint: "http://127.0.0.1:8081" + type: "GET" + URL: "/metrics/find?query=d&format=json" + expectedResponse: + httpCode: 503 + contentType: "text/plain; charset=utf-8" + + # 503, partial success + - endpoint: "http://127.0.0.1:8081" + type: "GET" + URL: "/metrics/find?query=a&query=d&format=json" + expectedResponse: + httpCode: 503 + contentType: "text/plain; charset=utf-8" + +listeners: + - address: ":9070" + expressions: + "a": + pathExpression: "a" + data: + - metricName: "a" + values: [0,1,2,2,3] + + # timeout + "c": + pathExpression: "b" + emptyBody: true + code: 404 + replyDelayMS: 7000 + data: + - metricName: "c" + values: [0,1,2,2,3] + + "d": + pathExpression: "d" + emptyBody: true + code: 503 diff --git a/cmd/mockbackend/testcases/render_error_all/carbonapi.yaml b/cmd/mockbackend/testcases/render_error_all/carbonapi.yaml index 9fcc83b04..ada9b85ea 100644 --- a/cmd/mockbackend/testcases/render_error_all/carbonapi.yaml +++ b/cmd/mockbackend/testcases/render_error_all/carbonapi.yaml @@ -42,7 +42,7 @@ upstreams: forceAttemptHTTP2: true maxIdleConnsPerHost: 1000 timeouts: - find: "3s" + find: "600s" render: "5s" connect: "200ms" servers: diff --git a/zipper/broadcast/broadcast_group.go b/zipper/broadcast/broadcast_group.go index 125a3ebb2..b41181b4b 100644 --- a/zipper/broadcast/broadcast_group.go +++ b/zipper/broadcast/broadcast_group.go @@ -624,25 +624,19 @@ func (bg *BroadcastGroup) Find(ctx context.Context, request *protov3.MultiGlobRe ) } - if len(result.Response.Metrics) == 0 { - nonNotFoundErrors := types.ReturnNonNotFoundError(result.Err) - if nonNotFoundErrors != nil { - err := types.ErrFailedToFetch.WithHTTPCode(500) - for _, e := range nonNotFoundErrors { - err = err.WithCause(e) - } - logger.Debug("non-404 errors while fetching data from backends", - zap.Any("errors", result.Err), + var err merry.Error + if result.Err != nil && len(result.Err) > 0 { + code, errors := helper.MergeHttpErrors(result.Err) + if len(errors) > 0 { + err = types.ErrFailedToFetch.WithHTTPCode(code).WithMessage(strings.Join(errors, "\n")) + logger.Debug("errors while fetching data from backends", + zap.Int("httpCode", code), + zap.Strings("errors", errors), ) - return &protov3.MultiGlobResponse{}, result.Stats, err + return nil, result.Stats, err } - - return &protov3.MultiGlobResponse{}, result.Stats, types.ErrNotFound.WithHTTPCode(404) - } - result.Stats.TotalMetricsCount = 0 - for _, x := range result.Response.Metrics { - result.Stats.TotalMetricsCount += uint64(len(x.Matches)) } + logger.Debug("got some find responses", zap.Int("backends_count", len(backends)), zap.Int("response_count", responseCount), @@ -651,16 +645,12 @@ func (bg *BroadcastGroup) Find(ctx context.Context, request *protov3.MultiGlobRe zap.Any("response", result.Response), ) - var err merry.Error - if result.Err != nil { - if bg.requireSuccessAll { - err = types.ErrFailedToFetch - } else { - err = types.ErrNonFatalErrors - } - for _, e := range result.Err { - err = err.WithCause(e) - } + if len(result.Response.Metrics) == 0 { + return &protov3.MultiGlobResponse{}, result.Stats, types.ErrNotFound.WithHTTPCode(404) + } + result.Stats.TotalMetricsCount = 0 + for _, x := range result.Response.Metrics { + result.Stats.TotalMetricsCount += uint64(len(x.Matches)) } return result.Response, result.Stats, err diff --git a/zipper/helper/requests.go b/zipper/helper/requests.go index 6b047a266..b31e99a13 100644 --- a/zipper/helper/requests.go +++ b/zipper/helper/requests.go @@ -98,6 +98,32 @@ func requestError(err error, server string) merry.Error { return types.ErrResponceError.WithValue("server", server) } +func HttpErrorCode(err merry.Error) (code int) { + if err == nil { + code = http.StatusOK + } else { + c := merry.RootCause(err) + if c == nil { + c = err + } + + code = merry.HTTPCode(err) + if code == http.StatusNotFound { + return + } else if code == http.StatusInternalServerError && merry.Is(c, parser.ErrInvalidArg) { + // check for invalid args, see applyByNode rewrite function + code = http.StatusBadRequest + } + + if code == http.StatusGatewayTimeout || code == http.StatusBadGateway || merry.Is(c, types.ErrFailedToFetch) { + // simplify code, one error type for communications errors, all we can retry + code = http.StatusServiceUnavailable + } + } + + return +} + func MergeHttpErrors(errors []merry.Error) (int, []string) { returnCode := http.StatusNotFound errMsgs := make([]string, 0) diff --git a/zipper/zipper.go b/zipper/zipper.go index fa1c6ea01..b1c3a4187 100644 --- a/zipper/zipper.go +++ b/zipper/zipper.go @@ -267,9 +267,9 @@ func (z Zipper) FindProtoV3(ctx context.Context, request *protov3.MultiGlobReque if len(findResponse.Err) > 0 { var e merry.Error if len(findResponse.Err) == 1 { - e = findResponse.Err[0] + e = helper.HttpErrorByCode(findResponse.Err[0]) } else { - e = findResponse.Err[1].WithCause(findResponse.Err[0]) + e = helper.HttpErrorByCode(findResponse.Err[1].WithCause(findResponse.Err[0])) } logger.Debug("had errors while fetching result", zap.Any("errors", e),