Don't return an error from Handler.ServeHTTP
#35
Conversation
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][1]). The rate limiter's `ServeHTTP` was doing just that, and so the metrics showed no trace of the 429 responses. [1]: https://github.com/caddyserver/caddy/blob/7d919af01b31aff6eb1086e87784bf59c52419bb/modules/caddyhttp/metrics.go#L140
|
Hmm, could we fix 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). |
|
Ah, breaking changes are no good. I'll see if I can put together a Caddy PR that does as you suggest |
|
The change should be fairly simple, let me know if you have questions! (Thanks as always Francis) |
If the error returned by the wrapped handler is a `Caddy.HandlerError`, that indicates a problem that still yielded a successful HTTP transaction, so it makes sense to observe request duration and size metrics for it. See mholt/caddy-ratelimit#35 for some context.
|
I think this change is obsoleted by caddyserver/caddy#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. |
|
Sounds good! :) |
If the error returned by the wrapped handler is a `Caddy.HandlerError`, that indicates a problem that still yielded a successful HTTP transaction, so it makes sense to observe request duration and size metrics for it. See mholt/caddy-ratelimit#35 for some context.
Caddy can be set up to export Prometheus metrics from HTTP servers, and middleware automatically gets wrapped in a
metricsInstrumentedHandlerwhich will emit histograms of requests times, labeled by the HTTP status. However, this only happens if a handler'sServeHTTPmethod doesn't return an error (source). The rate limiter'sServeHTTPwas doing just that, and so the metrics showed no trace of the 429 responses.