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: Support zstd encoding #1496
base: main
Are you sure you want to change the base?
Conversation
d9dd903
to
12e6e53
Compare
5378521
to
6a82962
Compare
This allows endpoints to respond with zstd compressed metric data, if the requester supports it. For backwards compatibility, gzip compression will take precedence. Signed-off-by: Manuel Rüger <manuel@rueg.eu>
6a82962
to
18f74ec
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.
Nice! I am supportive. I think adding zstd to some places in Prometheus turned out to be sometimes controversial due to higher CPU, but from the client side... we are happy to support more AS LONG as the dependency is not too problematic (zstd should be fine).
Thanks! I have some suggestions to the implementation though, see comments.
// gzipAccepted returns whether the client will accept gzip-encoded content. | ||
func gzipAccepted(header http.Header) bool { | ||
// EncodingAccepted returns whether the client will accept encoded content. | ||
func EncodingAccepted(header http.Header, encoding string) bool { |
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.
func EncodingAccepted(header http.Header, encoding string) bool { | |
func IsEncodingAccepted(header http.Header, encoding string) bool { |
if !opts.DisableCompression { | ||
// Gzip takes precedence over zstd | ||
// TODO(mrueg): Replace klauspost/compress with stdlib implementation once https://github.com/golang/go/issues/62513 is implemented. | ||
if EncodingAccepted(req.Header, "zstd") { |
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.
Something is off here. Shouldn't we take whatever is first on "Accept-Encoding" (or what has the biggest q?) as per RFC 9110 (https://www.rfc-editor.org/rfc/rfc9110.html#name-accept-encoding)
Also as it's implemented here zstd takes precedence I think (I assume EncodingAccepted checks if zstd is part of header, even if it's last in the order) .. or actually we may in case of "Accept-Encoding: gzip;zstd" -- pick zstd,allocate everything and then choose gzip for double compression?
Can we just do (pseudo code):
if !opts.DisableCompression {
accepted, err := parseAcceptEncoding(req.Header)
// Return 415 on err
for _, e := range accepted {
if e== "gzip" {
// gzip flow
break
}
if e== "zstd" {
// zstd flow
break
}
}
// Technically we should check if uncompressed is accepted, but we never checked it and we don't want to break existing clients and browsers, so assuming uncompressed is always accepted.
}
}{ | ||
{ | ||
name: "test with gzip accept-encoding", | ||
header: http.Header{"Accept-Encoding": {"gzip"}}, |
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.
Please add more complex cases like:
Accept-Encoding: gzip;q=1.0, identity; q=0.5, *;q=0
Accept-Encoding: gzip;zstd
Accept-Encoding: zstd;gzip
This allows endpoints to respond with zstd compressed metric data, if the requester supports it. For backwards compatibility, gzip compression will take precedence.
I added a benchmark to the http_test.go, that will test with a differently sized synthetic metric series.
See also: prometheus/prometheus#13866