Parse HTTP response directly

[importhuman]

Was something like this required for #977? (I'm aware tests are failing, will look into it more)

Got the following benchmark results (used the existing benchmark test in client_test.go, not sure why delta is nil)

name              old time/op    new time/op    delta
Client/4KB-12       62.7µs ± 0%   137.4µs ± 0%   ~     (p=1.000 n=1+1)
Client/50KB-12       119µs ± 0%     132µs ± 0%   ~     (p=1.000 n=1+1)
Client/1000KB-12    1.44ms ± 0%    0.13ms ± 0%   ~     (p=1.000 n=1+1)
Client/2000KB-12    2.68ms ± 0%    0.14ms ± 0%   ~     (p=1.000 n=1+1)

name              old alloc/op   new alloc/op   delta
Client/4KB-12       20.5kB ± 0%    18.2kB ± 0%   ~     (p=1.000 n=1+1)
Client/50KB-12       136kB ± 0%      18kB ± 0%   ~     (p=1.000 n=1+1)
Client/1000KB-12    2.11MB ± 0%    0.02MB ± 0%   ~     (p=1.000 n=1+1)
Client/2000KB-12    4.21MB ± 0%    0.02MB ± 0%   ~     (p=1.000 n=1+1)

name              old allocs/op  new allocs/op  delta
Client/4KB-12         74.0 ± 0%     128.0 ± 0%   ~     (p=1.000 n=1+1)
Client/50KB-12         101 ± 0%       137 ± 0%   ~     (p=1.000 n=1+1)
Client/1000KB-12       595 ± 0%       135 ± 0%   ~     (p=1.000 n=1+1)
Client/2000KB-12     1.10k ± 0%     0.14k ± 0%   ~     (p=1.000 n=1+1)

Thank you!

[importhuman]

cc @bboreham @kakkoyun can you please review if this is along correct lines?

[bboreham]

No, I don't expect the code to get much shorter when implementing this. In particular code like this which you removed: https://github.com/prometheus/client_golang/pull/1090/files#diff-aa9bfd1a638fbb706f8e8920297902937011160319d9679add5dca56e5ab8277L145-L153 is important to handle timeouts, and will need some functional replacement.

Possibly calling resp.Body.Close() will still work, but I think you will need a goroutine watching for context cancellation in order to do this.

[importhuman]

@bboreham will this work? I didn't use a select statement since this flow doesn't use a done channel. Apologies for the late update.

[importhuman]

Sorry for not closing this sooner, I've been unable to devote time. If this is high priority, happy to let someone else pick this up and help them. Will still try to finish it.

[importhuman]

Benchmark results:

main branch:

goos: linux
goarch: amd64
pkg: github.com/prometheus/client_golang/api
cpu: Intel(R) Core(TM) i7-10750H CPU @ 2.60GHz
BenchmarkClient/4KB-12         	   19268	     55355 ns/op	   20532 B/op	      74 allocs/op
BenchmarkClient/50KB-12        	   10000	    100465 ns/op	  135893 B/op	     101 allocs/op
BenchmarkClient/1000KB-12      	    1003	   1201450 ns/op	 2112968 B/op	     595 allocs/op
BenchmarkClient/2000KB-12      	     542	   2535752 ns/op	 4214170 B/op	    1097 allocs/op
PASS
ok  	github.com/prometheus/client_golang/api	5.644s

This branch:

goos: linux
goarch: amd64
pkg: github.com/prometheus/client_golang/api
cpu: Intel(R) Core(TM) i7-10750H CPU @ 2.60GHz
BenchmarkClient/4KB-12         	    8221	    137427 ns/op	   18519 B/op	     131 allocs/op
BenchmarkClient/50KB-12        	    8016	    136149 ns/op	   18442 B/op	     139 allocs/op
BenchmarkClient/1000KB-12      	    7760	    136296 ns/op	   18307 B/op	     138 allocs/op
BenchmarkClient/2000KB-12      	    8731	    137935 ns/op	   18318 B/op	     138 allocs/op
PASS
ok  	github.com/prometheus/client_golang/api	5.793s

Benchstat:

name              old time/op    new time/op    delta
Client/4KB-12       55.4µs ± 0%   137.4µs ± 0%   ~     (p=1.000 n=1+1)
Client/50KB-12       100µs ± 0%     136µs ± 0%   ~     (p=1.000 n=1+1)
Client/1000KB-12    1.20ms ± 0%    0.14ms ± 0%   ~     (p=1.000 n=1+1)
Client/2000KB-12    2.54ms ± 0%    0.14ms ± 0%   ~     (p=1.000 n=1+1)

name              old alloc/op   new alloc/op   delta
Client/4KB-12       20.5kB ± 0%    18.5kB ± 0%   ~     (p=1.000 n=1+1)
Client/50KB-12       136kB ± 0%      18kB ± 0%   ~     (p=1.000 n=1+1)
Client/1000KB-12    2.11MB ± 0%    0.02MB ± 0%   ~     (p=1.000 n=1+1)
Client/2000KB-12    4.21MB ± 0%    0.02MB ± 0%   ~     (p=1.000 n=1+1)

name              old allocs/op  new allocs/op  delta
Client/4KB-12         74.0 ± 0%     131.0 ± 0%   ~     (p=1.000 n=1+1)
Client/50KB-12         101 ± 0%       139 ± 0%   ~     (p=1.000 n=1+1)
Client/1000KB-12       595 ± 0%       138 ± 0%   ~     (p=1.000 n=1+1)
Client/2000KB-12     1.10k ± 0%     0.14k ± 0%   ~     (p=1.000 n=1+1)

Slightly worse for 4 and 50 KB, much better for 1000 and 2000 KB.

---

Failing tests:

Currently, they are failing due to a different error message. I have slightly changed the flow of the code to be in line what is currently in main branch, reading the response body after checking the status code. Are we allowed to change the expected error message? My concern is doing it would impact users who have structured their code with the current expected error messages.

[importhuman]

cc @bboreham @kakkoyun

[bboreham]

I've just noticed that the code calls the JSON parser twice to read each request. Once in `apiClientImpl.Do: https://github.com/prometheus/client_golang/blob/618194de6ad3db637313666104533639011b470d/api/prometheus/v1/api.go#L1140

and once in the individual API methods like Query: https://github.com/prometheus/client_golang/blob/618194de6ad3db637313666104533639011b470d/api/prometheus/v1/api.go#L859

Then, different data types make nested calls: https://github.com/prometheus/client_golang/blob/618194de6ad3db637313666104533639011b470d/api/prometheus/v1/api.go#L584

I suspect the first two could be merged without major change, but the last kind needs the code rewritten in "streaming" style to avoid building an intermediate buffer.

[importhuman]

Thanks for the detailed feedback! I don't have the answers currently, will update as I look into these issues more.

[stale[bot]]

Is this still relevant? If so, what is blocking it? Is there anything you can do to help move it forward?

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.