Querier: labels requests always fetch from store and respect time ranges (grafana#518)

* querier: always fetch labels from store

Remove description of the feature as experimental; make the option
`-querier.query-store-for-labels-enabled` always on.

* Querier: labels requests obey query-ingesters-within

If `-querier.query-ingesters-within` is set, make series, labels and
label-values requests to ingesters only within that time.

Ingesters may have data going back much further, depending on retention
setting, so it's more efficient to query just the window we won't get
from blocks storage.

Historically we only queried ingesters, so it was good to make the
request as long as possible, but now we always fetch labels from block
storage we don't need to.

* Use RegisterFlags with logger

* Review feedback

* Fix up calls to RegisterFlags

In other tools and places that use the config struct.

* Fix up test to match new behaviour.

* Ingesters: respect start/end time on labels&series requests

Previously ingester would return all current data if you asked for an
old range. Now it will return data in range.

With the new behaviour, if you don't set `EndTimestampMs` then it
is treated as zero and you will get nothing back. This is on the
internal gRPC API and it different to the Prometheus API which treats no
end time as "till the end of time".

One test needed fixing as it actually requested data from the wrong
time - the zero value `time.Time{}` is a specific point in the past.
Make it request data for all time instead.

Fix up other tests to match the new behaviour.

* Removed duplicated CHANGELOG entry

Signed-off-by: Marco Pracucci <marco@pracucci.com>

Co-authored-by: Marco Pracucci <marco@pracucci.com>

Author: bboreham

CHANGELOG.md

@@ -67,6 +67,7 @@
 * [CHANGE] Store-gateway: index cache now includes tenant in cache keys, this invalidates previous cached entries. #607
 * [CHANGE] Removed the deprecated `-<prefix>.fifocache.size` flag. #618
 * [CHANGE] Ruler: removed the support for the deprecated storage configuration via `-ruler.storage.*` CLI flags (and their respective YAML config options). Use `-ruler-storage.*` instead. #628
+* [CHANGE] Querier: always fetch labels from store and respect start/end times in request; the option `-querier.query-store-for-labels-enabled` is now always on. #518
 * [CHANGE] Query Frontend: range query response now omits the `data` field when it's empty (error case) like Prometheus does, previously it was `"data":{"resultType":"","result":null}`. #629
 * [CHANGE] Query Frontend: instant queries now honor the `-querier.max-retries-per-request` flag. #630
 * [CHANGE] Alertmanager: removed `-alertmanager.storage.*` configuration options, with the exception of the CLI flags `-alertmanager.storage.path` and `-alertmanager.storage.retention`. Use `-alertmanager-storage.*` instead. #632
@@ -161,6 +162,7 @@
 * [ENHANCEMENT] Ring/Memberlist: reduce CPU utilization for rings with a large number of members. #537 #563 #634
 * [ENHANCEMENT] Add histogram metrics `cortex_distributor_sample_delay_seconds` and `cortex_ingester_tsdb_sample_out_of_order_delta_seconds` #488
 * [ENHANCEMENT] Compactor: expose low-level concurrency options for compactor: `-compactor.max-opening-blocks-concurrency`, `-compactor.max-closing-blocks-concurrency`, `-compactor.symbols-flushers-concurrency`, used when using `split-and-merge` compaction strategy. #569
+* [ENHANCEMENT] Querier: labels requests now obey `-querier.query-ingesters-within`, making them a little more efficient. #518
 * [ENHANCEMENT] Store-gateway: label values with matchers now doesn't preload or list series, reducing latency and memory consumption. #534
 * [ENHANCEMENT] Azure client: expose option to configure MSI URL and user-assigned identity. #584
 * [ENHANCEMENT] Store-gateway: the results of `LabelNames()`, `LabelValues()` and `Series(skipChunks=true)` calls are now cached in the index cache. #590

cmd/mimir/main.go

@@ -76,7 +76,7 @@ func main() {
 
 	// This sets default values from flags to the config.
 	// It needs to be called before parsing the config file!
-	flagext.RegisterFlags(&cfg)
+	flagext.RegisterFlagsWithLogger(util_log.Logger, &cfg)
 
 	if configFile != "" {
 		if err := LoadConfig(configFile, expandENV, &cfg); err != nil {

docs/blocks-storage/querier.md

@@ -111,11 +111,6 @@ querier:
   # CLI flag: -querier.query-ingesters-within
   [query_ingesters_within: <duration> | default = 0s]
 
-  # Query long-term store for series, label values and label names APIs. Works
-  # only with blocks engine.
-  # CLI flag: -querier.query-store-for-labels-enabled
-  [query_store_for_labels_enabled: <boolean> | default = false]
-
   # True to enable queriers to use an optimized implementation which passes down
   # to ingesters the label matchers when running the label names API. Can be
   # enabled once all ingesters run a version >= the one where this option has

docs/configuration/config-file-reference.md

@@ -807,11 +807,6 @@ The `querier_config` configures the querier.
 # CLI flag: -querier.query-ingesters-within
 [query_ingesters_within: <duration> | default = 0s]
 
-# Query long-term store for series, label values and label names APIs. Works
-# only with blocks engine.
-# CLI flag: -querier.query-store-for-labels-enabled
-[query_store_for_labels_enabled: <boolean> | default = false]
-
 # True to enable queriers to use an optimized implementation which passes down
 # to ingesters the label matchers when running the label names API. Can be
 # enabled once all ingesters run a version >= the one where this option has been

docs/configuration/v1-guarantees.md

@@ -24,7 +24,6 @@ The Cortex maintainers commit to ensuring future version of Cortex can read data
 Cortex strives to be 100% API compatible with Prometheus (under `/prometheus/*` and `/api/prom/*`); any deviation from this is considered a bug, except:
 
 - Requiring the `__name__` label on queries when querying the [chunks storage](../chunks-storage/_index.md) (queries to ingesters or clusters running the blocks storage are not affected).
-- For queries to the `/api/v1/series`, `/api/v1/labels` and `/api/v1/label/{name}/values` endpoints, query's time range is ignored and the data is always fetched from ingesters. There is experimental support to query the long-term store with the _blocks_ storage engine when `-querier.query-store-for-labels-enabled` is set.
 - Additional API endpoints for creating, removing and modifying alerts and recording rules.
 - Additional API around pushing metrics (under `/api/push`).
 - Additional API endpoints for management of Cortex itself, such as the ring. These APIs are not part of the any compatibility guarantees.
@@ -54,7 +53,6 @@ Currently experimental features are:
 - OpenStack Swift storage support (both in blocks and chunks storage).
 - Metric relabeling in the distributor.
 - Scalable query-frontend (when using query-scheduler)
-- Querying store for series, labels APIs (`-querier.query-store-for-labels-enabled`)
 - Distributor: do not extend writes on unhealthy ingesters (`-distributor.extend-writes=false`)
 - Tenant Deletion in Purger, for blocks storage.
 - Query-frontend: query stats tracking (`-frontend.query-stats-enabled`)

integration/getting_started_single_process_config_test.go

@@ -8,6 +8,7 @@ package integration
 
 import (
 	"fmt"
+	"math"
 	"testing"
 	"time"
 
@@ -63,11 +64,15 @@ func TestGettingStartedSingleProcessConfigWithBlocksStorage(t *testing.T) {
 	require.Equal(t, model.ValVector, result.Type())
 	assert.Equal(t, expectedVector, result.(model.Vector))
 
-	labelValues, err := c.LabelValues("foo", time.Time{}, time.Time{}, nil)
+	// Work round Prometheus client lib not having a way to omit the start&end params
+	minTime := time.Unix(math.MinInt64/1000+62135596801, 0).UTC()
+	maxTime := time.Unix(math.MaxInt64/1000-62135596801, 999999999).UTC()
+
+	labelValues, err := c.LabelValues("foo", minTime, maxTime, nil)
 	require.NoError(t, err)
 	require.Equal(t, model.LabelValues{"bar"}, labelValues)
 
-	labelNames, err := c.LabelNames(time.Time{}, time.Time{})
+	labelNames, err := c.LabelNames(minTime, maxTime)
 	require.NoError(t, err)
 	require.Equal(t, []string{"__name__", "foo"}, labelNames)
 

integration/querier_test.go

@@ -674,13 +674,13 @@ func testMetadataQueriesWithBlocksStorage(
 			},
 			labelNames: []string{labels.MetricName, lastSeriesInStorageName, lastSeriesInIngesterBlocksName, firstSeriesInIngesterHeadName},
 		},
-		"query metadata entirely outside the ingester range should return the head data as well": {
+		"query metadata entirely outside the ingester range should not return the head data": {
 			from: lastSeriesInStorageTs.Add(-2 * blockRangePeriod),
 			to:   lastSeriesInStorageTs,
 			seriesTests: []seriesTest{
 				{
 					lookup: firstSeriesInIngesterHeadName,
-					ok:     true,
+					ok:     false,
 					resp:   firstSeriesInIngesterHead.Labels,
 				},
 				{
@@ -696,7 +696,7 @@ func testMetadataQueriesWithBlocksStorage(
 			labelValuesTests: []labelValuesTest{
 				{
 					label: labels.MetricName,
-					resp:  []string{lastSeriesInStorageName, firstSeriesInIngesterHeadName},
+					resp:  []string{lastSeriesInStorageName},
 				},
 				{
 					label:   labels.MetricName,
@@ -705,11 +705,11 @@ func testMetadataQueriesWithBlocksStorage(
 				},
 				{
 					label:   labels.MetricName,
-					resp:    []string{firstSeriesInIngesterHeadName},
+					resp:    []string{},
 					matches: []string{firstSeriesInIngesterHeadName},
 				},
 			},
-			labelNames: []string{labels.MetricName, lastSeriesInStorageName, firstSeriesInIngesterHeadName},
+			labelNames: []string{labels.MetricName, lastSeriesInStorageName},
 		},
 	}
 

integration/query_frontend_test.go

@@ -257,16 +257,14 @@ func runQueryFrontendTest(t *testing.T, cfg queryFrontendTestConfig) {
 			require.Regexp(t, "querier_wall_time;dur=[0-9.]*, response_time;dur=[0-9.]*$", res.Header.Values("Server-Timing")[0])
 		}
 
-		// In this test we do ensure that the /series start/end time is ignored and Mimir
-		// always returns series in ingesters memory. No need to repeat it for each user.
+		// Beyond the range of -querier.query-ingesters-within should return nothing. 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])
+			require.Len(t, result, 0)
 		}
 
 		for q := 0; q < numQueriesPerUser; q++ {

pkg/ingester/ingester_v2.go

@@ -9,7 +9,6 @@ import (
 	"context"
 	"fmt"
 	"io"
-	"math"
 	"net/http"
 	"os"
 	"path/filepath"
@@ -1164,12 +1163,7 @@ func (i *Ingester) LabelValues(ctx context.Context, req *client.LabelValuesReque
 		return &client.LabelValuesResponse{}, nil
 	}
 
-	mint, maxt, err := metadataQueryRange(startTimestampMs, endTimestampMs, db)
-	if err != nil {
-		return nil, err
-	}
-
-	q, err := db.Querier(ctx, mint, maxt)
+	q, err := db.Querier(ctx, startTimestampMs, endTimestampMs)
 	if err != nil {
 		return nil, err
 	}
@@ -1205,11 +1199,6 @@ func (i *Ingester) LabelNames(ctx context.Context, req *client.LabelNamesRequest
 		return nil, err
 	}
 
-	mint, maxt, err = metadataQueryRange(mint, maxt, db)
-	if err != nil {
-		return nil, err
-	}
-
 	q, err := db.Querier(ctx, mint, maxt)
 	if err != nil {
 		return nil, err
@@ -1247,11 +1236,7 @@ func (i *Ingester) MetricsForLabelMatchers(ctx context.Context, req *client.Metr
 		return nil, err
 	}
 
-	mint, maxt, err := metadataQueryRange(req.StartTimestampMs, req.EndTimestampMs, db)
-	if err != nil {
-		return nil, err
-	}
-
+	mint, maxt := req.StartTimestampMs, req.EndTimestampMs
 	q, err := db.Querier(ctx, mint, maxt)
 	if err != nil {
 		return nil, err
@@ -2370,33 +2355,6 @@ func (i *Ingester) FlushHandler(w http.ResponseWriter, r *http.Request) {
 	w.WriteHeader(http.StatusNoContent)
 }
 
-// metadataQueryRange returns the best range to query for metadata queries based on the timerange in the ingester.
-func metadataQueryRange(queryStart, queryEnd int64, db *userTSDB) (mint, maxt int64, err error) {
-	// Ingesters are run with limited retention and we don't support querying the store-gateway for labels yet.
-	// This means if someone loads a dashboard that is outside the range of the ingester, and we only return the
-	// data for the timerange requested (which will be empty), the dashboards will break. To fix this we should
-	// return the "head block" range until we can query the store-gateway.
-
-	// Now the question would be what to do when the query is partially in the ingester range. I would err on the side
-	// of caution and query the entire db, as I can't think of a good way to query the head + the overlapping range.
-	mint, maxt = queryStart, queryEnd
-
-	lowestTs, err := db.StartTime()
-	if err != nil {
-		return mint, maxt, err
-	}
-
-	// Completely outside.
-	if queryEnd < lowestTs {
-		mint, maxt = db.Head().MinTime(), db.Head().MaxTime()
-	} else if queryStart < lowestTs {
-		// Partially inside.
-		mint, maxt = 0, math.MaxInt64
-	}
-
-	return
-}
-
 func wrappedTSDBIngestErr(ingestErr error, timestamp model.Time, labels []mimirpb.LabelAdapter) error {
 	if ingestErr == nil {
 		return nil

pkg/ingester/ingester_v2_test.go

@@ -1248,7 +1248,7 @@ func Test_Ingester_LabelNames(t *testing.T) {
 		expected := []string{"__name__", "status", "route"}
 
 		// Get label names
-		res, err := i.LabelNames(ctx, &client.LabelNamesRequest{})
+		res, err := i.LabelNames(ctx, &client.LabelNamesRequest{EndTimestampMs: math.MaxInt64})
 		require.NoError(t, err)
 		assert.ElementsMatch(t, expected, res.LabelNames)
 	})
@@ -1311,7 +1311,7 @@ func Test_Ingester_LabelValues(t *testing.T) {
 
 	// Get label values
 	for labelName, expectedValues := range expected {
-		req := &client.LabelValuesRequest{LabelName: labelName}
+		req := &client.LabelValuesRequest{LabelName: labelName, EndTimestampMs: math.MaxInt64}
 		res, err := i.LabelValues(ctx, req)
 		require.NoError(t, err)
 		assert.ElementsMatch(t, expectedValues, res.LabelValues)
@@ -1802,7 +1802,7 @@ func TestIngester_LabelNames_ShouldNotCreateTSDBIfDoesNotExists(t *testing.T) {
 	// Mock request
 	userID := "test"
 	ctx := user.InjectOrgID(context.Background(), userID)
-	req := &client.LabelNamesRequest{}
+	req := &client.LabelNamesRequest{EndTimestampMs: math.MaxInt64}
 
 	res, err := i.LabelNames(ctx, req)
 	require.NoError(t, err)
@@ -1963,18 +1963,15 @@ func Test_Ingester_MetricsForLabelMatchers(t *testing.T) {
 				{Labels: mimirpb.FromLabelsToLabelAdapters(fixtures[2].lbls)},
 			},
 		},
-		"should NOT filter metrics by time range to always return known metrics even when queried for older time ranges": {
+		"should filter metrics by time range to return nothing when queried for older time ranges": {
 			from: 100,
 			to:   1000,
 			matchers: []*client.LabelMatchers{{
 				Matchers: []*client.LabelMatcher{
 					{Type: client.EQUAL, Name: model.MetricNameLabel, Value: "test_1"},
 				},
 			}},
-			expected: []*mimirpb.Metric{
-				{Labels: mimirpb.FromLabelsToLabelAdapters(fixtures[0].lbls)},
-				{Labels: mimirpb.FromLabelsToLabelAdapters(fixtures[1].lbls)},
-			},
+			expected: []*mimirpb.Metric{},
 		},
 		"should not return duplicated metrics on overlapping matchers": {
 			from: math.MinInt64,