From 6851027ca5690eda69b9c91f0d56bab77f2a8084 Mon Sep 17 00:00:00 2001 From: Koenraad Verheyden Date: Thu, 17 Aug 2023 17:39:07 +0200 Subject: [PATCH] Fix panic in the metrics-generator when using multiple tenants with default overrides (#2786) * Modify concurrency test to consider multitenant / concurrent updateProcessors calls * Copy desiredProcessors map before modifying it * Update CHANGELOG.md (cherry picked from commit 9af6bb2eccd7c04c2f06097278f6b92f8d74860b) --- CHANGELOG.md | 12 ++++ modules/generator/instance.go | 15 +++-- modules/generator/instance_test.go | 33 ++++++---- vendor/golang.org/x/exp/maps/maps.go | 94 ++++++++++++++++++++++++++++ vendor/modules.txt | 1 + 5 files changed, 139 insertions(+), 16 deletions(-) create mode 100644 vendor/golang.org/x/exp/maps/maps.go diff --git a/CHANGELOG.md b/CHANGELOG.md index ea54b32d001..8cc461e1dda 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/modules/generator/instance.go b/modules/generator/instance.go index f964b941e38..95807b8f3dd 100644 --- a/modules/generator/instance.go +++ b/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 { diff --git a/modules/generator/instance_test.go b/modules/generator/instance_test.go index 19495fe277e..2245b126f13 100644 --- a/modules/generator/instance_test.go +++ b/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) diff --git a/vendor/golang.org/x/exp/maps/maps.go b/vendor/golang.org/x/exp/maps/maps.go new file mode 100644 index 00000000000..ecc0dabb74d --- /dev/null +++ b/vendor/golang.org/x/exp/maps/maps.go @@ -0,0 +1,94 @@ +// Copyright 2021 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +// Package maps defines various functions useful with maps of any type. +package maps + +// Keys returns the keys of the map m. +// The keys will be in an indeterminate order. +func Keys[M ~map[K]V, K comparable, V any](m M) []K { + r := make([]K, 0, len(m)) + for k := range m { + r = append(r, k) + } + return r +} + +// Values returns the values of the map m. +// The values will be in an indeterminate order. +func Values[M ~map[K]V, K comparable, V any](m M) []V { + r := make([]V, 0, len(m)) + for _, v := range m { + r = append(r, v) + } + return r +} + +// Equal reports whether two maps contain the same key/value pairs. +// Values are compared using ==. +func Equal[M1, M2 ~map[K]V, K, V comparable](m1 M1, m2 M2) bool { + if len(m1) != len(m2) { + return false + } + for k, v1 := range m1 { + if v2, ok := m2[k]; !ok || v1 != v2 { + return false + } + } + return true +} + +// EqualFunc is like Equal, but compares values using eq. +// Keys are still compared with ==. +func EqualFunc[M1 ~map[K]V1, M2 ~map[K]V2, K comparable, V1, V2 any](m1 M1, m2 M2, eq func(V1, V2) bool) bool { + if len(m1) != len(m2) { + return false + } + for k, v1 := range m1 { + if v2, ok := m2[k]; !ok || !eq(v1, v2) { + return false + } + } + return true +} + +// Clear removes all entries from m, leaving it empty. +func Clear[M ~map[K]V, K comparable, V any](m M) { + for k := range m { + delete(m, k) + } +} + +// Clone returns a copy of m. This is a shallow clone: +// the new keys and values are set using ordinary assignment. +func Clone[M ~map[K]V, K comparable, V any](m M) M { + // Preserve nil in case it matters. + if m == nil { + return nil + } + r := make(M, len(m)) + for k, v := range m { + r[k] = v + } + return r +} + +// Copy copies all key/value pairs in src adding them to dst. +// When a key in src is already present in dst, +// the value in dst will be overwritten by the value associated +// with the key in src. +func Copy[M1 ~map[K]V, M2 ~map[K]V, K comparable, V any](dst M1, src M2) { + for k, v := range src { + dst[k] = v + } +} + +// DeleteFunc deletes any key/value pairs from m for which del returns true. +func DeleteFunc[M ~map[K]V, K comparable, V any](m M, del func(K, V) bool) { + for k, v := range m { + if del(k, v) { + delete(m, k) + } + } +} diff --git a/vendor/modules.txt b/vendor/modules.txt index d69f0c4c10f..d98a2686914 100644 --- a/vendor/modules.txt +++ b/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