-
Notifications
You must be signed in to change notification settings - Fork 468
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
feat: add gzip support for /metrics #477
Conversation
e75ffb5
to
37609ec
Compare
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.
Thanks for doing this. This seems to work in general, but most importantly, we have to add it in a way that the default behavior is the same as the current one (no compression, even if requested). See comments (which also include some nit-picking).
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.
Initially, you used github.com/NYTimes/gziphandler , but now you dropped it. Any reason for that? It seems reasonable to me to reuse that battle-tested middleware instead of implementing our own here.
In different news, you need to switch off gzip compression in the promhttp.HandlerOpts
(line 137). Otherwise you might get double gzip compression.
#84 need a feature, send |
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.
My apologies for only realizing now that #84 only talks about gzip'ing the body of a request, which is not handled at all by the usual gzip handlers (which are only concerned about response compression).
I think I understand now. Good news is that it makes things easier in terms of behavior change. We don't even need a flag because the request encoding isn't really connected to content negotiations. So far, a gzip'd request was just failing, so we can simply always allow it now. The response encoding will be untouched by all of that.
For remaining things to do, see comments below that, hopefully, make more sense now. My apologies again.
Also, could you add a short section to the README.md about the new feature? Perhaps at the end of the API section? Along the lines: "The body of a POST
or PUT
request may be gzip-compressed. Add a header Content-Encoding: gzip
to do so." And then perhaps a curl example like echo "some_metric 3.14" | gzip | curl -H 'Content-Encoding: gzip' --data-binary @- http://pushgateway.example.org:9091/metrics/job/some_job
OK |
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.
Thank you very much for your contribution and your patience. I only have a few formatting nits left for the README.md file.
Let pushgateway can handle gzip format request. Signed-off-by: fatelei <[email protected]>
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.
Thank you very much again. Merging now.
What is pr do ?
Let pushgateway server handle gzip content-encoding content.
Tests
Accept gzip
Forbid gzip