New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Prometheus: Remove cache, pass headers in request, simplify client creation for resource calls and custom client #51436
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
var gzipIgnoredPaths = []matcher{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because we reuse the httpClient and it's setup we don't need duplicated azure setup.
dda3e53
to
03e245d
Compare
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry my comments are about things that didn't really change in this PR but please consider them to make the overall result better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested the changes with wireshark to ensure that the headers were being passed as well. Looks good! So much simpler, and love the code comments and the tests. 🔥
@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.