From d5c6e6c7a869cd6378bd826f888c9855c7c5cb76 Mon Sep 17 00:00:00 2001 From: Mohammed Al Sahaf Date: Sat, 5 Oct 2024 20:19:29 +0300 Subject: [PATCH 1/2] metrics: move `metrics` up, outside `servers` This change moves the metrics configuration from per-server level to a single config knob within the `http` app. Enabling `metrics` in any of the configured servers inside `http` enables metrics for all servers. Fix #6604 Signed-off-by: Mohammed Al Sahaf --- caddyconfig/httpcaddyfile/httptype.go | 14 +++++++ caddyconfig/httpcaddyfile/options.go | 20 ++++++++++ caddyconfig/httpcaddyfile/serveroptions.go | 1 + .../metrics_merge_options.caddyfiletest | 39 +++++++++++++++++++ .../metrics_perhost.caddyfiletest | 8 ++-- modules/caddyhttp/app.go | 27 +++++++++---- modules/caddyhttp/server.go | 1 + 7 files changed, 99 insertions(+), 11 deletions(-) create mode 100644 caddytest/integration/caddyfile_adapt/metrics_merge_options.caddyfiletest diff --git a/caddyconfig/httpcaddyfile/httptype.go b/caddyconfig/httpcaddyfile/httptype.go index 4dacd9058d0..a238a33b42f 100644 --- a/caddyconfig/httpcaddyfile/httptype.go +++ b/caddyconfig/httpcaddyfile/httptype.go @@ -15,6 +15,7 @@ package httpcaddyfile import ( + "cmp" "encoding/json" "fmt" "net" @@ -186,12 +187,25 @@ func (st ServerType) Setup( return nil, warnings, err } + // hoist the metrics config from per-server to global + metrics, _ := options["metrics"].(*caddyhttp.Metrics) + for _, s := range servers { + if s.Metrics != nil { + metrics = cmp.Or[*caddyhttp.Metrics](metrics, &caddyhttp.Metrics{}) + metrics = &caddyhttp.Metrics{ + PerHost: metrics.PerHost || s.Metrics.PerHost, + } + s.Metrics = nil // we don't need it anymore + } + } + // now that each server is configured, make the HTTP app httpApp := caddyhttp.App{ HTTPPort: tryInt(options["http_port"], &warnings), HTTPSPort: tryInt(options["https_port"], &warnings), GracePeriod: tryDuration(options["grace_period"], &warnings), ShutdownDelay: tryDuration(options["shutdown_delay"], &warnings), + Metrics: metrics, Servers: servers, } diff --git a/caddyconfig/httpcaddyfile/options.go b/caddyconfig/httpcaddyfile/options.go index abbe8f4185e..4bdafb02738 100644 --- a/caddyconfig/httpcaddyfile/options.go +++ b/caddyconfig/httpcaddyfile/options.go @@ -24,6 +24,7 @@ import ( "github.com/caddyserver/caddy/v2" "github.com/caddyserver/caddy/v2/caddyconfig" "github.com/caddyserver/caddy/v2/caddyconfig/caddyfile" + "github.com/caddyserver/caddy/v2/modules/caddyhttp" "github.com/caddyserver/caddy/v2/modules/caddytls" ) @@ -53,6 +54,7 @@ func init() { RegisterGlobalOption("local_certs", parseOptTrue) RegisterGlobalOption("key_type", parseOptSingleString) RegisterGlobalOption("auto_https", parseOptAutoHTTPS) + RegisterGlobalOption("metrics", parseMetricsOptions) RegisterGlobalOption("servers", parseServerOptions) RegisterGlobalOption("ocsp_stapling", parseOCSPStaplingOptions) RegisterGlobalOption("cert_lifetime", parseOptDuration) @@ -472,6 +474,24 @@ func parseOptAutoHTTPS(d *caddyfile.Dispenser, _ any) (any, error) { return val, nil } +func unmarshalCaddyfileMetricsOptions(d *caddyfile.Dispenser) (any, error) { + d.Next() // consume option name + metrics := new(caddyhttp.Metrics) + for d.NextBlock(0) { + switch d.Val() { + case "per_host": + metrics.PerHost = true + default: + return nil, d.Errf("unrecognized servers option '%s'", d.Val()) + } + } + return metrics, nil +} + +func parseMetricsOptions(d *caddyfile.Dispenser, _ any) (any, error) { + return unmarshalCaddyfileMetricsOptions(d) +} + func parseServerOptions(d *caddyfile.Dispenser, _ any) (any, error) { return unmarshalCaddyfileServerOptions(d) } diff --git a/caddyconfig/httpcaddyfile/serveroptions.go b/caddyconfig/httpcaddyfile/serveroptions.go index b05af47f547..40a8af20962 100644 --- a/caddyconfig/httpcaddyfile/serveroptions.go +++ b/caddyconfig/httpcaddyfile/serveroptions.go @@ -240,6 +240,7 @@ func unmarshalCaddyfileServerOptions(d *caddyfile.Dispenser) (any, error) { } case "metrics": + caddy.Log().Warn("The nested 'metrics' option inside `servers` is deprecated and will be removed in the next major version. Use the global 'metrics' option instead.") serverOpts.Metrics = new(caddyhttp.Metrics) for nesting := d.Nesting(); d.NextBlock(nesting); { switch d.Val() { diff --git a/caddytest/integration/caddyfile_adapt/metrics_merge_options.caddyfiletest b/caddytest/integration/caddyfile_adapt/metrics_merge_options.caddyfiletest new file mode 100644 index 00000000000..946b3d0c83a --- /dev/null +++ b/caddytest/integration/caddyfile_adapt/metrics_merge_options.caddyfiletest @@ -0,0 +1,39 @@ +{ + metrics + servers :80 { + metrics { + per_host + } + } +} +:80 { + respond "Hello" +} + +---------- +{ + "apps": { + "http": { + "servers": { + "srv0": { + "listen": [ + ":80" + ], + "routes": [ + { + "handle": [ + { + "body": "Hello", + "handler": "static_response" + } + ] + } + ] + } + }, + "metrics": { + "per_host": true + } + } + } +} \ No newline at end of file diff --git a/caddytest/integration/caddyfile_adapt/metrics_perhost.caddyfiletest b/caddytest/integration/caddyfile_adapt/metrics_perhost.caddyfiletest index 49921551573..e362cecc93e 100644 --- a/caddytest/integration/caddyfile_adapt/metrics_perhost.caddyfiletest +++ b/caddytest/integration/caddyfile_adapt/metrics_perhost.caddyfiletest @@ -26,11 +26,11 @@ } ] } - ], - "metrics": { - "per_host": true - } + ] } + }, + "metrics": { + "per_host": true } } } diff --git a/modules/caddyhttp/app.go b/modules/caddyhttp/app.go index 7dc2bee72fa..c57e14e9992 100644 --- a/modules/caddyhttp/app.go +++ b/modules/caddyhttp/app.go @@ -15,6 +15,7 @@ package caddyhttp import ( + "cmp" "context" "crypto/tls" "fmt" @@ -142,6 +143,10 @@ type App struct { // affect functionality. Servers map[string]*Server `json:"servers,omitempty"` + // If set, metrics observations will be enabled. + // This setting is EXPERIMENTAL and subject to change. + Metrics *Metrics `json:"metrics,omitempty"` + ctx caddy.Context logger *zap.Logger tlsApp *caddytls.TLS @@ -184,6 +189,10 @@ func (app *App) Provision(ctx caddy.Context) error { return err } + if app.Metrics != nil { + app.Metrics.init = sync.Once{} + app.Metrics.httpMetrics = &httpMetrics{} + } // prepare each server oldContext := ctx.Context for srvName, srv := range app.Servers { @@ -196,6 +205,15 @@ func (app *App) Provision(ctx caddy.Context) error { srv.errorLogger = app.logger.Named("log.error") srv.shutdownAtMu = new(sync.RWMutex) + if srv.Metrics != nil { + srv.logger.Warn("per-server 'metrics' is deprecated; use 'metrics' in the root 'http' app instead") + app.Metrics = cmp.Or[*Metrics](app.Metrics, &Metrics{ + init: sync.Once{}, + httpMetrics: &httpMetrics{}, + }) + app.Metrics.PerHost = app.Metrics.PerHost || srv.Metrics.PerHost + } + // only enable access logs if configured if srv.Logs != nil { srv.accessLogger = app.logger.Named("log.access") @@ -342,16 +360,11 @@ func (app *App) Provision(ctx caddy.Context) error { srv.listenerWrappers = append([]caddy.ListenerWrapper{new(tlsPlaceholderWrapper)}, srv.listenerWrappers...) } } - // pre-compile the primary handler chain, and be sure to wrap it in our // route handler so that important security checks are done, etc. primaryRoute := emptyHandler if srv.Routes != nil { - if srv.Metrics != nil { - srv.Metrics.init = sync.Once{} - srv.Metrics.httpMetrics = &httpMetrics{} - } - err := srv.Routes.ProvisionHandlers(ctx, srv.Metrics) + err := srv.Routes.ProvisionHandlers(ctx, app.Metrics) if err != nil { return fmt.Errorf("server %s: setting up route handlers: %v", srvName, err) } @@ -370,7 +383,7 @@ func (app *App) Provision(ctx caddy.Context) error { // provision the named routes (they get compiled at runtime) for name, route := range srv.NamedRoutes { - err := route.Provision(ctx, srv.Metrics) + err := route.Provision(ctx, app.Metrics) if err != nil { return fmt.Errorf("server %s: setting up named route '%s' handlers: %v", name, srvName, err) } diff --git a/modules/caddyhttp/server.go b/modules/caddyhttp/server.go index 5aa7e0f6351..1b81bd98aa2 100644 --- a/modules/caddyhttp/server.go +++ b/modules/caddyhttp/server.go @@ -226,6 +226,7 @@ type Server struct { // If set, metrics observations will be enabled. // This setting is EXPERIMENTAL and subject to change. + // DEPRECATED: Use the app-level `metrics` field. Metrics *Metrics `json:"metrics,omitempty"` name string From 765d01893875a4930af91e149444f6ca72892a67 Mon Sep 17 00:00:00 2001 From: Mohammed Al Sahaf Date: Mon, 7 Oct 2024 07:03:54 +0000 Subject: [PATCH 2/2] normalize domain name --- modules/caddyhttp/metrics.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/modules/caddyhttp/metrics.go b/modules/caddyhttp/metrics.go index 947721429f4..9bb97e0b47b 100644 --- a/modules/caddyhttp/metrics.go +++ b/modules/caddyhttp/metrics.go @@ -4,6 +4,7 @@ import ( "context" "errors" "net/http" + "strings" "sync" "time" @@ -133,8 +134,8 @@ func (h *metricsInstrumentedHandler) ServeHTTP(w http.ResponseWriter, r *http.Re statusLabels := prometheus.Labels{"server": server, "handler": h.handler, "method": method, "code": ""} if h.metrics.PerHost { - labels["host"] = r.Host - statusLabels["host"] = r.Host + labels["host"] = strings.ToLower(r.Host) + statusLabels["host"] = strings.ToLower(r.Host) } inFlight := h.metrics.httpMetrics.requestInFlight.With(labels)