From 168e91c008dbc5b82180d6ab137c40942ba160a2 Mon Sep 17 00:00:00 2001 From: Koenraad Verheyden Date: Thu, 9 Nov 2023 18:19:38 +0100 Subject: [PATCH] Merge the processors overrides set through runtime overrides and user-configurable overrides (#3125) --- CHANGELOG.md | 1 + .../overrides/user_configurable_overrides.go | 10 +++--- .../user_configurable_overrides_test.go | 27 ++++++++++------ pkg/util/listtomap/list_to_map.go | 11 +++++++ pkg/util/listtomap/list_to_map_test.go | 31 +++++++++++++++++++ 5 files changed, 67 insertions(+), 13 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ffd0a424181..7101ea813cb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,7 @@ * [ENHANCEMENT] Introduced `AttributePolicyMatch` & `IntrinsicPolicyMatch` structures to match span attributes based on strongly typed values & precompiled regexp [#3025](https://github.com/grafana/tempo/pull/3025) (@andriusluk) * [CHANGE] TraceQL/Structural operators performance improvement. [#3088](https://github.com/grafana/tempo/pull/3088) (@joe-elliott) +* [CHANGE] Merge the processors overrides set through runtime overrides and user-configurable overrides [#3125](https://github.com/grafana/tempo/pull/3125) (@kvrhdn) * [FEATURE] Introduce list_blocks_concurrency on GCS and S3 backends to control backend load and performance. [#2652](https://github.com/grafana/tempo/pull/2652) (@zalegrala) * [BUGFIX] Include statusMessage intrinsic attribute in tag search. [#3084](https://github.com/grafana/tempo/pull/3084) (@rcrowe) * [ENHANCEMENT] Update poller to make use of previous results and reduce backend load. [#2652](https://github.com/grafana/tempo/pull/2652) (@zalegrala) diff --git a/modules/overrides/user_configurable_overrides.go b/modules/overrides/user_configurable_overrides.go index 2361cbf1bb6..58c953059b1 100644 --- a/modules/overrides/user_configurable_overrides.go +++ b/modules/overrides/user_configurable_overrides.go @@ -21,6 +21,7 @@ import ( userconfigurableoverrides "github.com/grafana/tempo/modules/overrides/userconfigurable/client" filterconfig "github.com/grafana/tempo/pkg/spanfilter/config" + "github.com/grafana/tempo/pkg/util/listtomap" tempo_log "github.com/grafana/tempo/pkg/util/log" "github.com/grafana/tempo/pkg/util/tracing" "github.com/grafana/tempo/tempodb/backend" @@ -202,10 +203,11 @@ func (o *userConfigurableOverridesManager) Forwarders(userID string) []string { } func (o *userConfigurableOverridesManager) MetricsGeneratorProcessors(userID string) map[string]struct{} { - if processors, ok := o.getTenantLimits(userID).GetMetricsGenerator().GetProcessors(); ok { - return processors.GetMap() - } - return o.Interface.MetricsGeneratorProcessors(userID) + // We merge settings from both layers meaning if a processor is enabled on any layer it will be always enabled (OR logic) + processorsUserConfigurable, _ := o.getTenantLimits(userID).GetMetricsGenerator().GetProcessors() + processorsRuntime := o.Interface.MetricsGeneratorProcessors(userID) + + return listtomap.Merge(processorsUserConfigurable, processorsRuntime) } func (o *userConfigurableOverridesManager) MetricsGeneratorDisableCollection(userID string) bool { diff --git a/modules/overrides/user_configurable_overrides_test.go b/modules/overrides/user_configurable_overrides_test.go index 5b44ca82716..ee463bcd7f9 100644 --- a/modules/overrides/user_configurable_overrides_test.go +++ b/modules/overrides/user_configurable_overrides_test.go @@ -11,6 +11,12 @@ import ( "time" "github.com/grafana/dskit/services" + "github.com/prometheus/client_golang/prometheus" + "github.com/prometheus/common/model" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "gopkg.in/yaml.v2" + userconfigurableoverrides "github.com/grafana/tempo/modules/overrides/userconfigurable/client" tempo_api "github.com/grafana/tempo/pkg/api" "github.com/grafana/tempo/pkg/sharedconfig" @@ -18,11 +24,6 @@ import ( "github.com/grafana/tempo/pkg/util/listtomap" "github.com/grafana/tempo/tempodb/backend" "github.com/grafana/tempo/tempodb/backend/local" - "github.com/prometheus/client_golang/prometheus" - "github.com/prometheus/common/model" - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" - "gopkg.in/yaml.v2" ) const ( @@ -371,15 +372,24 @@ func TestUserConfigOverridesManager_MergeRuntimeConfig(t *testing.T) { // Set Forwarders in UserConfigOverrides limits mgr.tenantLimits[tenantID] = &userconfigurableoverrides.Limits{ Forwarders: &[]string{"my-other-forwarder"}, + MetricsGenerator: &userconfigurableoverrides.LimitsMetricsGenerator{ + Processors: map[string]struct{}{"local-blocks": {}}, + }, } - // test all override methods + // Test all override methods + + // Forwarders are set in user-configurable overrides and will override runtime overrides + assert.NotEqual(t, mgr.Forwarders(tenantID), baseMgr.Forwarders(tenantID)) + + // Processors will be the merged result between runtime and user-configurable overrides + assert.Equal(t, mgr.MetricsGeneratorProcessors(tenantID), map[string]struct{}{"local-blocks": {}, "service-graphs": {}, "span-metrics": {}}) + + // For the remaining settings runtime overrides will bubble up assert.Equal(t, mgr.IngestionRateStrategy(), baseMgr.IngestionRateStrategy()) assert.Equal(t, mgr.MaxLocalTracesPerUser(tenantID), baseMgr.MaxLocalTracesPerUser(tenantID)) assert.Equal(t, mgr.MaxGlobalTracesPerUser(tenantID), baseMgr.MaxGlobalTracesPerUser(tenantID)) assert.Equal(t, mgr.MaxBytesPerTrace(tenantID), baseMgr.MaxBytesPerTrace(tenantID)) - // Forwarders are set in userconfigurableoverrides so not same as runtime overrides - assert.NotEqual(t, mgr.Forwarders(tenantID), baseMgr.Forwarders(tenantID)) assert.Equal(t, mgr.Forwarders(tenantID), []string{"my-other-forwarder"}) assert.Equal(t, baseMgr.Forwarders(tenantID), []string{"fwd", "fwd-2"}) @@ -389,7 +399,6 @@ func TestUserConfigOverridesManager_MergeRuntimeConfig(t *testing.T) { assert.Equal(t, mgr.IngestionBurstSizeBytes(tenantID), baseMgr.IngestionBurstSizeBytes(tenantID)) assert.Equal(t, mgr.MetricsGeneratorIngestionSlack(tenantID), baseMgr.MetricsGeneratorIngestionSlack(tenantID)) assert.Equal(t, mgr.MetricsGeneratorRingSize(tenantID), baseMgr.MetricsGeneratorRingSize(tenantID)) - assert.Equal(t, mgr.MetricsGeneratorProcessors(tenantID), baseMgr.MetricsGeneratorProcessors(tenantID)) assert.Equal(t, mgr.MetricsGeneratorMaxActiveSeries(tenantID), baseMgr.MetricsGeneratorMaxActiveSeries(tenantID)) assert.Equal(t, mgr.MetricsGeneratorCollectionInterval(tenantID), baseMgr.MetricsGeneratorCollectionInterval(tenantID)) assert.Equal(t, mgr.MetricsGeneratorDisableCollection(tenantID), baseMgr.MetricsGeneratorDisableCollection(tenantID)) diff --git a/pkg/util/listtomap/list_to_map.go b/pkg/util/listtomap/list_to_map.go index f5e1f6ee2e8..527ddff64fa 100644 --- a/pkg/util/listtomap/list_to_map.go +++ b/pkg/util/listtomap/list_to_map.go @@ -74,3 +74,14 @@ func (l *ListToMap) GetMap() map[string]struct{} { } return *l } + +func Merge(m1, m2 ListToMap) ListToMap { + merged := make(ListToMap) + for k := range m1 { + merged[k] = struct{}{} + } + for k := range m2 { + merged[k] = struct{}{} + } + return merged +} diff --git a/pkg/util/listtomap/list_to_map_test.go b/pkg/util/listtomap/list_to_map_test.go index 6c82d0dde76..9985a787c23 100644 --- a/pkg/util/listtomap/list_to_map_test.go +++ b/pkg/util/listtomap/list_to_map_test.go @@ -93,3 +93,34 @@ func TestListToMapMarshalOperationsJSON(t *testing.T) { }) } } + +func TestMerge(t *testing.T) { + testCases := []struct { + name string + m1, m2, merged ListToMap + }{ + { + "merge keys from both maps", + map[string]struct{}{"key1": {}, "key3": {}}, + map[string]struct{}{"key2": {}, "key3": {}}, + map[string]struct{}{"key1": {}, "key2": {}, "key3": {}}, + }, + { + "nil map", + nil, + map[string]struct{}{"key1": {}}, + map[string]struct{}{"key1": {}}, + }, + { + "both nil", + nil, + nil, + map[string]struct{}{}, + }, + } + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + assert.Equal(t, tc.merged, Merge(tc.m1, tc.m2)) + }) + } +}