-
Notifications
You must be signed in to change notification settings - Fork 498
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
Update to Mimir-Prometheus 0db77b4c7 #3682
Conversation
14b9fb9
to
47b2810
Compare
47b2810
to
021ce71
Compare
`tsdb/wal` was renamed to `tsdb/wlog`. Several other libraries were bumped by this, including prometheus/client-go and prometheus/exporter-toolkit.
Required to build against prometheus/exporter-toolkit.
Copied from the sparsehistograms branch. Co-authored-by: Levi Harrison <git@leviharrison.dev> Co-authored-by: Jeanette Tan <jeanette.tan@grafana.com>
They are no longer identical in memory layout. This puts up the cost of query sharding slightly.
fd1f1ef
to
4de050e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have reviewed the non-vendored changes. Mostly LGTM, with some minor comments.
My main concern is that we're taking some assumptions on values being floats that could translate into bugs once histograms are implemented.
I wonder if the people implementing histograms will just have to revisit every single usage of chunkenc.ValFloat
, or we could hint them by having our own mimirchunk.ValFloatTODO = chunkenc.ValFloat
so they would pay special attention to that ValFloatTODO
. This especially corresponds to every place where we do Next() == chunkenc.ValFloat
and Seek(...) == chunkenc.ValFloat
comparisons.
I'll try to take at least a quick look at the vendored changes before approving this.
res = append(res, SampleStream{ | ||
Labels: mimirpb.FromLabelsToLabelAdapters(series.Metric), | ||
Samples: mimirpb.FromPointsToSamples(series.Points), | ||
Samples: fromPointsToSamples(series.Points), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since SampleStream
is a local struct, can we change it in a way that it would still support the casting instead of creating a new slice of samples here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
probably yes, but needs some work to align the mimir.proto
definition with promql.Point definition.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Made an issue: #3738
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Going back to @colega original suggestion, I think adding a dummy pointer into the structure might make it possible to keep the original cast for cases when all samples are floats - so until we officially support native histograms basically.
@@ -271,7 +272,7 @@ func TestNewMockShardedqueryable(t *testing.T) { | |||
seriesCt++ | |||
iter := set.At().Iterator() | |||
samples := 0 | |||
for iter.Next() { | |||
for iter.Next() == chunkenc.ValFloat { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would say this should be:
for iter.Next() == chunkenc.ValFloat { | |
for iter.Next() != chunkenc.ValNone { |
We don't have histogram samples now, but this way the code should work once we start having them, wdyt?
(This is not a blocker comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm also adding an assert that specifies that this mock function is generating float values
@@ -450,7 +451,7 @@ func seriesSetToSampleStreams(set storage.SeriesSet) ([]SampleStream, error) { | |||
stream := SampleStream{Labels: mimirpb.FromLabelsToLabelAdapters(set.At().Labels())} | |||
|
|||
it := set.At().Iterator() | |||
for it.Next() { | |||
for it.Next() == chunkenc.ValFloat { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar here, I'd get the type of next, and maybe fail the test if we find one of those values? That will save debugging time for the engineers implementing histograms and fixing these tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changing, plus adding error return if series contains non float val
pkg/ingester/ingester.go
Outdated
for it.Next() != chunkenc.ValNone { | ||
t, v := it.At() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should fail if we get a native histogram here.
for it.Next() != chunkenc.ValNone { | |
t, v := it.At() | |
for valType := it.Next(); valType != chunkenc.ValNone; valType = it.Next() { | |
if valType != chunkenc.ValFloat { | |
return 0, 0, fmt.Errorf("unsupported value type: %v", valType) | |
t, v := it.At() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, adding this code
pkg/querier/batch/stream.go
Outdated
if !ensureBatchType(valueTypes) { | ||
ensureBatchType(valueTypes) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should do the same? Also it seems that this is the only usage of ensureBatchType
so the return value isn't considered.
if !ensureBatchType(valueTypes) { | |
ensureBatchType(valueTypes) | |
} | |
ensureBatchType(valueTypes) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a bit more complicated. There's a special case where upon noticing that the Batch type is wrong, we need to close the current Batch (call nextBatch) and then check again that the Batch is of the correct type. So basically run the algorithm twice, but on the second try we can be sure that we're not in the middle of a Batch. I'll try to make this nicer.
batch.Timestamps[j] = t | ||
batch.FloatHistogramValues[j] = fh | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we have a default: panic case here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, adding
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've also reviewed the non-prometheus vendored deps (I assume prometheus was reviewed when it was brought to mimir-prometheus, plus it's just an insane number of changes).
Changes LGTM modulo the previously mentioned comments, but unblocking with an approval.
@colega Thank you for your comments! These changes were picked from a branch that is still WIP and far from being ready for review, and many of these changes are just temporary placeholders to get things to work for now and will definitely be improved before the PR is marked ready, e.g. we do things like |
Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>
pkg/querier/batch/stream.go
Outdated
// if the current batch being appended is full. | ||
checkForFullBatch := func() { | ||
// Step to the next Batch in the result, create it if it does not exist | ||
nextBatch := func(valueTypes chunkenc.ValueType) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nextBatch := func(valueTypes chunkenc.ValueType) { | |
nextBatch := func(valueType chunkenc.ValueType) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reverted
pkg/querier/batch/stream.go
Outdated
@@ -113,3 +175,42 @@ func mergeStreams(left, right batchStream, result batchStream, size int) batchSt | |||
result.reset() | |||
return result | |||
} | |||
|
|||
func createBatch(valueTypes chunkenc.ValueType) (batch chunk.Batch) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
func createBatch(valueTypes chunkenc.ValueType) (batch chunk.Batch) { | |
func createBatch(valueType chunkenc.ValueType) (batch chunk.Batch) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reverted
pkg/querier/batch/stream.go
Outdated
} | ||
|
||
// Call on uninitialized Batch to allocate the correct valueType array | ||
func initBatchType(batch *chunk.Batch, valueTypes chunkenc.ValueType) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
func initBatchType(batch *chunk.Batch, valueTypes chunkenc.ValueType) { | |
func initBatchType(batch *chunk.Batch, valueType chunkenc.ValueType) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reverted
pkg/querier/batch/stream.go
Outdated
} | ||
} | ||
|
||
func populate(b *chunk.Batch, s batchStream, valueTypes chunkenc.ValueType) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
func populate(b *chunk.Batch, s batchStream, valueTypes chunkenc.ValueType) { | |
func populate(b *chunk.Batch, s batchStream, valueType chunkenc.ValueType) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reverted
Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>
pkg/querier/batch/stream.go
Outdated
right.next() | ||
} else { | ||
if (rt == chunkenc.ValHistogram || rt == chunkenc.ValFloatHistogram) && lt == chunkenc.ValFloat { | ||
// Prefer historgrams over floats. Take left side if both has histograms. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment "Take left side if both has histograms." should be moved inside the else
branch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reverted
pkg/querier/batch/stream.go
Outdated
|
||
// mergeStreams merges streams of Batches of the same series over time. | ||
// Samples are simply merged by time when they are the same type (float/histogram/...), with the left stream taking precedence if the timestamps are equal. | ||
// When sample are different type, batches are not merged. In case of equal timestamps, histograms take precedence since they have more information. | ||
func mergeStreams(left, right batchStream, result batchStream, size int) batchStream { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have a benchmark here to check if all the function calls indirection introduce a performance penalty? This is an hot path for us.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reverted
pkg/querier/batch/stream.go
Outdated
for { | ||
if t := bs.hasNext(); t != chunkenc.ValNone { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't easier this way?
for { | |
if t := bs.hasNext(); t != chunkenc.ValNone { | |
for t := bs.hasNext(); t != chunkenc.ValNone; t = bs.hasNext() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reverted
pkg/querier/batch/stream.go
Outdated
} | ||
|
||
// Ensure that the batch we're writing to is not full and of the right type. | ||
ensureBatch := func(valueTypes chunkenc.ValueType) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ensureBatch := func(valueTypes chunkenc.ValueType) { | |
ensureBatch := func(valueType chunkenc.ValueType) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reverted
pkg/querier/batch/stream.go
Outdated
b = &result[resultLen-1] | ||
} | ||
|
||
ensureBatchType := func(valueTypes chunkenc.ValueType) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ensureBatchType := func(valueTypes chunkenc.ValueType) { | |
ensureBatchType := func(valueType chunkenc.ValueType) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reverted
pkg/querier/batch/stream.go
Outdated
func mergeStreams(left, right batchStream, result batchStream, size int) batchStream { | ||
|
||
// Reset the Index and Length of existing batches. | ||
for i := range result { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we reset the value type as well? I think we should have a unit test covering the case result
is reused and data types are different.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reverted
pkg/querier/batch/stream.go
Outdated
// then the one provided. | ||
result = append(result, createBatch(valueTypes)) | ||
} | ||
b = &result[resultLen-1] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why don't we directly set the b
type here to valueTypes
? The function nextBatch()
looks inconsistent to me, because the input valueTypes
is honored only if a new batch is created, but not reused.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reverted
pkg/querier/batch/stream.go
Outdated
result = append(result, chunk.Batch{}) | ||
} | ||
b = &result[resultLen-1] | ||
nextBatch(valueTypes) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Assuming the suggestion I left in nextBatch()
is correct, then here you can return after calling nextBatch()
. If we do return, then I would remove ensureBatchType()
function at all and inline it below here, given it's just used here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reverted
I quickly checked the vendored changes and LGTM. Prometheus changes correctness not checked (too many changes), but assuming logic is correct then I didn't see any change that requires missing changes on Mimir side (excluding support for native histograms, which we're working on...). |
Error out if iterator is asked to merge non float chunks: New/Seek merge Otherwise handle non floats normally Next and nonOverlaping Seek Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>
The new code had issues and unknown performance. Mitigate risk by not handling histograms in Batches and restore the current way of working with additional checks to avoid histograms. Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>
This works out fine since the caller checks the ensuing batch.Length, thus propagating the emptiness of the iterator. Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>
Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>
Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>
Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>
Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>
Be explicit in asserts instead. Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good job! This simplified version looks definitely safer to merge to me. I left few last comments.
In few places you will find a suggestion about adding a "TODO comment". I'm not a big fan of TODO comments in the code, but I've seen it's a pattern you already adopted (in this PR there are already few TODOs). Up to you if you want to address these comments. My main fear is that we may forget about it in few places when we'll add native histogram support, but maybe it's a not justified fear, so I just defer to you.
pkg/querier/block_test.go
Outdated
@@ -482,7 +483,7 @@ func Benchmark_blockQuerierSeriesSet_iteration(b *testing.B) { | |||
set := blockQuerierSeriesSet{series: series} | |||
|
|||
for set.Next() { | |||
for t := set.At().Iterator(); t.Next(); { | |||
for t := set.At().Iterator(); t.Next() == chunkenc.ValFloat; { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't be != none?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, done
tools/tsdb-chunks/main.go
Outdated
ts, val := it.At() | ||
if ts < minTS { | ||
minTS = ts | ||
for valType := it.Next(); it.Err() == nil && valType != chunkenc.ValNone; valType = it.Next() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nit] By contract, Next()
must return ValNone
on error.
for valType := it.Next(); it.Err() == nil && valType != chunkenc.ValNone; valType = it.Next() { | |
for valType := it.Next(); valType != chunkenc.ValNone; valType = it.Next() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i thought of this, but wasn't sure, fixed
tools/tsdb-index-health/main.go
Outdated
@@ -338,8 +339,13 @@ func verifyChunks(l log.Logger, cr *chunks.Reader, lset labels.Labels, chks []ch | |||
prevTs := int64(-1) | |||
|
|||
it := ch.Iterator(nil) | |||
for it.Err() == nil && it.Next() { | |||
for valType := it.Next(); it.Err() == nil && valType != chunkenc.ValNone; valType = it.Next() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nit] By contract, Next()
must return ValNone
on error.
for valType := it.Next(); it.Err() == nil && valType != chunkenc.ValNone; valType = it.Next() { | |
for valType := it.Next(); valType != chunkenc.ValNone; valType = it.Next() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
tools/tsdb-print-chunk/main.go
Outdated
for it.Err() == nil && it.Next() { | ||
ts, val := it.At() | ||
fmt.Printf("%g\t%d (%s)\n", val, ts, timestamp.Time(ts).UTC().Format(time.RFC3339Nano)) | ||
for valType := it.Next(); it.Err() == nil && valType != chunkenc.ValNone; valType = it.Next() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nit] By contract, Next()
must return ValNone
on error.
for valType := it.Next(); it.Err() == nil && valType != chunkenc.ValNone; valType = it.Next() { | |
for valType := it.Next(); valType != chunkenc.ValNone; valType = it.Next() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
3..2..1.. LGTM! I just left a final nit. 🙇
c.h = c.h[:0] | ||
|
||
if c.currErr != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think should be moved above the line c.h = c.h[:0]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixing
Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>
Lots of interfaces change because of Sparse Histograms.
This PR doesn't make Mimir actually handle Sparse Histograms, but it lets us update to a newer Prometheus.
Possibly in line with upstream v2.40.2?
Much of the code in this PR was copied from the sparsehistograms branch.
Co-authored-by: Levi Harrison git@leviharrison.dev
Co-authored-by: Jeanette Tan jeanette.tan@grafana.com
Checklist
CHANGELOG.md
updated.