Add streaming for ingester metadata APIs #4725
Conversation
harry671003
force-pushed
the
streaming-metrics-for-label
branch
2 times, most recently
from
b186d2d to
774cba9
Compare
| @@ -871,6 +871,10 @@ The `querier_config` configures the Cortex querier. | |||
| # CLI flag: -querier.ingester-streaming | |||
| [ingester_streaming: <boolean> | default = true] | |||
|
|
|||
| # Use streaming RPCs for metadata APIs from ingester. | |||
| # CLI flag: -querier.ingester-metadata-streaming | |||
| [ingester_metadata_streaming: <boolean> | default = true] | |||
There was a problem hiding this comment.
I am pondering on if we should have a new flag or just use the ingester_streaming flag. What's your thought on this?
I also worry about are we going to create new configuration flag as we enable streaming for other APIs like get labels or get examplars.
There was a problem hiding this comment.
I am pondering on if we should have a new flag or just use the ingester_streaming flag. What's your thought on this?
We need a way to migrate... so i think a new flag is needed indeed...
But i agree if we have more APIs that will be converted to streaming, we should do all at once.
There was a problem hiding this comment.
I've added streaming in LabelNames and LabelValues as well in this PR. This should reduce the number of new flags we need for live migration.
There was a problem hiding this comment.
If the default value of ingester_metadata_streaming is true, won't a lot of customers gets auto migrate to metadata stream?
There was a problem hiding this comment.
Good catch. I can change that. :)
pkg/distributor/distributor.go
Outdated
| metrics := map[model.Fingerprint]model.Metric{} | ||
|
|
||
| _, err = d.ForReplicationSet(ctx, replicationSet, func(ctx context.Context, client ingester_client.IngesterClient) (interface{}, error) { | ||
| stream, err := client.MetricsForLabelMatchersStream(ctx, req) |
There was a problem hiding this comment.
I assume we were using queryIngesterStream as example for this functino. In queryIngesterStream I see there is defer stream.CLoseSend() how come we don't need it here?
pkg/distributor/distributor.go
Outdated
| @@ -991,6 +992,64 @@ func (d *Distributor) MetricsForLabelMatchers(ctx context.Context, from, through | |||
| return result, nil | |||
| } | |||
|
|
|||
| func (d *Distributor) MetricsForLabelMatchersStream(ctx context.Context, from, through model.Time, matchers ...*labels.Matcher) ([]metric.Metric, error) { | |||
There was a problem hiding this comment.
There is quite a bit of duplicate code to MetricsForLabelMatchers, I wonder if we can refactor out some common code.
harry671003
force-pushed
the
streaming-metrics-for-label
branch
2 times, most recently
from
5556106 to
2445896
Compare
harry671003
force-pushed
the
streaming-metrics-for-label
branch
2 times, most recently
from
10a7c2b to
56a4373
Compare
harry671003
changed the title
harry671003
force-pushed
the
streaming-metrics-for-label
branch
5 times, most recently
from
06bc0d4 to
f9d19a4
Compare
|
LGTM |
harry671003
force-pushed
the
streaming-metrics-for-label
branch
from
f9d19a4 to
0b8bc2e
Compare
Signed-off-by: 🌲 Harry 🌊 John 🏔 <[email protected]>
harry671003
force-pushed
the
streaming-metrics-for-label
branch
from
0b8bc2e to
329e726
Compare
Signed-off-by: 🌲 Harry 🌊 John 🏔 [email protected]
What this PR does:
Currently, the
MetricsForLabelMatchers,LabelNames,LabelValuesAPIs doesn't support streaming. Because of this queriers have to load all the time-series into memory before themax_fetched_series_per_querylimit can be applied. This causes queriers to OOM when a sufficiently large query is made. This PR introduces streaming for this API reducing the amount of data that must be fetched before the limit should be applied.Which issue(s) this PR fixes:
Fixes #
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]