Optionally compress the WAL using Snappy

[csmarchbanks]

Compress the WAL records using snappy. Running in test clusters shows that the WAL is half the size with minimal increase in CPU.

Benchmarks:
2012 macbook pro, SATA flash drive

go test ./wal -bench . -benchmem -benchtime=5s
goos: darwin
goarch: amd64
pkg: github.com/prometheus/tsdb/wal
BenchmarkWAL_LogBatched/compress=true-8                   500000             13234 ns/op         154.74 MB/s           0 B/op          0 allocs/op
BenchmarkWAL_LogBatched/compress=false-8                 1000000              5278 ns/op         387.96 MB/s           0 B/op          0 allocs/op
BenchmarkWAL_Log/compress=true-8                          500000             13209 ns/op         155.04 MB/s           0 B/op          0 allocs/op
BenchmarkWAL_Log/compress=false-8                         500000             19728 ns/op         103.81 MB/s           0 B/op          0 allocs/op

5th Gen Lenovo X1 Carbon, NVMe flash drive

go test ./wal -bench . -benchmem -benchtime=5s
goos: linux
goarch: amd64
pkg: github.com/prometheus/tsdb/wal
BenchmarkWAL_LogBatched/compress=true-4                  5000000              1590 ns/op        1287.93 MB/s           0 B/op          0 allocs/op
BenchmarkWAL_LogBatched/compress=false-4                 3000000              6327 ns/op         323.66 MB/s           0 B/op          0 allocs/op
BenchmarkWAL_Log/compress=true-4                         5000000              1555 ns/op        1316.45 MB/s           0 B/op          0 allocs/op
BenchmarkWAL_Log/compress=false-4                        1000000              5862 ns/op         349.36 MB/s           0 B/op          0 allocs/op

Closes #599

[krasi-georgiev]

I would say lets have it optional and enabled by default so that if people run into some sort of performance issues can always disable. I guess some people would prefer high performance where others would prefer lower disk usage.

[csmarchbanks]

My only concern with having it enabled by default is that a user will lose any compressed WAL data if they roll back their Prometheus version. Perhaps have one release of Prometheus with default false, and then default it to true?

[brian-brazil]

Thus far we've enabled this sort of thing by default, we only promise that you can roll forward.

[beorn7]

Thus far we've enabled this sort of thing by default, we only promise that you can roll forward.

Actually, we were very conservative at times with changes that prevent a roll-back. At other time, we were not. The latter has caused some headaches with users. At least initially, I'd always leave the default at the old (roll-back-able) setting. Once a minor release has worked fine with the new setting, we can consider flipping the default.

I do get the promise of not necessarily supporting a roll-back. However, we can still be conservative and introduce a new feature as an opt-in first, simply to avoid the scenario that a bug (which might or might not be related to the new feature) is discovered in the new minor release but then you cannot roll back.

[csmarchbanks]

First test of this in Prometheus is going well. Only ingesting 8k samples per second on this instance, but I see no significant increase in CPU usage. The compression ratio for my data is steady at 2.1x.

[krasi-georgiev]

In this case a potential bug would cause the user to delete the WAL folder(3h data loss) and starting Prometheus with the the compression disabled so I think this should be enough of an option to not need to roll back to a previous Prometheus version.

I am leaning towards having it enabled by default with an option to disable so that we can have as many people as possible to test it and report bugs.
Once considered stable(after 1-2 releases) make it permanently enabled and remove the option to disable it.

@csmarchbanks is this ready for a review?

[csmarchbanks]

This is ready for a review, thanks in advance!

[krasi-georgiev]

just few nits, overall very good PR and ready to be merged.

the RSS RAM usage has increased so maybe worth it to run some profiling. Maybe the buf is escaping in the heap.
http://prombench.prometheus.io/grafana/d/7gmLoNDmz/prombench?orgId=1&var-RuleGroup=All&var-pr-number=5592&from=1558629647622&to=1558690279046

[beorn7]

In this case a potential bug would cause the user to delete the WAL folder(3h data loss) and starting Prometheus with the the compression disabled so I think this should be enough of an option to not need to roll back to a previous Prometheus version.

