sidecar: fix issue #396: split response into chunks no bigger than 2^16 samples #718
Conversation
…2^16 to avoid overflowing XOR compression Signed-off-by: Robert Sullivan <[email protected]>
pkg/store/prometheus.go
Outdated
| @@ -176,6 +182,23 @@ func (p *PrometheusStore) Series(r *storepb.SeriesRequest, s storepb.Store_Serie | |||
| return nil | |||
| } | |||
|
|
|||
| // XoR encoding supports a max size of 2^16 - 1 samples, so we need | |||
| // to chunk all samples into groups of no more than 2^16 - 1 | |||
| func chunkSamples(series prompb.TimeSeries) [][]prompb.Sample { | |||
There was a problem hiding this comment.
This name could be misleading as proper chunking should be with max 120 samples (:
I think we should name it bit differently for reader to explicitly state it is fine to do biggestPossible chunks in our query case. Alternatively name it like this but add maxSamples int64 argument.
Also we could consider, as we have lot's of raw samples here to decrease mem consumption for sidecar, to reuse e.Samples instead of recreating multiple of arrays of prombp.Sample. Each prompb.Samples array with lenght 2^16 is 1MB worth of mem. Maybe not much to optimize this not sure. Depends on how many samples at the end you have. Plus with lazy GC and multiple series it might be quite large number.
Up to you, we can start with something readable like this and move for microoptimizations later on or move to slicing and operate on indexes only e.g (I wrote it in bus, so I did not test this)
var aggr []storepb.AggrChunk
var arr []prompb.Sample = e.Samples // arr will share same underlying array
for len(arr) > 0 {
len := len(arr)
if i > math.MaxUint16 {
len = math.MaxUint16
}
enc, cb, err := p.encodeChunk(arr[:len-1])
if err != nil {
return status.Error(codes.Unknown, err.Error())
}
aggr = append(aggr, storepb.AggrChunk{
MinTime: int64(arr[0].Timestamp),
MaxTime: int64(arr[len-1].Timestamp),
Raw: &storepb.Chunk{Type: enc, Data: cb},
})
arr = arr[len-1:]
}
What do you think?
pkg/store/prometheus.go
Outdated
| // We generally expect all samples of the requested range to be traversed | ||
| // so we just encode all samples into one big chunk regardless of size. | ||
| enc, cb, err := p.encodeChunk(e.Samples) | ||
| // XoR encoding supports a max size of 2^16 - 1 samples, so we need |
There was a problem hiding this comment.
| // XoR encoding supports a max size of 2^16 - 1 samples, so we need | |
| // XOR encoding supports a max size of 2^16 - 1 samples, so we need |
pkg/store/prometheus_test.go
Outdated
| testutil.Ok(t, err) | ||
| srv := newStoreSeriesServer(ctx) | ||
|
|
||
| err = proxy.Series(&storepb.SeriesRequest{ |
There was a problem hiding this comment.
you can inline testutil.Ok(t, proxy.Series... but not a blocker.
|
|
||
| // Regression test for https://github.com/improbable-eng/thanos/issues/396. | ||
| func TestPrometheusStore_Series_SplitSamplesIntoChunksWithMaxSizeOfUint16_e2e(t *testing.T) { | ||
| defer leaktest.CheckTimeout(t, 10*time.Second)() |
pkg/store/prometheus_test.go
Outdated
|
|
||
| a := p.Appender() | ||
|
|
||
| offset := int64(math.MaxUint16 + 5) |
There was a problem hiding this comment.
Worth to make it 3 chunks maybe for solid test? (:
|
@bwplotka I made those 3 changes you suggested |
split response into chunks no bigger than 2^16 to avoid overflowing XOR compression
Signed-off-by: Robert Sullivan [email protected]
Changes
In
prometheus.gowhen building the response, create multipleAggrChunkstructs each with a slice of samples not to exceed 2^16 number of samples. This is needed to not overflow the XOR encoding.Verification
We added a test to
prometheus_test.go, and we also shipped it to our environment where we were seeing the bug mentioned in issue #396. After pushing the changes, we saw a full 2 weeks of data in the same query that was previously cutting off the more recent week of data.