From 247089f30127cb1149507f894a2c219b06d98527 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Johannes=20Br=C3=BCderl?= Date: Thu, 4 Apr 2024 17:38:43 +0200 Subject: [PATCH] config: fix detailed metric keys missing in leading keys (#528) 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 455b0861..0c4152a6 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 3ed3984b..bceaa2e6 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) + }) + } +}