Query the entire ingester range for series. #3347
Conversation
|
Depending on #3345 |
There was a problem hiding this comment.
I'm a bit dubious about always querying 0, math.MaxInt64. We have clusters with several days retention on ingesters and we would end up querying all ingesters blocks every time, even when the start/end time range is way smaller (think about "Last 1h" dashboards).
What if we try to do something smarter, like:
- If start/end time range is within the retention period, honor start/end time range
- Otherwise, query only the head (we would return series/labels different than what the user expect anyway)
gouthamve
force-pushed
the
query-all-ingester-for-series
branch
from
2a9abd1 to
302b226
Compare
gouthamve
force-pushed
the
query-all-ingester-for-series
branch
2 times, most recently
from
35062b8 to
1824eed
Compare
There was a problem hiding this comment.
Good job, thanks. LGTM! Could you add a CHANGELOG entry, please?
Note: @pstibrany also plan to review it by the end of the day. Do you mind waiting for his review too, please?
integration/querier_test.go
Outdated
| @@ -414,6 +414,291 @@ func TestQuerierWithBlocksStorageRunningInSingleBinaryMode(t *testing.T) { | |||
| } | |||
| } | |||
|
|
|||
| func TestMetadataQueriesWithBlocksStorage(t *testing.T) { | |||
There was a problem hiding this comment.
This test has been copied from the above ones. I think you could just add your assertions to the two tests above. You could create a function to assert on metadata queries, and call it from the two tests above.
By my initial quick look it looks good, but haven't fully checked the integration test yet. I will do that later today. |
With prometheus/prometheus#8050 we shouldn't be paging in chunks for the series API lookups. This would mean much lower memory utilisation and is similar to doing a range query for that period with the same query. With that optimisation, we should allow series API to query all of the ingester instead of just head block. Signed-off-by: Goutham Veeramachaneni <[email protected]>
Signed-off-by: Goutham Veeramachaneni <[email protected]>
Signed-off-by: Goutham Veeramachaneni <[email protected]>
Signed-off-by: Goutham Veeramachaneni <[email protected]>
Signed-off-by: Marco Pracucci <[email protected]>
pracucci
force-pushed
the
query-all-ingester-for-series
branch
from
dc3755d to
ab92eaf
Compare
integration/querier_test.go
Outdated
| } | ||
| } | ||
|
|
||
| func testMetadataQueriesWithBlocksStorage(t *testing.T, c *e2ecortex.Client, series1, series2, series3 []prompb.TimeSeries, blockRangePeriod time.Duration) { |
There was a problem hiding this comment.
Nit: I've found it bit confusing to express from and to in terms of series, and would suggest passing three points in time instead, relevant to the test:
- minT of oldest block in ingester
- minT of head
- maxT of head
There was a problem hiding this comment.
Reason I passed the timeseries is because we also need labels (eg. series3[0].Labels)
Signed-off-by: Marco Pracucci <[email protected]>
Signed-off-by: Marco Pracucci <[email protected]>
Signed-off-by: Marco Pracucci <[email protected]>
Signed-off-by: Marco Pracucci <[email protected]>
With prometheus/prometheus#8050 we shouldn't be
paging in chunks for the series API lookups. This would mean much lower
memory utilisation and is similar to doing a range query for that
period with the same query.
With that optimisation, we should allow series API to query all of the
ingester instead of just head block.
Signed-off-by: Goutham Veeramachaneni [email protected]
What this PR does:
Which issue(s) this PR fixes:
Fixes #
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]