Migrate golang/snappy to klauspost/compress/snappy #13330
Conversation
chhetripradeep
commented
Dec 25, 2023
chhetripradeep
commented
- Since klauspost/compress/snappy is a drop-in replacement, well-maintained and claims to have better performance, it will be good to use it over golang/snappy.
- I currently don't have a production grade prometheus setup where I can try it but I hope prombench can give us those numbers.
Signed-off-by: Pradeep Chhetri <pradeepchhetri4444@gmail.com>
chhetripradeep
requested review from
dgl,
jesusvazquez,
csmarchbanks,
cstyan,
bwplotka and
tomwilkie
as code owners
|
/prombench main |
|
Something went wrong - the Prometheus in prombench for this PR has only 12,000 series (should be millions). |
|
/prombench cancel |
|
Benchmark cancel is in progress. |
|
/prombench main |
|
Running the benchmark again to see if it was a strange race condition. I'll watch closely to cancel earlier. |
| @@ -550,7 +550,8 @@ func TestCheckpointSeriesReset(t *testing.T) { | |||
| segments int | |||
| }{ | |||
| {compress: CompressionNone, segments: 14}, | |||
| {compress: CompressionSnappy, segments: 13}, | |||
| {compress: CompressionSnappy, segments: 14}, | |||
There was a problem hiding this comment.
Can you comment on why this changed?
There was a problem hiding this comment.
After changing from golang/snappy to klauspost/compress/snappy, this test started failing, so to fix that i had to change it to 14. Should snappy compression have lesser number of segments than one without compression.
There was a problem hiding this comment.
It does seem concerning if the data size goes up.
Can you investigate whether this is a general pattern? Maybe the library author has advice?
There was a problem hiding this comment.
Sure i will do some investigation about it.
|
Unfortunately, I don't think prombench exercises remote write, or does it? There is this script #13102 we developed to benchmark remote write as part of #13105 , that may help. Also, we actually tried klauspost/compress/snappy as part of that too, but AFAIR results were not really better. I am happy to be proven wrong though. |
|
/prombench cancel |
|
Benchmark cancel is in progress. |
|
This time the prombench got the timeseries right for both instances of prometheus but as @npazosmendez pointed out the benchmark isnt really going to give us evidence that the new lib performs better. @chhetripradeep please have a look and see if you could come up with a meaningful benchmark for this PR. I'm also curious about this comment #13330 (comment) |
This is true, however I believe the WAL is Snappy-compressed by default, so we should be able to see the difference on writing. |
|
Yep prombench doesn't test RW, but as nico pointed out we do have a benchmarking script for remote write. However, I'd prefer to leave any changes out of remote write for now. Again as Nico mentioned we've tested a number of options, including klaus's snappy/s2 packages. The main benefit for remote write to change from golang/snappy is going to be on the receivers side. We're doing some refactoring of how the compression package(s) is used as part of the 2.0 work and so I'd prefer to just continue with those and merge it in separately or as part of 2.0. |
|
Hello from the bug-scrub! This PR is waiting on a response to the point that compression seems to be worse. |
