bootstrap: apply --web.max-requests - #441
Conversation
The flag was registered, parsed, validated and stored, and then nothing acted on it. An exporter that supplies Config.MetricsHandler got a --web.max-requests=40 default in its help output that limited nothing. Bound the metrics handler with it. Requests arriving while the limit is reached are answered with 503 rather than queued, so a scrape that cannot be served fails quickly instead of piling up behind the ones already running, which matches what promhttp does for the same setting. A limit of zero leaves the handler unwrapped, as the flag help says. The bound is applied to the metrics endpoint only. Routes registered by a MetricsHandlerFactory are not covered, because the flag describes parallel scrape requests, and a health endpoint should still answer while the metrics endpoint is saturated. --web.disable-exporter-metrics cannot be applied the same way: only the metrics handler knows which collectors it gathers, and bootstrap is given that handler already built. It is reported to a MetricsHandlerFactory as Bootstrap.DisableExporterMetrics, so say that in the flag help rather than leaving it looking like something bootstrap acts on. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Manuel Rüger <manuel@rueg.eu>
ca97f74 to
e0d6023
Compare
| mux := http.NewServeMux() | ||
| metricsPath := t.MetricsPath | ||
| mux.Handle(metricsPath, metricsHandler) | ||
| mux.Handle(metricsPath, maxRequestsHandler(metricsHandler, t.MaxRequests)) |
There was a problem hiding this comment.
issue (blocking): --web.max-requests has defaulted to 40 for a while but never did anything. Turning it on means any exporter that bumps the toolkit and serves more than 40 concurrent scrapes starts getting 503s with no config change on their side. Can we get a CHANGELOG entry at minimum? Worth discussing whether the wrap should default to off for one release.
| mux := http.NewServeMux() | ||
| metricsPath := t.MetricsPath | ||
| mux.Handle(metricsPath, metricsHandler) | ||
| mux.Handle(metricsPath, maxRequestsHandler(metricsHandler, t.MaxRequests)) |
There was a problem hiding this comment.
issue (non-blocking): The 503 happens before the exporter's handler runs, so promhttp_metric_handler_requests_total and ..._in_flight stay flat while scrapes are being dropped. Anyone alerting on those sees nothing. A log line on rejection would help, even a rate limited one.
Side note: for exporters that already pass MaxRequests into HandlerOpts, the inner limiter is now dead weight.
| disableExporterMetrics: app.Flag( | ||
| "web.disable-exporter-metrics", | ||
| "Exclude metrics about the exporter itself (promhttp_*, process_*, go_*).", | ||
| "Exclude metrics about the exporter itself (promhttp_*, process_*, go_*). Applied by the exporter's metrics handler, which is the only thing that knows what it collects; it is reported to a MetricsHandlerFactory as Bootstrap.DisableExporterMetrics.", |
There was a problem hiding this comment.
nitpick: --help output isn't the place for Go type names. Also not true for exporters that set a static Config.MetricsHandler, since there's no factory involved at all. Suggest trimming the flag string and putting the API detail on the Bootstrap.DisableExporterMetrics doc comment.
| close(handler.release) | ||
| wg.Wait() | ||
|
|
||
| // With the slot free again, a scrape is served. |
There was a problem hiding this comment.
issue (non-blocking): This uses server2, so the semaphore is fresh and the release path never gets asserted. Drop the <-inFlight release from the wrapper and this still passes. Reusing server would actually test it.
| t.Errorf("first request: got status %d, expected %d", rec.Code, http.StatusOK) | ||
| } | ||
| }) | ||
| <-handler.entered |
There was a problem hiding this comment.
suggestion: If the limit regresses, this doesn't fail, it hangs until the 10 minute package timeout. Same for the drain loop at line 125. A select with a short time.After and t.Fatal around the blocking waits would make the failure readable.
2 participants
--web.max-requestsis registered, parsed, validated and stored, and then nothing acts on it. An exporter that suppliesConfig.MetricsHandlergets a--web.max-requests=40default in its help output that limits nothing.Change
Bound the metrics handler with it. Requests arriving while the limit is reached are answered with 503 rather than queued, so a scrape that cannot be served fails quickly instead of piling up behind the ones already running — matching what
promhttpdoes for the same setting. A limit of 0 leaves the handler unwrapped, as the flag help says.The bound applies to the metrics endpoint only. Routes registered by a
MetricsHandlerFactoryare not covered, because the flag describes parallel scrape requests and a health endpoint should still answer while the metrics endpoint is saturated. There is a test for that.This deliberately does not pull in
client_golangas a direct dependency — the limiter is a few lines ofchan struct{}.--web.disable-exporter-metrics
This one cannot be applied the same way: only the metrics handler knows which collectors it gathers, and
bootstrapis given that handler already built. It is reported to aMetricsHandlerFactoryasBootstrap.DisableExporterMetrics, so this says that in the flag help rather than leaving it looking like somethingbootstrapacts on. Happy to take a different approach if you would rather it did something more.Tests
Three tests: the limit is enforced, 0 disables it, and other routes are unaffected. The first hangs against master, since without the bound the second request enters the handler instead of being turned away.
🤖 Generated with Claude Code