search

miekg / caddy-prometheus

Prometheus metrics middleware for caddy
Apache License 2.0
65 stars 26 forks source link

429 status not reported from ratelimit middleware #29

Open sporkmonger opened 7 years ago

sporkmonger commented 7 years ago

The https://github.com/xuqingfeng/caddy-rate-limit plugin sends 429 responses when rate limits are hit, but as best as I can tell, the prometheus plugin doesn't report them. The middleware return value is here: https://github.com/xuqingfeng/caddy-rate-limit/blob/master/ratelimit.go#L84

And it seems like https://github.com/miekg/caddy-prometheus/blob/master/handler.go#L25-L35 ought to handle it correctly, but for reasons unknown, no "429" appears anywhere in our metrics even though they do appear in our logs and we've been able to successfully trigger them manually and observe that there is indeed a 429 response coming across the wire.

miekg commented 7 years ago

Are you running that code as middleware in Caddy? Can you test some some, i.e. does printing the status also say 429?

sporkmonger commented 7 years ago

Yup, printing status comes back 429.

sporkmonger commented 7 years ago

Is it possible that the middleware chain was short circuited by the rate limit hit and prometheus plugin never gets called?

miekg commented 7 years ago

[ Quoting notifications@github.com in "Re: [miekg/caddy-prometheus] 429 st..." ]

Is it possible that the middleware chain was short circuited by the rate limit hit and prometheus plugin never gets called?

depends on the order of our middleware... I've put this one first. caddyhttp/httpserver/plugin.go seems to have it, using the default, will not make it work, as "prometheus" is defined near the end.

sporkmonger commented 7 years ago

Just to confirm, prometheus should be first?

miekg commented 7 years ago

Yes. Early enough to catch all your middleware

On Aug 1, 2017 4:58 PM, "Bob Aman" notifications@github.com wrote:

Just to confirm, prometheus should be first?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/miekg/caddy-prometheus/issues/29#issuecomment-319414784, or mute the thread https://github.com/notifications/unsubscribe-auth/AAVkW089ToajcJjnf4a-3LdJEelP2zbqks5sT0sVgaJpZM4OpEO0 .

gonzalop commented 5 years ago

I have a similar issue with 304s. I've tried having prometheus as the first line for the servers to no avail.

krichprollsch commented 4 years ago

Hello, I have the same issue too. As @miekg said, I have fixed it by moving the prometheus declaration into caddyhttp/httpserver/plugin.go before ratelimit and re-building caddy from sources.

What about doing a PR on caddy to move prometheus earlier into the list? Do you think it's a good idea? I suppose some other users encounter this issue too...