Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix panic in the metrics-generator when using multiple tenants with default overrides #2786

Merged
merged 4 commits into from
Aug 17, 2023
Merged

Fix panic in the metrics-generator when using multiple tenants with default overrides #2786

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
32bd393
Modify concurrency test to consider multitenant / concurrent updatePr…
Aug 16, 2023
f956733
Copy desiredProcessors map before modifying it
Aug 11, 2023
c1aa60b
Update CHANGELOG.md
Aug 17, 2023
f68685f
Merge branch 'main' into kvrhdn/metrics-generator-panic
Aug 17, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
* [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 / 2023-07-31

Expand Down
15 changes: 10 additions & 5 deletions modules/generator/instance.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if desiredProcessors was capable of racing in the previous code, I don't see how this fixes it. it could still race here while copying to newDesiredProcessors

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This only reads the map, afaik concurrent map reads are fine. Later in this function we would modify desiredProcessors but with this change we only modify the copy newDesiredProcessors.

Copy link
Member

@joe-elliott joe-elliott Aug 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. A little googling and a quick test confirms what you're saying. If multiple goroutines have access to this map though there is the threat in the future of someone writing to it in a different goroutine and breaking this assumption.

I'm fine with the fix, but let's add a comment explaining why we are copying the map and what our expectations are for other goroutines holding a reference to the map.


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
Expand All @@ -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 {
Expand Down
33 changes: 22 additions & 11 deletions modules/generator/instance_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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{})
Expand All @@ -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)
Expand Down
94 changes: 94 additions & 0 deletions vendor/golang.org/x/exp/maps/maps.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions vendor/modules.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1478,6 +1478,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.9.0
## explicit; go 1.17
Expand Down