diff --git a/src/config/config_impl.go b/src/config/config_impl.go index 8663d5a01..ca7c6fe50 100644 --- a/src/config/config_impl.go +++ b/src/config/config_impl.go @@ -368,13 +368,13 @@ func (this *rateLimitConfigImpl) GetLimit( logger.Debugf("looking up key: %s", finalKey) nextDescriptor := descriptorsMap[finalKey] - matchedViaWildcard := false + var matchedWildcardKey string if nextDescriptor == nil && len(prevDescriptor.wildcardKeys) > 0 { for _, wildcardKey := range prevDescriptor.wildcardKeys { if strings.HasPrefix(finalKey, strings.TrimSuffix(wildcardKey, "*")) { nextDescriptor = descriptorsMap[wildcardKey] - matchedViaWildcard = true + matchedWildcardKey = wildcardKey break } } @@ -389,7 +389,7 @@ func (this *rateLimitConfigImpl) GetLimit( } // Track share_threshold pattern when matching via wildcard, even if no rate_limit at this level - if matchedViaWildcard && nextDescriptor != nil && nextDescriptor.shareThreshold && nextDescriptor.wildcardPattern != "" { + if matchedWildcardKey != "" && nextDescriptor != nil && nextDescriptor.shareThreshold && nextDescriptor.wildcardPattern != "" { // Extract the value part from the wildcard pattern (e.g., "key_files*" -> "files*") if shareThresholdPatterns == nil { shareThresholdPatterns = make(map[int]string) @@ -403,57 +403,38 @@ func (this *rateLimitConfigImpl) GetLimit( // Build value_to_metric metrics path for this level valueToMetricFullKey.WriteString(".") if nextDescriptor != nil { - // Check if share_threshold is enabled for this entry + // Determine the value to use for this entry + var valueToUse string hasShareThreshold := shareThresholdPatterns[i] != "" - if matchedViaWildcard { - // When share_threshold is enabled AND value_to_metric is enabled, use the prefix of the wildcard pattern - if hasShareThreshold && nextDescriptor.valueToMetric { - wildcardPrefix := strings.TrimSuffix(shareThresholdPatterns[i], "*") - valueToMetricFullKey.WriteString(entry.Key) - valueToMetricFullKey.WriteString("_") - valueToMetricFullKey.WriteString(wildcardPrefix) + + if matchedWildcardKey != "" { + // Matched via wildcard + if hasShareThreshold { + // share_threshold: always use wildcard pattern with * + valueToUse = shareThresholdPatterns[i] } else if nextDescriptor.valueToMetric { - valueToMetricFullKey.WriteString(entry.Key) - if entry.Value != "" { - valueToMetricFullKey.WriteString("_") - valueToMetricFullKey.WriteString(entry.Value) - } + // value_to_metric: use actual runtime value + valueToUse = entry.Value } else { - valueToMetricFullKey.WriteString(entry.Key) + // No flags: preserve wildcard pattern + valueToUse = strings.TrimPrefix(matchedWildcardKey, entry.Key+"_") } } else if matchedUsingValue { - // Matched explicit key+value in config - // When share_threshold is enabled AND value_to_metric is enabled, use the prefix of the wildcard pattern - if hasShareThreshold && nextDescriptor.valueToMetric { - wildcardPrefix := strings.TrimSuffix(shareThresholdPatterns[i], "*") - valueToMetricFullKey.WriteString(entry.Key) - valueToMetricFullKey.WriteString("_") - valueToMetricFullKey.WriteString(wildcardPrefix) - } else { - valueToMetricFullKey.WriteString(entry.Key) - if entry.Value != "" { - valueToMetricFullKey.WriteString("_") - valueToMetricFullKey.WriteString(entry.Value) - } - } + // Matched explicit key+value in config (share_threshold can't apply here) + valueToUse = entry.Value } else { // Matched default key (no value) in config - // When share_threshold is enabled AND value_to_metric is enabled, use the prefix of the wildcard pattern - if hasShareThreshold && nextDescriptor.valueToMetric { - wildcardPrefix := strings.TrimSuffix(shareThresholdPatterns[i], "*") - valueToMetricFullKey.WriteString(entry.Key) - valueToMetricFullKey.WriteString("_") - valueToMetricFullKey.WriteString(wildcardPrefix) - } else if nextDescriptor.valueToMetric { - valueToMetricFullKey.WriteString(entry.Key) - if entry.Value != "" { - valueToMetricFullKey.WriteString("_") - valueToMetricFullKey.WriteString(entry.Value) - } - } else { - valueToMetricFullKey.WriteString(entry.Key) + if nextDescriptor.valueToMetric { + valueToUse = entry.Value } } + + // Write key and value (if any) + valueToMetricFullKey.WriteString(entry.Key) + if valueToUse != "" { + valueToMetricFullKey.WriteString("_") + valueToMetricFullKey.WriteString(valueToUse) + } } else { // No next descriptor found; still append something deterministic valueToMetricFullKey.WriteString(entry.Key) @@ -510,22 +491,20 @@ func (this *rateLimitConfigImpl) GetLimit( } // Replace metric with detailed metric, if leaf descriptor is detailed. - // When share_threshold is enabled, expose the prefix (before *) of the wildcard pattern + // If share_threshold is enabled, always use wildcard pattern with * if rateLimit != nil && rateLimit.DetailedMetric { // Check if any entry has share_threshold enabled hasShareThreshold := rateLimit.ShareThresholdKeyPattern != nil && len(rateLimit.ShareThresholdKeyPattern) > 0 if hasShareThreshold { - // Build metric key with wildcard prefix for entries with share_threshold + // Build metric key with wildcard pattern (including *) for entries with share_threshold var shareThresholdMetricKey strings.Builder shareThresholdMetricKey.WriteString(domain) for i, entry := range descriptor.Entries { shareThresholdMetricKey.WriteString(".") if i < len(rateLimit.ShareThresholdKeyPattern) && rateLimit.ShareThresholdKeyPattern[i] != "" { - // Use the prefix of the wildcard pattern (before *) - wildcardPrefix := strings.TrimSuffix(rateLimit.ShareThresholdKeyPattern[i], "*") shareThresholdMetricKey.WriteString(entry.Key) shareThresholdMetricKey.WriteString("_") - shareThresholdMetricKey.WriteString(wildcardPrefix) + shareThresholdMetricKey.WriteString(rateLimit.ShareThresholdKeyPattern[i]) } else { // Include full key_value for entries without share_threshold shareThresholdMetricKey.WriteString(entry.Key) @@ -552,10 +531,8 @@ func (this *rateLimitConfigImpl) GetLimit( if enhancedKey != rateLimit.FullKey { // Recreate to ensure a clean stats struct, then set to enhanced stats originalShareThresholdKeyPattern := rateLimit.ShareThresholdKeyPattern - rateLimit = NewRateLimit(rateLimit.Limit.RequestsPerUnit, rateLimit.Limit.Unit, this.statsManager.NewStats(rateLimit.FullKey), rateLimit.Unlimited, rateLimit.ShadowMode, rateLimit.Name, rateLimit.Replaces, rateLimit.DetailedMetric) + rateLimit = NewRateLimit(rateLimit.Limit.RequestsPerUnit, rateLimit.Limit.Unit, this.statsManager.NewStats(enhancedKey), rateLimit.Unlimited, rateLimit.ShadowMode, rateLimit.Name, rateLimit.Replaces, rateLimit.DetailedMetric) rateLimit.ShareThresholdKeyPattern = originalShareThresholdKeyPattern - rateLimit.Stats = this.statsManager.NewStats(enhancedKey) - rateLimit.FullKey = enhancedKey } } diff --git a/test/config/config_test.go b/test/config/config_test.go index 19b4a6832..c32f84155 100644 --- a/test/config/config_test.go +++ b/test/config/config_test.go @@ -592,6 +592,10 @@ func TestWildcardConfig(t *testing.T) { }) assert.NotNil(withoutVal1) assert.Equal(withoutVal1, withoutVal2) + // Verify stats keys for no value descriptors + assert.Equal("test-domain.noVal", withoutVal1.Stats.Key, "No value descriptor should use just the key") + assert.Equal(withoutVal1.Stats.Key, withoutVal1.FullKey, "FullKey should match Stats.Key") + assert.Equal(withoutVal1.Stats.Key, withoutVal2.Stats.Key, "All no-value matches should have same stats key") // Matches multiple wildcard values and results are equal wildcard1 := rlConfig.GetLimit( @@ -611,7 +615,12 @@ func TestWildcardConfig(t *testing.T) { }) assert.NotNil(wildcard1) assert.Equal(wildcard1, wildcard2) + assert.Equal("test-domain.wild_foo*", wildcard1.Stats.Key, "Wildcard stats key should include the wildcard pattern with *") + assert.Equal("test-domain.wild_foo*", wildcard1.FullKey, "Wildcard FullKey should match Stats.Key") + assert.Equal("test-domain.wild_foo*", wildcard2.Stats.Key, "All wildcard matches should have same stats key with wildcard pattern") assert.NotNil(wildcard3) + assert.Equal("test-domain.nestedWild_val1.wild_goo*", wildcard3.Stats.Key, "Nested wildcard stats key should include the wildcard pattern with *") + assert.Equal("test-domain.nestedWild_val1.wild_goo*", wildcard3.FullKey, "Nested wildcard FullKey should match Stats.Key") // Doesn't match non-matching values noMatch := rlConfig.GetLimit( @@ -1615,21 +1624,24 @@ func TestShareThreshold(t *testing.T) { // Verify config: ShareThresholdKeyPattern is set correctly asrt.Equal("files/*", rl.ShareThresholdKeyPattern[0]) asrt.EqualValues(100, rl.Limit.RequestsPerUnit) - // Verify stats: All should have the same stats key - asrt.Equal("test-domain.files", rl.Stats.Key) + // Verify stats: All should have the same stats key with wildcard pattern + // share_threshold: stats key includes wildcard pattern with * + asrt.Equal("test-domain.files_files/*", rl.Stats.Key) rateLimits = append(rateLimits, rl) } // Verify all have same stats key for i := 1; i < len(rateLimits); i++ { asrt.Equal(rateLimits[0].Stats.Key, rateLimits[i].Stats.Key, "All values should have same stats key") + asrt.Equal(rateLimits[i].Stats.Key, rateLimits[i].FullKey, "FullKey should match Stats.Key for all rate limits") } }) // Test Case 2: share_threshold with metrics (value_to_metric and detailed_metric) // Tests that metrics correctly include wildcard prefix when share_threshold is enabled t.Run("share_threshold with metrics", func(t *testing.T) { - // Test value_to_metric + // Test value_to_metric with share_threshold + // share_threshold takes priority: should use wildcard pattern with * rl1 := rlConfig.GetLimit( context.TODO(), "test-domain", &pb_struct.RateLimitDescriptor{ @@ -1640,9 +1652,11 @@ func TestShareThreshold(t *testing.T) { }) asrt.NotNil(rl1) asrt.Equal("api/*", rl1.ShareThresholdKeyPattern[0]) - asrt.Equal("test-domain.route_api/.method", rl1.Stats.Key) + asrt.Equal("test-domain.route_api/*.method", rl1.Stats.Key, "share_threshold takes priority over value_to_metric, should use wildcard pattern with *") + asrt.Equal(rl1.Stats.Key, rl1.FullKey, "FullKey should match Stats.Key") - // Test detailed_metric + // Test detailed_metric with share_threshold + // share_threshold takes priority: should use wildcard pattern with * rl2 := rlConfig.GetLimit( context.TODO(), "test-domain", &pb_struct.RateLimitDescriptor{ @@ -1654,7 +1668,8 @@ func TestShareThreshold(t *testing.T) { asrt.NotNil(rl2) asrt.Equal("svc/*", rl2.ShareThresholdKeyPattern[0]) asrt.True(rl2.DetailedMetric) - asrt.Equal("test-domain.service_svc/.endpoint_get", rl2.Stats.Key) + asrt.Equal("test-domain.service_svc/*.endpoint_get", rl2.Stats.Key, "share_threshold takes priority over detailed_metric, should use wildcard pattern with *") + asrt.Equal(rl2.Stats.Key, rl2.FullKey, "FullKey should match Stats.Key") }) // Test Case 3: Nested wildcards with share_threshold @@ -1671,6 +1686,9 @@ func TestShareThreshold(t *testing.T) { asrt.NotNil(rl1) asrt.Equal("nested/*", rl1.ShareThresholdKeyPattern[1]) asrt.EqualValues(200, rl1.Limit.RequestsPerUnit) + // Verify stats key includes wildcard pattern for nested share_threshold + asrt.Equal("test-domain.nested_parent.files_nested/*", rl1.Stats.Key, "Nested share_threshold should include wildcard pattern with *") + asrt.Equal(rl1.Stats.Key, rl1.FullKey, "FullKey should match Stats.Key") // Multiple wildcards rl2 := rlConfig.GetLimit( @@ -1684,6 +1702,9 @@ func TestShareThreshold(t *testing.T) { asrt.NotNil(rl2) asrt.Equal("top/*", rl2.ShareThresholdKeyPattern[0]) asrt.Equal("nested/*", rl2.ShareThresholdKeyPattern[1]) + // Verify stats key includes wildcard patterns for multiple share_threshold entries + asrt.Equal("test-domain.files_top/*.files_nested/*", rl2.Stats.Key, "Multiple share_threshold entries should include all wildcard patterns") + asrt.Equal(rl2.Stats.Key, rl2.FullKey, "FullKey should match Stats.Key") // Parent has share_threshold, child does not rl3 := rlConfig.GetLimit( @@ -1698,6 +1719,10 @@ func TestShareThreshold(t *testing.T) { asrt.NotNil(rl3) asrt.Equal("path/*", rl3.ShareThresholdKeyPattern[0]) asrt.Equal("", rl3.ShareThresholdKeyPattern[1]) + // Verify stats key: parent has share_threshold (wildcard pattern), child is wildcard without share_threshold (preserves original behavior with *) + // Note: file has wildcard pattern file/* in config but share_threshold: false, so it preserves original wildcard behavior + asrt.Equal("test-domain.path_path/*.file_file/*.type", rl3.Stats.Key, "Parent with share_threshold uses wildcard pattern, child wildcard without share_threshold preserves original behavior") + asrt.Equal(rl3.Stats.Key, rl3.FullKey, "FullKey should match Stats.Key") // Both parent and child have share_threshold rl4 := rlConfig.GetLimit( @@ -1712,6 +1737,10 @@ func TestShareThreshold(t *testing.T) { asrt.NotNil(rl4) asrt.Equal("user/*", rl4.ShareThresholdKeyPattern[0]) asrt.Equal("res/*", rl4.ShareThresholdKeyPattern[1]) + // Verify stats key includes wildcard patterns for both parent and child with share_threshold + // Note: action is just a key with value, no wildcard, so it doesn't include the value in stats key (no value_to_metric or detailed_metric) + asrt.Equal("test-domain.user_user/*.resource_res/*.action", rl4.Stats.Key, "Both parent and child with share_threshold should include wildcard patterns, action key without flags uses just the key") + asrt.Equal(rl4.Stats.Key, rl4.FullKey, "FullKey should match Stats.Key") }) // Test Case 4: Explicit value takes precedence over wildcard @@ -1752,8 +1781,8 @@ func TestShareThreshold(t *testing.T) { asrt.Equal("test*", rl.ShareThresholdKeyPattern[0], "Pattern should be test*") asrt.EqualValues(100, rl.Limit.RequestsPerUnit, "Should have shared limit of 100") - // Verify Stats: All wildcard matches should have the same stats key - asrt.Equal("test-domain.file", rl.Stats.Key, "Stats key should be same for all wildcard values") + // Verify Stats: All wildcard matches should have the same stats key with wildcard pattern + asrt.Equal("test-domain.file_test*", rl.Stats.Key, "Stats key should include wildcard pattern with * when share_threshold is true") rateLimits = append(rateLimits, rl) } @@ -1767,7 +1796,7 @@ func TestShareThreshold(t *testing.T) { for _, rl := range rateLimits { rl.Stats.TotalHits.Inc() } - counterValue := stats.NewCounter("test-domain.file.total_hits").Value() + counterValue := stats.NewCounter("test-domain.file_test*.total_hits").Value() asrt.GreaterOrEqual(counterValue, uint64(len(testWildcardValues)), "All wildcard values should increment the same counter") // test1 should have its own counter @@ -1776,3 +1805,177 @@ func TestShareThreshold(t *testing.T) { asrt.GreaterOrEqual(counterTest1, uint64(1), "test1 should increment its own counter") }) } + +// TestWildcardStatsBehavior verifies the stats key behavior for wildcards under different flag combinations +func TestWildcardStatsBehavior(t *testing.T) { + asrt := assert.New(t) + store := stats.NewStore(stats.NewNullSink(), false) + + t.Run("No flags - preserves original behavior with wildcard pattern", func(t *testing.T) { + cfg := []config.RateLimitConfigToLoad{ + { + Name: "inline", + ConfigYaml: &config.YamlRoot{ + Domain: "test-domain", + Descriptors: []config.YamlDescriptor{ + { + Key: "wild", + Value: "foo*", + RateLimit: &config.YamlRateLimit{ + RequestsPerUnit: 20, + Unit: "minute", + }, + }, + }, + }, + }, + } + + rlConfig := config.NewRateLimitConfigImpl(cfg, mockstats.NewMockStatManager(store), false) + rl := rlConfig.GetLimit(context.TODO(), "test-domain", + &pb_struct.RateLimitDescriptor{ + Entries: []*pb_struct.RateLimitDescriptor_Entry{{Key: "wild", Value: "foo1"}}, + }) + asrt.NotNil(rl) + asrt.Equal("test-domain.wild_foo*", rl.Stats.Key) + asrt.Equal(rl.Stats.Key, rl.FullKey) + }) + + t.Run("value_to_metric - uses runtime value", func(t *testing.T) { + cfg := []config.RateLimitConfigToLoad{ + { + Name: "inline", + ConfigYaml: &config.YamlRoot{ + Domain: "test-domain", + Descriptors: []config.YamlDescriptor{ + { + Key: "wild", + Value: "foo*", + ValueToMetric: true, + RateLimit: &config.YamlRateLimit{ + RequestsPerUnit: 20, + Unit: "minute", + }, + }, + }, + }, + }, + } + + rlConfig := config.NewRateLimitConfigImpl(cfg, mockstats.NewMockStatManager(store), false) + rl := rlConfig.GetLimit(context.TODO(), "test-domain", + &pb_struct.RateLimitDescriptor{ + Entries: []*pb_struct.RateLimitDescriptor_Entry{{Key: "wild", Value: "foo1"}}, + }) + asrt.NotNil(rl) + asrt.Equal("test-domain.wild_foo1", rl.Stats.Key) + }) + + t.Run("share_threshold - uses wildcard pattern with *", func(t *testing.T) { + cfg := []config.RateLimitConfigToLoad{ + { + Name: "inline", + ConfigYaml: &config.YamlRoot{ + Domain: "test-domain", + Descriptors: []config.YamlDescriptor{ + { + Key: "wild", + Value: "foo*", + ShareThreshold: true, + RateLimit: &config.YamlRateLimit{ + RequestsPerUnit: 20, + Unit: "minute", + }, + }, + }, + }, + }, + } + + rlConfig := config.NewRateLimitConfigImpl(cfg, mockstats.NewMockStatManager(store), false) + rl := rlConfig.GetLimit(context.TODO(), "test-domain", + &pb_struct.RateLimitDescriptor{ + Entries: []*pb_struct.RateLimitDescriptor_Entry{{Key: "wild", Value: "foo1"}}, + }) + asrt.NotNil(rl) + asrt.Equal("test-domain.wild_foo*", rl.Stats.Key) + asrt.Equal(rl.Stats.Key, rl.FullKey) + }) + + t.Run("share_threshold with value_to_metric - share_threshold takes priority", func(t *testing.T) { + cfg := []config.RateLimitConfigToLoad{ + { + Name: "inline", + ConfigYaml: &config.YamlRoot{ + Domain: "test-domain", + Descriptors: []config.YamlDescriptor{ + { + Key: "wild", + Value: "foo*", + ShareThreshold: true, + ValueToMetric: true, + RateLimit: &config.YamlRateLimit{ + RequestsPerUnit: 20, + Unit: "minute", + }, + }, + }, + }, + }, + } + + rlConfig := config.NewRateLimitConfigImpl(cfg, mockstats.NewMockStatManager(store), false) + rl := rlConfig.GetLimit(context.TODO(), "test-domain", + &pb_struct.RateLimitDescriptor{ + Entries: []*pb_struct.RateLimitDescriptor_Entry{{Key: "wild", Value: "foo1"}}, + }) + asrt.NotNil(rl) + asrt.Equal("test-domain.wild_foo*", rl.Stats.Key) + asrt.Equal(rl.Stats.Key, rl.FullKey) + }) + + t.Run("value_to_metric in other descriptor - wildcard pattern preserved", func(t *testing.T) { + cfg := []config.RateLimitConfigToLoad{ + { + Name: "inline", + ConfigYaml: &config.YamlRoot{ + Domain: "test-domain", + Descriptors: []config.YamlDescriptor{ + { + Key: "wild", + Value: "foo*", + Descriptors: []config.YamlDescriptor{ + { + Key: "other", + ValueToMetric: true, + Descriptors: []config.YamlDescriptor{ + { + Key: "limit", + RateLimit: &config.YamlRateLimit{RequestsPerUnit: 20, Unit: "minute"}, + }, + }, + }, + }, + }, + }, + }, + }, + } + + rlConfig := config.NewRateLimitConfigImpl(cfg, mockstats.NewMockStatManager(store), false) + rl := rlConfig.GetLimit( + context.TODO(), "test-domain", + &pb_struct.RateLimitDescriptor{ + Entries: []*pb_struct.RateLimitDescriptor_Entry{ + {Key: "wild", Value: "foo1"}, + {Key: "other", Value: "bar"}, + {Key: "limit", Value: "X"}, + }, + }, + ) + asrt.NotNil(rl) + // Wildcard pattern should be preserved even though value_to_metric is enabled on "other" + asrt.Equal("test-domain.wild_foo*.other_bar.limit", rl.Stats.Key, "Should preserve wildcard pattern when value_to_metric is only on other descriptor") + asrt.Equal(rl.Stats.Key, rl.FullKey, "FullKey should match Stats.Key") + }) +}