Prometheus: Remove cache, pass headers in request, simplify client creation for resource calls and custom client #51436
Conversation
|
|
||
| index := 1 | ||
| for { | ||
| headerNameSuffix := fmt.Sprintf("httpHeaderName%d", index) |
There was a problem hiding this comment.
Custom headers are already handled in http client creation, but before there was a bug where we overwritten the headers which were supplied.
|
Drone build failed: https://drone.grafana.net/grafana/grafana-enterprise/24152 |
|
|
||
| // Check that the server actually sent compressed data | ||
| var reader io.ReadCloser | ||
| switch resp.Header.Get("Content-Encoding") { |
There was a problem hiding this comment.
This does not seem to be needed. If the server responds with gzipped data, we don't care here as we don't really need to read it into specific representation. I assume problem before was that we did not sent the response headers back which would probably mean it wouldn't be decoded in the browser.
@marefr I assume this is correct thing to do. I see in
grafana/pkg/middleware/gziper.go
Line 44 in 57fcfd5
There was a problem hiding this comment.
Hmm. Good question. You probably want/need to test that to verify it works as expected. Given gzip header are returned and the bytes is gzipped it should work.
| } | ||
|
|
||
| // New version using custom client and better response parsing | ||
| qd, err := querydata.New(httpClient, features, tracer, settings, plog) |
There was a problem hiding this comment.
Because we reuse the httpClient and it's setup we don't need duplicated azure setup.
aocenas
force-pushed
the
aocenas/prometheus/simplify-resource-streaming-client
branch
from
dda3e53
to
03e245d
Compare
aocenas
added
type/refactor
no-changelog
pkg/tsdb/prometheus/prometheus.go
Outdated
| b, err := buffered.New(roundTripper, tracer, settings, plog) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| qd, err := querydata.New(httpClientProvider, cfg, features, tracer, settings, plog) | ||
| httpClient, err := httpClientProvider.New(*opts) |
There was a problem hiding this comment.
Maybe you can use this on line 50 above and then use httpClient.Transport to get the roundTripper - then you're reusing the transport for both query and resources.
There was a problem hiding this comment.
LGTM. Quite a lot of changes, but looks good I think. But easy I've missed something important. Would be good with integration test as discussed earlier to verify bigger refactorings doesn't introduce any bugs, but I haven't put any additional thoughts into this.
aocenas
requested review from
sakjur,
papagian and
kylebrandt
and removed request for
a team
|
Drone build failed: https://drone.grafana.net/grafana/grafana-enterprise/24555 |
|
@marefr agree with the integration test but have to think about how to do it. I added a unit test for the HTTP setup here that should at least make sure we don't overwrite something again. |
|
Nice 👍 We can consider integration test later no worries |
|
@bboreham thanks for the suggestions 👍 |
|
@bboreham ah hell, I forgot to push changed you suggested, will do that in separate PR. |
Similar to #51061 where we could simplify the client creation with contextual middleware. Here though instead of middleware we are just passing the request to the custom client we are using for the resource calls and the custom implementation supporting streaming parsing.
This means we don't have to bind any data to the http client and we don't have to create a new client for each request and we don't need to cache it for performance reasons. This simplifies the client creation.
There are some smaller fixes too like passing of errors, status codes and response headers in resource calls.