Ingester: Metadata APIs should honour QueryIngestersWithin when QueryStoreForLabels is enabled #5027
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
harry671003
force-pushed
the
ingester_querying
branch
from
30e447a to
829e403
Compare
yeya24
approved these changes
Dec 8, 2022
harry671003
force-pushed
the
ingester_querying
branch
from
829e403 to
43db2a7
Compare
harry671003
force-pushed
the
ingester_querying
branch
2 times, most recently
from
5d1ab32 to
1737e53
Compare
alanprot
reviewed
Dec 9, 2022
pkg/querier/distributor_queryable.go
Outdated
| // older time ranges (while in Cortex we do ignore the start/end and always return | ||
| // series in ingesters). | ||
| // Also, in the recent versions of Prometheus, we pass in the hint but with Func set to "series". | ||
| if q.queryIngestersWithin > 0 && !(sp != nil && sp.Func == "series" && !q.queryStoreForLabels) { |
There was a problem hiding this comment.
nit: Is this condition too hard to read?
Maybe we could do something like:
// We should query store if `queryStoreForLabels` it set OR we need the chunks
shouldQueryStore := q.queryStoreForLabels || (sp != nil && sp.Func != "series")
if shouldQueryStore && queryIngestersWithin {
// Do the time manipulation
}
|
Should we enable query store by default? We may even consider deprecate this flag (and enable always) so we stop doing all these |
@alanprot - I can mark the flag as deprecated. Until we remove this flag, we need to do this checks in both ingesters and queriers so that the default behaviour (of querying all the blocks on ingesters) remains the same. Lmk, if I should deprecate this flag in this PR. |
harry671003
force-pushed
the
ingester_querying
branch
from
1737e53 to
745b808
Compare
Signed-off-by: 🌲 Harry 🌊 John 🏔 <[email protected]>
harry671003
force-pushed
the
ingester_querying
branch
from
d5e77d8 to
2a3d07a
Compare
alanprot
approved these changes
Dec 14, 2022
alexqyle
pushed a commit
to alexqyle/cortex
that referenced
this pull request
May 2, 2023
…StoreForLabels is enabled (cortexproject#5027) * Ingester Metadata APIs should honour QueryIngestersWithin Signed-off-by: 🌲 Harry 🌊 John 🏔 <[email protected]> * Deprecate query-store-for-labels-enabled flag Signed-off-by: 🌲 Harry 🌊 John 🏔 <[email protected]> Signed-off-by: 🌲 Harry 🌊 John 🏔 <[email protected]> Signed-off-by: Alex Le <[email protected]>
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.
What this PR does:
Before adding the feature to query
Ingesters will keep the blocks on disk for up-to:
-blocks-storage.tsdb.retention-period. This can be a lot more than theQueryIngestersWithinduration. The query path is already optimized to only query the blocks falling withinnow() - QueryIngestersWithinfrom ingesters. This PR adds the optimization also for the metadata APIs.Which issue(s) this PR fixes:
Fixes #
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]