metrics: Fix hidden panic while observing with bad exemplars (#3733)

* metrics: Fixing panic while observing with bad exemplars

Signed-off-by: Dave Henderson <dhenderson@gmail.com>

* 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 <mholt@users.noreply.github.com>

caddytest/integration/autohttps_test.go

@@ -0,0 +1,22 @@
+package integration
+
+import (
+	"net/http"
+	"testing"
+
+	"github.com/caddyserver/caddy/v2/caddytest"
+)
+
+func TestAutoHTTPtoHTTPSRedirects(t *testing.T) {
+	tester := caddytest.NewTester(t)
+	tester.InitServer(`
+	{
+		http_port     9080
+		https_port    9443
+	}
+	localhost:9443
+	respond "Yahaha! You found me!"
+  `, "caddyfile")
+
+	tester.AssertRedirect("http://localhost:9080/", "https://localhost/", http.StatusPermanentRedirect)
+}

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")

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"

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()

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

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