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(common/loki): switch Snappy compression from golang/snappy to klauspost/compress/snappy for better compression #955

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

hainenber
Copy link
Contributor

@hainenber hainenber commented May 29, 2024

PR Description

I discovered that Snappy compression could be done from github.com/klauspost/compress/snappy. It's advertised as having "better compression" so I think the switch could be a performance win.

In addition, there's still usage of old Shopify/sarama, which is IBM/sarama now.

Hijack this PR are also replacements of golang.org/x/exp/maps and golang.org/x/exp/slices to stdlib's equivalences.

Which issue(s) this PR fixes

N/A

Notes to the Reviewer

PR Checklist

  • CHANGELOG.md updated

hainenber and others added 3 commits May 29, 2024 23:25
…`klauspost/compress/snappy` for better compression

Signed-off-by: hainenber <dotronghai96@gmail.com>
Signed-off-by: hainenber <dotronghai96@gmail.com>
Copy link
Contributor

github-actions bot commented Jul 3, 2024

This PR has not had any activity in the past 30 days, so the needs-attention label has been added to it.
If you do not have enough time to follow up on this PR or you think it's no longer relevant, consider closing it.
The needs-attention label signals to maintainers that something has fallen through the cracks. No action is needed by you; your PR will be kept open and you do not have to respond to this comment. The label will be removed the next time this job runs if there is new activity.
Thank you for your contributions!

@rfratto
Copy link
Member

rfratto commented Jul 10, 2024

Hey, thanks for the PR! I don't doubt the compression is better, but there's no information in the package for what the comparison of CPU/memory consumption is.

I'd like to see some tests here:

  • A benchmark comparing golang/snappy and klauspost/compress/snappy, with a custom stat for post-compression size.
  • A fuzz test that ensures anything compressed using klauspost/compress/snappy can be decompressed with golang/snappy

@rfratto
Copy link
Member

rfratto commented Jul 10, 2024

It looks like both Prometheus and Loki have explored switching to klauspost/compress/snappy, but found that compression performs worse in those cases.

Those were done at the storage-level and not at the client-side, so that's not necessarily an indication that it'll perform worse in Alloy, but definitely an indication that we should add benchmarks and validate.

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.

2 participants