Skip to content

Remove query_store_for_labels_enabled configuration #5984

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
## master / unreleased

* [CHANGE] Upgrade Dockerfile Node version from 14x to 18x. #5906
* [CHANGE] Ingester: Remove `-querier.query-store-for-labels-enabled` flag. Querying long-term store for labels is always enabled. #5984
* [ENHANCEMENT] Query Frontend/Querier: Added store gateway postings touched count and touched size in Querier stats and log in Query Frontend. #5892
* [ENHANCEMENT] Query Frontend/Querier: Returns `warnings` on prometheus query responses. #5916
* [ENHANCEMENT] Ingester: Allowing to configure `-blocks-storage.tsdb.head-compaction-interval` flag up to 30 min and add a jitter on the first head compaction. #5919 #5928
Expand Down
6 changes: 0 additions & 6 deletions docs/blocks-storage/querier.md
Original file line number Diff line number Diff line change
Expand Up @@ -117,12 +117,6 @@ querier:
# CLI flag: -querier.query-ingesters-within
[query_ingesters_within: <duration> | default = 0s]

# Deprecated (Querying long-term store for labels will be always enabled in
# the future.): Query long-term store for series, label values and label names
# APIs.
# CLI flag: -querier.query-store-for-labels-enabled
[query_store_for_labels_enabled: <boolean> | default = false]

# Enable returning samples stats per steps in query response.
# CLI flag: -querier.per-step-stats-enabled
[per_step_stats_enabled: <boolean> | default = false]
Expand Down
5 changes: 0 additions & 5 deletions docs/configuration/config-file-reference.md
Original file line number Diff line number Diff line change
Expand Up @@ -3644,11 +3644,6 @@ The `querier_config` configures the Cortex querier.
# CLI flag: -querier.query-ingesters-within
[query_ingesters_within: <duration> | default = 0s]

# Deprecated (Querying long-term store for labels will be always enabled in the
# future.): Query long-term store for series, label values and label names APIs.
# CLI flag: -querier.query-store-for-labels-enabled
[query_store_for_labels_enabled: <boolean> | default = false]

# Enable returning samples stats per steps in query response.
# CLI flag: -querier.per-step-stats-enabled
[per_step_stats_enabled: <boolean> | default = false]
Expand Down
14 changes: 1 addition & 13 deletions integration/query_frontend_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -361,18 +361,6 @@ func runQueryFrontendTest(t *testing.T, cfg queryFrontendTestConfig) {
require.Equal(t, http.StatusBadRequest, resp.StatusCode)
}

// In this test we do ensure that the /series start/end time is ignored and Cortex
// always returns series in ingesters memory. No need to repeat it for each user.
if userID == 0 {
start := now.Add(-1000 * time.Hour)
end := now.Add(-999 * time.Hour)

result, err := c.Series([]string{"series_1"}, start, end)
require.NoError(t, err)
require.Len(t, result, 1)
assert.Equal(t, model.LabelSet{labels.MetricName: "series_1"}, result[0])
}

