QubitProducts / exporter_exporter

A reverse proxy designed for Prometheus exporters
Apache License 2.0
336 stars 55 forks source link

implement common http transport for modules with http method #38

Closed cyril-s closed 4 years ago

cyril-s commented 4 years ago

Current implementation handles modules with verify=true and verify=false very differently. For the former, it uses promhttp.HandlerFor helper with http.Client and acts as some kind of gateway while for the latter, it uses httputil.ReverseProxy and acts as usual http proxy. Some problems are:

1) Inconsistent http headers handling. http.Client makes request anew while httputil.ReverseProxy copies end-to-end headers 2) Inconsistent error handling. promhttp.HandlerFor always returns 500 Internal Server Error when 302 and 304 codes are appropriate, while promhttp.HandlerFor of current implementation returns 302 always.

I suggest get rid of promhttp.HandlerFor and move verification logic into ModifyResponse hook of httputil.ReverseProxy. This allows for usual http proxy headers handling and better error handling.

Error handling changes table:

Old verify Old no verify New verify New no verify
Bad format 500, empty body 200 500, error message in body 200
Timeout 500, “context deadline exceeded” in body 502, empty body 504, Gateway Timeout 504, Gateway Timeout
Network error 500, “connection refused” 502, empty body 502, Bad Gateway 502, Bad Gateway
Default 500, some detailed error message 502, empty body 502, Bad Gateway 502, Bad Gateway

In summary, this PR changes:

Closes #37

tcolgate commented 4 years ago

This looks like an excellent approach. I'll need a little time to review to PR though.

tcolgate commented 4 years ago

What happen in the case of a 401 (or other non-200/500)?

tcolgate commented 4 years ago

Generally happy, but I'd like to keep reporting of non-200 responses if possible. Debugging things like 401s, and/or https issues needs to be as easy as possible. I've not had time to read the code, so I apologise if this is dealt with and I'm missing it. Thanks especially for the benchmark tests.

cyril-s commented 4 years ago

@tcolgate only errors originating from exporter_exporter itself are logged. They are:

All other errors are passed as is from proxied server to the client. But I agree that logging would be useful. I made another PR with access logging. Take a look at #40

tcolgate commented 4 years ago

okay, we can review logging in the other PR.