feat: Support zstd compression #1496

mrueg · 2024-04-10T14:55:38Z

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

/kind feature

Release note:

promhttp: Support zstd compression

bwplotka

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.

prometheus/promhttp/http.go

prometheus/promhttp/http_test.go

mrueg · 2024-05-13T12:01:56Z

Thanks for the feedback!

I haven't thought about enhancing the Accept-Encoding header further and have replaced it now with an internal copy of the archived https://github.com/golang/gddo/blob/master/httputil/negotiate.go#L19

I see prometheus is using an internal copy of goautoneg: https://github.com/prometheus/common/blob/main/internal/bitbucket.org/ww/goautoneg/autoneg.go this might be an option to switch it to the gddo/httputil implementation and move all the code into prometheus/common.

Users of the library can now control what compression they offer via opts.EncodingOffers as well.

Cross-linking these issues from internal TODOs:
golang/go#19307
golang/go#62513

Open question:
If the user of the client supplies an not-yet implemented compression, should this be visible via an error (and if so, how)?

bwplotka

Beautiful work, had to find time to review it

If the user of the client supplies an not-yet implemented compression, should this be visible via an error (and if so, how)?

Good question. First of all I would use enum so it's less likely it happens. Then.. I propose something in comment, should be good enough. WDYT?

Thanks! Almost there!

prometheus/promhttp/http.go

prometheus/promhttp/http_test.go

prometheus/promhttp/http.go

mrueg · 2024-06-03T21:32:33Z

prometheus/promhttp/http.go

