From 0cad83883a88c949826f3bdd980b5ff856280671 Mon Sep 17 00:00:00 2001 From: Vladimir Smirnov Date: Thu, 10 Aug 2023 23:20:25 +0200 Subject: [PATCH] Revert "find,tags: return detailed (non-internal server) error" This reverts commit 73b1eb298bdcba488391b087ebb91cd24560cd45. Related to #791 --- cmd/carbonapi/http/find_handlers.go | 24 +++++++++----------- cmd/carbonapi/http/helper.go | 5 ----- cmd/carbonapi/http/tags_handler.go | 24 +++++++++----------- zipper/broadcast/broadcast_group.go | 9 ++++---- zipper/helper/requests.go | 35 ----------------------------- zipper/zipper.go | 1 + 6 files changed, 28 insertions(+), 70 deletions(-) diff --git a/cmd/carbonapi/http/find_handlers.go b/cmd/carbonapi/http/find_handlers.go index a36d8ae33..e4141bf4c 100644 --- a/cmd/carbonapi/http/find_handlers.go +++ b/cmd/carbonapi/http/find_handlers.go @@ -23,7 +23,6 @@ import ( "github.com/go-graphite/carbonapi/date" "github.com/go-graphite/carbonapi/intervalset" utilctx "github.com/go-graphite/carbonapi/util/ctx" - "github.com/go-graphite/carbonapi/zipper/helper" ) // Find handler and it's helper functions @@ -176,7 +175,6 @@ func findList(multiGlobs *pbv3.MultiGlobResponse) ([]byte, error) { func findHandler(w http.ResponseWriter, r *http.Request) { t0 := time.Now() uid := uuid.NewV4() - carbonapiUUID := uid.String() // TODO: Migrate to context.WithTimeout // ctx, _ := context.WithTimeout(context.TODO(), config.Config.ZipperTimeout) ctx := utilctx.SetUUID(r.Context(), uid.String()) @@ -199,7 +197,7 @@ func findHandler(w http.ResponseWriter, r *http.Request) { var accessLogDetails = carbonapipb.AccessLogDetails{ Handler: "find", Username: username, - CarbonapiUUID: carbonapiUUID, + CarbonapiUUID: uid.String(), URL: r.URL.RequestURI(), PeerIP: srcIP, PeerPort: srcPort, @@ -216,9 +214,10 @@ 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 - writeErrorResponse(w, int(accessLogDetails.HTTPCode), accessLogDetails.Reason, carbonapiUUID) + logAsError = true return } @@ -241,16 +240,15 @@ func findHandler(w http.ResponseWriter, r *http.Request) { if err != nil { accessLogDetails.HTTPCode = http.StatusBadRequest accessLogDetails.Reason = "failed to parse message body: " + err.Error() - writeErrorResponse(w, http.StatusBadRequest, accessLogDetails.Reason, carbonapiUUID) + http.Error(w, "bad request (failed to parse format): "+err.Error(), http.StatusBadRequest) return } err = pv3Request.Unmarshal(body) if err != nil { - w.Header().Set(ctxHeaderUUID, carbonapiUUID) accessLogDetails.HTTPCode = http.StatusBadRequest accessLogDetails.Reason = "failed to parse message body: " + err.Error() - writeErrorResponse(w, http.StatusBadRequest, accessLogDetails.Reason, carbonapiUUID) + http.Error(w, "bad request (failed to parse format): "+err.Error(), http.StatusBadRequest) return } } else { @@ -260,9 +258,10 @@ 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`" - writeErrorResponse(w, http.StatusBadRequest, accessLogDetails.Reason, carbonapiUUID) + logAsError = true return } @@ -274,7 +273,7 @@ func findHandler(w http.ResponseWriter, r *http.Request) { accessLogDetails.TotalMetricsCount += stats.TotalMetricsCount } if err != nil { - returnCode := merry.HTTPCode(helper.HttpErrorByCode(err)) + returnCode := merry.HTTPCode(err) if returnCode != http.StatusOK || multiGlobs == nil { // Allow override status code for 404-not-found replies. if returnCode == http.StatusNotFound { @@ -284,9 +283,9 @@ 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() - writeErrorResponse(w, returnCode, accessLogDetails.Reason, carbonapiUUID) // 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 @@ -366,13 +365,12 @@ 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() - writeErrorResponse(w, http.StatusInternalServerError, accessLogDetails.Reason, carbonapiUUID) - logAsError = true return } - writeResponse(w, http.StatusOK, b, format, jsonp, carbonapiUUID) + writeResponse(w, http.StatusOK, b, format, jsonp, uid.String()) } diff --git a/cmd/carbonapi/http/helper.go b/cmd/carbonapi/http/helper.go index e7dcab15d..8d239b216 100644 --- a/cmd/carbonapi/http/helper.go +++ b/cmd/carbonapi/http/helper.go @@ -158,11 +158,6 @@ func getFormat(r *http.Request, defaultFormat responseFormat) (responseFormat, b return f, ok, format } -func writeErrorResponse(w http.ResponseWriter, returnCode int, err, carbonapiUUID string) { - w.Header().Set(ctxHeaderUUID, carbonapiUUID) - http.Error(w, err, returnCode) -} - func writeResponse(w http.ResponseWriter, returnCode int, b []byte, format responseFormat, jsonp, carbonapiUUID string) { //TODO: Simplify that switch w.Header().Set(ctxHeaderUUID, carbonapiUUID) diff --git a/cmd/carbonapi/http/tags_handler.go b/cmd/carbonapi/http/tags_handler.go index a5e06d73e..1fbcca3c3 100644 --- a/cmd/carbonapi/http/tags_handler.go +++ b/cmd/carbonapi/http/tags_handler.go @@ -20,16 +20,15 @@ import ( func tagHandler(w http.ResponseWriter, r *http.Request) { t0 := time.Now() - uid := uuid.NewV4() - carbonapiUUID := uid.String() + uuid := uuid.NewV4() // TODO: Migrate to context.WithTimeout - ctx := utilctx.SetUUID(r.Context(), uid.String()) + ctx := utilctx.SetUUID(r.Context(), uuid.String()) requestHeaders := utilctx.GetLogHeaders(ctx) username, _, _ := r.BasicAuth() logger := zapwriter.Logger("tag").With( - zap.String("carbonapi_uuid", uid.String()), + zap.String("carbonapi_uuid", uuid.String()), zap.String("username", username), zap.Any("request_headers", requestHeaders), ) @@ -40,7 +39,7 @@ func tagHandler(w http.ResponseWriter, r *http.Request) { var accessLogDetails = &carbonapipb.AccessLogDetails{ Handler: "tags", Username: username, - CarbonapiUUID: carbonapiUUID, + CarbonapiUUID: uuid.String(), URL: r.URL.Path, PeerIP: srcIP, PeerPort: srcPort, @@ -58,8 +57,8 @@ func tagHandler(w http.ResponseWriter, r *http.Request) { err := r.ParseForm() if err != nil { logAsError = true - accessLogDetails.HTTPCode = int32(http.StatusBadRequest) - writeErrorResponse(w, http.StatusBadRequest, accessLogDetails.Reason, carbonapiUUID) + w.Header().Set("Content-Type", contentTypeJSON) + _, _ = w.Write([]byte{'[', ']'}) return } @@ -88,16 +87,15 @@ func tagHandler(w http.ResponseWriter, r *http.Request) { } else if strings.HasSuffix(r.URL.Path, "values") || strings.HasSuffix(r.URL.Path, "values/") { res, err = config.Config.ZipperInstance.TagValues(ctx, rawQuery, limit) } else { - writeErrorResponse(w, http.StatusNotFound, http.StatusText(http.StatusNotFound), carbonapiUUID) + http.Error(w, http.StatusText(http.StatusNotFound), http.StatusNotFound) accessLogDetails.HTTPCode = http.StatusNotFound return } // TODO(civil): Implement stats if err != nil && !merry.Is(err, types.ErrNoMetricsFetched) && !merry.Is(err, types.ErrNonFatalErrors) { - code := merry.HTTPCode(err) - writeErrorResponse(w, code, http.StatusText(code), carbonapiUUID) - accessLogDetails.HTTPCode = int32(code) + http.Error(w, http.StatusText(http.StatusInternalServerError), http.StatusInternalServerError) + accessLogDetails.HTTPCode = http.StatusInternalServerError accessLogDetails.Reason = err.Error() logAsError = true return @@ -111,7 +109,7 @@ func tagHandler(w http.ResponseWriter, r *http.Request) { } if err != nil { - writeErrorResponse(w, http.StatusInternalServerError, http.StatusText(http.StatusInternalServerError), carbonapiUUID) + http.Error(w, http.StatusText(http.StatusInternalServerError), http.StatusInternalServerError) accessLogDetails.HTTPCode = http.StatusInternalServerError accessLogDetails.Reason = err.Error() logAsError = true @@ -119,7 +117,7 @@ func tagHandler(w http.ResponseWriter, r *http.Request) { } w.Header().Set("Content-Type", contentTypeJSON) - w.Header().Set(ctxHeaderUUID, uid.String()) + w.Header().Set(ctxHeaderUUID, uuid.String()) _, _ = w.Write(b) accessLogDetails.Runtime = time.Since(t0).Seconds() accessLogDetails.HTTPCode = http.StatusOK diff --git a/zipper/broadcast/broadcast_group.go b/zipper/broadcast/broadcast_group.go index f64b3c9b3..35efed554 100644 --- a/zipper/broadcast/broadcast_group.go +++ b/zipper/broadcast/broadcast_group.go @@ -607,8 +607,7 @@ func (bg *BroadcastGroup) Find(ctx context.Context, request *protov3.MultiGlobRe if len(result.Response.Metrics) == 0 { nonNotFoundErrors := types.ReturnNonNotFoundError(result.Err) if nonNotFoundErrors != nil { - code := helper.MergeHttpErrorsCode(result.Err) - err := types.ErrFailedToFetch.WithHTTPCode(code) + err := types.ErrFailedToFetch.WithHTTPCode(500) for _, e := range nonNotFoundErrors { err = err.WithCause(e) } @@ -819,8 +818,10 @@ func (bg *BroadcastGroup) tagEverything(ctx context.Context, isTagName bool, que var err merry.Error if result.Err != nil { - code := helper.MergeHttpErrorsCode(result.Err) - err = types.ErrFailedToFetch.WithHTTPCode(code) + err = types.ErrNonFatalErrors + for _, e := range result.Err { + err = err.WithCause(e) + } } return result.Response, err diff --git a/zipper/helper/requests.go b/zipper/helper/requests.go index 6854493eb..6b047a266 100644 --- a/zipper/helper/requests.go +++ b/zipper/helper/requests.go @@ -98,41 +98,6 @@ func requestError(err error, server string) merry.Error { return types.ErrResponceError.WithValue("server", server) } -func MergeHttpErrorsCode(errors []merry.Error) (returnCode int) { - returnCode = http.StatusNotFound - for _, err := range errors { - c := merry.RootCause(err) - if c == nil { - c = err - } - - code := merry.HTTPCode(err) - if code == http.StatusNotFound { - continue - } 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 { - // simplify code, one error type for communications errors, all we can retry - code = http.StatusServiceUnavailable - } - - if code == http.StatusBadRequest { - // The 400 is returned on wrong requests, e.g. non-existent functions - returnCode = code - } else if returnCode == http.StatusNotFound || code == http.StatusForbidden { - // First error or access denied (may be limits or other) - returnCode = code - } else if code != http.StatusServiceUnavailable { - returnCode = code - } - } - - return returnCode -} - 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 33cf033f7..70d769b7a 100644 --- a/zipper/zipper.go +++ b/zipper/zipper.go @@ -161,6 +161,7 @@ func NewZipper(sender func(*types.Stats), cfg *config.Config, logger *zap.Logger ) } + logger.Error("DEBUG ERROR LOGGGGG", zap.Any("cfg", cfg)) broadcastGroup, err := broadcast.New( broadcast.WithLogger(logger),