From 1272392fbea1e0e5213f70b2641b1bc2a3e4634b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Johannes=20Br=C3=BCderl?= Date: Wed, 3 Apr 2024 12:56:41 +0200 Subject: [PATCH] config: fix detailed metric keys missing in leading keys MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit only the last descriptor uses the provided descriptor value in case of detailed metrics. when traversing the list of descriptors, the code "loses" the previous keys. this leads to metrics like: "test-domain.first-key_.second-key_second-value", where the last descriptor properly uses the detailed metric descriptor value, but all other descriptors (the first one here) are missing the value. this patch introduces a new string builder, that builds the detailed metric as the iteration of the input descriptor is happening. a unit test is attached to show the behavior. it fails without the new code, and successfully preserves all descriptor keys with the patched code. Signed-off-by: Johannes Brüderl --- src/config/config_impl.go | 16 +- test/config/config_test.go | 316 +++++++++++++++++++++++++++++++++++++ 2 files changed, 331 insertions(+), 1 deletion(-) diff --git a/src/config/config_impl.go b/src/config/config_impl.go index 455b0861a..0c4152a60 100644 --- a/src/config/config_impl.go +++ b/src/config/config_impl.go @@ -306,10 +306,19 @@ func (this *rateLimitConfigImpl) GetLimit( descriptorsMap := value.descriptors prevDescriptor := &value.rateLimitDescriptor + + // Build detailed metric as we traverse the list of descriptors + var detailedMetricFullKey strings.Builder + detailedMetricFullKey.WriteString(domain) + for i, entry := range descriptor.Entries { // First see if key_value is in the map. If that isn't in the map we look for just key // to check for a default value. finalKey := entry.Key + "_" + entry.Value + + detailedMetricFullKey.WriteString(".") + detailedMetricFullKey.WriteString(finalKey) + logger.Debugf("looking up key: %s", finalKey) nextDescriptor := descriptorsMap[finalKey] @@ -343,7 +352,7 @@ func (this *rateLimitConfigImpl) GetLimit( descriptorsMap = nextDescriptor.descriptors } else { if rateLimit != nil && rateLimit.DetailedMetric { - rateLimit = NewRateLimit(rateLimit.Limit.RequestsPerUnit, rateLimit.Limit.Unit, this.statsManager.NewStats(rateLimit.FullKey+"_"+entry.Value), rateLimit.Unlimited, rateLimit.ShadowMode, rateLimit.Name, rateLimit.Replaces, false) + rateLimit = NewRateLimit(rateLimit.Limit.RequestsPerUnit, rateLimit.Limit.Unit, this.statsManager.NewStats(rateLimit.FullKey), rateLimit.Unlimited, rateLimit.ShadowMode, rateLimit.Name, rateLimit.Replaces, rateLimit.DetailedMetric) } break @@ -351,6 +360,11 @@ func (this *rateLimitConfigImpl) GetLimit( prevDescriptor = nextDescriptor } + // Replace metric with detailed metric, if leaf descriptor is detailed. + if rateLimit != nil && rateLimit.DetailedMetric { + rateLimit.Stats = this.statsManager.NewStats(detailedMetricFullKey.String()) + } + return rateLimit } diff --git a/test/config/config_test.go b/test/config/config_test.go index 3ed3984ba..bceaa2e6d 100644 --- a/test/config/config_test.go +++ b/test/config/config_test.go @@ -12,6 +12,7 @@ import ( pb_type "github.com/envoyproxy/go-control-plane/envoy/type/v3" stats "github.com/lyft/gostats" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" "github.com/envoyproxy/ratelimit/src/config" mockstats "github.com/envoyproxy/ratelimit/test/mocks/stats" @@ -631,3 +632,318 @@ func TestWildcardConfig(t *testing.T) { }) assert.Nil(midWildcard) } + +func TestDetailedMetric(t *testing.T) { + assert := require.New(t) + stats := stats.NewStore(stats.NewNullSink(), false) + + // Descriptor config with a realistic nested setup, that is re-used across + // multiple test cases. + realisticExampleConfig := &config.YamlRoot{ + Domain: "nested", + Descriptors: []config.YamlDescriptor{ + { + Key: "key_1", + RateLimit: &config.YamlRateLimit{ + RequestsPerUnit: 500, + Unit: "minute", + }, + DetailedMetric: true, + }, + { + Key: "key_1", + Value: "some-value-for-key-1", + RateLimit: &config.YamlRateLimit{ + RequestsPerUnit: 500, + Unit: "minute", + }, + }, + { + Key: "key_2", + RateLimit: &config.YamlRateLimit{ + RequestsPerUnit: 5000, + Unit: "minute", + }, + Descriptors: []config.YamlDescriptor{ + { + Key: "key_3", + DetailedMetric: true, + }, + { + Key: "key_3", + Value: "requested-key3-value", + RateLimit: &config.YamlRateLimit{ + RequestsPerUnit: 5, + Unit: "minute", + }, + DetailedMetric: true, + }, + }, + DetailedMetric: true, + }, + { + Key: "key_2", + Value: "specific-id", + RateLimit: &config.YamlRateLimit{ + RequestsPerUnit: 50000, + Unit: "minute", + }, + Descriptors: []config.YamlDescriptor{ + { + Key: "key_3", + DetailedMetric: true, + }, + { + Key: "key_3", + Value: "requested-key3-value", + RateLimit: &config.YamlRateLimit{ + RequestsPerUnit: 100, + Unit: "minute", + }, + DetailedMetric: true, + }, + }, + DetailedMetric: true, + }, + }, + } + + tests := []struct { + name string + config []config.RateLimitConfigToLoad + request *pb_struct.RateLimitDescriptor + expectedStatsKey string + }{ + { + name: "nested with no values but request only top-level key", + config: []config.RateLimitConfigToLoad{ + { + ConfigYaml: &config.YamlRoot{ + Domain: "nested", + Descriptors: []config.YamlDescriptor{ + { + Key: "key-1", + RateLimit: &config.YamlRateLimit{ + RequestsPerUnit: 100, + Unit: "minute", + }, + Descriptors: []config.YamlDescriptor{ + { + Key: "key-2", + RateLimit: &config.YamlRateLimit{ + RequestsPerUnit: 5, + Unit: "minute", + }, + }, + }, + DetailedMetric: true, + }, + }, + }, + }, + }, + request: &pb_struct.RateLimitDescriptor{ + Entries: []*pb_struct.RateLimitDescriptor_Entry{ + { + Key: "key-1", + Value: "value-1", + }, + }, + }, + expectedStatsKey: "nested.key-1_value-1", + }, + { + name: "nested with no values but request only top-level key with no detailed metric", + config: []config.RateLimitConfigToLoad{ + { + ConfigYaml: &config.YamlRoot{ + Domain: "nested", + Descriptors: []config.YamlDescriptor{ + { + Key: "key-1", + RateLimit: &config.YamlRateLimit{ + RequestsPerUnit: 100, + Unit: "minute", + }, + Descriptors: []config.YamlDescriptor{ + { + Key: "key-2", + RateLimit: &config.YamlRateLimit{ + RequestsPerUnit: 5, + Unit: "minute", + }, + }, + }, + DetailedMetric: false, + }, + }, + }, + }, + }, + request: &pb_struct.RateLimitDescriptor{ + Entries: []*pb_struct.RateLimitDescriptor_Entry{ + { + Key: "key-1", + Value: "value-1", + }, + }, + }, + expectedStatsKey: "nested.key-1", + }, + { + name: "nested with no values and request fully nested descriptors", + config: []config.RateLimitConfigToLoad{ + { + ConfigYaml: &config.YamlRoot{ + Domain: "nested", + Descriptors: []config.YamlDescriptor{ + { + Key: "key-1", + RateLimit: &config.YamlRateLimit{ + RequestsPerUnit: 100, + Unit: "minute", + }, + Descriptors: []config.YamlDescriptor{ + { + Key: "key-2", + RateLimit: &config.YamlRateLimit{ + RequestsPerUnit: 5, + Unit: "minute", + }, + DetailedMetric: true, + }, + }, + DetailedMetric: true, + }, + }, + }, + }, + }, + request: &pb_struct.RateLimitDescriptor{ + Entries: []*pb_struct.RateLimitDescriptor_Entry{ + { + Key: "key-1", + Value: "value-1", + }, + { + Key: "key-2", + Value: "value-2", + }, + }, + }, + expectedStatsKey: "nested.key-1_value-1.key-2_value-2", + }, + { + name: "nested with no values and request fully nested descriptors with no detailed metric", + config: []config.RateLimitConfigToLoad{ + { + ConfigYaml: &config.YamlRoot{ + Domain: "nested", + Descriptors: []config.YamlDescriptor{ + { + Key: "key-1", + RateLimit: &config.YamlRateLimit{ + RequestsPerUnit: 100, + Unit: "minute", + }, + Descriptors: []config.YamlDescriptor{ + { + Key: "key-2", + RateLimit: &config.YamlRateLimit{ + RequestsPerUnit: 5, + Unit: "minute", + }, + DetailedMetric: false, + }, + }, + DetailedMetric: false, + }, + }, + }, + }, + }, + request: &pb_struct.RateLimitDescriptor{ + Entries: []*pb_struct.RateLimitDescriptor_Entry{ + { + Key: "key-1", + Value: "value-1", + }, + { + Key: "key-2", + Value: "value-2", + }, + }, + }, + expectedStatsKey: "nested.key-1.key-2", + }, + { + name: "test nested descriptors with simple request", + config: []config.RateLimitConfigToLoad{ + { + ConfigYaml: realisticExampleConfig, + }, + }, + request: &pb_struct.RateLimitDescriptor{ + Entries: []*pb_struct.RateLimitDescriptor_Entry{ + { + Key: "key_1", + Value: "value-for-key-1", + }, + }, + }, + expectedStatsKey: "nested.key_1_value-for-key-1", + }, + { + name: "test nested only second descriptor request not nested", + config: []config.RateLimitConfigToLoad{ + { + ConfigYaml: realisticExampleConfig, + }, + }, + request: &pb_struct.RateLimitDescriptor{ + Entries: []*pb_struct.RateLimitDescriptor_Entry{ + { + Key: "key_2", + Value: "key-2-value", + }, + }, + }, + expectedStatsKey: "nested.key_2_key-2-value", + }, + { + name: "test nested descriptors with nested request", + config: []config.RateLimitConfigToLoad{ + { + ConfigYaml: realisticExampleConfig, + }, + }, + request: &pb_struct.RateLimitDescriptor{ + Entries: []*pb_struct.RateLimitDescriptor_Entry{ + { + Key: "key_2", + Value: "key-2-value", + }, + { + Key: "key_3", + Value: "requested-key3-value", + }, + }, + }, + expectedStatsKey: "nested.key_2_key-2-value.key_3_requested-key3-value", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + rlConfig := config.NewRateLimitConfigImpl(tt.config, mockstats.NewMockStatManager(stats), true) + rlConfig.Dump() + + rl := rlConfig.GetLimit( + context.TODO(), "nested", + tt.request, + ) + assert.NotNil(rl) + assert.Equal(tt.expectedStatsKey, rl.Stats.Key) + }) + } +}