Prevent OOMs in the chunk store.#873
Conversation
- Merge and dedupe set of strings, not sets of parse chunks. - Limit number of chunks fetched in a single query. - Add lots more debug logging to the querier to help track this all down. Signed-off-by: Tom Wilkie <tom.wilkie@gmail.com>
tomwilkie
force-pushed
the
chunk-store-oom
branch
from
d483fd0 to
e42d765
Compare
bboreham
left a comment
bboreham
left a comment
There was a problem hiding this comment.
Looking good - some small points
| } | ||
|
|
||
| var chunkSet ByKey | ||
| func (c *Store) parseIndexEntries(ctx context.Context, entries []IndexEntry, matcher *labels.Matcher) ([]string, error) { |
There was a problem hiding this comment.
I think it's worth retaining the comment that entries are returned in order, and here seems to be the right place for it.
pkg/chunk/storage/by_key_test.go
Outdated
| func (cs ByKey) Less(i, j int) bool { return lessByKey(cs[i], cs[j]) } | ||
|
|
||
| // This comparison uses all the same information as Chunk.ExternalKey() | ||
| func lessByKey(a, b chunk.Chunk) bool { |
There was a problem hiding this comment.
This function was a good idea when it saved the creation of millions of temp strings in queries; as part of a test it looks over-complicated.
pkg/chunk/chunk_store_test.go
Outdated
|
|
||
| func init() { | ||
| var al util.AllowedLevel | ||
| al.Set("debug") |
pkg/chunk/chunk_store.go
Outdated
| func (c *Store) Get(ctx context.Context, from, through model.Time, allMatchers ...*labels.Matcher) (model.Matrix, error) { | ||
|
|
||
| logger := util.WithContext(ctx, util.Logger) | ||
| level.Debug(logger).Log("msg", "ChunkStore.Get", "from", from, "through", through, "matchers", len(allMatchers)) |
There was a problem hiding this comment.
This log line is very similar to the tracing log line below; perhaps we should have a wrapper so we don't repeat very similar code?
pkg/chunk/chunk_store.go
Outdated
| return nil, err | ||
| } | ||
|
|
||
| level.Debug(logger).Log("func", "ChunkStore.getMetricNameChunks", "msg", "Chunks in index", "n", len(chunks)) |
There was a problem hiding this comment.
"msg" and "n" seem like an unnecessary level of indirection
pkg/chunk/chunk_store.go
Outdated
| } | ||
|
|
||
| // Merge entries in order because we wish to keep label series together consecutively | ||
| chunkIDs := nWayIntersectStrings(chunkIDSets) |
There was a problem hiding this comment.
Rather than assembling a list then breaking it down to intersect, I'd think it is cheaper to do the intersecting as the results come in. But that could be a subsequent change.
Signed-off-by: Tom Wilkie <tom.wilkie@gmail.com>
Signed-off-by: Tom Wilkie <tom.wilkie@gmail.com>
Signed-off-by: Tom Wilkie <tom.wilkie@gmail.com>
Signed-off-by: Tom Wilkie <tom.wilkie@gmail.com>
tomwilkie
force-pushed
the
chunk-store-oom
branch
from
323d857 to
16e1167
Compare
|
@bboreham PTAL? |
| return writeReqs, nil | ||
| } | ||
|
|
||
| type spanLogger struct { |
There was a problem hiding this comment.
comment describing the intent of this struct ?
Signed-off-by: Tom Wilkie <tom.wilkie@gmail.com>
|
Thanks! |
2 participants
This change moves the parsing and instantiation of the
struct Chunksafter the intersection of their IDs - we were finding on very high cardinality timeseries (~8m) and queriers would OOM just building the array of chunks from the index, even if the intersection of label matchers was relatively small.We also add a limit to the number of chunks fetched in a single query.
The big casualty here is removing the support for
metadataInIndexchunks - comment says these were last used in Nov 2016, so this is probably safe.Signed-off-by: Tom Wilkie tom.wilkie@gmail.com