From aeeb5037ebeaaf69d38da8b5e435711f51c4deea Mon Sep 17 00:00:00 2001 From: Samuel Laferriere Date: Thu, 21 Nov 2024 23:54:15 +0400 Subject: [PATCH] fix(middleware): proper handling of handler status codes in log/metrics middlewares (#200) * fix(middleware): proper handling of handler status codes in log/metrics middlewares * style: remove trailing whitespace (fix lint) * fix(middleware) cert version byte handling * style: make format --- Makefile | 2 +- server/middleware.go | 65 ++++++++++++++++++++++++++++++++------------ server/server.go | 16 +++++++---- 3 files changed, 59 insertions(+), 24 deletions(-) diff --git a/Makefile b/Makefile index 2de9c461..d91bf998 100644 --- a/Makefile +++ b/Makefile @@ -21,7 +21,7 @@ docker-build: @docker build -t ghcr.io/layr-labs/eigenda-proxy:dev . run-memstore-server: - ./bin/eigenda-proxy --memstore.enabled --eigenda.cert-verification-disabled --eigenda.eth-rpc http://localhost:8545 --eigenda.svc-manager-addr 0x123 + ./bin/eigenda-proxy --memstore.enabled --eigenda.cert-verification-disabled --eigenda.eth-rpc http://localhost:8545 --eigenda.svc-manager-addr 0x123 --metrics.enabled disperse-test-blob: curl -X POST -d my-blob-content http://127.0.0.1:3100/put/ diff --git a/server/middleware.go b/server/middleware.go index 65f389bc..68ffdc84 100644 --- a/server/middleware.go +++ b/server/middleware.go @@ -4,6 +4,7 @@ import ( "errors" "fmt" "net/http" + "strconv" "time" "github.com/Layr-Labs/eigenda-proxy/commitments" @@ -11,6 +12,30 @@ import ( "github.com/ethereum/go-ethereum/log" ) +// Used to capture the status code of the response, so that we can use it in middlewares. +// See https://github.com/golang/go/issues/18997 +// TODO: right now instantiating a separate scw for logging and metrics... is there a better way? +type statusCaptureWriter struct { + http.ResponseWriter + status int +} + +func (scw *statusCaptureWriter) WriteHeader(status int) { + scw.status = status + scw.ResponseWriter.WriteHeader(status) +} + +func newStatusCaptureWriter(w http.ResponseWriter) *statusCaptureWriter { + return &statusCaptureWriter{ + ResponseWriter: w, + // 200 status code is only added to response by outer layer http framework, + // since WriteHeader(200) is typically not called by handlers. + // So we initialize status as 200, and assume that any other status code + // will be set by the handler. + status: http.StatusOK, + } +} + // withMetrics is a middleware that records metrics for the route path. // It does not write anything to the response, that is the job of the handlers. func withMetrics( @@ -21,24 +46,25 @@ func withMetrics( return func(w http.ResponseWriter, r *http.Request) error { recordDur := m.RecordRPCServerRequest(r.Method) - err := handleFn(w, r) + scw := newStatusCaptureWriter(w) + err := handleFn(scw, r) if err != nil { + commitMode := "unknown" + certVersion := "unknown" var metaErr MetaError if errors.As(err, &metaErr) { - recordDur(w.Header().Get("status"), string(metaErr.Meta.Mode), string(metaErr.Meta.CertVersion)) - } else { - recordDur(w.Header().Get("status"), string("unknown"), string("unknown")) + commitMode = string(metaErr.Meta.Mode) + certVersion = string(metaErr.Meta.CertVersion) } + recordDur(strconv.Itoa(scw.status), commitMode, certVersion) return err } - // TODO: some routes don't have a version byte, such as /put simple commitments versionByte, err := parseVersionByte(w, r) if err != nil { - // we assume that every route will set the status header - recordDur(w.Header().Get("status"), string(mode), string(versionByte)) - return fmt.Errorf("metrics middleware: error parsing version byte: %w", err) + recordDur(strconv.Itoa(scw.status), string(mode), "unknown") + return fmt.Errorf("metrics middleware: parsing version byte: %w", err) } - recordDur(w.Header().Get("status"), string(mode), string(versionByte)) + recordDur(strconv.Itoa(scw.status), string(mode), string(versionByte)) return nil } } @@ -53,17 +79,20 @@ func withLogging( ) func(http.ResponseWriter, *http.Request) { return func(w http.ResponseWriter, r *http.Request) { start := time.Now() - err := handleFn(w, r) + + scw := newStatusCaptureWriter(w) + err := handleFn(scw, r) + + args := []any{ + "method", r.Method, "url", r.URL, "status", scw.status, "duration", time.Since(start), + } + if err != nil { + args = append(args, "err", err) + } var metaErr MetaError - //nolint:gocritic // ifElseChain is not a good replacement with errors.As if errors.As(err, &metaErr) { - log.Info("request", "method", r.Method, "url", r.URL, "duration", time.Since(start), - "err", err, "status", w.Header().Get("status"), - "commitment_mode", metaErr.Meta.Mode, "cert_version", metaErr.Meta.CertVersion) - } else if err != nil { - log.Info("request", "method", r.Method, "url", r.URL, "duration", time.Since(start), "err", err) - } else { - log.Info("request", "method", r.Method, "url", r.URL, "duration", time.Since(start)) + args = append(args, "commitment_mode", metaErr.Meta.Mode, "cert_version", metaErr.Meta.CertVersion) } + log.Info("request", args...) } } diff --git a/server/server.go b/server/server.go index 2bf7527e..10b7e073 100644 --- a/server/server.go +++ b/server/server.go @@ -115,14 +115,20 @@ func (svr *Server) Port() int { func parseVersionByte(w http.ResponseWriter, r *http.Request) (byte, error) { vars := mux.Vars(r) - // decode version byte - versionByteHex, ok := vars[routingVarNameVersionByteHex] - if !ok { - return 0, fmt.Errorf("version byte not found in path: %s", r.URL.Path) + // only GET routes use gorilla parsed vars to separate header bytes from the raw commitment bytes. + // POST routes parse them by hand because they neeed to send the entire + // request (including the type/version header bytes) to the server. + // TODO: perhaps for consistency we should also use gorilla vars for POST routes, + // and then just reconstruct the full commitment in the handlers? + versionByteHex, isGETRoute := vars[routingVarNameVersionByteHex] + if !isGETRoute { + // v0 is hardcoded in POST routes for now (see handlers.go that also have this hardcoded) + // TODO: change this once we introduce v1/v2 certs + return byte(commitments.CertV0), nil } versionByte, err := hex.DecodeString(versionByteHex) if err != nil { - return 0, fmt.Errorf("failed to decode version byte %s: %w", versionByteHex, err) + return 0, fmt.Errorf("decode version byte %s: %w", versionByteHex, err) } if len(versionByte) != 1 { return 0, fmt.Errorf("version byte is not a single byte: %s", versionByteHex)