Skip to content
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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mrueg
Copy link
Contributor

@mrueg mrueg commented Apr 10, 2024

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.

Running tool: go test -benchmem -run=^$ -bench ^BenchmarkEncoding$ github.com/prometheus/client_golang/prometheus/promhttp

goos: linux
goarch: amd64
pkg: github.com/prometheus/client_golang/prometheus/promhttp
cpu: Intel(R) Core(TM) i7-1065G7 CPU @ 1.30GHz
BenchmarkEncoding/test_with_gzip_encoding_small-8         	   20670	     53641 ns/op	  111770 B/op	      49 allocs/op
BenchmarkEncoding/test_with_zstd_encoding_small-8         	    7838	    178033 ns/op	 1121493 B/op	      75 allocs/op
BenchmarkEncoding/test_with_no_encoding_small-8           	   56409	     20998 ns/op	   38030 B/op	      43 allocs/op
BenchmarkEncoding/test_with_gzip_encoding_medium-8        	    8493	    143460 ns/op	   78918 B/op	      49 allocs/op
BenchmarkEncoding/test_with_zstd_encoding_medium-8        	    3650	    345559 ns/op	 1155737 B/op	      76 allocs/op
BenchmarkEncoding/test_with_no_encoding_medium-8          	    8816	    122759 ns/op	   72252 B/op	      44 allocs/op
BenchmarkEncoding/test_with_gzip_encoding_large-8         	     944	   1206069 ns/op	  479829 B/op	      52 allocs/op
BenchmarkEncoding/test_with_zstd_encoding_large-8         	     817	   1482854 ns/op	 1429911 B/op	      75 allocs/op
BenchmarkEncoding/test_with_no_encoding_large-8           	     994	   1213586 ns/op	  346837 B/op	      45 allocs/op
BenchmarkEncoding/test_with_gzip_encoding_extra-large-8   	      85	  14008798 ns/op	 2972152 B/op	      56 allocs/op
BenchmarkEncoding/test_with_zstd_encoding_extra-large-8   	      90	  14638520 ns/op	 3731521 B/op	      74 allocs/op
BenchmarkEncoding/test_with_no_encoding_extra-large-8     	      93	  13897780 ns/op	 2648745 B/op	      45 allocs/op
PASS
ok  	github.com/prometheus/client_golang/prometheus/promhttp	17.534s

See also: prometheus/prometheus#13866

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>
Copy link
Member

@bwplotka bwplotka left a 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 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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") {
Copy link
Member

@bwplotka bwplotka May 9, 2024

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"}},
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants