Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

perf: use zap's Check() to prevent useless allocs #6560

Merged
merged 7 commits into from
Sep 13, 2024
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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
92 changes: 64 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, "no index file in directory"); 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,20 @@ 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 +476,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 +578,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
Loading