TraceByID: don't allow concurrent_shards greater than query_shards (#…

…4074)

concurrent shards grater then query shards should not be allowed because it would create more goroutines then the jobs to send these jobs to queriers.

if that happens, set concurrent_shards=query_shards and log a warning.

CHANGELOG.md

@@ -1,5 +1,6 @@
 ## main / unreleased
 
+* [CHANGE] TraceByID: don't allow concurrent_shards greater than query_shards. [#4074](https://github.com/grafana/tempo/pull/4074) (@electron0zero)
 * **BREAKING CHANGE** tempo-query is no longer a jaeger instance with grpcPlugin. Its now a standalone server. Serving a grpc api for jaeger on `0.0.0.0:7777` by default. [#3840](https://github.com/grafana/tempo/issues/3840) (@frzifus)
 * [CHANGE] **BREAKING CHANGE** The dynamic injection of X-Scope-OrgID header for metrics generator remote-writes is changed. If the header is aleady set in per-tenant overrides or global tempo configuration, then it is honored and not overwritten. [#4021](https://github.com/grafana/tempo/pull/4021) (@mdisibio)
 * [CHANGE] **BREAKING CHANGE** Migrate from OpenTracing to OpenTelemetry instrumentation. Removed the `use_otel_tracer` configuration option. Use the OpenTelemetry environment variables to configure the span exporter [#3646](https://github.com/grafana/tempo/pull/3646) (@andreasgerstmayr)

cmd/tempo/app/config.go

@@ -219,6 +219,10 @@ func (c *Config) CheckConfig() []ConfigWarning {
 		warnings = append(warnings, warnConfiguredLegacyCache)
 	}
 
+	if c.Frontend.TraceByID.ConcurrentShards > c.Frontend.TraceByID.QueryShards {
+		warnings = append(warnings, warnTraceByIDConcurrentShards)
+	}
+
 	return warnings
 }
 
@@ -293,6 +297,11 @@ var (
 		Message: "c.StorageConfig.Trace.Cache is deprecated and will be removed in a future release.",
 		Explain: "Please migrate to the top level cache settings config.",
 	}
+
+	warnTraceByIDConcurrentShards = ConfigWarning{
+		Message: "c.Frontend.TraceByID.ConcurrentShards greater than query_shards is invalid. concurrent_shards will be set to query_shards",
+		Explain: "Please remove ConcurrentShards or set it to a value less than or equal to QueryShards",
+	}
 )
 
 func newV2Warning(setting string) ConfigWarning {

cmd/tempo/app/config_test.go

@@ -4,6 +4,7 @@ import (
 	"testing"
 	"time"
 
+	"github.com/grafana/tempo/modules/frontend"
 	"github.com/grafana/tempo/tempodb/backend/s3"
 	"github.com/stretchr/testify/assert"
 
@@ -52,6 +53,12 @@ func TestConfig_CheckConfig(t *testing.T) {
 						Enabled: true,
 					},
 				},
+				Frontend: frontend.Config{
+					TraceByID: frontend.TraceByIDConfig{
+						QueryShards:      100,
+						ConcurrentShards: 200,
+					},
+				},
 			},
 			expect: []ConfigWarning{
 				warnCompleteBlockTimeout,
@@ -63,6 +70,7 @@ func TestConfig_CheckConfig(t *testing.T) {
 				warnLogDiscardedTraces,
 				warnNativeAWSAuthEnabled,
 				warnConfiguredLegacyCache,
+				warnTraceByIDConcurrentShards,
 			},
 		},
 		{

modules/frontend/traceid_sharder.go

@@ -53,10 +53,18 @@ func (s asyncTraceSharder) RoundTrip(pipelineRequest pipeline.Request) (pipeline
 
 	// execute requests
 	concurrentShards := uint(s.cfg.QueryShards)
+	// if concurrent shards is set, respect that value
 	if s.cfg.ConcurrentShards > 0 {
 		concurrentShards = uint(s.cfg.ConcurrentShards)
 	}
 
+	// concurrent_shards grater then query_shards should not be allowed because it would create
+	// more goroutines then the jobs to send these jobs to queriers.
+	if concurrentShards > uint(s.cfg.QueryShards) {
+		// set the concurrent shards to the total shards
+		concurrentShards = uint(s.cfg.QueryShards)
+	}
+
 	return pipeline.NewAsyncSharderFunc(ctx, int(concurrentShards), len(reqs), func(i int) pipeline.Request {
 		return pipeline.NewHTTPRequest(reqs[i])
 	}, s.next), nil