Use selectParams in Select() to tell if we need samples or metadata

Move from one brittle assumption about Prometheus to another.

Author: bboreham

cmd/lite/main.go

@@ -146,12 +146,11 @@ func main() {
 		defer rulerServer.Stop()
 	}
 
-	sampleQueryable := querier.NewQueryable(dist, chunkStore, false)
-	metadataQueryable := querier.NewQueryable(dist, chunkStore, true)
+	sampleQueryable := querier.NewQueryable(dist, chunkStore)
 
 	api := v1.NewAPI(
 		engine,
-		metadataQueryable,
+		sampleQueryable,
 		querier.DummyTargetRetriever{},
 		querier.DummyAlertmanagerRetriever{},
 		func() config.Config { return config.Config{} },

cmd/querier/main.go

@@ -88,13 +88,12 @@ func main() {
 	}
 	defer chunkStore.Stop()
 
-	sampleQueryable := querier.NewQueryable(dist, chunkStore, false)
-	metadataQueryable := querier.NewQueryable(dist, chunkStore, true)
+	queryable := querier.NewQueryable(dist, chunkStore)
 
 	engine := promql.NewEngine(util.Logger, nil, querierConfig.MaxConcurrent, querierConfig.Timeout)
 	api := v1.NewAPI(
 		engine,
-		metadataQueryable,
+		queryable,
 		querier.DummyTargetRetriever{},
 		querier.DummyAlertmanagerRetriever{},
 		func() config.Config { return config.Config{} },
@@ -108,7 +107,7 @@ func main() {
 
 	subrouter := server.HTTP.PathPrefix("/api/prom").Subrouter()
 	subrouter.PathPrefix("/api/v1").Handler(middleware.AuthenticateUser.Wrap(promRouter))
-	subrouter.Path("/read").Handler(middleware.AuthenticateUser.Wrap(http.HandlerFunc(sampleQueryable.RemoteReadHandler)))
+	subrouter.Path("/read").Handler(middleware.AuthenticateUser.Wrap(http.HandlerFunc(queryable.RemoteReadHandler)))
 	subrouter.Path("/validate_expr").Handler(middleware.AuthenticateUser.Wrap(http.HandlerFunc(dist.ValidateExprHandler)))
 	subrouter.Path("/user_stats").Handler(middleware.AuthenticateUser.Wrap(http.HandlerFunc(dist.UserStatsHandler)))
 

pkg/querier/querier.go

@@ -38,21 +38,20 @@ type ChunkStore interface {
 
 // NewEngine creates a new promql.Engine for cortex.
 func NewEngine(distributor Querier, chunkStore ChunkStore, reg prometheus.Registerer, maxConcurrent int, timeout time.Duration) (*promql.Engine, storage.Queryable) {
-	queryable := NewQueryable(distributor, chunkStore, false)
+	queryable := NewQueryable(distributor, chunkStore)
 	engine := promql.NewEngine(util.Logger, reg, maxConcurrent, timeout)
 	return engine, queryable
 }
 
 // NewQueryable creates a new Queryable for cortex.
-func NewQueryable(distributor Querier, chunkStore ChunkStore, mo bool) MergeQueryable {
+func NewQueryable(distributor Querier, chunkStore ChunkStore) MergeQueryable {
 	return MergeQueryable{
 		queriers: []Querier{
 			distributor,
 			&chunkQuerier{
 				store: chunkStore,
 			},
 		},
-		metadataOnly: mo,
 	}
 }
 
@@ -125,18 +124,16 @@ func mergeMatrices(matrices chan model.Matrix, errors chan error, n int) (model.
 // A MergeQueryable is a storage.Queryable that produces a storage.Querier which merges
 // results from multiple underlying Queriers.
 type MergeQueryable struct {
-	queriers     []Querier
-	metadataOnly bool
+	queriers []Querier
 }
 
 // Querier implements storage.Queryable.
 func (q MergeQueryable) Querier(ctx context.Context, mint, maxt int64) (storage.Querier, error) {
 	return mergeQuerier{
-		ctx:          ctx,
-		queriers:     q.queriers,
-		mint:         mint,
-		maxt:         maxt,
-		metadataOnly: q.metadataOnly,
+		ctx:      ctx,
+		queriers: q.queriers,
+		mint:     mint,
+		maxt:     maxt,
 	}, nil
 }
 
@@ -205,16 +202,12 @@ type mergeQuerier struct {
 	queriers []Querier
 	mint     int64
 	maxt     int64
-	// Whether this querier should only load series metadata in Select().
-	// Necessary for remote storage implementations of the storage.Querier
-	// interface because both metadata and bulk data loading happens via
-	// the Select() method.
-	metadataOnly bool
 }
 
-func (mq mergeQuerier) Select(_ *storage.SelectParams, matchers ...*labels.Matcher) (storage.SeriesSet, error) {
+func (mq mergeQuerier) Select(sp *storage.SelectParams, matchers ...*labels.Matcher) (storage.SeriesSet, error) {
 	// TODO: Update underlying selectors to return errors directly.
-	if mq.metadataOnly {
+	// Kludge: Prometheus passes nil SelectParams if it is doing a 'series' operation, which needs only metadata
+	if sp == nil {
 		return mq.selectMetadata(matchers...), nil
 	}
 	return mq.selectSamples(matchers...), nil

pkg/querier/querier_test.go

@@ -12,6 +12,7 @@ import (
 	"github.com/golang/snappy"
 	"github.com/prometheus/common/model"
 	"github.com/prometheus/prometheus/pkg/labels"
+	"github.com/prometheus/prometheus/storage"
 	"github.com/stretchr/testify/require"
 	"github.com/weaveworks/cortex/pkg/ingester/client"
 	"github.com/weaveworks/cortex/pkg/prom1/storage/metric"
@@ -104,13 +105,13 @@ func TestMergeQuerierSortsMetricLabels(t *testing.T) {
 				},
 			},
 		},
-		mint:         0,
-		maxt:         0,
-		metadataOnly: false,
+		mint: 0,
+		maxt: 0,
 	}
 	m, err := labels.NewMatcher(labels.MatchEqual, model.MetricNameLabel, "testmetric")
 	require.NoError(t, err)
-	ss, err := mq.Select(nil, m)
+	dummyParams := storage.SelectParams{}
+	ss, err := mq.Select(&dummyParams, m)
 	require.NoError(t, err)
 	require.NoError(t, ss.Err())
 	ss.Next()