feat(tsdb): allow using ST capable XOR chunks - set format on read#18026
feat(tsdb): allow using ST capable XOR chunks - set format on read#18026krajorama wants to merge 2 commits into
Conversation
643cc12 to
ac2cd89
Compare
|
trivial rebase, no conflicts |
ac2cd89 to
33a08b3
Compare
ywwg
left a comment
ywwg
left a comment
There was a problem hiding this comment.
As you said, this is mostly adding bools and tests, with just a little uncontroversial logic changes. I have a couple questions, mostly around:
- some of the code seems to imply that the st-storage-enabled setting could maybe change at run time, but I don't think we want that to be possible, so maybe those calls could be simplified? But I want to understand better
- I don't quite understand the distinction between EncXOROptST and EncodingForFloatST and when one or the other is meant to be used.
| func (pb *Block) SetSTStorageEnabled(enabled bool) { | ||
| if cr, ok := pb.chunkr.(*chunks.Reader); ok { | ||
| cr.SetSTStorageEnabled(enabled) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
does this imply the setting could be changed at runtime, or is this only used during setup? I worry about keeping things consistent everywhere if the st storage setting could be changed while prometheus is already started up
There was a problem hiding this comment.
You cannot change feature flag in Prometheus on the fly, but this is for multi tenant derivatives like Cortex.
| block, err := OpenBlock(db.logger, filepath.Join(db.dir, blockID), nil, postingsDecoderFactory) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| block.SetSTStorageEnabled(db.stStorageEnabled) |
There was a problem hiding this comment.
ok I guess this answers my question above. I guess, why is ststorage not a parameter on the openBlock call?
(one day I will refactor prometheus to use the .WithOption() pattern for object creation to avoid these long constructors)
|
|
||
| db.mtx.RLock() | ||
| loadable, corrupted, err := openBlocks(db.logger, db.dir, db.blocks, db.chunkPool, db.opts.PostingsDecoderFactory) | ||
| loadable, corrupted, err := openBlocks(db.logger, db.dir, db.blocks, db.chunkPool, db.opts.PostingsDecoderFactory, db.opts.EnableSTStorage) |
There was a problem hiding this comment.
like here it's an option on the open call...
| // EnableSTStorage determines whether databases (WAL/WBL, tsdb, | ||
| // agent) should set a Start Time value per sample. | ||
| // Represents 'st-storage' feature flag. | ||
| EnableSTStorage atomic.Bool |
There was a problem hiding this comment.
To be in line with the other settings that can be set per tenant.
542fbfa to
c68e204
Compare
I'm mindful of multi tenant implementations where you'd want this setting to be dynamic and also per DB (aka tenant).
You're right |
6a04151 to
df77ed3
Compare
| ConvertClassicHistogramsToNHCB: sl.convertClassicHistToNHCB, | ||
| KeepClassicOnClassicAndNativeHistograms: sl.alwaysScrapeClassicHist, | ||
| OpenMetricsSkipSTSeries: sl.enableSTZeroIngestion, | ||
| OpenMetricsSkipSTSeries: sl.enableSTZeroIngestion || sl.enableSTStorage, |
There was a problem hiding this comment.
| OpenMetricsSkipSTSeries: sl.enableSTZeroIngestion || sl.enableSTStorage, | |
| OpenMetricsSkipSTSeries: sl.enableSTZeroIngestion | | sl.parseST, |
Signed-off-by: bwplotka <[email protected]>
|
This turned out to be a bit more complicated then the alternative #18013 . Putting it aside for now, we'll try the alternative. |
Co-Authored-By: Claude Sonnet 4.5 <[email protected]> Signed-off-by: György Krajcsovits <[email protected]>
67d4228 to
363e89b
Compare
7f5534f to
e25dc28
Compare
|
Alternative merged. |
3 participants
Two big changes
--st-storageflag for promtool) to decide what chunk encoding to use when re-coding chunks.The second change is there to make sure we don't convert existing chunks to a new format accidentally.
Based on #17909.
Finishes #17791 for floats.
Which issue(s) does the PR fix:
Does this PR introduce a user-facing change?