cmd: validate telemetry.path and scrape.timeout-margin, warn on unknown disabled collectors (#2733)

Signed-off-by: Jan-Otto Kröpke <mail@jkroepke.de>
2026-10-17 18:19:04 +02:00 · 2026-10-16 09:11:02 +02:00
parent a3692d335a
commit 41f8bfb352
5 changed files with 268 additions and 25 deletions
@@ -65,6 +65,9 @@ const (
 	writeTimeoutMargin = 30 * time.Second
 )

+// healthPath is the URL path of the health endpoint.
+const healthPath = "/health"
+
 func main() {
 	// os.Kill can't be caught. On Windows, Go delivers console close, logoff and shutdown events as SIGTERM.
 	//nolint:forbidigo // os/signal only accepts syscall.Signal values, not windows.Signal.
@@ -177,6 +180,20 @@ func run(ctx context.Context, args []string) int {
 		return 1
 	}

+	if err := validateTimeoutMargin(*timeoutMargin); err != nil {
+		logStartupError(ctx, "Invalid scrape timeout margin", err)
+
+		return 1
+	}
+
+	// Check the routes before the collectors are built, so that an invalid
+	// telemetry path fails the startup instead of panicking in the router.
+	if _, err := newServeMux(*metricsPath, *debugEnabled, http.NotFoundHandler()); err != nil {
+		logStartupError(ctx, "Invalid telemetry path", err)
+
+		return 1
+	}
+
 	// When scraping to stdout, redirect logs to stderr so they don't
 	// mix with the metrics output. Only override if the user hasn't
 	// explicitly set --log.file to something other than the default.
@@ -237,8 +254,18 @@ func run(ctx context.Context, args []string) int {

 	var disabledCollectorList []string
 	if *disabledCollectors != "" {
-		disabledCollectorList = slices.Compact(strings.Split(*disabledCollectors, ","))
+		disabledCollectorList = splitCollectorList(*disabledCollectors)
 		collectors.Disable(disabledCollectorList)
+
+		// An unknown name in --collectors.enabled fails the startup. Only warn about one in
+		// --collectors.disabled: disabling a collector that does not exist leaves nothing
+		// running that should not, and a failure would break configurations that disable a
+		// collector which a later release removed.
+		if unknown := unknownCollectors(disabledCollectorList); len(unknown) > 0 {
+			logger.LogAttrs(ctx, slog.LevelWarn, "ignoring unknown collectors in --collectors.disabled",
+				slog.Any("collectors", unknown),
+			)
+		}
 	}

 	// Close the collectors on every return from here on. On a regular shutdown, this runs after the HTTP server has shut down.
@@ -276,16 +303,13 @@ func run(ctx context.Context, args []string) int {
 		return scrapeToStdout(ctx, os.Stdout, logger, collectors, handlerOpts)
 	}

-	mux := http.NewServeMux()
-	mux.Handle("GET /health", httphandler.NewHealthHandler())
-	mux.Handle("GET "+*metricsPath, httphandler.New(logger, collectors, handlerOpts))
+	mux, err := newServeMux(*metricsPath, *debugEnabled, httphandler.New(logger, collectors, handlerOpts))
+	if err != nil {
+		logger.LogAttrs(ctx, slog.LevelError, "failed to build router",
+			slog.Any("err", err),
+		)

-	if *debugEnabled {
-		mux.HandleFunc("GET /debug/pprof/", pprof.Index)
-		mux.HandleFunc("GET /debug/pprof/cmdline", pprof.Cmdline)
-		mux.HandleFunc("GET /debug/pprof/profile", pprof.Profile)
-		mux.HandleFunc("GET /debug/pprof/symbol", pprof.Symbol)
-		mux.HandleFunc("GET /debug/pprof/trace", pprof.Trace)
+		return 1
 	}

 	logger.LogAttrs(ctx, slog.LevelInfo, "started windows_exporter",
@@ -481,10 +505,94 @@ func effectiveCollectors(enabled, disabled []string) []string {
 	})
 }

-func expandEnabledCollectors(enabled string) []string {
-	expanded := strings.ReplaceAll(enabled, "[defaults]", collector.DefaultCollectors)
+// unknownCollectors returns the names that are not available collectors.
+// Empty names, as in "cpu,,net", are skipped.
+func unknownCollectors(names []string) []string {
+	available := collector.Available()

-	return slices.Compact(strings.Split(expanded, ","))
+	var unknown []string
+
+	for _, name := range names {
+		if name != "" && !slices.Contains(available, name) && !slices.Contains(unknown, name) {
+			unknown = append(unknown, name)
+		}
+	}
+
+	return unknown
+}
+
+// validateTimeoutMargin rejects a negative, NaN or infinite margin. A negative
+// margin would extend the scrape timeout beyond the timeout of the client.
+func validateTimeoutMargin(margin float64) error {
+	if math.IsNaN(margin) || math.IsInf(margin, 0) || margin < 0 {
+		return fmt.Errorf("--scrape.timeout-margin must be a non-negative number of seconds, got %v", margin)
+	}
+
+	return nil
+}
+
+// newServeMux returns the router of the exporter. http.ServeMux panics on a
+// pattern it cannot parse or that conflicts with another route, so newServeMux
+// returns an error for a telemetry path that cannot be registered instead.
+func newServeMux(metricsPath string, debugEnabled bool, metricsHandler http.Handler) (*http.ServeMux, error) {
+	if !strings.HasPrefix(metricsPath, "/") {
+		return nil, fmt.Errorf("--telemetry.path %q must start with /", metricsPath)
+	}
+
+	if metricsPath == healthPath {
+		return nil, fmt.Errorf("--telemetry.path %q is reserved for the health endpoint", metricsPath)
+	}
+
+	mux := http.NewServeMux()
+	mux.Handle("GET "+healthPath, httphandler.NewHealthHandler())
+
+	if debugEnabled {
+		mux.HandleFunc("GET /debug/pprof/", pprof.Index)
+		mux.HandleFunc("GET /debug/pprof/cmdline", pprof.Cmdline)
+		mux.HandleFunc("GET /debug/pprof/profile", pprof.Profile)
+		mux.HandleFunc("GET /debug/pprof/symbol", pprof.Symbol)
+		mux.HandleFunc("GET /debug/pprof/trace", pprof.Trace)
+	}
+
+	// The metrics route is registered last, so that a conflict with any other route is detected.
+	if err := handle(mux, "GET "+metricsPath, metricsHandler); err != nil {
+		return nil, fmt.Errorf("--telemetry.path %q cannot be registered: %w", metricsPath, err)
+	}
+
+	return mux, nil
+}
+
+// handle registers handler for pattern and returns the panic of
+// http.ServeMux.Handle as an error.
+func handle(mux *http.ServeMux, pattern string, handler http.Handler) error {
+	var err error
+
+	func() {
+		defer func() {
+			if r := recover(); r != nil {
+				err = fmt.Errorf("%v", r)
+			}
+		}()
+
+		mux.Handle(pattern, handler)
+	}()
+
+	return err
+}
+
+func expandEnabledCollectors(enabled string) []string {
+	return splitCollectorList(strings.ReplaceAll(enabled, "[defaults]", collector.DefaultCollectors))
+}
+
+// splitCollectorList splits a comma-separated list of collector names and trims
+// the whitespace around each name, so that "cpu, net" names net.
+func splitCollectorList(list string) []string {
+	names := strings.Split(list, ",")
+	for i, name := range names {
+		names[i] = strings.TrimSpace(name)
+	}
+
+	return slices.Compact(names)
 }

 // scrapeToStdout performs a single scrape through the HTTP handler and writes
@@ -24,8 +24,10 @@ import (
 	"fmt"
 	"io"
 	"log/slog"
+	"math"
 	"net"
 	"net/http"
+	"net/http/httptest"
 	"net/url"
 	"os"
 	"strings"
@@ -77,6 +79,21 @@ func TestRun(t *testing.T) {
 			config:          `{"web":{"listen-address":"127.0.0.1:8083"}}`,
 			metricsEndpoint: "http://127.0.0.1:8084/metrics",
 		},
+		{
+			name:     "invalid telemetry.path without leading slash",
+			args:     []string{"--web.listen-address=127.0.0.1:8085", "--telemetry.path=metrics"},
+			exitCode: 1,
+		},
+		{
+			name:     "invalid telemetry.path health",
+			args:     []string{"--web.listen-address=127.0.0.1:8085", "--telemetry.path=/health"},
+			exitCode: 1,
+		},
+		{
+			name:     "invalid scrape.timeout-margin",
+			args:     []string{"--web.listen-address=127.0.0.1:8085", "--scrape.timeout-margin=-1"},
+			exitCode: 1,
+		},
 	} {
 		t.Run(tc.name, func(t *testing.T) {
 			ctx, cancel := context.WithCancel(t.Context())
@@ -184,6 +201,114 @@ func waitUntilListening(tb testing.TB, network, address string) error {
 	return fmt.Errorf("listener not listening: %w", err)
 }

+func TestNewServeMux(t *testing.T) {
+	t.Parallel()
+
+	for _, tc := range []struct {
+		path    string
+		debug   bool
+		wantErr string
+	}{
+		{path: "/metrics"},
+		{path: "/"},
+		{path: "/custom/metrics"},
+		{path: "/metrics/"},
+		{path: "/debug/pprof/"},
+		{path: "metrics", wantErr: "must start with /"},
+		{path: "", wantErr: "must start with /"},
+		{path: "/health", wantErr: "reserved for the health endpoint"},
+		{path: "/debug/pprof/", debug: true, wantErr: "cannot be registered"},
+		{path: "/debug/pprof/trace", debug: true, wantErr: "cannot be registered"},
+		{path: "/metrics/{name", wantErr: "cannot be registered"},
+	} {
+		t.Run(fmt.Sprintf("%s debug=%t", tc.path, tc.debug), func(t *testing.T) {
+			t.Parallel()
+
+			metricsHandler := http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
+				_, _ = w.Write([]byte("metrics"))
+			})
+
+			mux, err := newServeMux(tc.path, tc.debug, metricsHandler)
+			if tc.wantErr != "" {
+				require.ErrorContains(t, err, tc.wantErr)
+				require.Nil(t, mux)
+
+				return
+			}
+
+			require.NoError(t, err)
+
+			for _, path := range []string{tc.path, healthPath} {
+				rec := httptest.NewRecorder()
+				mux.ServeHTTP(rec, httptest.NewRequestWithContext(t.Context(), http.MethodGet, path, nil))
+				require.Equal(t, http.StatusOK, rec.Code, path)
+
+				if path == tc.path {
+					require.Equal(t, "metrics", rec.Body.String(), path)
+				}
+			}
+		})
+	}
+}
+
+func TestValidateTimeoutMargin(t *testing.T) {
+	t.Parallel()
+
+	for _, margin := range []float64{0, 0.5, 30} {
+		require.NoError(t, validateTimeoutMargin(margin), margin)
+	}
+
+	for _, margin := range []float64{-0.5, math.NaN(), math.Inf(1), math.Inf(-1)} {
+		require.Error(t, validateTimeoutMargin(margin), margin)
+	}
+}
+
+func TestUnknownCollectors(t *testing.T) {
+	t.Parallel()
+
+	require.Empty(t, unknownCollectors([]string{"cpu", "", "net"}))
+	require.Equal(t, []string{"nope", "CPU"}, unknownCollectors([]string{"cpu", "nope", "", "nope", "CPU"}))
+}
+
+func TestSplitCollectorList(t *testing.T) {
+	t.Parallel()
+
+	for list, want := range map[string][]string{
+		"cpu,net":          {"cpu", "net"},
+		"cpu, net":         {"cpu", "net"},
+		" cpu ,\tnet , os": {"cpu", "net", "os"},
+		"cpu,cpu, cpu":     {"cpu"},
+		// An empty name is kept, so that an empty --collectors.enabled still fails.
+		"": {""},
+	} {
+		require.Equal(t, want, splitCollectorList(list), list)
+	}
+
+	require.Equal(t, []string{"cpu", "net"}, expandEnabledCollectors("cpu, net"))
+	require.Contains(t, expandEnabledCollectors("[defaults], net"), "net")
+	require.NotContains(t, expandEnabledCollectors("[defaults], net"), " net")
+}
+
+// TestCollectorListsWithSpaces checks that names with spaces around them enable
+// and disable the collectors they name.
+func TestCollectorListsWithSpaces(t *testing.T) {
+	t.Parallel()
+
+	collectors := collector.NewWithFlags(kingpin.New("test", "test"))
+	require.NoError(t, collectors.Enable(expandEnabledCollectors("cpu, net, os")))
+
+	disabled := splitCollectorList("cpu, net")
+	collectors.Disable(disabled)
+	require.Empty(t, unknownCollectors(disabled), "no warning for trimmed names")
+
+	limited, err := collectors.WithCollectors([]string{"os"})
+	require.NoError(t, err)
+	require.NotNil(t, limited)
+
+	_, err = collectors.WithCollectors([]string{"net"})
+	require.Error(t, err, "net must be disabled")
+}
+
 func TestScrapeStdout(t *testing.T) {
 	t.Parallel()