search

miekg / caddy-prometheus

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

correctly displaying response statuses #25

Closed hackeryarn closed 7 years ago

hackeryarn commented 7 years ago

This resolves #22

@miekg Could you please review?

My company is depending on this library in production. If you need any help with maintenance, please let me know.

This implementation was suggested in https://github.com/mholt/caddy/issues/1662.

jancel commented 7 years ago

+1

miekg commented 7 years ago

I've made to opposite change a while back: https://github.com/miekg/caddy-prometheus/commit/6f7750eab689fa5a0dd6770308b5c556f173e710 So we might need some testing for this to see if the normal stuff works. Also the reason why there is no integrating testing is because it is probably hard :-(

hackeryarn commented 7 years ago

Interesting. Maybe we need to use rw.Status() only if the status is 0. That would run it only for proxies or other requests that don't have a default status.

miekg commented 7 years ago

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

Interesting. Maybe we need to use rw.Status() only if the status is 0. That would run it only for proxies or other requests that don't have a default status.

Yeah, thinking along the same lines.

Is there a light weight (without building all of caddy) to test this that you can think of?

hackeryarn commented 7 years ago

I will work through this today and see what I can do for tests.

miekg commented 7 years ago

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

I will work through this today and see what I can do for tests.

Nice, thank you!

miekg commented 7 years ago

Thinks this looks legit. Have you locally tested it without proxy and with proxy? And got 404 etc?

hackeryarn commented 7 years ago

Yes, local tests looked good for all proxy and non proxy responses. I also validated that the previous implementation always returned 200 on non proxies.

miekg commented 7 years ago

OK, let's worry about proper tests in a later PR.

hackeryarn commented 7 years ago

Thanks for the quick merge! I've scaffold the tests to a degree, just ran I to a couple issues that I'm trying to resolve now.

hackeryarn commented 7 years ago

@miekg what is the process for releases to caddy plugins? Specifically when would the updated version be available to install with caddy?

miekg commented 7 years ago

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

@miekg what is the process for releases to caddy plugins? Specifically when would the updated version be available to install with caddy?

All manual :( Although @mholt made an REST interface. But, alas, I have zero time to properly set this up.

Compiling caddy is not that hard btw :) But I'll push some buttons.

mholt commented 7 years ago

Manual?? It used to be manual before the new site. Now it's just a button push and you do have a (yet undocumented) REST API. :wink: And besides, git webhooks are almost ready. Would that be preferable?

miekg commented 7 years ago

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

Manual?? It used to be manual before the new site. Now it's just a button push. And besides, git webhooks are almost ready. Would that be preferable?

Yes! If we can do webhooks I'll set that up asap. (Nice!)

miekg commented 7 years ago

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

@miekg what is the process for releases to caddy plugins? Specifically when would the updated version be available to install with caddy?

should be deployed on the caddy site

hackeryarn commented 7 years ago

@miekg Thank you so much!