tsdb(wal): st-per-sample initial code and benchmarks#17671
Conversation
|
/prombench |
|
Incorrect Available Commands:
Advanced Flags for
Examples:
|
|
/prombench main |
|
⏱️ Welcome to Prometheus Benchmarking Tool. ⏱️ Compared versions: After the successful deployment (check status here), the benchmarking results can be viewed at: Available Commands:
|
|
Do you still need Prombench running? |
|
/prombench cancel |
|
Benchmark cancel is in progress. |
|
/prombench cancel |
|
Benchmark cancel is in progress. |
|
oh nevermind, the dashboard defaults to showing a valid prombench, in this case 17716. |
|
Changes to the serialization of samples:
As long as my assumptions about what these slices of SampleRefs usually look like, this should be an improvement across the board: "v0" here is a naive encoding of ST with every record similar to how T is recorded, as a delta with the first sample. |
|
some of these numbers are a little suspicious (98%?) so I will keep checking |
|
In very promising news, I had to make some tests bigger because the new written WAL size is so much smaller |
bwplotka
left a comment
There was a problem hiding this comment.
Thanks for this! Looks really good, just minor naming suggestions.
I am curious about using a more efficient per-sample delta encoding statements. Is V1 really less efficient than V2? How? 🤔 (V2 only adds more info in general, except first sample)
yes you're right, this text was leftover from my previous version which turned out not to be correct. I'll change the wording. |
|
What's the latest benchmark against V1 without ST support? |
|
I'm still working on the changes. I'll mark as ready for review when I've got the updated numbers. |
|
ok here's the benchstat comparison: only valid for the non-st examples since v1 doesn't support st: decode: |
|
raw stats for v2: encode: decode: |
|
/prombench |
|
/prombench cancel |
|
Benchmark cancel is in progress. |
|
What we've decided to do is create a new feature branch based on this PR instead of merging it to main. so @bwplotka, let's do a last review round, and then instead of merging I'll push to a feature branch and close this PR. |
6bba202 to
9842814
Compare
bwplotka
left a comment
There was a problem hiding this comment.
Thanks! Overall good. Reviewed mostly, but still didn't review the exact code for enc/Dec in details.
My point so far is to probably start using st on appenderV2 with the flag in this PR. Changing tests when this is not yet implemented is a bit odd, although we expect to do in the follow up 🙈
| require.NoError(t, err) | ||
| walSeriesCount += len(series) | ||
|
|
||
| case record.Samples, record.SamplesV2: |
There was a problem hiding this comment.
Should we actually assert V2 for enabled storage? I guess that means we change appender code to actually use flag. Let's either do this or not change tests, perhaps?
There was a problem hiding this comment.
I added asserts to confirm the correct type is used based on the test run
| s3 := labels.FromStrings("e", "f") | ||
| s4 := labels.FromStrings("g", "h") | ||
| for _, enableStStorage := range []bool{false, true} { | ||
| t.Run("enableStStorage="+strconv.FormatBool(enableStStorage), func(t *testing.T) { |
There was a problem hiding this comment.
I'm confused here, doesn't this follow the format since it's enableStStorage=[true/false]? Maybe I misunderstand the "case=a" format you describe.
| } | ||
| // Add a sample so that the series is not garbage collected. | ||
| samplesRec := record.RefSample{Ref: ref, T: 1000, V: 1000} | ||
| enc := record.Encoder{EnableSTStorage: enableStStorage} |
There was a problem hiding this comment.
We can skip appendV1 tests changes. Appender V1 will never use new WAL formats
There was a problem hiding this comment.
does that mean I can revert all v2 tests in non _v2 files?
There was a problem hiding this comment.
ah it looks like this is the only one anyway
|
Addressed notes, thanks! |
bwplotka
left a comment
There was a problem hiding this comment.
Something went wrong with this PR?
Maybe you tried merging main changes? It's very hard to review this form.
Let's focus on ST WAL only change in one PR, then main merges in another PRs (also I'd prefer if we honestly rebase the feature branch (much more cleaner)).
2114639 to
542fbfa
Compare
|
diff should be clean now |
bwplotka
left a comment
There was a problem hiding this comment.
LGTM just tiny comment nits, we can fix later. Thanks!
Initial implementation of #17790. Only implements ST-per-sample for Counters. Tests and benchmarks updated. Note: This increases the size of the RefSample object for all users, whether st-per-sample is turned on or not. Signed-off-by: Owen Williams <[email protected]>
542fbfa to
c68e204
Compare
…ncoding` (#18062) * feat(tsdb/chunkenc): add float chunk format with start timestamp support Signed-off-by: György Krajcsovits <[email protected]> * optimize code path and layout Signed-off-by: György Krajcsovits <[email protected]> * make new format usable in head Signed-off-by: György Krajcsovits <[email protected]> * fix issue with seeking to last sample again Signed-off-by: György Krajcsovits <[email protected]> * fix iterator benchmark for chunks not supporting ST Signed-off-by: György Krajcsovits <[email protected]> * reduce footprint of the xoroptst chunk iterator object It was 80 bytes with a lot of padding compared to the 56 bytes of the original xor chunk iterator. Made it 64 bytes, tightly packed. Signed-off-by: György Krajcsovits <[email protected]> * Fix benchmark expectations on ST in interator Signed-off-by: György Krajcsovits <[email protected]> * add inclusive delta test case Signed-off-by: György Krajcsovits <[email protected]> * make testcases independent of order Signed-off-by: György Krajcsovits <[email protected]> * drop unused code Signed-off-by: György Krajcsovits <[email protected]> * Drop commented out line Signed-off-by: György Krajcsovits <[email protected]> * documentation Signed-off-by: György Krajcsovits <[email protected]> * Small simplification in the doc Signed-off-by: György Krajcsovits <[email protected]> * Add delta st inclusive test case for random vt Signed-off-by: György Krajcsovits <[email protected]> * Switch to delta of difference of st to prev t from delta of delta of st. Signed-off-by: György Krajcsovits <[email protected]> * Write ST after T and V so we can write a single bit on the second sample Signed-off-by: György Krajcsovits <[email protected]> * verify chunk sample len function Signed-off-by: György Krajcsovits <[email protected]> * Reduce size of first st stored a little Signed-off-by: György Krajcsovits <[email protected]> * test the case where st equals the t Signed-off-by: György Krajcsovits <[email protected]> * add st equal t to bechmarks Signed-off-by: György Krajcsovits <[email protected]> * test(chunkenc): test that appender can contonue chunks Test that initializing a chunk appender from an existing chunk works correctly. Signed-off-by: György Krajcsovits <[email protected]> * fix(chunkenc): bug in initializing appender on existing chunk Signed-off-by: György Krajcsovits <[email protected]> * Add cases with jitter in the start time as well Signed-off-by: György Krajcsovits <[email protected]> * tsdb: ST-in-WAL: Counter implementation and benchmarks (#17671) Initial implementation of #17790. Only implements ST-per-sample for Counters. Tests and benchmarks updated. Note: This increases the size of the RefSample object for all users, whether st-per-sample is turned on or not. Signed-off-by: Owen Williams <[email protected]> * refactor: sed enableStStorage/enableSTStorage Signed-off-by: bwplotka <[email protected]> * feat[scrape]: add ST parsing support to scrape AppenderV2 flow (#18103) Signed-off-by: bwplotka <[email protected]> * feat(tsdb): change head opt EnableSTStorage to atomic (#18107) In downstream projects this needs to be set dynamically per tenant. Signed-off-by: György Krajcsovits <[email protected]> * Merge pull request #18108 from prometheus/bwplotka/fix scrape: add tests for ST appending; add warnings for ST feature flag users around _created drop * refact(tsdb): trivial rename (#18109) Signed-off-by: György Krajcsovits <[email protected]> * fix(tsdb): missing passing head option to wal/wbl write (#18113) Signed-off-by: György Krajcsovits <[email protected]> * feat(tsdb): allow using ST capable XOR chunks - retain format on read (#18013) * feat(tsdb): allow appending to ST capable XOR chunk optionally Only for float samples as of now. Supports for in-order and out-of-order samples. Make sure that on readout the ST capable chunks are returned automatically. When the chunks are returned as is, this is trivially true. When a chunk needs to be re-coded due to deletion (tombstone) markers, we take the encoding of the original chunk. When a chunk needs to be created from overlapping chunks, we observe whether ST is zero or not and create the new chunk based on that. Signed-off-by: György Krajcsovits <[email protected]> * fix test after merge Signed-off-by: bwplotka <[email protected]> * feat: RW2 sending ST support Signed-off-by: bwplotka <[email protected]> tmp Signed-off-by: bwplotka <[email protected]> * tests: test ST in a cheapest way possible Signed-off-by: bwplotka <[email protected]> * tests: add bench CLI recommended invokations Signed-off-by: bwplotka <[email protected]> * fixed tests after rebase Signed-off-by: bwplotka <[email protected]> * feat(chunkenc): replace xoroptst chunk encoding with xor2 XOR2 is based on #18238 With additional ST support. Signed-off-by: György Krajcsovits <[email protected]> * feat: add compliance RW sender test for agent Signed-off-by: bwplotka <[email protected]> * feat(agent): add support for appending ST Signed-off-by: bwplotka <[email protected]> * replace stray xoroptst words Signed-off-by: György Krajcsovits <[email protected]> * Apply suggestions from code review Co-authored-by: Copilot <[email protected]> Signed-off-by: Bartlomiej Plotka <[email protected]> * post merge conflict fixes Signed-off-by: bwplotka <[email protected]> * feat(tsdb): register st_storage in feature API Register the st-storage feature flag in the feature registry via the TSDB options, consistent with how other TSDB features like exemplar_storage and delayed_compaction are registered. Signed-off-by: György Krajcsovits <[email protected]> Coded with Claude Sonnet 4.6. * Document xor2-encoding feature flag Signed-off-by: Carrie Edwards <[email protected]> * Add xor2-encoding feature flag Signed-off-by: Carrie Edwards <[email protected]> * Update CHANGELOG Signed-off-by: Carrie Edwards <[email protected]> * Fix linting Signed-off-by: Carrie Edwards <[email protected]> * Remove setting of xor2 encoding option in db open Signed-off-by: Carrie Edwards <[email protected]> * Fix tests Signed-off-by: Carrie Edwards <[email protected]> * Fix linting Signed-off-by: Carrie Edwards <[email protected]> * Update feature flag description Signed-off-by: Carrie Edwards <[email protected]> * Update comments and feature flag description Signed-off-by: Carrie Edwards <[email protected]> * Update documentation for st-storage feature Signed-off-by: Carrie Edwards <[email protected]> * st: disconnect st-storage with xor2-encoding given planned experiments (#18316) * st: disconnect st-storage with xor2-encoding given planned experiments Signed-off-by: bwplotka <[email protected]> * Update docs/feature_flags.md Co-authored-by: George Krajcsovits <[email protected]> Signed-off-by: Bartlomiej Plotka <[email protected]> * Update docs/feature_flags.md Co-authored-by: George Krajcsovits <[email protected]> Signed-off-by: Bartlomiej Plotka <[email protected]> * Update docs/feature_flags.md Co-authored-by: George Krajcsovits <[email protected]> Signed-off-by: Bartlomiej Plotka <[email protected]> * Update docs/feature_flags.md Co-authored-by: George Krajcsovits <[email protected]> Signed-off-by: Bartlomiej Plotka <[email protected]> --------- Signed-off-by: bwplotka <[email protected]> Signed-off-by: Bartlomiej Plotka <[email protected]> Co-authored-by: George Krajcsovits <[email protected]> --------- Signed-off-by: György Krajcsovits <[email protected]> Signed-off-by: Ganesh Vernekar <[email protected]> Signed-off-by: Bryan Boreham <[email protected]> Signed-off-by: Aleksandr Smirnov <[email protected]> Signed-off-by: Mohammad Abbasi <[email protected]> Signed-off-by: matt-gp <[email protected]> Signed-off-by: Ian Kerins <[email protected]> Signed-off-by: SuperQ <[email protected]> Signed-off-by: dependabot[bot] <[email protected]> Signed-off-by: Arve Knudsen <[email protected]> Signed-off-by: ffgan <[email protected]> Signed-off-by: Patryk Prus <[email protected]> Signed-off-by: Owen Williams <[email protected]> Signed-off-by: bwplotka <[email protected]> Signed-off-by: 3Juhwan <[email protected]> Signed-off-by: Sammy Tran <[email protected]> Signed-off-by: Casie Chen <[email protected]> Signed-off-by: Dan Cech <[email protected]> Signed-off-by: kakabisht <[email protected]> Signed-off-by: Jeanette Tan <[email protected]> Signed-off-by: Divyansh Mishra <[email protected]> Signed-off-by: Varun Chawla <[email protected]> Signed-off-by: Martin Valiente Ainz <[email protected]> Signed-off-by: Kyle Eckhart <[email protected]> Signed-off-by: Julien Pivotto <[email protected]> Signed-off-by: Matthieu MOREL <[email protected]> Signed-off-by: Linas Medziunas <[email protected]> Signed-off-by: Björn Rabenstein <[email protected]> Signed-off-by: beorn7 <[email protected]> Signed-off-by: Sayuru <[email protected]> Signed-off-by: Matt <[email protected]> Signed-off-by: Bartlomiej Plotka <[email protected]> Signed-off-by: Carrie Edwards <[email protected]> Co-authored-by: Bartlomiej Plotka <[email protected]> Co-authored-by: Ganesh Vernekar <[email protected]> Co-authored-by: Bryan Boreham <[email protected]> Co-authored-by: Sasha <[email protected]> Co-authored-by: Mohammad Abbasi <[email protected]> Co-authored-by: matt-gp <[email protected]> Co-authored-by: Ian Kerins <[email protected]> Co-authored-by: SuperQ <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Arve Knudsen <[email protected]> Co-authored-by: Julien <[email protected]> Co-authored-by: ffgan <[email protected]> Co-authored-by: Patryk Prus <[email protected]> Co-authored-by: Ganesh Vernekar <[email protected]> Co-authored-by: Joe Adams <[email protected]> Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Co-authored-by: Owen Williams <[email protected]> Co-authored-by: 3Juhwan <[email protected]> Co-authored-by: Casie Chen <[email protected]> Co-authored-by: Dan Cech <[email protected]> Co-authored-by: hridyesh bisht <[email protected]> Co-authored-by: zenador <[email protected]> Co-authored-by: Divyansh Mishra <[email protected]> Co-authored-by: Varun Chawla <[email protected]> Co-authored-by: Martin Valiente Ainz <[email protected]> Co-authored-by: Kyle Eckhart <[email protected]> Co-authored-by: Matthieu MOREL <[email protected]> Co-authored-by: Linas Medžiūnas <[email protected]> Co-authored-by: Björn Rabenstein <[email protected]> Co-authored-by: beorn7 <[email protected]> Co-authored-by: Sayuru <[email protected]> Co-authored-by: Copilot <[email protected]> Co-authored-by: Carrie Edwards <[email protected]>

Implement an experimental "SamplesV2" encoding method that includes support for Start Time per sample. Part of #17790. Since each slice of refs is only one sample per series, compression quality will be highly dependent on how similar data is between series, ie similar scrape times or start times.
This is a very partial implementation, consisting only of regular counter support as proof-of-concept and not for histograms. This feature is off by default, but tests have been updated to use it. Configuration options exist for the feature for future use by a feature flag in the binary.
Benchmarks
A naive implementation should result in a 33% increase in data size since each record goes from
3*64to4*64bits. Numbers better than 33% are an improvement. In this case, because we add a marker byte when the ST is not easily predictable, we would expect a 37% increase at worst.Current benchmarks... I think I still need to work on generating representative artificial data samples, it's too easy to construct data that compresses much too well.
Does this PR introduce a user-facing change?
Increases size of all WAL RefSample objects by an int64 (a 33% increase) regardless of whether the new ST Storage option is enabled. Prombench does not show significant increase in memory usage, however.