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

config: fix detailed metric keys missing in leading keys #528

Merged
merged 1 commit into from
Apr 4, 2024
Merged

config: fix detailed metric keys missing in leading keys #528

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
16 changes: 15 additions & 1 deletion src/config/config_impl.go
View file
Open in desktop
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)
Comment on lines +310 to +312
Copy link
Member

Choose a reason for hiding this comment

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

perf nit: can we only do this if detailed metrics are enabled?

Copy link
Contributor Author

@birdayz birdayz Apr 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

i fear it's not easily possible. only once we traversed the full "tree", we know if the leaf is a detailed metric (it can be hierarchical, and the leaf is the deciding factor).

so we build this string, even if we discard it later (line 364).

the previous solution loses this information because of exactly this reason (keys of previous descriptors in the "tree" are lost). so, basically, we need to memorize the path we have taken (including the keys AND values of the descriptors) we have seen, even if we drop it later.

i can think of two alternatives:

  • we could traverse the tree again, after we have traversed it once, and see that the leaf has detailed metrics on (line 364). I haven't benchmarked, but i don't feel that this is faster than doing a bit of string concatenation (which cost most likely way less than the entire walk through the hierarchy)
  • add a settings option to ratelimit-service, that steers detailed metrics in general. i.e. you can completely turn it off if you wish. however, even here we would spend a little more time doing this string concat, if somebody wants to have detailed metrics only for a small subset of their metrics.

I'm not sure if any of these is simpler than "just" doing the string concat and dropping later.

i tried to avoid refactoring this function more, because i am not very familiar with the codebase - and want to avoid introducing bugs i did not think of.

I personally think it should be fine to add this additional string concat (we do a few already, e.g. line 317). But of course, i'll ultimately defer to your judgment, how do you think about this?

Copy link
Member

Choose a reason for hiding this comment

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

Sure fair enough, thanks for the detailed explanation. SGTM.


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
Open in desktop
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)
})
}
}