Fix panic in the metrics-generator when using multiple tenants with d…

…efault overrides (#2786)

* Modify concurrency test to consider multitenant / concurrent updateProcessors calls

* Copy desiredProcessors map before modifying it

* Update CHANGELOG.md

(cherry picked from commit 9af6bb2)

CHANGELOG.md

@@ -1,4 +1,16 @@
 ## main / unreleased
+* [FEATURE] Add the `/api/status/buildinfo` endpoint [#2702](https://github.com/grafana/tempo/pull/2702) (@fabrizio-grafana)
+* [FEATURE] New encoding vParquet3 with support for dedicated attribute columns (@mapno, @stoewer) [#2649](https://github.com/grafana/tempo/pull/2649)
+* [FEATURE] Add filtering support to Generic Forwarding [#2742](https://github.com/grafana/tempo/pull/2742) (@Blinkuu)
+* [ENHANCEMENT] Assert ingestion rate limits as early as possible [#2640](https://github.com/grafana/tempo/pull/2703) (@mghildiy)
+* [ENHANCEMENT] Add several metrics-generator fields to user-configurable overrides [#2711](https://github.com/grafana/tempo/pull/2711) (@kvrhdn)
+* [ENHANCEMENT] Update /api/metrics/summary to correctly handle missing attributes and improve performance of TraceQL `select()` queries. [#2765](https://github.com/grafana/tempo/pull/2765) (@mdisibio)
+* [ENHANCEMENT] Add `TempoUserConfigurableOverridesReloadFailing` alert [#2784](https://github.com/grafana/tempo/pull/2784) (@kvrhdn)
+* [BUGFIX] Fix panic in metrics summary api [#2738](https://github.com/grafana/tempo/pull/2738) (@mdisibio)
+* [BUGFIX] Fix node role auth IDMSv1 [#2760](https://github.com/grafana/tempo/pull/2760) (@coufalja)
+* [BUGFIX] Only search ingester blocks that fall within the request time range. [#2783](https://github.com/grafana/tempo/pull/2783) (@joe-elliott)
+* [BUGFIX] Fix incorrect metrics for index failures [#2781](https://github.com/grafana/tempo/pull/2781) (@zalegrala)
+* [BUGFIX] Fix panic in the metrics-generator when using multiple tenants with default overrides [#2786](https://github.com/grafana/tempo/pull/2786) (@kvrhdn)
 
 ## v2.2.0-rc0 / 2023-07-21
 

modules/generator/instance.go

@@ -12,6 +12,7 @@ import (
 	"github.com/go-kit/log/level"
 	"github.com/prometheus/client_golang/prometheus"
 	"github.com/prometheus/client_golang/prometheus/promauto"
+	"golang.org/x/exp/maps"
 
 	"github.com/grafana/tempo/modules/generator/processor"
 	"github.com/grafana/tempo/modules/generator/processor/localblocks"
@@ -151,8 +152,12 @@ func (i *instance) updateSubprocessors(desiredProcessors map[string]struct{}, de
 	_, latencyOk := desiredProcessors[spanmetrics.Latency.String()]
 	_, sizeOk := desiredProcessors[spanmetrics.Size.String()]
 
+	// Copy the map before modifying it. This map can be shared by multiple instances and is not safe to write to.
+	newDesiredProcessors := map[string]struct{}{}
+	maps.Copy(newDesiredProcessors, desiredProcessors)
+
 	if !allOk {
-		desiredProcessors[spanmetrics.Name] = struct{}{}
+		newDesiredProcessors[spanmetrics.Name] = struct{}{}
 		desiredCfg.SpanMetrics.Subprocessors[spanmetrics.Count] = false
 		desiredCfg.SpanMetrics.Subprocessors[spanmetrics.Latency] = false
 		desiredCfg.SpanMetrics.Subprocessors[spanmetrics.Size] = false
@@ -170,11 +175,11 @@ func (i *instance) updateSubprocessors(desiredProcessors map[string]struct{}, de
 		}
 	}
 
-	delete(desiredProcessors, spanmetrics.Latency.String())
-	delete(desiredProcessors, spanmetrics.Count.String())
-	delete(desiredProcessors, spanmetrics.Size.String())
+	delete(newDesiredProcessors, spanmetrics.Latency.String())
+	delete(newDesiredProcessors, spanmetrics.Count.String())
+	delete(newDesiredProcessors, spanmetrics.Size.String())
 
-	return desiredProcessors, desiredCfg
+	return newDesiredProcessors, desiredCfg
 }
 
 func (i *instance) updateProcessors() error {

modules/generator/instance_test.go

@@ -25,8 +25,17 @@ import (
 )
 
 func Test_instance_concurrency(t *testing.T) {
+	// Both instances use the same overrides, this map will be accessed by both
 	overrides := &mockOverrides{}
-	instance, err := newInstance(&Config{}, "test", overrides, &noopStorage{}, prometheus.DefaultRegisterer, log.NewNopLogger(), nil)
+	overrides.processors = map[string]struct{}{
+		spanmetrics.Name:   {},
+		servicegraphs.Name: {},
+	}
+
+	instance1, err := newInstance(&Config{}, "test", overrides, &noopStorage{}, prometheus.DefaultRegisterer, log.NewNopLogger(), nil)
+	assert.NoError(t, err)
+
+	instance2, err := newInstance(&Config{}, "test", overrides, &noopStorage{}, prometheus.DefaultRegisterer, log.NewNopLogger(), nil)
 	assert.NoError(t, err)
 
 	end := make(chan struct{})
@@ -44,26 +53,28 @@ func Test_instance_concurrency(t *testing.T) {
 
 	go accessor(func() {
 		req := test.MakeBatch(1, nil)
-		instance.pushSpans(context.Background(), &tempopb.PushSpansRequest{Batches: []*v1.ResourceSpans{req}})
+		instance1.pushSpans(context.Background(), &tempopb.PushSpansRequest{Batches: []*v1.ResourceSpans{req}})
 	})
 
 	go accessor(func() {
-		overrides.processors = map[string]struct{}{
-			"span-metrics": {},
-		}
-		err := instance.updateProcessors()
+		req := test.MakeBatch(1, nil)
+		instance2.pushSpans(context.Background(), &tempopb.PushSpansRequest{Batches: []*v1.ResourceSpans{req}})
+	})
+
+	go accessor(func() {
+		err := instance1.updateProcessors()
 		assert.NoError(t, err)
+	})
 
-		overrides.processors = map[string]struct{}{
-			"service-graphs": {},
-		}
-		err = instance.updateProcessors()
+	go accessor(func() {
+		err := instance2.updateProcessors()
 		assert.NoError(t, err)
 	})
 
 	time.Sleep(100 * time.Millisecond)
 
-	instance.shutdown()
+	instance1.shutdown()
+	instance2.shutdown()
 
 	time.Sleep(10 * time.Millisecond)
 	close(end)

vendor/modules.txt

@@ -1321,6 +1321,7 @@ golang.org/x/crypto/pkcs12/internal/rc2
 # golang.org/x/exp v0.0.0-20230321023759-10a507213a29
 ## explicit; go 1.18
 golang.org/x/exp/constraints
+golang.org/x/exp/maps
 golang.org/x/exp/slices
 # golang.org/x/mod v0.8.0
 ## explicit; go 1.17