search

mholt / caddy-ratelimit

HTTP rate limiting module for Caddy 2
Apache License 2.0
255 stars 17 forks source link

Don't return an error from `Handler.ServeHTTP` #35

Closed tgeoghegan closed 11 months ago

tgeoghegan commented 11 months ago

Caddy can be set up to export Prometheus metrics from HTTP servers, and middleware automatically gets wrapped in a metricsInstrumentedHandler which will emit histograms of requests times, labeled by the HTTP status. However, this only happens if a handler's ServeHTTP method doesn't return an error (source). The rate limiter's ServeHTTP was doing just that, and so the metrics showed no trace of the 429 responses.

francislavoie commented 11 months ago

Hmm, could we fix metricsInstrumentedHandler to check for caddyhttp.Error instead? That way it's an actual recoverable error that can be handled with handle_errors routes in Caddy.

Technically this is a breaking change because it means error routes can no longer be used to implement custom behaviour on rate limiting (e.g. render an error page or w/e).

tgeoghegan commented 11 months ago

Ah, breaking changes are no good. I'll see if I can put together a Caddy PR that does as you suggest

mholt commented 11 months ago

The change should be fairly simple, let me know if you have questions!

(Thanks as always Francis)

tgeoghegan commented 11 months ago

I think this change is obsoleted by https://github.com/caddyserver/caddy/pull/5979, which I've verified solves my problem. I'm going to close this PR for the sake of tidiness and we can re-open this discussion should the linked Caddy PR not go forward for some reason.

mholt commented 11 months ago

Sounds good! :)