Address PR comments

cmd/mimir/config-descriptor.json

@@ -4373,7 +4373,7 @@
           "kind": "field",
           "name": "instant_queries_with_subquery_spin_off",
           "required": false,
-          "desc": "List of regexp patterns matching instant queries. Subqueries within those instant queries will be spun off as range queries to optimize their execution.",
+          "desc": "List of regular expression patterns matching instant queries. Subqueries within those instant queries will be spun off as range queries to optimize their performance.",
           "fieldValue": null,
           "fieldDefaultValue": [],
           "fieldType": "list of strings",
@@ -6600,7 +6600,7 @@
           "kind": "field",
           "name": "spin_off_instant_subqueries_to_url",
           "required": false,
-          "desc": "If set, subqueries in instant queries will be spun off as range queries and sent to the given URL. Also requires `instant_queries_with_subquery_spin_off` to be set for the tenant.",
+          "desc": "If set, subqueries in instant queries are spun off as range queries and sent to the given URL. This parameter also requires you to set `instant_queries_with_subquery_spin_off` for the tenant.",
           "fieldValue": null,
           "fieldDefaultValue": "",
           "fieldFlag": "query-frontend.spin-off-instant-subqueries-to-url",

cmd/mimir/help-all.txt.tmpl

@@ -2430,7 +2430,7 @@ Usage of ./cmd/mimir/mimir:
   -query-frontend.shard-active-series-queries
     	[experimental] True to enable sharding of active series queries.
   -query-frontend.spin-off-instant-subqueries-to-url string
-    	[experimental] If set, subqueries in instant queries will be spun off as range queries and sent to the given URL. Also requires `instant_queries_with_subquery_spin_off` to be set for the tenant.
+    	[experimental] If set, subqueries in instant queries are spun off as range queries and sent to the given URL. This parameter also requires you to set `instant_queries_with_subquery_spin_off` for the tenant.
   -query-frontend.split-instant-queries-by-interval duration
     	[experimental] Split instant queries by an interval and execute in parallel. 0 to disable it.
   -query-frontend.split-queries-by-interval duration

docs/sources/mimir/configure/configuration-parameters/index.md

@@ -1723,9 +1723,9 @@ results_cache:
 # CLI flag: -query-frontend.use-active-series-decoder
 [use_active_series_decoder: <boolean> | default = false]
 
-# (experimental) If set, subqueries in instant queries will be spun off as range
-# queries and sent to the given URL. Also requires
-# `instant_queries_with_subquery_spin_off` to be set for the tenant.
+# (experimental) If set, subqueries in instant queries are spun off as range
+# queries and sent to the given URL. This parameter also requires you to set
+# `instant_queries_with_subquery_spin_off` for the tenant.
 # CLI flag: -query-frontend.spin-off-instant-subqueries-to-url
 [spin_off_instant_subqueries_to_url: <string> | default = ""]
 
@@ -3610,9 +3610,9 @@ The `limits` block configures default and per-tenant limits imposed by component
 # CLI flag: -query-frontend.prom2-range-compat
 [prom2_range_compat: <boolean> | default = false]
 
-# (experimental) List of regexp patterns matching instant queries. Subqueries
-# within those instant queries will be spun off as range queries to optimize
-# their execution.
+# (experimental) List of regular expression patterns matching instant queries.
+# Subqueries within those instant queries will be spun off as range queries to
+# optimize their performance.
 [instant_queries_with_subquery_spin_off: <list of strings> | default = ]
 
 # Enables endpoints used for cardinality analysis.

pkg/frontend/querymiddleware/astmapper/subquery_spin_off.go

@@ -31,7 +31,7 @@ type subquerySpinOffMapper struct {
 	stats  *SubquerySpinOffMapperStats
 }
 
