Skip to content

feat: Add field to add unspecified value to metric #996

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

Merged
collin-lee merged 4 commits into from
Nov 29, 2025
Merged

feat: Add field to add unspecified value to metric #996

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
9e9d3d5
Add field to add unspecified value to metric
xuannam230201 Oct 31, 2025
17dbaf6
Update README.md to pass docs_check_format check
xuannam230201 Nov 11, 2025
10d3389
Update format to pass pre-commit check
xuannam230201 Nov 11, 2025
f60829f
Update based on comments and add more unit tests
xuannam230201 Nov 28, 2025
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
63 changes: 63 additions & 0 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
- [Replaces](#replaces)
- [ShadowMode](#shadowmode)
- [Including detailed metrics for unspecified values](#including-detailed-metrics-for-unspecified-values)
- [Including descriptor values in metrics](#including-descriptor-values-in-metrics)
- [Examples](#examples)
- [Example 1](#example-1)
- [Example 2](#example-2)
Expand All @@ -31,6 +32,7 @@
- [Example 7](#example-7)
- [Example 8](#example-8)
- [Example 9](#example-9)
- [Example 10](#example-10)
- [Loading Configuration](#loading-configuration)
- [File Based Configuration Loading](#file-based-configuration-loading)
- [xDS Management Server Based Configuration Loading](#xds-management-server-based-configuration-loading)
Expand Down Expand Up @@ -282,6 +284,7 @@ descriptors:
requests_per_unit: <see below: required>
shadow_mode: (optional)
detailed_metric: (optional)
value_to_metric: (optional)
descriptors: (optional block)
- ... (nested repetition of above)
```
Expand Down Expand Up @@ -336,6 +339,14 @@ Setting the `detailed_metric: true` for a descriptor will extend the metrics tha

NB! This should only be enabled in situations where the potentially large cardinality of metrics that this can lead to is acceptable.

### Including descriptor values in metrics

Setting `value_to_metric: true` (default: `false`) for a descriptor will include the descriptor's runtime value in the metric key, even when the descriptor value is not explicitly defined in the configuration. This allows you to track metrics per descriptor value when the value comes from the runtime request, providing visibility into different rate limit scenarios without needing to pre-define every possible value.

**Note:** If a value is explicitly specified in a descriptor (e.g., `value: "GET"`), that value is always included in the metric key regardless of the `value_to_metric` setting. The `value_to_metric` flag only affects descriptors where the value is not explicitly defined in the configuration.

When combined with wildcard matching, the full runtime value is included in the metric key, not just the wildcard prefix. This feature works independently of `detailed_metric` - when `detailed_metric` is set, it takes precedence and `value_to_metric` is ignored.

### Examples

#### Example 1
Expand Down Expand Up @@ -629,6 +640,58 @@ descriptors:
requests_per_unit: 20
```

#### Example 10

Using `value_to_metric: true` to include descriptor values in metrics when values are not explicitly defined in the configuration:

```yaml
domain: example10
descriptors:
- key: route
value_to_metric: true
descriptors:
- key: http_method
value_to_metric: true
descriptors:
- key: subject_id
rate_limit:
unit: minute
requests_per_unit: 60
```

With this configuration, requests with different runtime values for `route` and `http_method` will generate separate metrics:

- Request: `route=api`, `http_method=GET`, `subject_id=123`
- Metric key: `example10.route_api.http_method_GET.subject_id`

- Request: `route=web`, `http_method=POST`, `subject_id=456`
- Metric key: `example10.route_web.http_method_POST.subject_id`

Without `value_to_metric: true`, both requests would use the same metric key: `example10.route.http_method.subject_id`.

When combined with wildcard matching, the full runtime value is included:

```yaml
domain: example10_wildcard
descriptors:
- key: user
value_to_metric: true
descriptors:
- key: action
value: read*
value_to_metric: true
descriptors:
- key: resource
rate_limit:
unit: minute
requests_per_unit: 100
```

- Request: `user=alice`, `action=readfile`, `resource=documents`
- Metric key: `example10_wildcard.user_alice.action_readfile.resource`

Note: When `detailed_metric: true` is set on a descriptor, it takes precedence and `value_to_metric` is ignored for that descriptor.

## Loading Configuration

Rate limit service supports following configuration loading methods. You can define which methods to use by configuring environment variable `CONFIG_TYPE`.
Expand Down
74 changes: 68 additions & 6 deletions src/config/config_impl.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ type YamlDescriptor struct {
Descriptors []YamlDescriptor
ShadowMode bool `yaml:"shadow_mode"`
DetailedMetric bool `yaml:"detailed_metric"`
ValueToMetric bool `yaml:"value_to_metric"`
}

type YamlRoot struct {
Expand All @@ -40,9 +41,10 @@ type YamlRoot struct {
}

type rateLimitDescriptor struct {
descriptors map[string]*rateLimitDescriptor
limit *RateLimit
wildcardKeys []string
descriptors map[string]*rateLimitDescriptor
limit *RateLimit
wildcardKeys []string
valueToMetric bool
}

type rateLimitDomain struct {
Expand All @@ -68,6 +70,7 @@ var validKeys = map[string]bool{
"name": true,
"replaces": true,
"detailed_metric": true,
"value_to_metric": true,
}

// Create a new rate limit config entry.
Expand Down Expand Up @@ -185,7 +188,7 @@ func (this *rateLimitDescriptor) loadDescriptors(config RateLimitConfigToLoad, p

logger.Debugf(
"loading descriptor: key=%s%s", newParentKey, rateLimitDebugString)
newDescriptor := &rateLimitDescriptor{map[string]*rateLimitDescriptor{}, rateLimit, nil}
newDescriptor := &rateLimitDescriptor{map[string]*rateLimitDescriptor{}, rateLimit, nil, descriptorConfig.ValueToMetric}
newDescriptor.loadDescriptors(config, newParentKey+".", descriptorConfig.Descriptors, statsManager)
this.descriptors[finalKey] = newDescriptor

Expand Down Expand Up @@ -262,7 +265,7 @@ func (this *rateLimitConfigImpl) loadConfig(config RateLimitConfigToLoad) {
}

logger.Debugf("loading domain: %s", root.Domain)
newDomain := &rateLimitDomain{rateLimitDescriptor{map[string]*rateLimitDescriptor{}, nil, nil}}
newDomain := &rateLimitDomain{rateLimitDescriptor{map[string]*rateLimitDescriptor{}, nil, nil, false}}
newDomain.loadDescriptors(config, root.Domain+".", root.Descriptors, this.statsManager)
this.domains[root.Domain] = newDomain
}
Expand Down Expand Up @@ -313,6 +316,10 @@ func (this *rateLimitConfigImpl) GetLimit(
var detailedMetricFullKey strings.Builder
detailedMetricFullKey.WriteString(domain)

// Build value_to_metric-enhanced metric key as we traverse
var valueToMetricFullKey strings.Builder
valueToMetricFullKey.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.
Expand All @@ -323,20 +330,61 @@ func (this *rateLimitConfigImpl) GetLimit(

logger.Debugf("looking up key: %s", finalKey)
nextDescriptor := descriptorsMap[finalKey]
matchedViaWildcard := false

if nextDescriptor == nil && len(prevDescriptor.wildcardKeys) > 0 {
for _, wildcardKey := range prevDescriptor.wildcardKeys {
if strings.HasPrefix(finalKey, strings.TrimSuffix(wildcardKey, "*")) {
nextDescriptor = descriptorsMap[wildcardKey]
matchedViaWildcard = true
break
}
}
}

matchedUsingValue := nextDescriptor != nil
if nextDescriptor == nil {
finalKey = entry.Key
logger.Debugf("looking up key: %s", finalKey)
nextDescriptor = descriptorsMap[finalKey]
matchedUsingValue = false
}

// Build value_to_metric metrics path for this level
valueToMetricFullKey.WriteString(".")
if nextDescriptor != nil {
if matchedViaWildcard {
if nextDescriptor.valueToMetric {
valueToMetricFullKey.WriteString(entry.Key)
if entry.Value != "" {
valueToMetricFullKey.WriteString("_")
valueToMetricFullKey.WriteString(entry.Value)
}
} else {
valueToMetricFullKey.WriteString(entry.Key)
}
} else if matchedUsingValue {
// Matched explicit key+value in config
valueToMetricFullKey.WriteString(entry.Key)
if entry.Value != "" {
valueToMetricFullKey.WriteString("_")
valueToMetricFullKey.WriteString(entry.Value)
}
} else {
// Matched default key (no value) in config
if nextDescriptor.valueToMetric {
valueToMetricFullKey.WriteString(entry.Key)
if entry.Value != "" {
valueToMetricFullKey.WriteString("_")
valueToMetricFullKey.WriteString(entry.Value)
}
} else {
valueToMetricFullKey.WriteString(entry.Key)
}
}
} else {
// No next descriptor found; still append something deterministic
valueToMetricFullKey.WriteString(entry.Key)
}

if nextDescriptor != nil && nextDescriptor.limit != nil {
Expand Down Expand Up @@ -364,7 +412,21 @@ func (this *rateLimitConfigImpl) GetLimit(

// Replace metric with detailed metric, if leaf descriptor is detailed.
if rateLimit != nil && rateLimit.DetailedMetric {
rateLimit.Stats = this.statsManager.NewStats(detailedMetricFullKey.String())
detailedKey := detailedMetricFullKey.String()
rateLimit.Stats = this.statsManager.NewStats(detailedKey)
rateLimit.FullKey = detailedKey
}

// If not using detailed metric, but any value_to_metric path produced a different key,
// override stats to use the value_to_metric-enhanced key
if rateLimit != nil && !rateLimit.DetailedMetric {
enhancedKey := valueToMetricFullKey.String()
if enhancedKey != rateLimit.FullKey {
// Recreate to ensure a clean stats struct, then set to enhanced stats
rateLimit = NewRateLimit(rateLimit.Limit.RequestsPerUnit, rateLimit.Limit.Unit, this.statsManager.NewStats(rateLimit.FullKey), rateLimit.Unlimited, rateLimit.ShadowMode, rateLimit.Name, rateLimit.Replaces, rateLimit.DetailedMetric)
rateLimit.Stats = this.statsManager.NewStats(enhancedKey)
rateLimit.FullKey = enhancedKey
}
Copy link
Copy Markdown
Contributor

@collin-lee collin-lee Nov 12, 2025

Choose a reason for hiding this comment

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

@xuannam230201

Should we add after 419:

rateLimit.FullKey = enhancedKey

FullKey is used for logging/debugging (e.g., in base_limiter.go), Stats.Key is the actual metric key used for statistics. They should match so logging reflects the actual metrics being tracked

// Without the fix:
rateLimit.FullKey = "domain.route.http_method" // Old key (shown in logs)
rateLimit.Stats.Key = "domain.route_api.http_method_GET" // New key (used for metrics)

Copy link
Copy Markdown
Contributor

@collin-lee collin-lee Nov 26, 2025

Choose a reason for hiding this comment

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

@xuannam230201 see this comment about adding rateLimit.FullKey = enhancedKey after

Here's a test we could add to config_test.go:

`// TestValueToMetric_FullKeyMatchesStatsKey verifies that rateLimit.FullKey always matches
// rateLimit.Stats.Key. This is important for debugging and log/metric correlation.
// FullKey is used in debug logs, while Stats.Key is used for actual metrics.
func TestValueToMetric_FullKeyMatchesStatsKey(t *testing.T) {
asrt := assert.New(t)
store := stats.NewStore(stats.NewNullSink(), false)

cfg := []config.RateLimitConfigToLoad{
	{
		Name: "inline",
		ConfigYaml: &config.YamlRoot{
			Domain: "test-domain",
			Descriptors: []config.YamlDescriptor{
				{
					Key:           "route",
					ValueToMetric: true,
					Descriptors: []config.YamlDescriptor{
						{
							Key:           "http_method",
							ValueToMetric: true,
							Descriptors: []config.YamlDescriptor{
								{
									Key: "subject_id",
									RateLimit: &config.YamlRateLimit{
										RequestsPerUnit: 60,
										Unit:            "minute",
									},
								},
							},
						},
					},
				},
			},
		},
	},
}

rlConfig := config.NewRateLimitConfigImpl(cfg, mockstats.NewMockStatManager(store), false)

// Test case 1: value_to_metric enabled - FullKey should match Stats.Key
rl := rlConfig.GetLimit(
	context.TODO(), "test-domain",
	&pb_struct.RateLimitDescriptor{
		Entries: []*pb_struct.RateLimitDescriptor_Entry{
			{Key: "route", Value: "api"},
			{Key: "http_method", Value: "GET"},
			{Key: "subject_id", Value: "user123"},
		},
	},
)
asrt.NotNil(rl)
asrt.Equal(rl.FullKey, rl.Stats.Key, "FullKey should match Stats.Key when value_to_metric is enabled")
expectedKey := "test-domain.route_api.http_method_GET.subject_id"
asrt.Equal(expectedKey, rl.FullKey)
asrt.Equal(expectedKey, rl.Stats.Key)

// Test case 2: value_to_metric disabled - FullKey should still match Stats.Key
cfgNoValueToMetric := []config.RateLimitConfigToLoad{
	{
		Name: "inline",
		ConfigYaml: &config.YamlRoot{
			Domain: "test-domain-2",
			Descriptors: []config.YamlDescriptor{
				{
					Key: "route",
					Descriptors: []config.YamlDescriptor{
						{
							Key: "http_method",
							Descriptors: []config.YamlDescriptor{
								{
									Key: "subject_id",
									RateLimit: &config.YamlRateLimit{
										RequestsPerUnit: 60,
										Unit:            "minute",
									},
								},
							},
						},
					},
				},
			},
		},
	},
}

rlConfig2 := config.NewRateLimitConfigImpl(cfgNoValueToMetric, mockstats.NewMockStatManager(store), false)
rl2 := rlConfig2.GetLimit(
	context.TODO(), "test-domain-2",
	&pb_struct.RateLimitDescriptor{
		Entries: []*pb_struct.RateLimitDescriptor_Entry{
			{Key: "route", Value: "api"},
			{Key: "http_method", Value: "GET"},
			{Key: "subject_id", Value: "user123"},
		},
	},
)
asrt.NotNil(rl2)
asrt.Equal(rl2.FullKey, rl2.Stats.Key, "FullKey should match Stats.Key even when value_to_metric is disabled")

// Test case 3: detailed_metric enabled - FullKey should match Stats.Key
cfgDetailedMetric := []config.RateLimitConfigToLoad{
	{
		Name: "inline",
		ConfigYaml: &config.YamlRoot{
			Domain: "test-domain-3",
			Descriptors: []config.YamlDescriptor{
				{
					Key: "route",
					Descriptors: []config.YamlDescriptor{
						{
							Key: "http_method",
							Descriptors: []config.YamlDescriptor{
								{
									Key:            "subject_id",
									DetailedMetric: true,
									RateLimit: &config.YamlRateLimit{
										RequestsPerUnit: 60,
										Unit:            "minute",
									},
								},
							},
						},
					},
				},
			},
		},
	},
}

rlConfig3 := config.NewRateLimitConfigImpl(cfgDetailedMetric, mockstats.NewMockStatManager(store), false)
rl3 := rlConfig3.GetLimit(
	context.TODO(), "test-domain-3",
	&pb_struct.RateLimitDescriptor{
		Entries: []*pb_struct.RateLimitDescriptor_Entry{
			{Key: "route", Value: "api"},
			{Key: "http_method", Value: "GET"},
			{Key: "subject_id", Value: "user123"},
		},
	},
)
asrt.NotNil(rl3)
asrt.Equal(rl3.FullKey, rl3.Stats.Key, "FullKey should match Stats.Key when detailed_metric is enabled")

}
`

Copy link
Copy Markdown
Contributor Author

@xuannam230201 xuannam230201 Nov 28, 2025

Choose a reason for hiding this comment

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

Good to know that!

However, currently, I see that FullKey and Stat.Key in detailed_metric are not the same (because current code doesn't set FullKey to Stat.Key in detailed_metric).

I set FullKey to Stat.Key in both cases (detailed_metric and value_to_metric), and also added unit tests for these cases. Is it okay?

Copy link
Copy Markdown
Contributor

@collin-lee collin-lee Nov 29, 2025

Choose a reason for hiding this comment

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

ok great - thanks! LGTM

}

return rateLimit
Expand Down
Loading
Loading