Don't query high cardinality labels. #886
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This will allow us to vary the store implementation over time, and not just the schema. This will unblock the new bigtable storage adapter (using columns instead of rows), and allow us to more easily implement the iterative intersections and indexing of series instead of chunks. Signed-off-by: Tom Wilkie <[email protected]>
…r this schema. I tried to adapt the original chunk store to support this style of indexing - easy on the right path, but the read path became even more of a rats nest. So I factored out the common bits as best I could and made a new chunk store - the seriesStore. Signed-off-by: Tom Wilkie <[email protected]>
Signed-off-by: Tom Wilkie <[email protected]>
Signed-off-by: Tom Wilkie <[email protected]>
Signed-off-by: Tom Wilkie <[email protected]>
Signed-off-by: Tom Wilkie <[email protected]>
tomwilkie
changed the title
|
After some testing, this improved latency for those particular queries mentioned in #884 by >2x. |
bboreham
reviewed
Jul 19, 2018
pkg/chunk/chunk_store_test.go
Outdated
| var schemas = []struct { | ||
| name string | ||
| fn func(cfg SchemaConfig) Schema | ||
| schemaFn schemaFactory | ||
| storeFn storeFactory |
There was a problem hiding this comment.
I was expecting this member to come in as part of the refactor to multiple stores.
pkg/chunk/chunk_store_test.go
Outdated
| var schemas = []struct { | ||
| name string | ||
| fn func(cfg SchemaConfig) Schema | ||
| requireMetricName bool |
There was a problem hiding this comment.
I expected storeFn to arrive in this commit, even though they would all be the same.
| "time" | ||
| ) | ||
|
|
||
| // heapCache is a simple string -> interface{} cache which uses a heap to manage |
There was a problem hiding this comment.
Do we really need to write another cache?
| ) | ||
|
|
||
| // heapCache is a simple string -> interface{} cache which uses a heap to manage | ||
| // evictions. O(log N) inserts, O(n) get and update. |
There was a problem hiding this comment.
It looks like it should be O(1) get - is that a typo or am I missing something?
|
Going to roll this into #875 as I'm struggling to keep track of all my open PRs. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #884
This change limits the cardinality for labels; if the limit is exceeded, queries can still work, but there must be another selector with lower cardinality.
Notably, after this change, queries on two high-cardinality labels that would have results in a small number of series will fail.
Builds on seriesStores, so includes #875.