From 35974c81fe283d483f3f1585eba603179650ad3e Mon Sep 17 00:00:00 2001 From: Michail Safronov Date: Wed, 13 Mar 2024 23:06:49 +0500 Subject: [PATCH] fixup! feat(render): return error on partial targets fetch --- cmd/mockbackend/e2etesting.go | 22 ++++++++++++---------- cmd/mockbackend/find.go | 18 ++++++++++-------- zipper/broadcast/broadcast_group.go | 27 ++++++++++++++++++--------- zipper/types/response.go | 10 ++++++---- 4 files changed, 46 insertions(+), 31 deletions(-) diff --git a/cmd/mockbackend/e2etesting.go b/cmd/mockbackend/e2etesting.go index 4baa82205..1cbc378b4 100644 --- a/cmd/mockbackend/e2etesting.go +++ b/cmd/mockbackend/e2etesting.go @@ -212,14 +212,6 @@ func doTest(logger *zap.Logger, t *Query) []error { return failures } - if resp.StatusCode != t.ExpectedResponse.HttpCode { - failures = append(failures, merry2.Errorf("unexpected status code, got %v, expected %v", - resp.StatusCode, - t.ExpectedResponse.HttpCode, - ), - ) - } - contentType = resp.Header.Get("Content-Type") if t.ExpectedResponse.ContentType != contentType { failures = append(failures, @@ -237,6 +229,14 @@ func doTest(logger *zap.Logger, t *Query) []error { return failures } + if resp.StatusCode != t.ExpectedResponse.HttpCode { + failures = append(failures, merry2.Errorf("unexpected status code, got %v, expected %v", + resp.StatusCode, + t.ExpectedResponse.HttpCode, + ), + ) + } + // We don't need to actually check body of response if we expect any sort of error (4xx/5xx) if t.ExpectedResponse.HttpCode >= 300 { return failures @@ -256,7 +256,7 @@ func doTest(logger *zap.Logger, t *Query) []error { } if !sha256matched { encodedBody := base64.StdEncoding.EncodeToString(b) - failures = append(failures, merry2.Errorf("sha256 mismatch, got '%v', expected '%v', encodedBodyy: '%v'", hashStr, t.ExpectedResponse.ExpectedResults[0].SHA256, encodedBody)) + failures = append(failures, merry2.Errorf("sha256 mismatch, got '%v', expected '%v', encodedBody: '%v'", hashStr, t.ExpectedResponse.ExpectedResults[0].SHA256, encodedBody)) return failures } case "application/json": @@ -323,7 +323,9 @@ func doTest(logger *zap.Logger, t *Query) []error { } default: - failures = append(failures, merry2.Errorf("unsupported content-type: got '%v'", contentType)) + if resp.StatusCode == http.StatusOK { + failures = append(failures, merry2.Errorf("unsupported content-type: got '%v'", contentType)) + } } return failures diff --git a/cmd/mockbackend/find.go b/cmd/mockbackend/find.go index e31e78eb5..74ba2729a 100644 --- a/cmd/mockbackend/find.go +++ b/cmd/mockbackend/find.go @@ -46,14 +46,7 @@ func (cfg *listener) findHandler(wr http.ResponseWriter, req *http.Request) { return } - query := req.Form["query"] - - if len(query) == 0 { - logger.Error("Bad request (no query)") - http.Error(wr, "Bad request (no query)", - http.StatusBadRequest) - return - } + var query []string if format == protoV3Format { body, err := io.ReadAll(req.Body) @@ -70,6 +63,15 @@ func (cfg *listener) findHandler(wr http.ResponseWriter, req *http.Request) { _ = pv3Request.Unmarshal(body) query = pv3Request.Metrics + } else { + query = req.Form["query"] + } + + if len(query) == 0 { + logger.Error("Bad request (no query)") + http.Error(wr, "Bad request (no query)", + http.StatusBadRequest) + return } logger.Info("request details", diff --git a/zipper/broadcast/broadcast_group.go b/zipper/broadcast/broadcast_group.go index 322bf49f7..b475a8e24 100644 --- a/zipper/broadcast/broadcast_group.go +++ b/zipper/broadcast/broadcast_group.go @@ -307,12 +307,14 @@ func (bg *BroadcastGroup) doSingleFetch(ctx context.Context, logger *zap.Logger, // TODO(Civil): migrate limiter to merry requests, splitErr := bg.splitRequest(ctx, request, backend) - if len(requests) == 0 && splitErr != nil { - response := types.NewServerFetchResponse() - response.Server = backend.Name() - response.AddError(splitErr) - resCh <- response - return + if len(requests) == 0 { + if splitErr != nil { + response := types.NewServerFetchResponse() + response.Server = backend.Name() + response.AddError(splitErr) + resCh <- response + return + } } logger = logger.With(zap.String("backend_name", backend.Name())) @@ -510,7 +512,7 @@ func (bg *BroadcastGroup) Fetch(ctx context.Context, request *protov3.MultiFetch ) } - if len(result.Response.Metrics) == 0 { + if len(result.Response.Metrics) == 0 || (bg.requireSuccessAll && len(result.Err) > 0) { code, errors := helper.MergeHttpErrors(result.Err) if len(errors) > 0 { err := types.ErrFailedToFetch.WithHTTPCode(code).WithMessage(strings.Join(errors, "\n")) @@ -537,7 +539,7 @@ func (bg *BroadcastGroup) Fetch(ctx context.Context, request *protov3.MultiFetch ) var err merry.Error - if result.Err != nil && len(result.Err) > 0 { + if len(result.Err) > 0 { if bg.requireSuccessAll { code, errors := helper.MergeHttpErrors(result.Err) if len(errors) > 0 { @@ -626,7 +628,7 @@ func (bg *BroadcastGroup) Find(ctx context.Context, request *protov3.MultiGlobRe } var err merry.Error - if result.Err != nil && len(result.Err) > 0 { + if len(result.Response.Metrics) == 0 || (bg.requireSuccessAll && len(result.Err) > 0) { code, errors := helper.MergeHttpErrors(result.Err) if len(errors) > 0 { err = types.ErrFailedToFetch.WithHTTPCode(code).WithMessage(strings.Join(errors, "\n")) @@ -654,6 +656,13 @@ func (bg *BroadcastGroup) Find(ctx context.Context, request *protov3.MultiGlobRe result.Stats.TotalMetricsCount += uint64(len(x.Matches)) } + if result.Err != nil { + err = types.ErrNonFatalErrors + for _, e := range result.Err { + err = err.WithCause(e) + } + } + return result.Response, result.Stats, err } diff --git a/zipper/types/response.go b/zipper/types/response.go index 3a3d6e3d3..9227fdce4 100644 --- a/zipper/types/response.go +++ b/zipper/types/response.go @@ -12,7 +12,7 @@ import ( ) // type Fetcher func(ctx context.Context, logger *zap.Logger, client types.BackendServer, reqs interface{}, resCh chan<- types.ServerFetchResponse) { -//type Fetcher func(ctx context.Context, logger *zap.Logger, client BackendServer, reqs interface{}, resCh chan ServerFetchResponse) { +// type Fetcher func(ctx context.Context, logger *zap.Logger, client BackendServer, reqs interface{}, resCh chan ServerFetchResponse) { type Fetcher func(ctx context.Context, logger *zap.Logger, client BackendServer, reqs interface{}, resCh chan ServerFetcherResponse) type ServerFetcherResponse interface { @@ -52,9 +52,11 @@ GATHER: select { case res := <-resCh: answeredServers[res.GetServer()] = struct{}{} - _ = result.MergeI(res) - responseCount++ - + if err := result.MergeI(res); err == nil { + responseCount++ + } else { + result.AddError(err) + } case <-ctx.Done(): err := ErrTimeoutExceeded.WithValue("timedout_backends", NoAnswerBackends(clients, answeredServers)) result.AddError(err)