+		w, encodingHeader, closeWriter, err := NegotiateEncodingWriter(req, rsp, opts.DisableCompression, compressions)
+
+		if closeWriter != nil {
+			defer func() {


@bwplotka Test will fail if you comment out this defer.
Needed to do it that way instead of calling the function directly for golangci-lint's errcheck.

I think there is potentially better way (do _ := if we don't care about this err, or pack opt.Err log in helper func), but that's already good enough, Thanks!

Still not resolved, plus we can kill this if != nil check for close Writer I assume (let's ensure negotiateEncodingWriter always sets it, even if noop).

Sounds good, I added the empty functions you suggested also if an error is returned.

mrueg · 2024-06-03T21:39:32Z

@bwplotka I believe I addressed everything, so this is ready for another round of review. :)
Appreciate the feedback, helped me to ramp up my golang quite a bit.

bwplotka

Amazing, thank you!

I think this is good enough to merge in this state. I think there are some things e.g. number of if statements to handle response from Negotiate yields more code vs if we just inline this function (: But I think there might be ways to simplify this, mostly for readability.

Happy to merge, and allow others and myself to simplify bit in another PR, or propose PR to your PR, what do you prefer? Or if you want you could try to simplify too if you know what I mean (added some suggestions), but happy to help here to save your time! 🤗

Thanks!

prometheus/promhttp/http.go

bwplotka · 2024-06-04T07:34:45Z

prometheus/promhttp/http.go

+		w, encodingHeader, closeWriter, err := NegotiateEncodingWriter(req, rsp, opts.DisableCompression, compressions)
+
+		if closeWriter != nil {
+			defer func() {


I think there is potentially better way (do _ := if we don't care about this err, or pack opt.Err log in helper func), but that's already good enough, Thanks!

bwplotka · 2024-06-04T07:35:08Z

prometheus/promhttp/http.go

+				}
+			}()
+		}
+		if err != nil {


Technically no defer is needed on this error, no? So we can move closeWriter to after err != nil and skip closerWriter if nil check?

This one isn't the deferred one, this is the one from NegotiateEncodingWriter. I reordered it for better readability.

bwplotka · 2024-06-04T07:35:22Z

prometheus/promhttp/http.go

+			if opts.ErrorLog != nil {
+				opts.ErrorLog.Println("error getting writer", err)
+			}
+			// Since the writer received from NegotiateEncodingWriter will be nil, in case there's an error, we set it here


No need for this comment TBH

Removed it. :)

bwplotka · 2024-06-04T07:40:19Z

prometheus/promhttp/http.go


-			w = gz
+		if encodingHeader == "" {


After err check we expect this to be always valid, can we ensure in func?

Good idea, I moved it into the err check.

mrueg · 2024-06-04T08:59:42Z

Amazing, thank you!

I think this is good enough to merge in this state. I think there are some things e.g. number of if statements to handle response from Negotiate yields more code vs if we just inline this function (: But I think there might be ways to simplify this, mostly for readability.

Happy to merge, and allow others and myself to simplify bit in another PR, or propose PR to your PR, what do you prefer? Or if you want you could try to simplify too if you know what I mean (added some suggestions), but happy to help here to save your time! 🤗

Thanks!

Can we keep the NegotiateEncodingWriter func separate and not inline it? I want to reuse it in kube-state-metrics to avoid code duplication there (unfortunately we cannot fully use ServeHTTP there).

If there are any other bits that could make the code simpler and easier to read, please let me know, happy to implement them as well.

Do you want me to squash the commits into a single one?

bwplotka

Great work! Let's keep going then ;p

I think I found one more bug 🙈 And added some suggestions inline. No need to squash, we will do it while merging. Function can be separate, added suggestions, but I think we have to make it private... sorry. Exporting NegotiateEncodingWriter would be not ideal. Commented why.

bwplotka · 2024-06-04T18:08:15Z

prometheus/promhttp/http.go

+		w, encodingHeader, closeWriter, err := NegotiateEncodingWriter(req, rsp, opts.DisableCompression, compressions)
+
+		if closeWriter != nil {
+			defer func() {


Still not resolved, plus we can kill this if != nil check for close Writer I assume (let's ensure negotiateEncodingWriter always sets it, even if noop).

prometheus/promhttp/http.go

bwplotka · 2024-06-04T18:12:52Z

prometheus/promhttp/http.go

+// compressions. It returns a writer implementing the compression and an the
+// correct value that the caller can set in the response header.
+func NegotiateEncodingWriter(r *http.Request, rw io.Writer, disableCompression bool, compressions []string) (_ io.Writer, encodingHeaderValue string, closeWriter func() error, _ error) {
+	w := rw


Hm.. I feel this can be removed? We can return those z and gz things?

Yes, removed it. :)

prometheus/promhttp/http.go

bwplotka · 2024-06-04T18:15:07Z

prometheus/promhttp/http.go

+		return w, compression, z.Close, nil
+	case "gzip":
+		gz := gzipPool.Get().(*gzip.Writer)
+		defer gzipPool.Put(gz)


This has to be done on Close. It's a subtle bug, but concurrent Negotiate calls might corrupt gzip writers (hard to repro on tests I guess). "Put" means "I won't use gz anymore", which will happen on return of this function. But that's not true is it?

Good catch! I was wondering about that as well. Thanks for the suggestions on returning a function here that calls both. TIL :)

prometheus/promhttp/http.go

This allows endpoints to respond with zstd compressed metric data, if the requester supports it. I have imported a content-encoding parser from https://github.com/golang/gddo which is an archived repository to support different content-encoding headers. Signed-off-by: Manuel Rüger <manuel@rueg.eu>

Co-authored-by: Bartlomiej Plotka <bwplotka@gmail.com> Signed-off-by: Manuel Rüger <manuel@rueg.eu>

* String typed enum Signed-off-by: Manuel Rüger <manuel@rueg.eu>

Signed-off-by: Manuel Rüger <manuel@rueg.eu>

Co-authored-by: Bartlomiej Plotka <bwplotka@gmail.com> Signed-off-by: Manuel Rüger <manuel@rueg.eu>

Signed-off-by: Manuel Rüger <manuel@rueg.eu>

Co-authored-by: Bartlomiej Plotka <bwplotka@gmail.com> Signed-off-by: Manuel Rüger <manuel@rueg.eu>

mrueg force-pushed the support-zstd-encoding branch from d9dd903 to 12e6e53 Compare April 10, 2024 14:57

mrueg mentioned this pull request Apr 10, 2024

Allow Content-Encoding zstandard / zstd for scraping metrics prometheus/prometheus#13866

Open

mrueg force-pushed the support-zstd-encoding branch 3 times, most recently from 6a82962 to 18f74ec Compare April 10, 2024 15:56

bwplotka requested changes May 9, 2024

View reviewed changes

prometheus/promhttp/http.go Outdated Show resolved Hide resolved

prometheus/promhttp/http.go Outdated Show resolved Hide resolved

prometheus/promhttp/http_test.go Outdated Show resolved Hide resolved

mrueg force-pushed the support-zstd-encoding branch from 18f74ec to 562606c Compare May 13, 2024 11:58

mrueg force-pushed the support-zstd-encoding branch 11 times, most recently from 0d37c09 to 03af4da Compare May 13, 2024 12:51

bwplotka reviewed May 17, 2024

View reviewed changes

prometheus/promhttp/http.go Outdated Show resolved Hide resolved

prometheus/promhttp/http.go Outdated Show resolved Hide resolved

prometheus/promhttp/http_test.go Outdated Show resolved Hide resolved

prometheus/promhttp/http.go Outdated Show resolved Hide resolved

mrueg force-pushed the support-zstd-encoding branch 2 times, most recently from 48c16ce to ce851d0 Compare May 17, 2024 14:03

mrueg changed the title ~~feat: Support zstd encoding~~ feat: Support zstd compression May 17, 2024

mrueg force-pushed the support-zstd-encoding branch 2 times, most recently from bd79bd6 to b1dbb01 Compare May 17, 2024 14:10

mrueg force-pushed the support-zstd-encoding branch 2 times, most recently from 1716212 to 789178e Compare May 31, 2024 12:53

github-actions bot added the kind/FEATURE label May 31, 2024

mrueg force-pushed the support-zstd-encoding branch 2 times, most recently from 175c33d to 70157ae Compare May 31, 2024 12:57

github-actions bot removed the kind/FEATURE label Jun 3, 2024

mrueg force-pushed the support-zstd-encoding branch from 5d1b662 to 7cfc1f3 Compare June 3, 2024 21:12

github-actions bot added the kind/FEATURE label Jun 3, 2024

mrueg force-pushed the support-zstd-encoding branch from 7cfc1f3 to ad8f8a3 Compare June 3, 2024 21:17

github-actions bot removed the kind/FEATURE label Jun 3, 2024

mrueg force-pushed the support-zstd-encoding branch from ad8f8a3 to 99037c1 Compare June 3, 2024 21:29

github-actions bot added the kind/FEATURE label Jun 3, 2024

mrueg commented Jun 3, 2024

View reviewed changes

bwplotka approved these changes Jun 4, 2024

View reviewed changes

github-actions bot added kind/FEATURE and removed kind/FEATURE labels Jun 4, 2024

bwplotka reviewed Jun 4, 2024

View reviewed changes

github-actions bot removed the kind/FEATURE label Jun 4, 2024

mrueg force-pushed the support-zstd-encoding branch from 1209cbd to 88ad686 Compare June 4, 2024 19:47

github-actions bot added the kind/FEATURE label Jun 4, 2024

mrueg and others added 5 commits June 6, 2024 00:09

Update prometheus/promhttp/http.go

6ad6d34

Co-authored-by: Bartlomiej Plotka <bwplotka@gmail.com> Signed-off-by: Manuel Rüger <manuel@rueg.eu>

Update prometheus/promhttp/http.go

b0aadb6

Co-authored-by: Bartlomiej Plotka <bwplotka@gmail.com> Signed-off-by: Manuel Rüger <manuel@rueg.eu>

Update prometheus/promhttp/http.go

07c2ea2

Co-authored-by: Bartlomiej Plotka <bwplotka@gmail.com> Signed-off-by: Manuel Rüger <manuel@rueg.eu>

Integrate review comments

bf0f3dc

* String typed enum Signed-off-by: Manuel Rüger <manuel@rueg.eu>

mrueg force-pushed the support-zstd-encoding branch from 88ad686 to fac3bb7 Compare June 5, 2024 22:09

github-actions bot removed the kind/FEATURE label Jun 5, 2024

mrueg and others added 4 commits June 6, 2024 00:13

Test with gzip compression

c40f12a

Signed-off-by: Manuel Rüger <manuel@rueg.eu>

Update prometheus/promhttp/http.go

f0c8656

Co-authored-by: Bartlomiej Plotka <bwplotka@gmail.com> Signed-off-by: Manuel Rüger <manuel@rueg.eu>

Reorder error handling

9fea575

Signed-off-by: Manuel Rüger <manuel@rueg.eu>

Apply suggestions from code review

140db7a

Co-authored-by: Bartlomiej Plotka <bwplotka@gmail.com> Signed-off-by: Manuel Rüger <manuel@rueg.eu>

mrueg force-pushed the support-zstd-encoding branch from fac3bb7 to 140db7a Compare June 5, 2024 22:14

github-actions bot added the kind/FEATURE label Jun 5, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Support zstd compression #1496

feat: Support zstd compression #1496

mrueg commented Apr 10, 2024 •

edited by ArthurSens

bwplotka left a comment

mrueg commented May 13, 2024 •

edited

bwplotka left a comment

mrueg Jun 3, 2024

bwplotka Jun 4, 2024

bwplotka Jun 4, 2024

mrueg Jun 4, 2024

mrueg commented Jun 3, 2024

bwplotka left a comment •

edited

bwplotka Jun 4, 2024

bwplotka Jun 4, 2024

mrueg Jun 4, 2024

bwplotka Jun 4, 2024

mrueg Jun 4, 2024

bwplotka Jun 4, 2024

mrueg Jun 4, 2024

mrueg commented Jun 4, 2024

bwplotka left a comment

bwplotka Jun 4, 2024

bwplotka Jun 4, 2024

mrueg Jun 4, 2024

bwplotka Jun 4, 2024

mrueg Jun 4, 2024

feat: Support zstd compression #1496

Are you sure you want to change the base?

feat: Support zstd compression #1496

Conversation

mrueg commented Apr 10, 2024 • edited by ArthurSens

bwplotka left a comment

Choose a reason for hiding this comment

mrueg commented May 13, 2024 • edited

bwplotka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrueg commented Jun 3, 2024

bwplotka left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrueg commented Jun 4, 2024

bwplotka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrueg commented Apr 10, 2024 •

edited by ArthurSens

mrueg commented May 13, 2024 •

edited

bwplotka left a comment •

edited