Skip to content

Commit

Permalink
config: fix detailed metric keys missing in leading keys (#528)
Browse files Browse the repository at this point in the history 
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 <johannes.bruederl@gmail.com>
  • Loading branch information
@birdayz
birdayz authored Apr 4, 2024
1 parent 3654bfd commit 247089f
Show file tree
Hide file tree
Showing 2 changed files with 331 additions and 1 deletion.
16 changes: 15 additions & 1 deletion src/config/config_impl.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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]

Expand Down Expand Up @@ -343,14 +352,19 @@ 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
}
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
}

Expand Down
316 changes: 316 additions & 0 deletions test/config/config_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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)
})
}
}

0 comments on commit 247089f

Please sign in to comment.