Skip to content

Commit

Permalink
perf: use zap's Check() to prevent useless allocs (#6560)
Browse files Browse the repository at this point in the history
* perf: use zap's Check() to prevent useless allocs

* fix

* fix

* fix

* fix

* restore previous replacer behavior

* fix linter
  • Loading branch information
dunglas authored Sep 13, 2024
1 parent 21f9c20 commit f4bf4e0
Show file tree
Hide file tree
Showing 30 changed files with 599 additions and 282 deletions.
7 changes: 4 additions & 3 deletions modules/caddyhttp/caddyauth/caddyauth.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import (
"net/http"

"go.uber.org/zap"
"go.uber.org/zap/zapcore"

"github.com/caddyserver/caddy/v2"
"github.com/caddyserver/caddy/v2/modules/caddyhttp"
Expand Down Expand Up @@ -76,9 +77,9 @@ func (a Authentication) ServeHTTP(w http.ResponseWriter, r *http.Request, next c
for provName, prov := range a.Providers {
user, authed, err = prov.Authenticate(w, r)
if err != nil {
a.logger.Error("auth provider returned error",
zap.String("provider", provName),
zap.Error(err))
if c := a.logger.Check(zapcore.ErrorLevel, "auth provider returned error"); c != nil {
c.Write(zap.String("provider", provName), zap.Error(err))
}
continue
}
if authed {
Expand Down
11 changes: 7 additions & 4 deletions modules/caddyhttp/fileserver/browse.go
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ import (
"time"

"go.uber.org/zap"
"go.uber.org/zap/zapcore"

"github.com/caddyserver/caddy/v2"
"github.com/caddyserver/caddy/v2/modules/caddyhttp"
Expand Down Expand Up @@ -68,9 +69,9 @@ type Browse struct {
}

func (fsrv *FileServer) serveBrowse(fileSystem fs.FS, root, dirPath string, w http.ResponseWriter, r *http.Request, next caddyhttp.Handler) error {
fsrv.logger.Debug("browse enabled; listing directory contents",
zap.String("path", dirPath),
zap.String("root", root))
if c := fsrv.logger.Check(zapcore.DebugLevel, "browse enabled; listing directory contents"); c != nil {
c.Write(zap.String("path", dirPath), zap.String("root", root))
}

// Navigation on the client-side gets messed up if the
// URL doesn't end in a trailing slash because hrefs to
Expand All @@ -92,7 +93,9 @@ func (fsrv *FileServer) serveBrowse(fileSystem fs.FS, root, dirPath string, w ht
origReq := r.Context().Value(caddyhttp.OriginalRequestCtxKey).(http.Request)
if r.URL.Path == "" || path.Base(origReq.URL.Path) == path.Base(r.URL.Path) {
if !strings.HasSuffix(origReq.URL.Path, "/") {
fsrv.logger.Debug("redirecting to trailing slash to preserve hrefs", zap.String("request_path", r.URL.Path))
if c := fsrv.logger.Check(zapcore.DebugLevel, "redirecting to trailing slash to preserve hrefs"); c != nil {
c.Write(zap.String("request_path", r.URL.Path))
}
return redirect(w, r, origReq.URL.Path+"/")
}
}
Expand Down
7 changes: 4 additions & 3 deletions modules/caddyhttp/fileserver/browsetplcontext.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import (

"github.com/dustin/go-humanize"
"go.uber.org/zap"
"go.uber.org/zap/zapcore"

"github.com/caddyserver/caddy/v2"
"github.com/caddyserver/caddy/v2/modules/caddyhttp"
Expand Down Expand Up @@ -57,9 +58,9 @@ func (fsrv *FileServer) directoryListing(ctx context.Context, fileSystem fs.FS,

info, err := entry.Info()
if err != nil {
fsrv.logger.Error("could not get info about directory entry",
zap.String("name", entry.Name()),
zap.String("root", root))
if c := fsrv.logger.Check(zapcore.ErrorLevel, "could not get info about directory entry"); c != nil {
c.Write(zap.String("name", entry.Name()), zap.String("root", root))
}
continue
}

Expand Down
14 changes: 11 additions & 3 deletions modules/caddyhttp/fileserver/matcher.go
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ import (
"github.com/google/cel-go/common/types/ref"
"github.com/google/cel-go/parser"
"go.uber.org/zap"
"go.uber.org/zap/zapcore"

"github.com/caddyserver/caddy/v2"
"github.com/caddyserver/caddy/v2/caddyconfig/caddyfile"
Expand Down Expand Up @@ -326,7 +327,9 @@ func (m MatchFile) selectFile(r *http.Request) (matched bool) {

fileSystem, ok := m.fsmap.Get(fsName)
if !ok {
m.logger.Error("use of unregistered filesystem", zap.String("fs", fsName))
if c := m.logger.Check(zapcore.ErrorLevel, "use of unregistered filesystem"); c != nil {
c.Write(zap.String("fs", fsName))
}
return false
}
type matchCandidate struct {
Expand Down Expand Up @@ -356,7 +359,10 @@ func (m MatchFile) selectFile(r *http.Request) (matched bool) {
return val, nil
})
if err != nil {
m.logger.Error("evaluating placeholders", zap.Error(err))
if c := m.logger.Check(zapcore.ErrorLevel, "evaluating placeholders"); c != nil {
c.Write(zap.Error(err))
}

expandedFile = file // "oh well," I guess?
}

Expand All @@ -379,7 +385,9 @@ func (m MatchFile) selectFile(r *http.Request) (matched bool) {
} else {
globResults, err = fs.Glob(fileSystem, fullPattern)
if err != nil {
m.logger.Error("expanding glob", zap.Error(err))
if c := m.logger.Check(zapcore.ErrorLevel, "expanding glob"); c != nil {
c.Write(zap.Error(err))
}
}
}

Expand Down
91 changes: 63 additions & 28 deletions modules/caddyhttp/fileserver/staticfiles.go
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ import (
"strings"

"go.uber.org/zap"
"go.uber.org/zap/zapcore"

"github.com/caddyserver/caddy/v2"
"github.com/caddyserver/caddy/v2/modules/caddyhttp"
Expand Down Expand Up @@ -286,11 +287,14 @@ func (fsrv *FileServer) ServeHTTP(w http.ResponseWriter, r *http.Request, next c
// remove any trailing `/` as it breaks fs.ValidPath() in the stdlib
filename := strings.TrimSuffix(caddyhttp.SanitizedPathJoin(root, r.URL.Path), "/")

fsrv.logger.Debug("sanitized path join",
zap.String("site_root", root),
zap.String("fs", fsName),
zap.String("request_path", r.URL.Path),
zap.String("result", filename))
if c := fsrv.logger.Check(zapcore.DebugLevel, "sanitized path join"); c != nil {
c.Write(
zap.String("site_root", root),
zap.String("fs", fsName),
zap.String("request_path", r.URL.Path),
zap.String("result", filename),
)
}

// get information about the file
info, err := fs.Stat(fileSystem, filename)
Expand All @@ -313,9 +317,12 @@ func (fsrv *FileServer) ServeHTTP(w http.ResponseWriter, r *http.Request, next c
indexPath := caddyhttp.SanitizedPathJoin(filename, indexPage)
if fileHidden(indexPath, filesToHide) {
// pretend this file doesn't exist
fsrv.logger.Debug("hiding index file",
zap.String("filename", indexPath),
zap.Strings("files_to_hide", filesToHide))
if c := fsrv.logger.Check(zapcore.DebugLevel, "hiding index file"); c != nil {
c.Write(
zap.String("filename", indexPath),
zap.Strings("files_to_hide", filesToHide),
)
}
continue
}

Expand All @@ -335,17 +342,22 @@ func (fsrv *FileServer) ServeHTTP(w http.ResponseWriter, r *http.Request, next c
info = indexInfo
filename = indexPath
implicitIndexFile = true
fsrv.logger.Debug("located index file", zap.String("filename", filename))
if c := fsrv.logger.Check(zapcore.DebugLevel, "located index file"); c != nil {
c.Write(zap.String("filename", filename))
}
break
}
}

// if still referencing a directory, delegate
// to browse or return an error
if info.IsDir() {
fsrv.logger.Debug("no index file in directory",
zap.String("path", filename),
zap.Strings("index_filenames", fsrv.IndexNames))
if c := fsrv.logger.Check(zapcore.DebugLevel, "no index file in directory"); c != nil {
c.Write(
zap.String("path", filename),
zap.Strings("index_filenames", fsrv.IndexNames),
)
}
if fsrv.Browse != nil && !fileHidden(filename, filesToHide) {
return fsrv.serveBrowse(fileSystem, root, filename, w, r, next)
}
Expand All @@ -355,9 +367,12 @@ func (fsrv *FileServer) ServeHTTP(w http.ResponseWriter, r *http.Request, next c
// one last check to ensure the file isn't hidden (we might
// have changed the filename from when we last checked)
if fileHidden(filename, filesToHide) {
fsrv.logger.Debug("hiding file",
zap.String("filename", filename),
zap.Strings("files_to_hide", filesToHide))
if c := fsrv.logger.Check(zapcore.DebugLevel, "hiding file"); c != nil {
c.Write(
zap.String("filename", filename),
zap.Strings("files_to_hide", filesToHide),
)
}
return fsrv.notFound(w, r, next)
}

Expand All @@ -375,15 +390,21 @@ func (fsrv *FileServer) ServeHTTP(w http.ResponseWriter, r *http.Request, next c
if path.Base(origReq.URL.Path) == path.Base(r.URL.Path) {
if implicitIndexFile && !strings.HasSuffix(origReq.URL.Path, "/") {
to := origReq.URL.Path + "/"
fsrv.logger.Debug("redirecting to canonical URI (adding trailing slash for directory)",
zap.String("from_path", origReq.URL.Path),
zap.String("to_path", to))
if c := fsrv.logger.Check(zapcore.DebugLevel, "redirecting to canonical URI (adding trailing slash for directory"); c != nil {
c.Write(
zap.String("from_path", origReq.URL.Path),
zap.String("to_path", to),
)
}
return redirect(w, r, to)
} else if !implicitIndexFile && strings.HasSuffix(origReq.URL.Path, "/") {
to := origReq.URL.Path[:len(origReq.URL.Path)-1]
fsrv.logger.Debug("redirecting to canonical URI (removing trailing slash for file)",
zap.String("from_path", origReq.URL.Path),
zap.String("to_path", to))
if c := fsrv.logger.Check(zapcore.DebugLevel, "redirecting to canonical URI (removing trailing slash for file"); c != nil {
c.Write(
zap.String("from_path", origReq.URL.Path),
zap.String("to_path", to),
)
}
return redirect(w, r, to)
}
}
Expand Down Expand Up @@ -411,13 +432,19 @@ func (fsrv *FileServer) ServeHTTP(w http.ResponseWriter, r *http.Request, next c
compressedFilename := filename + precompress.Suffix()
compressedInfo, err := fs.Stat(fileSystem, compressedFilename)
if err != nil || compressedInfo.IsDir() {
fsrv.logger.Debug("precompressed file not accessible", zap.String("filename", compressedFilename), zap.Error(err))
if c := fsrv.logger.Check(zapcore.DebugLevel, "precompressed file not accessible"); c != nil {
c.Write(zap.String("filename", compressedFilename), zap.Error(err))
}
continue
}
fsrv.logger.Debug("opening compressed sidecar file", zap.String("filename", compressedFilename), zap.Error(err))
if c := fsrv.logger.Check(zapcore.DebugLevel, "opening compressed sidecar file"); c != nil {
c.Write(zap.String("filename", compressedFilename), zap.Error(err))
}
file, err = fsrv.openFile(fileSystem, compressedFilename, w)
if err != nil {
fsrv.logger.Warn("opening precompressed file failed", zap.String("filename", compressedFilename), zap.Error(err))
if c := fsrv.logger.Check(zapcore.WarnLevel, "opening precompressed file failed"); c != nil {
c.Write(zap.String("filename", compressedFilename), zap.Error(err))
}
if caddyErr, ok := err.(caddyhttp.HandlerError); ok && caddyErr.StatusCode == http.StatusServiceUnavailable {
return err
}
Expand Down Expand Up @@ -448,7 +475,9 @@ func (fsrv *FileServer) ServeHTTP(w http.ResponseWriter, r *http.Request, next c

// no precompressed file found, use the actual file
if file == nil {
fsrv.logger.Debug("opening file", zap.String("filename", filename))
if c := fsrv.logger.Check(zapcore.DebugLevel, "opening file"); c != nil {
c.Write(zap.String("filename", filename))
}

// open the file
file, err = fsrv.openFile(fileSystem, filename, w)
Expand Down Expand Up @@ -548,18 +577,24 @@ func (fsrv *FileServer) openFile(fileSystem fs.FS, filename string, w http.Respo
if err != nil {
err = fsrv.mapDirOpenError(fileSystem, err, filename)
if errors.Is(err, fs.ErrNotExist) {
fsrv.logger.Debug("file not found", zap.String("filename", filename), zap.Error(err))
if c := fsrv.logger.Check(zapcore.DebugLevel, "file not found"); c != nil {
c.Write(zap.String("filename", filename), zap.Error(err))
}
return nil, caddyhttp.Error(http.StatusNotFound, err)
} else if errors.Is(err, fs.ErrPermission) {
fsrv.logger.Debug("permission denied", zap.String("filename", filename), zap.Error(err))
if c := fsrv.logger.Check(zapcore.DebugLevel, "permission denied"); c != nil {
c.Write(zap.String("filename", filename), zap.Error(err))
}
return nil, caddyhttp.Error(http.StatusForbidden, err)
}
// maybe the server is under load and ran out of file descriptors?
// have client wait arbitrary seconds to help prevent a stampede
//nolint:gosec
backoff := weakrand.Intn(maxBackoff-minBackoff) + minBackoff
w.Header().Set("Retry-After", strconv.Itoa(backoff))
fsrv.logger.Debug("retry after backoff", zap.String("filename", filename), zap.Int("backoff", backoff), zap.Error(err))
if c := fsrv.logger.Check(zapcore.DebugLevel, "retry after backoff"); c != nil {
c.Write(zap.String("filename", filename), zap.Int("backoff", backoff), zap.Error(err))
}
return nil, caddyhttp.Error(http.StatusServiceUnavailable, err)
}
return file, nil
Expand Down
5 changes: 4 additions & 1 deletion modules/caddyhttp/intercept/intercept.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import (
"sync"

"go.uber.org/zap"
"go.uber.org/zap/zapcore"

"github.com/caddyserver/caddy/v2"
"github.com/caddyserver/caddy/v2/caddyconfig/caddyfile"
Expand Down Expand Up @@ -165,7 +166,9 @@ func (ir Intercept) ServeHTTP(w http.ResponseWriter, r *http.Request, next caddy
}
repl.Set("http.intercept.status_code", rec.Status())

ir.logger.Debug("handling response", zap.Int("handler", rec.handlerIndex))
if c := ir.logger.Check(zapcore.DebugLevel, "handling response"); c != nil {
c.Write(zap.Int("handler", rec.handlerIndex))
}

// pass the request through the response handler routes
return rec.handler.Routes.Compile(next).ServeHTTP(w, r)
Expand Down
10 changes: 8 additions & 2 deletions modules/caddyhttp/ip_matchers.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import (
"github.com/google/cel-go/cel"
"github.com/google/cel-go/common/types/ref"
"go.uber.org/zap"
"go.uber.org/zap/zapcore"

"github.com/caddyserver/caddy/v2"
"github.com/caddyserver/caddy/v2/caddyconfig/caddyfile"
Expand Down Expand Up @@ -150,12 +151,17 @@ func (m MatchRemoteIP) Match(r *http.Request) bool {
address := r.RemoteAddr
clientIP, zoneID, err := parseIPZoneFromString(address)
if err != nil {
m.logger.Error("getting remote IP", zap.Error(err))
if c := m.logger.Check(zapcore.ErrorLevel, "getting remote "); c != nil {
c.Write(zap.Error(err))
}

return false
}
matches, zoneFilter := matchIPByCidrZones(clientIP, zoneID, m.cidrs, m.zones)
if !matches && !zoneFilter {
m.logger.Debug("zone ID from remote IP did not match", zap.String("zone", zoneID))
if c := m.logger.Check(zapcore.DebugLevel, "zone ID from remote IP did not match"); c != nil {
c.Write(zap.String("zone", zoneID))
}
}
return matches
}
Expand Down
12 changes: 7 additions & 5 deletions modules/caddyhttp/logging.go
Original file line number Diff line number Diff line change
Expand Up @@ -193,7 +193,7 @@ func (sa *StringArray) UnmarshalJSON(b []byte) error {
// to use, the error log message, and any extra fields.
// If err is a HandlerError, the returned values will
// have richer information.
func errLogValues(err error) (status int, msg string, fields []zapcore.Field) {
func errLogValues(err error) (status int, msg string, fields func() []zapcore.Field) {
var handlerErr HandlerError
if errors.As(err, &handlerErr) {
status = handlerErr.StatusCode
Expand All @@ -202,10 +202,12 @@ func errLogValues(err error) (status int, msg string, fields []zapcore.Field) {
} else {
msg = handlerErr.Err.Error()
}
fields = []zapcore.Field{
zap.Int("status", handlerErr.StatusCode),
zap.String("err_id", handlerErr.ID),
zap.String("err_trace", handlerErr.Trace),
fields = func() []zapcore.Field {
return []zapcore.Field{
zap.Int("status", handlerErr.StatusCode),
zap.String("err_id", handlerErr.ID),
zap.String("err_trace", handlerErr.Trace),
}
}
return
}
Expand Down
Loading

0 comments on commit f4bf4e0

Please sign in to comment.