From b35cb293f57476e2ae1abb3bbcb39dee68de6a6c Mon Sep 17 00:00:00 2001 From: Aliaksandr Valialkin Date: Mon, 20 Jul 2020 14:00:33 +0300 Subject: [PATCH] lib/httpserver: log remote address in error message from `httpserver.Errorf` This should improve detection of the root cause of errors. Thanks to Anant for the idea. --- app/vmagent/main.go | 10 +++--- app/vmalert/web.go | 41 ++++++++++++++----------- app/vmauth/main.go | 6 ++-- app/vminsert/main.go | 10 +++--- app/vmselect/main.go | 10 +++--- app/vmstorage/main.go | 2 +- lib/httpserver/httpserver.go | 7 +++-- lib/ingestserver/opentsdbhttp/server.go | 12 ++++---- 8 files changed, 52 insertions(+), 46 deletions(-) diff --git a/app/vmagent/main.go b/app/vmagent/main.go index 6552ca7b07..0e0a1e47f0 100644 --- a/app/vmagent/main.go +++ b/app/vmagent/main.go @@ -136,7 +136,7 @@ func requestHandler(w http.ResponseWriter, r *http.Request) bool { prometheusWriteRequests.Inc() if err := promremotewrite.InsertHandler(r); err != nil { prometheusWriteErrors.Inc() - httpserver.Errorf(w, "error in %q: %s", r.URL.Path, err) + httpserver.Errorf(w, r, "error in %q: %s", r.URL.Path, err) return true } w.WriteHeader(http.StatusNoContent) @@ -145,7 +145,7 @@ func requestHandler(w http.ResponseWriter, r *http.Request) bool { vmimportRequests.Inc() if err := vmimport.InsertHandler(r); err != nil { vmimportErrors.Inc() - httpserver.Errorf(w, "error in %q: %s", r.URL.Path, err) + httpserver.Errorf(w, r, "error in %q: %s", r.URL.Path, err) return true } w.WriteHeader(http.StatusNoContent) @@ -154,7 +154,7 @@ func requestHandler(w http.ResponseWriter, r *http.Request) bool { csvimportRequests.Inc() if err := csvimport.InsertHandler(r); err != nil { csvimportErrors.Inc() - httpserver.Errorf(w, "error in %q: %s", r.URL.Path, err) + httpserver.Errorf(w, r, "error in %q: %s", r.URL.Path, err) return true } w.WriteHeader(http.StatusNoContent) @@ -163,7 +163,7 @@ func requestHandler(w http.ResponseWriter, r *http.Request) bool { prometheusimportRequests.Inc() if err := prometheusimport.InsertHandler(r); err != nil { prometheusimportErrors.Inc() - httpserver.Errorf(w, "error in %q: %s", r.URL.Path, err) + httpserver.Errorf(w, r, "error in %q: %s", r.URL.Path, err) return true } w.WriteHeader(http.StatusNoContent) @@ -172,7 +172,7 @@ func requestHandler(w http.ResponseWriter, r *http.Request) bool { influxWriteRequests.Inc() if err := influx.InsertHandlerForHTTP(r); err != nil { influxWriteErrors.Inc() - httpserver.Errorf(w, "error in %q: %s", r.URL.Path, err) + httpserver.Errorf(w, r, "error in %q: %s", r.URL.Path, err) return true } w.WriteHeader(http.StatusNoContent) diff --git a/app/vmalert/web.go b/app/vmalert/web.go index 07d46beba4..dbd90fa9aa 100644 --- a/app/vmalert/web.go +++ b/app/vmalert/web.go @@ -27,7 +27,6 @@ var pathList = [][]string{ } func (rh *requestHandler) handler(w http.ResponseWriter, r *http.Request) bool { - resph := responseHandler{w} switch r.URL.Path { case "/": for _, path := range pathList { @@ -36,10 +35,22 @@ func (rh *requestHandler) handler(w http.ResponseWriter, r *http.Request) bool { } return true case "/api/v1/groups": - resph.handle(rh.listGroups()) + data, err := rh.listGroups() + if err != nil { + httpserver.Errorf(w, r, "error in %q: %s", r.URL.Path, err) + return true + } + w.Header().Set("Content-Type", "application/json") + w.Write(data) return true case "/api/v1/alerts": - resph.handle(rh.listAlerts()) + data, err := rh.listAlerts() + if err != nil { + httpserver.Errorf(w, r, "error in %q: %s", r.URL.Path, err) + return true + } + w.Header().Set("Content-Type", "application/json") + w.Write(data) return true case "/-/reload": logger.Infof("api config reload was called, sending sighup") @@ -47,12 +58,18 @@ func (rh *requestHandler) handler(w http.ResponseWriter, r *http.Request) bool { w.WriteHeader(http.StatusOK) return true default: + if !strings.HasSuffix(r.URL.Path, "/status") { + return false + } // /api/v1///status - if strings.HasSuffix(r.URL.Path, "/status") { - resph.handle(rh.alert(r.URL.Path)) + data, err := rh.alert(r.URL.Path) + if err != nil { + httpserver.Errorf(w, r, "error in %q: %s", r.URL.Path, err) return true } - return false + w.Header().Set("Content-Type", "application/json") + w.Write(data) + return true } } @@ -151,18 +168,6 @@ func (rh *requestHandler) alert(path string) ([]byte, error) { return json.Marshal(resp) } -// responseHandler wrapper on http.ResponseWriter with sugar -type responseHandler struct{ http.ResponseWriter } - -func (w responseHandler) handle(b []byte, err error) { - if err != nil { - httpserver.Errorf(w, "%s", err) - return - } - w.Header().Set("Content-Type", "application/json") - w.Write(b) -} - func uint64FromPath(path string) (uint64, error) { s := strings.TrimRight(path, "/") return strconv.ParseUint(s, 10, 0) diff --git a/app/vmauth/main.go b/app/vmauth/main.go index 9b58cc5e36..e7b43ef6a1 100644 --- a/app/vmauth/main.go +++ b/app/vmauth/main.go @@ -49,20 +49,20 @@ func main() { func requestHandler(w http.ResponseWriter, r *http.Request) bool { username, password, ok := r.BasicAuth() if !ok { - httpserver.Errorf(w, "Missing `Authorization: Basic *` header") + httpserver.Errorf(w, r, "missing `Authorization: Basic *` header") return true } ac := authConfig.Load().(map[string]*UserInfo) info := ac[username] if info == nil || info.Password != password { - httpserver.Errorf(w, "Cannot find the provided username %q or password in config", username) + httpserver.Errorf(w, r, "cannot find the provided username %q or password in config", username) return true } info.requests.Inc() targetURL := createTargetURL(info.URLPrefix, r.URL) if _, err := url.Parse(targetURL); err != nil { - httpserver.Errorf(w, "Invalid targetURL=%q: %s", targetURL, err) + httpserver.Errorf(w, r, "invalid targetURL=%q: %s", targetURL, err) return true } r.Header.Set("vm-target-url", targetURL) diff --git a/app/vminsert/main.go b/app/vminsert/main.go index 61587a97e5..b581596c14 100644 --- a/app/vminsert/main.go +++ b/app/vminsert/main.go @@ -92,7 +92,7 @@ func RequestHandler(w http.ResponseWriter, r *http.Request) bool { prometheusWriteRequests.Inc() if err := promremotewrite.InsertHandler(r); err != nil { prometheusWriteErrors.Inc() - httpserver.Errorf(w, "error in %q: %s", r.URL.Path, err) + httpserver.Errorf(w, r, "error in %q: %s", r.URL.Path, err) return true } w.WriteHeader(http.StatusNoContent) @@ -101,7 +101,7 @@ func RequestHandler(w http.ResponseWriter, r *http.Request) bool { vmimportRequests.Inc() if err := vmimport.InsertHandler(r); err != nil { vmimportErrors.Inc() - httpserver.Errorf(w, "error in %q: %s", r.URL.Path, err) + httpserver.Errorf(w, r, "error in %q: %s", r.URL.Path, err) return true } w.WriteHeader(http.StatusNoContent) @@ -110,7 +110,7 @@ func RequestHandler(w http.ResponseWriter, r *http.Request) bool { csvimportRequests.Inc() if err := csvimport.InsertHandler(r); err != nil { csvimportErrors.Inc() - httpserver.Errorf(w, "error in %q: %s", r.URL.Path, err) + httpserver.Errorf(w, r, "error in %q: %s", r.URL.Path, err) return true } w.WriteHeader(http.StatusNoContent) @@ -119,7 +119,7 @@ func RequestHandler(w http.ResponseWriter, r *http.Request) bool { prometheusimportRequests.Inc() if err := prometheusimport.InsertHandler(r); err != nil { prometheusimportErrors.Inc() - httpserver.Errorf(w, "error in %q: %s", r.URL.Path, err) + httpserver.Errorf(w, r, "error in %q: %s", r.URL.Path, err) return true } w.WriteHeader(http.StatusNoContent) @@ -128,7 +128,7 @@ func RequestHandler(w http.ResponseWriter, r *http.Request) bool { influxWriteRequests.Inc() if err := influx.InsertHandlerForHTTP(r); err != nil { influxWriteErrors.Inc() - httpserver.Errorf(w, "error in %q: %s", r.URL.Path, err) + httpserver.Errorf(w, r, "error in %q: %s", r.URL.Path, err) return true } w.WriteHeader(http.StatusNoContent) diff --git a/app/vmselect/main.go b/app/vmselect/main.go index 615241bf0b..baca764e21 100644 --- a/app/vmselect/main.go +++ b/app/vmselect/main.go @@ -90,7 +90,7 @@ func RequestHandler(w http.ResponseWriter, r *http.Request) bool { *maxConcurrentRequests, *maxQueueDuration), StatusCode: http.StatusServiceUnavailable, } - httpserver.Errorf(w, "%s", err) + httpserver.Errorf(w, r, "%s", err) return true } } @@ -191,7 +191,7 @@ func RequestHandler(w http.ResponseWriter, r *http.Request) bool { exportRequests.Inc() if err := prometheus.ExportHandler(startTime, w, r); err != nil { exportErrors.Inc() - httpserver.Errorf(w, "error in %q: %s", r.URL.Path, err) + httpserver.Errorf(w, r, "error in %q: %s", r.URL.Path, err) return true } return true @@ -199,7 +199,7 @@ func RequestHandler(w http.ResponseWriter, r *http.Request) bool { federateRequests.Inc() if err := prometheus.FederateHandler(startTime, w, r); err != nil { federateErrors.Inc() - httpserver.Errorf(w, "error in %q: %s", r.URL.Path, err) + httpserver.Errorf(w, r, "error in %q: %s", r.URL.Path, err) return true } return true @@ -225,12 +225,12 @@ func RequestHandler(w http.ResponseWriter, r *http.Request) bool { deleteRequests.Inc() authKey := r.FormValue("authKey") if authKey != *deleteAuthKey { - httpserver.Errorf(w, "invalid authKey %q. It must match the value from -deleteAuthKey command line flag", authKey) + httpserver.Errorf(w, r, "invalid authKey %q. It must match the value from -deleteAuthKey command line flag", authKey) return true } if err := prometheus.DeleteHandler(startTime, r); err != nil { deleteErrors.Inc() - httpserver.Errorf(w, "error in %q: %s", r.URL.Path, err) + httpserver.Errorf(w, r, "error in %q: %s", r.URL.Path, err) return true } w.WriteHeader(http.StatusNoContent) diff --git a/app/vmstorage/main.go b/app/vmstorage/main.go index c400257c14..0400f1a58f 100644 --- a/app/vmstorage/main.go +++ b/app/vmstorage/main.go @@ -181,7 +181,7 @@ func RequestHandler(w http.ResponseWriter, r *http.Request) bool { } authKey := r.FormValue("authKey") if authKey != *snapshotAuthKey { - httpserver.Errorf(w, "invalid authKey %q. It must match the value from -snapshotAuthKey command line flag", authKey) + httpserver.Errorf(w, r, "invalid authKey %q. It must match the value from -snapshotAuthKey command line flag", authKey) return true } path = path[len("/snapshot"):] diff --git a/lib/httpserver/httpserver.go b/lib/httpserver/httpserver.go index e52ea7f2ed..c4fdd37f15 100644 --- a/lib/httpserver/httpserver.go +++ b/lib/httpserver/httpserver.go @@ -172,7 +172,7 @@ func handlerWrapper(s *server, w http.ResponseWriter, r *http.Request, rh Reques requestsTotal.Inc() path, err := getCanonicalPath(r.URL.Path) if err != nil { - Errorf(w, "cannot get canonical path: %s", err) + Errorf(w, r, "cannot get canonical path: %s", err) unsupportedRequestErrors.Inc() return } @@ -238,7 +238,7 @@ func handlerWrapper(s *server, w http.ResponseWriter, r *http.Request, rh Reques return } - Errorf(w, "unsupported path requested: %q", r.URL.Path) + Errorf(w, r, "unsupported path requested: %q", r.URL.Path) unsupportedRequestErrors.Inc() return } @@ -482,8 +482,9 @@ var ( ) // Errorf writes formatted error message to w and to logger. -func Errorf(w http.ResponseWriter, format string, args ...interface{}) { +func Errorf(w http.ResponseWriter, r *http.Request, format string, args ...interface{}) { errStr := fmt.Sprintf(format, args...) + errStr = fmt.Sprintf("remoteAddr: %s; %s", r.RemoteAddr, errStr) logger.WarnfSkipframes(1, "%s", errStr) // Extract statusCode from args diff --git a/lib/ingestserver/opentsdbhttp/server.go b/lib/ingestserver/opentsdbhttp/server.go index d3d9384db5..591ee42328 100644 --- a/lib/ingestserver/opentsdbhttp/server.go +++ b/lib/ingestserver/opentsdbhttp/server.go @@ -28,7 +28,7 @@ type Server struct { // MustStart starts HTTP OpenTSDB server on the given addr. // // MustStop must be called on the returned server when it is no longer needed. -func MustStart(addr string, insertHandler func(req *http.Request) error) *Server { +func MustStart(addr string, insertHandler func(r *http.Request) error) *Server { logger.Infof("starting HTTP OpenTSDB server at %q", addr) lnTCP, err := netutil.NewTCPListener("opentsdbhttp", addr) if err != nil { @@ -40,7 +40,7 @@ func MustStart(addr string, insertHandler func(req *http.Request) error) *Server // MustServe serves OpenTSDB HTTP put requests from ln. // // MustStop must be called on the returned server when it is no longer needed. -func MustServe(ln net.Listener, insertHandler func(req *http.Request) error) *Server { +func MustServe(ln net.Listener, insertHandler func(r *http.Request) error) *Server { h := newRequestHandler(insertHandler) hs := &http.Server{ Handler: h, @@ -84,12 +84,12 @@ func (s *Server) MustStop() { logger.Infof("OpenTSDB HTTP server at %q has been stopped", s.ln.Addr()) } -func newRequestHandler(insertHandler func(req *http.Request) error) http.Handler { - rh := func(w http.ResponseWriter, req *http.Request) { +func newRequestHandler(insertHandler func(r *http.Request) error) http.Handler { + rh := func(w http.ResponseWriter, r *http.Request) { writeRequests.Inc() - if err := insertHandler(req); err != nil { + if err := insertHandler(r); err != nil { writeErrors.Inc() - httpserver.Errorf(w, "error in %q: %s", req.URL.Path, err) + httpserver.Errorf(w, r, "error in %q: %s", r.URL.Path, err) return } w.WriteHeader(http.StatusNoContent)