[ISSUE] Fix: Wildcard Stats Key Behavior Changes#1017
[ISSUE] Fix: Wildcard Stats Key Behavior Changes#1017collin-lee merged 7 commits intoenvoyproxy:mainfrom
Conversation
711a97d to
40f3048
Compare
|
Hi @collin-lee , this issue was introduced in my another PR (this line and this line). As a result, I create this PR to address this issue (to maintain backward compatibility for metrics of wild card). Could you please help to take a look at this? Thanks in advance! |
40f3048 to
430727e
Compare
Signed-off-by: Nam Dang <xuannam230201@gmail.com>
430727e to
5a4a039
Compare
Signed-off-by: Nam Dang <xuannam230201@gmail.com>
Signed-off-by: Nam Dang <xuannam230201@gmail.com>
…uest Signed-off-by: Nam Dang <xuannam230201@gmail.com>
Signed-off-by: Nam Dang <xuannam230201@gmail.com>
| 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) |
There was a problem hiding this comment.
Remove these codes because each time this.statsManager.NewStats is called, logger prints debug log "Creating stats..." once. As a result, with current code, each request makes logger log that twice.
FullKey is automatically assigned to stat.string when rateLimit is initialized; as a result, remove the assignment (redundant).
Signed-off-by: Nam Dang <xuannam230201@gmail.com>
| // Build value_to_metric metrics path for this level | ||
| valueToMetricFullKey.WriteString(".") | ||
| if nextDescriptor != nil { | ||
| // Check if share_threshold is enabled for this entry |
There was a problem hiding this comment.
Refactor the logic to include value in stats. It's not necessary to repeat:
valueToMetricFullKey.WriteString(entry.Key)
valueToMetricFullKey.WriteString("_")
valueToMetricFullKey.WriteString(...)
several times in this logic
Signed-off-by: Nam Dang <xuannam230201@gmail.com>
a4a9d77 to
e06479e
Compare
[ISSUE]: Wildcard Stats Key Behavior Changes
Problem
Previously, when
value_to_metriclogic was added (in my another PR), the wildcard stats key behavior was accidentally changed. Wildcard patterns withoutvalue_to_metricorshare_thresholdwere generating stats keys without the wildcard value pattern (e.g.,test-domain.wildinstead oftest-domain.wild_foo*). This broke backward compatibility for wildcard configurations.Additionally, when share_threshold was implemented, stats keys for wildcard patterns with share_threshold were also not including the wildcard pattern with * (e.g., test-domain.files instead of test-domain.files_files/*), which was inconsistent with the expected behavior for wildcard stats.
Note: The lack of unit tests for wildcard stats keys made it difficult to detect this behavioral change. Adding comprehensive unit tests can help avoid such issues in the future.
Solution
The code now preserves the original wildcard stats behavior while properly handling
share_thresholdandvalue_to_metricflags according to the following rules:value_to_metricORdetailed_metric: Stats key includes wildcard pattern with*(e.g.,test-domain.wild_foo*)value_to_metricORdetailed_metric: Stats key includes actual runtime value (e.g.,test-domain.wild_foo1)share_threshold: true: Stats key always includes wildcard pattern with*, regardless of other flags (e.g.,test-domain.wild_foo*)This change ensures that:
Test Coverage
Added & Updated comprehensive unit tests to prevent future regressions.