// No need to repeat the query 400 test for each user.
if userID == 0 {
start := time.Unix(1595846748, 806*1e6)
Expand Down Expand Up @@ -400,7 +388,7 @@ func runQueryFrontendTest(t *testing.T, cfg queryFrontendTestConfig) {

wg.Wait()

extra := float64(3)
extra := float64(2)
if cfg.testMissingMetricName {
extra++
}
Expand Down
1 change: 0 additions & 1 deletion pkg/cortex/modules.go
Original file line number Diff line number Diff line change
Expand Up @@ -403,7 +403,6 @@ func (t *Cortex) initIngesterService() (serv services.Service, err error) {
t.Cfg.Ingester.DistributorShardingStrategy = t.Cfg.Distributor.ShardingStrategy
t.Cfg.Ingester.DistributorShardByAllLabels = t.Cfg.Distributor.ShardByAllLabels
t.Cfg.Ingester.InstanceLimitsFn = ingesterInstanceLimits(t.RuntimeConfig)
t.Cfg.Ingester.QueryStoreForLabels = t.Cfg.Querier.QueryStoreForLabels
t.Cfg.Ingester.QueryIngestersWithin = t.Cfg.Querier.QueryIngestersWithin
t.tsdbIngesterConfig()

Expand Down
11 changes: 5 additions & 6 deletions pkg/ingester/ingester.go
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,6 @@ type Config struct {
DistributorShardByAllLabels bool `yaml:"-"`

// Injected at runtime and read from querier config.
QueryStoreForLabels bool `yaml:"-"`
QueryIngestersWithin time.Duration `yaml:"-"`

DefaultLimits InstanceLimits `yaml:"instance_limits"`
Expand Down Expand Up @@ -1426,7 +1425,7 @@ func (i *Ingester) labelsValuesCommon(ctx context.Context, req *client.LabelValu
return &client.LabelValuesResponse{}, cleanup, nil
}

mint, maxt, err := metadataQueryRange(startTimestampMs, endTimestampMs, db, i.cfg.QueryStoreForLabels, i.cfg.QueryIngestersWithin)
mint, maxt, err := metadataQueryRange(startTimestampMs, endTimestampMs, db, i.cfg.QueryIngestersWithin)
if err != nil {
return nil, cleanup, err
}
Expand Down Expand Up @@ -1506,7 +1505,7 @@ func (i *Ingester) labelNamesCommon(ctx context.Context, req *client.LabelNamesR
return &client.LabelNamesResponse{}, cleanup, nil
}

mint, maxt, err := metadataQueryRange(req.StartTimestampMs, req.EndTimestampMs, db, i.cfg.QueryStoreForLabels, i.cfg.QueryIngestersWithin)
mint, maxt, err := metadataQueryRange(req.StartTimestampMs, req.EndTimestampMs, db, i.cfg.QueryIngestersWithin)
if err != nil {
return nil, cleanup, err
}
Expand Down Expand Up @@ -1586,7 +1585,7 @@ func (i *Ingester) metricsForLabelMatchersCommon(ctx context.Context, req *clien
return nil, cleanup, err
}

mint, maxt, err := metadataQueryRange(req.StartTimestampMs, req.EndTimestampMs, db, i.cfg.QueryStoreForLabels, i.cfg.QueryIngestersWithin)
mint, maxt, err := metadataQueryRange(req.StartTimestampMs, req.EndTimestampMs, db, i.cfg.QueryIngestersWithin)
if err != nil {
return nil, cleanup, err
}
Expand Down Expand Up @@ -2755,8 +2754,8 @@ func (i *Ingester) flushHandler(w http.ResponseWriter, r *http.Request) {
}

// metadataQueryRange returns the best range to query for metadata queries based on the timerange in the ingester.
func metadataQueryRange(queryStart, queryEnd int64, db *userTSDB, queryStoreForLabels bool, queryIngestersWithin time.Duration) (mint, maxt int64, err error) {
if queryIngestersWithin > 0 && queryStoreForLabels {
func metadataQueryRange(queryStart, queryEnd int64, db *userTSDB, queryIngestersWithin time.Duration) (mint, maxt int64, err error) {
if queryIngestersWithin > 0 {
// If the feature for querying metadata from store-gateway is enabled,
// then we don't want to manipulate the mint and maxt.
return queryStart, queryEnd, nil
Expand Down
3 changes: 0 additions & 3 deletions pkg/ingester/ingester_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2184,7 +2184,6 @@ func Test_Ingester_MetricsForLabelMatchers(t *testing.T) {
to int64
matchers []*client.LabelMatchers
expected []*cortexpb.Metric
queryStoreForLabels bool
queryIngestersWithin time.Duration
}{
"should return an empty response if no metric match": {
Expand Down Expand Up @@ -2254,7 +2253,6 @@ func Test_Ingester_MetricsForLabelMatchers(t *testing.T) {
expected: []*cortexpb.Metric{
{Labels: cortexpb.FromLabelsToLabelAdapters(fixtures[0].lbls)},
},
queryStoreForLabels: true,
queryIngestersWithin: time.Hour,
},
"should not return duplicated metrics on overlapping matchers": {
Expand Down Expand Up @@ -2323,7 +2321,6 @@ func Test_Ingester_MetricsForLabelMatchers(t *testing.T) {
EndTimestampMs: testData.to,
MatchersSet: testData.matchers,
}
i.cfg.QueryStoreForLabels = testData.queryStoreForLabels
i.cfg.QueryIngestersWithin = testData.queryIngestersWithin
res, err := i.MetricsForLabelMatchers(ctx, req)
require.NoError(t, err)
Expand Down
18 changes: 3 additions & 15 deletions pkg/querier/distributor_queryable.go
Original file line number Diff line number Diff line change
Expand Up @@ -36,13 +36,12 @@ type Distributor interface {
MetricsMetadata(ctx context.Context) ([]scrape.MetricMetadata, error)
}

func newDistributorQueryable(distributor Distributor, streamingMetdata bool, iteratorFn chunkIteratorFunc, queryIngestersWithin time.Duration, queryStoreForLabels bool) QueryableWithFilter {
func newDistributorQueryable(distributor Distributor, streamingMetdata bool, iteratorFn chunkIteratorFunc, queryIngestersWithin time.Duration) QueryableWithFilter {
return distributorQueryable{
distributor: distributor,
streamingMetdata: streamingMetdata,
iteratorFn: iteratorFn,
queryIngestersWithin: queryIngestersWithin,
queryStoreForLabels: queryStoreForLabels,
}
}

Expand All @@ -51,7 +50,6 @@ type distributorQueryable struct {
streamingMetdata bool
iteratorFn chunkIteratorFunc
queryIngestersWithin time.Duration
queryStoreForLabels bool
}

func (d distributorQueryable) Querier(mint, maxt int64) (storage.Querier, error) {
Expand All @@ -62,7 +60,6 @@ func (d distributorQueryable) Querier(mint, maxt int64) (storage.Querier, error)
streamingMetadata: d.streamingMetdata,
chunkIterFn: d.iteratorFn,
queryIngestersWithin: d.queryIngestersWithin,
queryStoreForLabels: d.queryStoreForLabels,
}, nil
}

Expand All @@ -77,7 +74,6 @@ type distributorQuerier struct {
streamingMetadata bool
chunkIterFn chunkIteratorFunc
queryIngestersWithin time.Duration
queryStoreForLabels bool
}

// Select implements storage.Querier interface.
Expand All @@ -91,19 +87,11 @@ func (q *distributorQuerier) Select(ctx context.Context, sortSeries bool, sp *st
minT, maxT = sp.Start, sp.End
}

// If the querier receives a 'series' query, it means only metadata is needed.
// For the specific case where queryStoreForLabels is disabled
// we shouldn't apply the queryIngestersWithin time range manipulation.
// Otherwise we'll end up returning no series at all for
// older time ranges (while in Cortex we do ignore the start/end and always return
// series in ingesters).
shouldNotQueryStoreForMetadata := (sp != nil && sp.Func == "series" && !q.queryStoreForLabels)

// If queryIngestersWithin is enabled, we do manipulate the query mint to query samples up until
// We should manipulate the query mint to query samples up until
// now - queryIngestersWithin, because older time ranges are covered by the storage. This
// optimization is particularly important for the blocks storage where the blocks retention in the
// ingesters could be way higher than queryIngestersWithin.
if q.queryIngestersWithin > 0 && !shouldNotQueryStoreForMetadata {
if q.queryIngestersWithin > 0 {
now := time.Now()
origMinT := minT
minT = max(minT, util.TimeToMillis(now.Add(-q.queryIngestersWithin)))
Expand Down
20 changes: 5 additions & 15 deletions pkg/querier/distributor_queryable_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,6 @@ func TestDistributorQuerier_SelectShouldHonorQueryIngestersWithin(t *testing.T)

tests := map[string]struct {
querySeries bool
queryStoreForLabels bool
queryIngestersWithin time.Duration
queryMinT int64
queryMaxT int64
Expand Down Expand Up @@ -69,19 +68,10 @@ func TestDistributorQuerier_SelectShouldHonorQueryIngestersWithin(t *testing.T)
expectedMinT: 0,
expectedMaxT: 0,
},
"should not manipulate query time range if queryIngestersWithin is enabled and query max time is older, but the query is for /series": {
"should manipulate query time range if queryIngestersWithin is enabled": {
querySeries: true,
queryIngestersWithin: time.Hour,
queryMinT: util.TimeToMillis(now.Add(-100 * time.Minute)),
queryMaxT: util.TimeToMillis(now.Add(-90 * time.Minute)),
expectedMinT: util.TimeToMillis(now.Add(-100 * time.Minute)),
expectedMaxT: util.TimeToMillis(now.Add(-90 * time.Minute)),
},
"should manipulate query time range if queryIngestersWithin is enabled and queryStoreForLabels is enabled": {
querySeries: true,
queryStoreForLabels: true,
queryIngestersWithin: time.Hour,
queryMinT: util.TimeToMillis(now.Add(-100 * time.Minute)),
queryMaxT: util.TimeToMillis(now.Add(-30 * time.Minute)),
expectedMinT: util.TimeToMillis(now.Add(-60 * time.Minute)),
expectedMaxT: util.TimeToMillis(now.Add(-30 * time.Minute)),
Expand All @@ -100,7 +90,7 @@ func TestDistributorQuerier_SelectShouldHonorQueryIngestersWithin(t *testing.T)
distributor.On("MetricsForLabelMatchersStream", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return([]model.Metric{}, nil)

ctx := user.InjectOrgID(context.Background(), "test")
queryable := newDistributorQueryable(distributor, streamingMetadataEnabled, nil, testData.queryIngestersWithin, testData.queryStoreForLabels)
queryable := newDistributorQueryable(distributor, streamingMetadataEnabled, nil, testData.queryIngestersWithin)
querier, err := queryable.Querier(testData.queryMinT, testData.queryMaxT)
require.NoError(t, err)

Expand Down Expand Up @@ -139,7 +129,7 @@ func TestDistributorQueryableFilter(t *testing.T) {
t.Parallel()

d := &MockDistributor{}
dq := newDistributorQueryable(d, false, nil, 1*time.Hour, true)
dq := newDistributorQueryable(d, false, nil, 1*time.Hour)

now := time.Now()

Expand Down Expand Up @@ -189,7 +179,7 @@ func TestIngesterStreaming(t *testing.T) {
nil)

ctx := user.InjectOrgID(context.Background(), "0")
queryable := newDistributorQueryable(d, true, batch.NewChunkMergeIterator, 0, true)
queryable := newDistributorQueryable(d, true, batch.NewChunkMergeIterator, 0)
querier, err := queryable.Querier(mint, maxt)
require.NoError(t, err)

Expand Down Expand Up @@ -230,7 +220,7 @@ func TestDistributorQuerier_LabelNames(t *testing.T) {
d.On("MetricsForLabelMatchersStream", mock.Anything, model.Time(mint), model.Time(maxt), someMatchers).
Return(metrics, nil)

queryable := newDistributorQueryable(d, streamingEnabled, nil, 0, true)
queryable := newDistributorQueryable(d, streamingEnabled, nil, 0)
querier, err := queryable.Querier(mint, maxt)
require.NoError(t, err)

Expand Down
39 changes: 10 additions & 29 deletions pkg/querier/querier.go
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,6 @@ type Config struct {
IngesterMetadataStreaming bool `yaml:"ingester_metadata_streaming"`
MaxSamples int `yaml:"max_samples"`
QueryIngestersWithin time.Duration `yaml:"query_ingesters_within"`
QueryStoreForLabels bool `yaml:"query_store_for_labels_enabled"`
AtModifierEnabled bool `yaml:"at_modifier_enabled" doc:"hidden"`
EnablePerStepStats bool `yaml:"per_step_stats_enabled"`

Expand Down Expand Up @@ -103,14 +102,15 @@ func (cfg *Config) RegisterFlags(f *flag.FlagSet) {
flagext.DeprecatedFlag(f, "querier.iterators", "Deprecated: Use iterators to execute query. This flag is no longer functional; Batch iterator is always enabled instead.", util_log.Logger)
//lint:ignore faillint Need to pass the global logger like this for warning on deprecated methods
flagext.DeprecatedFlag(f, "querier.batch-iterators", "Deprecated: Use batch iterators to execute query. This flag is no longer functional; Batch iterator is always enabled now.", util_log.Logger)
//lint:ignore faillint Need to pass the global logger like this for warning on deprecated methods
flagext.DeprecatedFlag(f, "querier.query-store-for-labels-enabled", "Deprecated: Querying long-term store is always enabled.", util_log.Logger)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this intended?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah .. thi give a warning if the flag is still present on the cli.., make sense ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we set this flag when we start to deprecate the flag? I think the title is a bit of misleading...
Remove query_store_for_labels_enabled configuration we are actually deprecating the config here. Not removing.

Removing means to remove the flag entirely and it should throw error if specified


cfg.StoreGatewayClient.RegisterFlagsWithPrefix("querier.store-gateway-client", f)
f.IntVar(&cfg.MaxConcurrent, "querier.max-concurrent", 20, "The maximum number of concurrent queries.")
f.DurationVar(&cfg.Timeout, "querier.timeout", 2*time.Minute, "The timeout for a query.")
f.BoolVar(&cfg.IngesterMetadataStreaming, "querier.ingester-metadata-streaming", false, "Use streaming RPCs for metadata APIs from ingester.")
f.IntVar(&cfg.MaxSamples, "querier.max-samples", 50e6, "Maximum number of samples a single query can load into memory.")
f.DurationVar(&cfg.QueryIngestersWithin, "querier.query-ingesters-within", 0, "Maximum lookback beyond which queries are not sent to ingester. 0 means all queries are sent to ingester.")
f.BoolVar(&cfg.QueryStoreForLabels, "querier.query-store-for-labels-enabled", false, "Deprecated (Querying long-term store for labels will be always enabled in the future.): Query long-term store for series, label values and label names APIs.")
f.BoolVar(&cfg.EnablePerStepStats, "querier.per-step-stats-enabled", false, "Enable returning samples stats per steps in query response.")
f.DurationVar(&cfg.MaxQueryIntoFuture, "querier.max-query-into-future", 10*time.Minute, "Maximum duration into the future you can query. 0 to disable.")
f.DurationVar(&cfg.DefaultEvaluationInterval, "querier.default-evaluation-interval", time.Minute, "The default evaluation interval or step size for subqueries.")
Expand Down Expand Up @@ -159,7 +159,7 @@ func getChunksIteratorFunction(_ Config) chunkIteratorFunc {
func New(cfg Config, limits *validation.Overrides, distributor Distributor, stores []QueryableWithFilter, reg prometheus.Registerer, logger log.Logger) (storage.SampleAndChunkQueryable, storage.ExemplarQueryable, promql.QueryEngine) {
iteratorFunc := getChunksIteratorFunction(cfg)

distributorQueryable := newDistributorQueryable(distributor, cfg.IngesterMetadataStreaming, iteratorFunc, cfg.QueryIngestersWithin, cfg.QueryStoreForLabels)
distributorQueryable := newDistributorQueryable(distributor, cfg.IngesterMetadataStreaming, iteratorFunc, cfg.QueryIngestersWithin)

ns := make([]QueryableWithFilter, len(stores))
for ix, s := range stores {
Expand Down Expand Up @@ -261,7 +261,6 @@ func NewQueryable(distributor QueryableWithFilter, stores []QueryableWithFilter,
chunkIterFn: chunkIterFn,
limits: limits,
maxQueryIntoFuture: cfg.MaxQueryIntoFuture,
queryStoreForLabels: cfg.QueryStoreForLabels,
ignoreMaxQueryLength: cfg.IgnoreMaxQueryLength,
distributor: distributor,
stores: stores,
Expand All @@ -277,12 +276,11 @@ type querier struct {
now time.Time
mint, maxt int64

limits *validation.Overrides
maxQueryIntoFuture time.Duration
queryStoreForLabels bool
distributor QueryableWithFilter
stores []QueryableWithFilter
limiterHolder *limiterHolder
limits *validation.Overrides
maxQueryIntoFuture time.Duration
distributor QueryableWithFilter
stores []QueryableWithFilter
limiterHolder *limiterHolder

ignoreMaxQueryLength bool
}
Expand Down Expand Up @@ -361,15 +359,6 @@ func (q querier) Select(ctx context.Context, sortSeries bool, sp *storage.Select
}
// if SelectHints is null, rely on minT, maxT of querier to scope in range for Select stmt
sp = &storage.SelectHints{Start: mint, End: maxt}
} else if sp.Func == "series" && !q.queryStoreForLabels {
// Else if the querier receives a 'series' query, it means only metadata is needed.
// Here we expect that metadataQuerier querier will handle that.
// Also, in the recent versions of Prometheus, we pass in the hint but with Func set to "series".
// See: https://github.com/prometheus/prometheus/pull/8050

// In this case, the query time range has already been validated when the querier has been
// created.
return metadataQuerier.Select(ctx, true, sp, matchers...)
}

// Validate query time range. Even if the time range has already been validated when we created
Expand Down Expand Up @@ -433,7 +422,7 @@ func (q querier) Select(ctx context.Context, sortSeries bool, sp *storage.Select

// LabelValues implements storage.Querier.
func (q querier) LabelValues(ctx context.Context, name string, matchers ...*labels.Matcher) ([]string, annotations.Annotations, error) {
ctx, stats, _, _, _, metadataQuerier, queriers, err := q.setupFromCtx(ctx)
ctx, stats, _, _, _, _, queriers, err := q.setupFromCtx(ctx)
if err == errEmptyTimeRange {
return nil, nil, nil
} else if err != nil {
Expand All @@ -444,10 +433,6 @@ func (q querier) LabelValues(ctx context.Context, name string, matchers ...*labe
stats.AddQueryStorageWallTime(time.Since(startT))
}()

if !q.queryStoreForLabels {
return metadataQuerier.LabelValues(ctx, name, matchers...)
}

if len(queriers) == 1 {
return queriers[0].LabelValues(ctx, name, matchers...)
}
Expand Down Expand Up @@ -487,7 +472,7 @@ func (q querier) LabelValues(ctx context.Context, name string, matchers ...*labe
}

func (q querier) LabelNames(ctx context.Context, matchers ...*labels.Matcher) ([]string, annotations.Annotations, error) {
ctx, stats, _, _, _, metadataQuerier, queriers, err := q.setupFromCtx(ctx)
ctx, stats, _, _, _, _, queriers, err := q.setupFromCtx(ctx)
if err == errEmptyTimeRange {
return nil, nil, nil
} else if err != nil {
Expand All @@ -498,10 +483,6 @@ func (q querier) LabelNames(ctx context.Context, matchers ...*labels.Matcher) ([
stats.AddQueryStorageWallTime(time.Since(startT))
}()

if !q.queryStoreForLabels {
return metadataQuerier.LabelNames(ctx, matchers...)
}

if len(queriers) == 1 {
return queriers[0].LabelNames(ctx, matchers...)
}
Expand Down
Loading
Loading