From d16ede358a2ec049bda28bf37e79c9cfcaa64c29 Mon Sep 17 00:00:00 2001 From: Dave Henderson Date: Thu, 17 Sep 2020 23:46:24 -0400 Subject: metrics: Fix hidden panic while observing with bad exemplars (#3733) * metrics: Fixing panic while observing with bad exemplars Signed-off-by: Dave Henderson * Minor cleanup The server is already added to the context. So, we can simply use that to get the server name, which is a field on the server. * Add integration test for auto HTTP->HTTPS redirects A test like this would have caught the problem in the first place Co-authored-by: Matthew Holt --- modules/caddyhttp/app.go | 1 + modules/caddyhttp/metrics.go | 52 ++++++++++++++------------------------- modules/caddyhttp/metrics_test.go | 17 ++++++++++++- modules/caddyhttp/routes.go | 9 +++---- modules/caddyhttp/server.go | 2 ++ 5 files changed, 40 insertions(+), 41 deletions(-) (limited to 'modules/caddyhttp') diff --git a/modules/caddyhttp/app.go b/modules/caddyhttp/app.go index 375ca4d..41820ea 100644 --- a/modules/caddyhttp/app.go +++ b/modules/caddyhttp/app.go @@ -155,6 +155,7 @@ func (app *App) Provision(ctx caddy.Context) error { // prepare each server for srvName, srv := range app.Servers { + srv.name = srvName srv.tlsApp = app.tlsApp srv.logger = app.logger.Named("log") srv.errorLogger = app.logger.Named("log.error") diff --git a/modules/caddyhttp/metrics.go b/modules/caddyhttp/metrics.go index 2b3b7c7..74be135 100644 --- a/modules/caddyhttp/metrics.go +++ b/modules/caddyhttp/metrics.go @@ -82,44 +82,38 @@ func initHTTPMetrics() { }, httpLabels) } -type ctxKeyServerName struct{} - // serverNameFromContext extracts the current server name from the context. -// Returns "UNKNOWN" if none is available (should probably never happen?) +// Returns "UNKNOWN" if none is available (should probably never happen). func serverNameFromContext(ctx context.Context) string { - srvName, ok := ctx.Value(ctxKeyServerName{}).(string) - if !ok { + srv, ok := ctx.Value(ServerCtxKey).(*Server) + if !ok || srv == nil || srv.name == "" { return "UNKNOWN" } - return srvName + return srv.name } type metricsInstrumentedHandler struct { - labels prometheus.Labels - statusLabels prometheus.Labels - mh MiddlewareHandler + handler string + mh MiddlewareHandler } -func newMetricsInstrumentedHandler(server, handler string, mh MiddlewareHandler) *metricsInstrumentedHandler { +func newMetricsInstrumentedHandler(handler string, mh MiddlewareHandler) *metricsInstrumentedHandler { httpMetrics.init.Do(func() { initHTTPMetrics() }) - labels := prometheus.Labels{"server": server, "handler": handler} - statusLabels := prometheus.Labels{"server": server, "handler": handler, "code": "", "method": ""} - return &metricsInstrumentedHandler{labels, statusLabels, mh} + return &metricsInstrumentedHandler{handler, mh} } func (h *metricsInstrumentedHandler) ServeHTTP(w http.ResponseWriter, r *http.Request, next Handler) error { - inFlight := httpMetrics.requestInFlight.With(h.labels) + server := serverNameFromContext(r.Context()) + labels := prometheus.Labels{"server": server, "handler": h.handler} + statusLabels := prometheus.Labels{"server": server, "handler": h.handler, "method": r.Method} + + inFlight := httpMetrics.requestInFlight.With(labels) inFlight.Inc() defer inFlight.Dec() - statusLabels := prometheus.Labels{"method": r.Method} - for k, v := range h.labels { - statusLabels[k] = v - } - start := time.Now() // This is a _bit_ of a hack - it depends on the ShouldBufferFunc always @@ -128,35 +122,25 @@ func (h *metricsInstrumentedHandler) ServeHTTP(w http.ResponseWriter, r *http.Re writeHeaderRecorder := ShouldBufferFunc(func(status int, header http.Header) bool { statusLabels["code"] = sanitizeCode(status) ttfb := time.Since(start).Seconds() - observeWithExemplar(statusLabels, httpMetrics.responseDuration, ttfb) + httpMetrics.responseDuration.With(statusLabels).Observe(ttfb) return false }) wrec := NewResponseRecorder(w, nil, writeHeaderRecorder) err := h.mh.ServeHTTP(wrec, r, next) dur := time.Since(start).Seconds() - httpMetrics.requestCount.With(h.labels).Inc() + httpMetrics.requestCount.With(labels).Inc() if err != nil { - httpMetrics.requestErrors.With(h.labels).Inc() + httpMetrics.requestErrors.With(labels).Inc() return err } - observeWithExemplar(statusLabels, httpMetrics.requestDuration, dur) - observeWithExemplar(statusLabels, httpMetrics.requestSize, float64(computeApproximateRequestSize(r))) + httpMetrics.requestDuration.With(statusLabels).Observe(dur) + httpMetrics.requestSize.With(statusLabels).Observe(float64(computeApproximateRequestSize(r))) httpMetrics.responseSize.With(statusLabels).Observe(float64(wrec.Size())) return nil } -func observeWithExemplar(l prometheus.Labels, o *prometheus.HistogramVec, value float64) { - obs := o.With(l) - if oe, ok := obs.(prometheus.ExemplarObserver); ok { - oe.ObserveWithExemplar(value, l) - return - } - // _should_ be a noop, but here just in case... - obs.Observe(value) -} - func sanitizeCode(code int) string { if code == 0 { return "200" diff --git a/modules/caddyhttp/metrics_test.go b/modules/caddyhttp/metrics_test.go index 14248a3..6ac591f 100644 --- a/modules/caddyhttp/metrics_test.go +++ b/modules/caddyhttp/metrics_test.go @@ -1,6 +1,7 @@ package caddyhttp import ( + "context" "errors" "net/http" "net/http/httptest" @@ -9,6 +10,20 @@ import ( "github.com/prometheus/client_golang/prometheus/testutil" ) +func TestServerNameFromContext(t *testing.T) { + ctx := context.Background() + expected := "UNKNOWN" + if actual := serverNameFromContext(ctx); actual != expected { + t.Errorf("Not equal: expected %q, but got %q", expected, actual) + } + + in := "foo" + ctx = context.WithValue(ctx, ServerCtxKey, &Server{name: in}) + if actual := serverNameFromContext(ctx); actual != in { + t.Errorf("Not equal: expected %q, but got %q", in, actual) + } +} + func TestMetricsInstrumentedHandler(t *testing.T) { handlerErr := errors.New("oh noes") response := []byte("hello world!") @@ -26,7 +41,7 @@ func TestMetricsInstrumentedHandler(t *testing.T) { return h.ServeHTTP(w, r) }) - ih := newMetricsInstrumentedHandler("foo", "bar", mh) + ih := newMetricsInstrumentedHandler("bar", mh) r := httptest.NewRequest("GET", "/", nil) w := httptest.NewRecorder() diff --git a/modules/caddyhttp/routes.go b/modules/caddyhttp/routes.go index be23d39..83e6354 100644 --- a/modules/caddyhttp/routes.go +++ b/modules/caddyhttp/routes.go @@ -243,12 +243,9 @@ func wrapRoute(route Route) Middleware { // pointer into its own stack frame to preserve it so it // won't be overwritten in future loop iterations. func wrapMiddleware(ctx caddy.Context, mh MiddlewareHandler) Middleware { - // first, wrap the middleware with metrics instrumentation - metricsHandler := newMetricsInstrumentedHandler( - serverNameFromContext(ctx.Context), - caddy.GetModuleName(mh), - mh, - ) + // wrap the middleware with metrics instrumentation + metricsHandler := newMetricsInstrumentedHandler(caddy.GetModuleName(mh), mh) + return func(next Handler) Handler { // copy the next handler (it's an interface, so it's // just a very lightweight copy of a pointer); this diff --git a/modules/caddyhttp/server.go b/modules/caddyhttp/server.go index 6851e98..4f198e5 100644 --- a/modules/caddyhttp/server.go +++ b/modules/caddyhttp/server.go @@ -122,6 +122,8 @@ type Server struct { // ⚠️ Experimental feature; subject to change or removal. AllowH2C bool `json:"allow_h2c,omitempty"` + name string + primaryHandlerChain Handler errorHandlerChain Handler listenerWrappers []caddy.ListenerWrapper -- cgit v1.2.3