Merge branch 'main' into issue-4960

conf/example.yaml

@@ -155,8 +155,8 @@ querier:
   cache_ttl: 168h
   # The interval for recent data that should not be cached.
   flux_interval: 5m
-  # The maximum number of concurrent queries for missing ranges.
-  max_concurrent_queries: 4
+  # The maximum number of queries a single query range request runs at once.
+  max_concurrent_queries: 8
   # When filtering logs by trace_id, clamp the query window to the trace time
   # range with padding to include slightly delayed log exports. Logs only; set
   # to 0 to disable.

pkg/querier/config.go

@@ -7,6 +7,8 @@ import (
 	"github.com/SigNoz/signoz/pkg/factory"
 )
 
+const DefaultMaxConcurrentQueries = 8
+
 type SkipResourceFingerprint struct {
 	Enabled bool `yaml:"enabled" mapstructure:"enabled"`
 	// If count of fingerprint is above threshold, skip the fingerprint subquery and filter on main table instead.
@@ -37,7 +39,7 @@ func newConfig() factory.Config {
 		// Default values
 		CacheTTL:             168 * time.Hour,
 		FluxInterval:         5 * time.Minute,
-		MaxConcurrentQueries: 4,
+		MaxConcurrentQueries: DefaultMaxConcurrentQueries,
 		SkipResourceFingerprint: SkipResourceFingerprint{
 			Enabled:   false,
 			Threshold: 100000,

pkg/querier/querier.go

@@ -13,6 +13,7 @@ import (
 
 	"github.com/dustin/go-humanize"
 	"golang.org/x/exp/maps"
+	"golang.org/x/sync/errgroup"
 
 	"github.com/SigNoz/signoz/pkg/errors"
 	"github.com/SigNoz/signoz/pkg/factory"
@@ -35,20 +36,21 @@ var (
 )
 
 type querier struct {
-	logger                    *slog.Logger
-	fl                        flagger.Flagger
-	telemetryStore            telemetrystore.TelemetryStore
-	metadataStore             telemetrytypes.MetadataStore
-	promEngine                prometheus.Prometheus
-	traceStmtBuilder          qbtypes.StatementBuilder[qbtypes.TraceAggregation]
-	logStmtBuilder            qbtypes.StatementBuilder[qbtypes.LogAggregation]
-	auditStmtBuilder          qbtypes.StatementBuilder[qbtypes.LogAggregation]
-	metricStmtBuilder         qbtypes.StatementBuilder[qbtypes.MetricAggregation]
-	meterStmtBuilder          qbtypes.StatementBuilder[qbtypes.MetricAggregation]
-	traceOperatorStmtBuilder  qbtypes.TraceOperatorStatementBuilder
-	bucketCache               BucketCache
-	liveDataRefresh           time.Duration
-	builderConfig             builderConfig
+	logger                   *slog.Logger
+	fl                       flagger.Flagger
+	telemetryStore           telemetrystore.TelemetryStore
+	metadataStore            telemetrytypes.MetadataStore
+	promEngine               prometheus.Prometheus
+	traceStmtBuilder         qbtypes.StatementBuilder[qbtypes.TraceAggregation]
+	logStmtBuilder           qbtypes.StatementBuilder[qbtypes.LogAggregation]
+	auditStmtBuilder         qbtypes.StatementBuilder[qbtypes.LogAggregation]
+	metricStmtBuilder        qbtypes.StatementBuilder[qbtypes.MetricAggregation]
+	meterStmtBuilder         qbtypes.StatementBuilder[qbtypes.MetricAggregation]
+	traceOperatorStmtBuilder qbtypes.TraceOperatorStatementBuilder
+	bucketCache              BucketCache
+	liveDataRefresh          time.Duration
+	builderConfig            builderConfig
+	maxConcurrentQueries     int
 }
 
 var _ Querier = (*querier)(nil)
@@ -67,25 +69,30 @@ func New(
 	bucketCache BucketCache,
 	flagger flagger.Flagger,
 	logTraceIDWindowPadding time.Duration,
+	maxConcurrentQueries int,
 ) *querier {
 	querierSettings := factory.NewScopedProviderSettings(settings, "github.com/SigNoz/signoz/pkg/querier")
+	if maxConcurrentQueries <= 0 {
+		maxConcurrentQueries = DefaultMaxConcurrentQueries
+	}
 	return &querier{
-		logger:                    querierSettings.Logger(),
-		fl:                        flagger,
-		telemetryStore:            telemetryStore,
-		metadataStore:             metadataStore,
-		promEngine:                promEngine,
-		traceStmtBuilder:          traceStmtBuilder,
-		logStmtBuilder:            logStmtBuilder,
-		auditStmtBuilder:          auditStmtBuilder,
-		metricStmtBuilder:         metricStmtBuilder,
-		meterStmtBuilder:          meterStmtBuilder,
-		traceOperatorStmtBuilder:  traceOperatorStmtBuilder,
-		bucketCache:               bucketCache,
-		liveDataRefresh:           5 * time.Second,
+		logger:                   querierSettings.Logger(),
+		fl:                       flagger,
+		telemetryStore:           telemetryStore,
+		metadataStore:            metadataStore,
+		promEngine:               promEngine,
+		traceStmtBuilder:         traceStmtBuilder,
+		logStmtBuilder:           logStmtBuilder,
+		auditStmtBuilder:         auditStmtBuilder,
+		metricStmtBuilder:        metricStmtBuilder,
+		meterStmtBuilder:         meterStmtBuilder,
+		traceOperatorStmtBuilder: traceOperatorStmtBuilder,
+		bucketCache:              bucketCache,
+		liveDataRefresh:          5 * time.Second,
 		builderConfig: builderConfig{
 			logTraceIDWindowPaddingMS: uint64(logTraceIDWindowPadding.Milliseconds()),
 		},
+		maxConcurrentQueries: maxConcurrentQueries,
 	}
 }
 
@@ -607,30 +614,40 @@ func (q *querier) run(
 		return false
 	}
 
-	for name, query := range qs {
-		// Skip cache if NoCache is set, or if cache is not available
-		if req.NoCache || q.bucketCache == nil || query.Fingerprint() == "" {
-			if req.NoCache {
-				q.logger.DebugContext(ctx, "NoCache flag set, bypassing cache", slog.String("query", name))
-			} else {
-				q.logger.InfoContext(ctx, "no bucket cache or fingerprint, executing query", slog.String("fingerprint", query.Fingerprint()))
+	names := maps.Keys(qs)
+	slices.Sort(names)
+	queryResults := make([]*qbtypes.Result, len(names))
+
+	// sem limits how many queries run at once for this request. The same
+	// limit covers the missing-range queries in executeWithCache. sem is held
+	// only while a query is running, never while waiting for other
+	// goroutines, so the two levels cannot deadlock.
+	sem := make(chan struct{}, q.maxConcurrentQueries)
+
+	eg, egCtx := errgroup.WithContext(ctx)
+	for i, name := range names {
+		query := qs[name]
+		eg.Go(func() error {
+			// Skip cache if NoCache is set, or if cache is not available
+			if req.NoCache || q.bucketCache == nil || query.Fingerprint() == "" {
+				if req.NoCache {
+					q.logger.DebugContext(egCtx, "NoCache flag set, bypassing cache", slog.String("query", name))
+				} else {
+					q.logger.InfoContext(egCtx, "no bucket cache or fingerprint, executing query", slog.String("fingerprint", query.Fingerprint()))
+				}
+				sem <- struct{}{}
+				result, err := query.Execute(egCtx)
+				<-sem
+				if err != nil {
+					return err
+				}
+				queryResults[i] = result
+				return nil
 			}
-			result, err := query.Execute(ctx)
-			qbEvent.HasData = qbEvent.HasData || hasData(result)
+
+			result, err := q.executeWithCache(egCtx, orgID, query, steps[name], sem)
 			if err != nil {
-				return nil, err
-			}
-			results[name] = result.Value
-			warnings = append(warnings, result.Warnings...)
-			warningsDocURL = result.WarningsDocURL
-			stats.RowsScanned += result.Stats.RowsScanned
-			stats.BytesScanned += result.Stats.BytesScanned
-			stats.DurationMS += result.Stats.DurationMS
-		} else {
-			result, err := q.executeWithCache(ctx, orgID, query, steps[name], req.NoCache)
-			qbEvent.HasData = qbEvent.HasData || hasData(result)
-			if err != nil {
-				return nil, err
+				return err
 			}
 			switch v := result.Value.(type) {
 			case *qbtypes.TimeSeriesData:
@@ -640,14 +657,23 @@ func (q *querier) run(
 			case *qbtypes.RawData:
 				v.QueryName = name
 			}
+			queryResults[i] = result
+			return nil
+		})
+	}
+	if err := eg.Wait(); err != nil {
+		return nil, err
+	}
 
-			results[name] = result.Value
-			warnings = append(warnings, result.Warnings...)
-			warningsDocURL = result.WarningsDocURL
-			stats.RowsScanned += result.Stats.RowsScanned
-			stats.BytesScanned += result.Stats.BytesScanned
-			stats.DurationMS += result.Stats.DurationMS
-		}
+	for i, name := range names {
+		result := queryResults[i]
+		qbEvent.HasData = qbEvent.HasData || hasData(result)
+		results[name] = result.Value
+		warnings = append(warnings, result.Warnings...)
+		warningsDocURL = result.WarningsDocURL
+		stats.RowsScanned += result.Stats.RowsScanned
+		stats.BytesScanned += result.Stats.BytesScanned
+		stats.DurationMS += result.Stats.DurationMS
 	}
 
 	gomaps.Copy(results, preseededResults)
@@ -707,8 +733,9 @@ func (q *querier) run(
 	return resp, nil
 }
 
-// executeWithCache executes a query using the bucket cache.
-func (q *querier) executeWithCache(ctx context.Context, orgID valuer.UUID, query qbtypes.Query, step qbtypes.Step, _ bool) (*qbtypes.Result, error) {
+// executeWithCache executes a query using the bucket cache. sem limits how
+// many queries run at once for the whole request.
+func (q *querier) executeWithCache(ctx context.Context, orgID valuer.UUID, query qbtypes.Query, step qbtypes.Step, sem chan struct{}) (*qbtypes.Result, error) {
 	// Get cached data and missing ranges
 	cachedResult, missingRanges := q.bucketCache.GetMissRanges(ctx, orgID, query, step)
 
@@ -721,7 +748,9 @@ func (q *querier) executeWithCache(ctx context.Context, orgID valuer.UUID, query
 	if cachedResult == nil && len(missingRanges) == 1 {
 		startMs, endMs := query.Window()
 		if missingRanges[0].From == startMs && missingRanges[0].To == endMs {
+			sem <- struct{}{}
 			result, err := query.Execute(ctx)
+			<-sem
 			if err != nil {
 				return nil, err
 			}
@@ -740,7 +769,6 @@ func (q *querier) executeWithCache(ctx context.Context, orgID valuer.UUID, query
 		slog.Int("missing_ranges_count", len(missingRanges)),
 		slog.Any("ranges", missingRanges))
 
-	sem := make(chan struct{}, 4)
 	var wg sync.WaitGroup
 
 	for i, timeRange := range missingRanges {
@@ -777,7 +805,9 @@ func (q *querier) executeWithCache(ctx context.Context, orgID valuer.UUID, query
 		if err != nil {
 			// If any query failed, fall back to full execution
 			q.logger.ErrorContext(ctx, "parallel query execution failed", errors.Attr(err))
+			sem <- struct{}{}
 			result, err := query.Execute(ctx)
+			<-sem
 			if err != nil {
 				return nil, err
 			}

pkg/querier/querier_test.go

@@ -2,11 +2,13 @@ package querier
 
 import (
 	"context"
+	"sync/atomic"
 	"testing"
 	"time"
 
 	cmock "github.com/SigNoz/clickhouse-go-mock"
 
+	"github.com/SigNoz/signoz/pkg/errors"
 	"github.com/SigNoz/signoz/pkg/flagger/flaggertest"
 	"github.com/SigNoz/signoz/pkg/instrumentation/instrumentationtest"
 	"github.com/SigNoz/signoz/pkg/telemetrystore"
@@ -54,7 +56,8 @@ func TestQueryRange_MetricTypeMissing(t *testing.T) {
 		nil,                // traceOperatorStmtBuilder
 		nil,                // bucketCache
 		flaggertest.New(t), // flagger
-		0,
+		0,                  // logTraceIDWindowPadding
+		0,                  // maxConcurrentQueries
 	)
 
 	req := &qbtypes.QueryRangeRequest{
@@ -125,7 +128,8 @@ func TestQueryRange_MetricTypeFromStore(t *testing.T) {
 		nil,                      // traceOperatorStmtBuilder
 		nil,                      // bucketCache
 		flaggertest.New(t),       // flagger
-		0,
+		0,                        // logTraceIDWindowPadding
+		0,                        // maxConcurrentQueries
 	)
 
 	req := &qbtypes.QueryRangeRequest{
@@ -155,3 +159,149 @@ func TestQueryRange_MetricTypeFromStore(t *testing.T) {
 	require.NoError(t, err)
 	require.NotNil(t, resp)
 }
+
+type fakeQuery struct {
+	execute func(ctx context.Context) (*qbtypes.Result, error)
+}
+
+func (f *fakeQuery) Fingerprint() string                                  { return "" }
+func (f *fakeQuery) Window() (uint64, uint64)                             { return 0, 0 }
+func (f *fakeQuery) Execute(ctx context.Context) (*qbtypes.Result, error) { return f.execute(ctx) }
+
+func chQueryEnvelopes(names []string) []qbtypes.QueryEnvelope {
+	envelopes := make([]qbtypes.QueryEnvelope, 0, len(names))
+	for _, name := range names {
+		envelopes = append(envelopes, qbtypes.QueryEnvelope{
+			Type: qbtypes.QueryTypeClickHouseSQL,
+			Spec: qbtypes.ClickHouseQuery{Name: name},
+		})
+	}
+	return envelopes
+}
+
+func TestRunExecutesQueriesConcurrently(t *testing.T) {
+	names := []string{"A", "B", "C", "D", "E"}
+	numQueries := len(names)
+
+	q := &querier{
+		logger:               instrumentationtest.New().Logger(),
+		maxConcurrentQueries: numQueries,
+	}
+
+	ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second)
+	defer cancel()
+
+	var started atomic.Int32
+	allStarted := make(chan struct{})
+
+	qs := make(map[string]qbtypes.Query, numQueries)
+	for _, name := range names {
+		qs[name] = &fakeQuery{execute: func(ctx context.Context) (*qbtypes.Result, error) {
+			if int(started.Add(1)) == numQueries {
+				close(allStarted)
+			}
+			select {
+			case <-allStarted:
+			case <-ctx.Done():
+				return nil, ctx.Err()
+			}
+			return &qbtypes.Result{
+				Type:  qbtypes.RequestTypeScalar,
+				Value: &qbtypes.ScalarData{QueryName: name},
+				Stats: qbtypes.ExecStats{RowsScanned: 1, BytesScanned: 2, DurationMS: 3},
+			}, nil
+		}}
+	}
+
+	req := &qbtypes.QueryRangeRequest{
+		RequestType:    qbtypes.RequestTypeScalar,
+		CompositeQuery: qbtypes.CompositeQuery{Queries: chQueryEnvelopes(names)},
+	}
+	resp, err := q.run(ctx, valuer.GenerateUUID(), qs, req, nil, &qbtypes.QBEvent{}, nil)
+	require.NoError(t, err)
+	require.NotNil(t, resp)
+	assert.Len(t, resp.Data.Results, numQueries)
+	assert.Equal(t, uint64(numQueries), resp.Meta.RowsScanned)
+	assert.Equal(t, uint64(2*numQueries), resp.Meta.BytesScanned)
+	assert.Equal(t, uint64(3*numQueries), resp.Meta.DurationMS)
+}
+
+func TestRunRespectsMaxConcurrentQueries(t *testing.T) {
+	const limit = 2
+	names := []string{"A", "B", "C", "D", "E", "F", "G", "H"}
+
+	q := &querier{
+		logger:               instrumentationtest.New().Logger(),
+		maxConcurrentQueries: limit,
+	}
+
+	var running, maxRunning atomic.Int32
+	qs := make(map[string]qbtypes.Query, len(names))
+	for _, name := range names {
+		qs[name] = &fakeQuery{execute: func(ctx context.Context) (*qbtypes.Result, error) {
+			cur := running.Add(1)
+			defer running.Add(-1)
+			for {
+				m := maxRunning.Load()
+				if cur <= m || maxRunning.CompareAndSwap(m, cur) {
+					break
+				}
+			}
+			time.Sleep(20 * time.Millisecond)
+			return &qbtypes.Result{
+				Type:  qbtypes.RequestTypeScalar,
+				Value: &qbtypes.ScalarData{QueryName: name},
+			}, nil
+		}}
+	}
+
+	req := &qbtypes.QueryRangeRequest{
+		RequestType:    qbtypes.RequestTypeScalar,
+		CompositeQuery: qbtypes.CompositeQuery{Queries: chQueryEnvelopes(names)},
+	}
+	resp, err := q.run(context.Background(), valuer.GenerateUUID(), qs, req, nil, &qbtypes.QBEvent{}, nil)
+	require.NoError(t, err)
+	require.NotNil(t, resp)
+	assert.Len(t, resp.Data.Results, len(names))
+	assert.LessOrEqual(t, maxRunning.Load(), int32(limit), "running queries must not exceed maxConcurrentQueries")
+}
+
+func TestRunQueryErrorCancelsSiblings(t *testing.T) {
+	q := &querier{
+		logger:               instrumentationtest.New().Logger(),
+		maxConcurrentQueries: 4,
+	}
+
+	bStarted := make(chan struct{})
+	var bCanceled atomic.Bool
+
+	qs := map[string]qbtypes.Query{
+		// fails once B is running.
+		"A": &fakeQuery{execute: func(ctx context.Context) (*qbtypes.Result, error) {
+			select {
+			case <-bStarted:
+			case <-ctx.Done():
+			}
+			return nil, errors.NewInternalf(errors.CodeInternal, "query A failed")
+		}},
+		// blocks until its context is canceled by A's failure.
+		"B": &fakeQuery{execute: func(ctx context.Context) (*qbtypes.Result, error) {
+			close(bStarted)
+			select {
+			case <-ctx.Done():
+				bCanceled.Store(true)
+				return nil, ctx.Err()
+			case <-time.After(10 * time.Second):
+				return nil, errors.NewInternalf(errors.CodeInternal, "query B was never canceled")
+			}
+		}},
+	}
+
+	req := &qbtypes.QueryRangeRequest{
+		RequestType:    qbtypes.RequestTypeScalar,
+		CompositeQuery: qbtypes.CompositeQuery{Queries: chQueryEnvelopes([]string{"A", "B"})},
+	}
+	_, err := q.run(context.Background(), valuer.GenerateUUID(), qs, req, nil, &qbtypes.QBEvent{}, nil)
+	require.ErrorContains(t, err, "query A failed")
+	assert.True(t, bCanceled.Load(), "query B should be canceled once query A fails")
+}

pkg/querier/signozquerier/provider.go

@@ -193,5 +193,6 @@ func newProvider(
 		bucketCache,
 		flagger,
 		cfg.LogTraceIDWindowPadding,
+		cfg.MaxConcurrentQueries,
 	), nil
 }

pkg/query-service/rules/setups_test.go

@@ -56,6 +56,7 @@ func prepareQuerierForMetrics(t *testing.T, telemetryStore telemetrystore.Teleme
 		nil, // bucketCache
 		flagger,
 		0,
+		0, // maxConcurrentQueries (0 means default)
 	), metadataStore
 }
 
@@ -110,6 +111,7 @@ func prepareQuerierForLogs(t *testing.T, telemetryStore telemetrystore.Telemetry
 		nil,            // bucketCache
 		fl,
 		5*time.Minute, // logTraceIDWindowPadding
+		0,             // maxConcurrentQueries (0 means default)
 	)
 }
 
@@ -158,5 +160,6 @@ func prepareQuerierForTraces(t *testing.T, telemetryStore telemetrystore.Telemet
 		nil,              // bucketCache
 		fl,
 		0,
+		0, // maxConcurrentQueries (0 means default)
 	)
 }