-// NewSubqueryExtractor creates a new instant query mapper.
+// NewSubquerySpinOffMapper creates a new instant query mapper.
 func NewSubquerySpinOffMapper(ctx context.Context, defaultStepFunc func(rangeMillis int64) int64, logger log.Logger, stats *SubquerySpinOffMapperStats) ASTMapper {
 	queryMapper := NewASTExprMapper(
 		&subquerySpinOffMapper{
@@ -47,6 +47,15 @@ func NewSubquerySpinOffMapper(ctx context.Context, defaultStepFunc func(rangeMil
 	)
 }
 
+// MapExpr implements the ASTMapper interface.
+// The strategy here is to look for aggregated subqueries (all subqueries should be aggregated) and spin them off into separate queries.
+// The frontend does not have internal control of the engine,
+// so MapExpr has to remap subqueries into "fake metrics" that can be queried by a Queryable that we can inject into the engine.
+// This "fake metric selector" is the "__subquery_spinoff__" metric.
+// For everything else, we have to pass it through to the downstream execution path (other instant middlewares),
+// so we remap them into a "__downstream_query__" selector.
+//
+// See sharding.go and embedded.go for another example of mapping into a fake metric selector.
 func (m *subquerySpinOffMapper) MapExpr(expr parser.Expr) (mapped parser.Expr, finished bool, err error) {
 	if err := m.ctx.Err(); err != nil {
 		return nil, false, err
@@ -78,6 +87,8 @@ func (m *subquerySpinOffMapper) MapExpr(expr parser.Expr) (mapped parser.Expr, f
 			return expr, false, nil
 		}
 		lastArgIdx := len(e.Args) - 1
+		// The last argument will typically contain the subquery in an aggregation function
+		// Examples: last_over_time(<subquery>[5m:]) or quantile_over_time(0.5, <subquery>[5m:])
 		if sq, ok := e.Args[lastArgIdx].(*parser.SubqueryExpr); ok {
 			// Filter out subqueries with offsets, not supported yet
 			if sq.OriginalOffset > 0 {

pkg/frontend/querymiddleware/roundtrip.go

@@ -102,7 +102,7 @@ func (cfg *Config) RegisterFlags(f *flag.FlagSet) {
 	f.StringVar(&cfg.QueryResultResponseFormat, "query-frontend.query-result-response-format", formatProtobuf, fmt.Sprintf("Format to use when retrieving query results from queriers. Supported values: %s", strings.Join(allFormats, ", ")))
 	f.BoolVar(&cfg.ShardActiveSeriesQueries, "query-frontend.shard-active-series-queries", false, "True to enable sharding of active series queries.")
 	f.BoolVar(&cfg.UseActiveSeriesDecoder, "query-frontend.use-active-series-decoder", false, "Set to true to use the zero-allocation response decoder for active series queries.")
-	f.StringVar(&cfg.SpinOffInstantSubqueriesToURL, "query-frontend.spin-off-instant-subqueries-to-url", "", "If set, subqueries in instant queries will be spun off as range queries and sent to the given URL. Also requires `instant_queries_with_subquery_spin_off` to be set for the tenant.")
+	f.StringVar(&cfg.SpinOffInstantSubqueriesToURL, "query-frontend.spin-off-instant-subqueries-to-url", "", "If set, subqueries in instant queries are spun off as range queries and sent to the given URL. This parameter also requires you to set `instant_queries_with_subquery_spin_off` for the tenant.")
 	cfg.ResultsCacheConfig.RegisterFlags(f)
 }
 

pkg/frontend/querymiddleware/spin_off_subqueries.go

@@ -155,7 +155,7 @@ func (s *spinOffSubqueriesMiddleware) Do(ctx context.Context, req MetricsQueryRe
 		return s.next.Do(ctx, req)
 	}
 
-	// Increment total number of instant queries attempted to split metrics
+	// Increment total number of instant queries attempted to spin off subqueries from.
 	s.metrics.spinOffAttempts.Inc()
 
 	mapperStats := astmapper.NewSubquerySpinOffMapperStats()
@@ -173,23 +173,24 @@ func (s *spinOffSubqueriesMiddleware) Do(ctx context.Context, req MetricsQueryRe
 	spinOffQuery, err := mapper.Map(expr)
 	if err != nil {
 		if errors.Is(err, context.DeadlineExceeded) && ctx.Err() == nil {
-			level.Error(spanLog).Log("msg", "timeout while splitting query by instant interval, please fill in a bug report with this query, falling back to try executing without splitting", "err", err)
+			level.Error(spanLog).Log("msg", "timeout while spinning off subqueries, please fill in a bug report with this query, falling back to try executing without spin-off", "err", err)
 		} else {
-			level.Error(spanLog).Log("msg", "failed to map the input query, falling back to try executing without splitting", "err", err)
+			level.Error(spanLog).Log("msg", "failed to map the input query, falling back to try executing without spin-off", "err", err)
 		}
 		s.metrics.spinOffSkipped.WithLabelValues(subquerySpinoffSkippedReasonMappingFailed).Inc()
 		return s.next.Do(ctx, req)
 	}
 
 	if mapperStats.SpunOffSubqueries() == 0 {
-		// the query cannot be split, so continue
+		// the query has no subqueries, so continue downstream
 		spanLog.DebugLog("msg", "input query resulted in a no operation, falling back to try executing without spinning off subqueries")
 		s.metrics.spinOffSkipped.WithLabelValues(subquerySpinoffSkippedReasonNoSubqueries).Inc()
 		return s.next.Do(ctx, req)
 	}
 
 	if mapperStats.DownstreamQueries() > mapperStats.SpunOffSubqueries() {
-		// the query cannot be split, so continue
+		// the query has more downstream queries than subqueries, so continue downstream
+		// It's probably more efficient to just execute the query as is
 		spanLog.DebugLog("msg", "input query resulted in more downstream queries than subqueries, falling back to try executing without spinning off subqueries")
 		s.metrics.spinOffSkipped.WithLabelValues(subquerySpinoffSkippedReasonDownstreamQueries).Inc()
 		return s.next.Do(ctx, req)
@@ -218,14 +219,14 @@ func (s *spinOffSubqueriesMiddleware) Do(ctx context.Context, req MetricsQueryRe
 
 	qry, err := newQuery(ctx, req, s.engine, lazyquery.NewLazyQueryable(queryable))
 	if err != nil {
-		level.Warn(spanLog).Log("msg", "failed to create new query from splittable request", "err", err)
+		level.Warn(spanLog).Log("msg", "failed to create new query from subquery spin request", "err", err)
 		return nil, apierror.New(apierror.TypeBadData, err.Error())
 	}
 
 	res := qry.Exec(ctx)
 	extracted, err := promqlResultToSamples(res)
 	if err != nil {
-		level.Warn(spanLog).Log("msg", "failed to execute split instant query", "err", err)
+		level.Warn(spanLog).Log("msg", "failed to execute spun off subquery", "err", err)
 		return nil, mapEngineError(err)
 	}
 

pkg/util/validation/limits.go

@@ -191,7 +191,7 @@ type Limits struct {
 	AlignQueriesWithStep                   bool                   `yaml:"align_queries_with_step" json:"align_queries_with_step"`
 	EnabledPromQLExperimentalFunctions     flagext.StringSliceCSV `yaml:"enabled_promql_experimental_functions" json:"enabled_promql_experimental_functions" category:"experimental"`
 	Prom2RangeCompat                       bool                   `yaml:"prom2_range_compat" json:"prom2_range_compat" category:"experimental"`
-	InstantQueriesWithSubquerySpinOff      []string               `yaml:"instant_queries_with_subquery_spin_off" json:"instant_queries_with_subquery_spin_off" doc:"nocli|description=List of regexp patterns matching instant queries. Subqueries within those instant queries will be spun off as range queries to optimize their execution." category:"experimental"`
+	InstantQueriesWithSubquerySpinOff      []string               `yaml:"instant_queries_with_subquery_spin_off" json:"instant_queries_with_subquery_spin_off" doc:"nocli|description=List of regular expression patterns matching instant queries. Subqueries within those instant queries will be spun off as range queries to optimize their performance." category:"experimental"`
 
 	// Cardinality
 	CardinalityAnalysisEnabled                    bool `yaml:"cardinality_analysis_enabled" json:"cardinality_analysis_enabled"`