I am leaning towards having it enabled by default with an option to disable so that we can have as many people as possible to test it and report bugs.
Once considered stable(after 1-2 releases) make it permanently enabled and remove the option to disable it.

Sorry for beating this seemingly dead horse again.

I agree that the stakes are not very high in this case. But that's perhaps a good opportunity to practice how we introduce un-rollback-able changes. We can also re-build the trust that we might have lost during the problematic v2.1 to v2.3 releases.

How does the following look to you:

• v2.11: Introduce WAL compression as opt-in via flag. If v2.11 has a possibly unrelated bug that requires a rollback, only those are affected that explicitly opted in. We'll still get a fair amount of users testing it, but the blast radius is limited (and in particular, nobody will feel treated unfairly).
• v2.12: If things look good in v2.11, make the compression the new default value of the flag. If v2.12 has an unrelated bug, roll-back is not a problem if you set the flag explicitly in v2.11. We will now got almost everybody using to reach the final level of confidence.
• v2.13: If there is no reason to ever switch off compression (which we should know by now), make the flag a no-op. (I wouldn't be surprised if some people don't want compression. We have seen users being CPU-bound during WAL replaying, complaining about the long time it took. They will prefer an uncompressed WAL, while those being I/O bound will prefer the compressed WAL.)

This is not much slower (if at all) than what you suggested (make it default and remove flag two releases later).

[krasi-georgiev]

yep looks like a good plan.

We'll still get a fair amount of users testing it, but the blast radius is limited

I hope enough people really enable it to report bugs

I wouldn't be surprised if some people don't want compression. We have seen users being CPU-bound during WAL replaying, complaining about the long time it took. They will prefer an uncompressed WAL, while those being I/O bound will prefer the compressed WAL.

so far the tests suggest that CPU usage hasn't increased. Looks like snappy is a very light weight compression.

[krasi-georgiev]

LGTM

ping @gouthamve

[krasi-georgiev]

actually do you think you can also add a test to ensure that the compression actually works.
Maybe:

1. Open a db with uncompressed WAL.
2. Write some samples.
3. Take a reading of the WAL.
4. Open a db with compressed WAL.
5. Write some samples.
6. Compare the size of the WAL.

[csmarchbanks]

Added a test and removed the metrics. Let me know if there is anything else you would like me to do!

[SuperQ]

👍 to @beorn7's timeline proposal. Having a quick rollback for users that hit other bugs is a valid reason to not jump right to default compression.

[krasi-georgiev]

LGTM
ping @gouthamve @codesome

[csmarchbanks]

Rebased this to fix conflicts.

Bump for a second set of eyes: @gouthamve @codesome

[codesome]

I gave a quick look at the main implementation and it looks good. Just some nits.
I trust @krasi-georgiev's review on the things that I didn't touch.

[csmarchbanks]

Thanks for the review @codesome! I made the changes, and rebased once more due to conflicts in head_test.go.

[csmarchbanks]

Ping for one final look from @codesome or @gouthamve since my last updates.

[csmarchbanks]

Rebased again, @codesome and @krasi-georgiev would love to get this merged soon!

[krasi-georgiev]

@csmarchbanks yes I reminder @codesome to have a final look and should get it in in the next few days.

[codesome]

Need to add an entry in CHANGELOG.

[csmarchbanks]

Thanks @codesome! I have added a CHANGELOG entry.

[krasi-georgiev]

Thanks, hopefully we didn't brake anything with this compression 😺

[krasi-georgiev]

@csmarchbanks I just remembered that we didn't add a test to check the behaviour when starting Prometheus with an existing uncompressed wal. In other words make sure it handles well a WAL file that contains compressed and uncompressed records

[csmarchbanks]

@krasi-georgiev Since #608 I don't think there will ever be a mixed WAL file. Also, since the reader is the same for a compressed or uncompressed WAL file (it looks at whether each record is compressed or not), the code path should be tested whenever we test with compress=false.

If you think it is worth an explicit test I can write one soon though.

[krasi-georgiev]

you are right #608 is enough. Lets leave as is.