Platform api changes to support consumer based rl and cost based rl#1818
Platform api changes to support consumer based rl and cost based rl#1818
Conversation
📝 WalkthroughSummaryThis PR extends the platform API to support consumer-based and cost-based rate limiting for LLM deployments. Key Functional ChangesConsumer-Level Rate Limiting
Cost-Based Rate Limiting
Policy Management Improvements
Test CoverageAdds comprehensive unit test coverage including:
WalkthroughThe LLM deployment service extends its policy generation logic to support cost-based budgeting and enhanced consumer-scoped rate limiting. Provider-level cost configurations now generate 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.11.4)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain modules listed in go.work or their selected dependencies" Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
platform-api/src/internal/service/llm_deployment.go (1)
1172-1187:⚠️ Potential issue | 🟠 Major | ⚡ Quick winScope matching still misses consumer request policies in resource-wise merges.
On Line 1172 and Line 1178, scope detection uses only
consumerBased. Consumer request-limit paths don’t set that flag, so backend and consumer request policies for the same path can be treated as duplicates and one can be dropped at Line 1184.Suggested patch
+func isConsumerScopedPath(path api.LLMPolicyPath) bool { + if v, ok := path.Params["consumerBased"].(bool); ok { + return v + } + if quotas, ok := path.Params["quotas"].([]map[string]interface{}); ok { + for _, q := range quotas { + if name, _ := q["name"].(string); name == "consumer-request-limit" { + return true + } + } + } + return false +} + func addOrAppendPolicyPath(policies *[]api.LLMPolicy, name, version string, path api.LLMPolicyPath) { - newConsumerBased, _ := path.Params["consumerBased"].(bool) + newConsumerBased := isConsumerScopedPath(path) for i := range *policies { if (*policies)[i].Name == name && (*policies)[i].Version == version { // Only merge entries that share the same scope (backend vs consumer) if len((*policies)[i].Paths) > 0 { - existingConsumerBased, _ := (*policies)[i].Paths[0].Params["consumerBased"].(bool) + existingConsumerBased := isConsumerScopedPath((*policies)[i].Paths[0]) if existingConsumerBased != newConsumerBased { continue // different scope — skip, look for another entry } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@platform-api/src/internal/service/llm_deployment.go` around lines 1172 - 1187, The scope comparison currently only compares boolean values (newConsumerBased vs existingConsumerBased) which treats a missing consumerBased key as equal to false and causes consumer request policies without that flag to be merged/dropped; change the check in the loop over (*policies)[i].Paths to compare both the presence of the consumerBased key and its boolean value (i.e., inspect whether Params contains "consumerBased" for new and existing paths as well as their bool values) and treat a mismatch in presence as a scope difference so the code continues instead of merging; update the logic around newConsumerBased, existingConsumerBased and path.Params lookups to use the existence flag plus value when deciding to continue.
🧹 Nitpick comments (2)
platform-api/src/internal/service/llm_deployment_test.go (2)
49-86: ⚡ Quick winAdd an assertion for
fallback: defaultin consumer request tests.The consumer request test validates
x-wso2-application-idbut not the fallback field, so this requirement can regress silently.Suggested patch
if !strings.Contains(yaml, "x-wso2-application-id") { t.Error("expected x-wso2-application-id in key extraction for consumer request limit") } +if !strings.Contains(yaml, "fallback: default") { + t.Error("expected fallback: default in key extraction for consumer request limit") +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@platform-api/src/internal/service/llm_deployment_test.go` around lines 49 - 86, Add an assertion in TestGenerateYAML_ConsumerRequestLimit to ensure the generated YAML includes the consumer key-extraction fallback value by checking that the string "fallback: default" appears (alongside the existing check for "x-wso2-application-id"); modify the test around generateLLMProviderDeploymentYAML/providerWithConsumerLimits to assert the presence of "fallback: default" so this requirement cannot regress.
345-373: ⚡ Quick winAdd a resource-wise same-path backend+consumer request regression test.
Current combined request coverage is global-only. A resource-wise case with the same path for backend and consumer request limits is needed to validate non-merging behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@platform-api/src/internal/service/llm_deployment.go`:
- Around line 934-937: The metadata extractor entries inside the "keyExtraction"
maps for consumer request key generation are missing the fallback field; update
the metadata extractor objects that currently read {"type": "metadata", "key":
"x-wso2-application-id"} to include the fallback property (e.g., "fallback":
"default") so the generated policy matches the intended consumer request-limit
shape—apply this change to both occurrences where "keyExtraction" contains the
routename and metadata extractor entries.
---
Outside diff comments:
In `@platform-api/src/internal/service/llm_deployment.go`:
- Around line 1172-1187: The scope comparison currently only compares boolean
values (newConsumerBased vs existingConsumerBased) which treats a missing
consumerBased key as equal to false and causes consumer request policies without
that flag to be merged/dropped; change the check in the loop over
(*policies)[i].Paths to compare both the presence of the consumerBased key and
its boolean value (i.e., inspect whether Params contains "consumerBased" for new
and existing paths as well as their bool values) and treat a mismatch in
presence as a scope difference so the code continues instead of merging; update
the logic around newConsumerBased, existingConsumerBased and path.Params lookups
to use the existence flag plus value when deciding to continue.
---
Nitpick comments:
In `@platform-api/src/internal/service/llm_deployment_test.go`:
- Around line 49-86: Add an assertion in TestGenerateYAML_ConsumerRequestLimit
to ensure the generated YAML includes the consumer key-extraction fallback value
by checking that the string "fallback: default" appears (alongside the existing
check for "x-wso2-application-id"); modify the test around
generateLLMProviderDeploymentYAML/providerWithConsumerLimits to assert the
presence of "fallback: default" so this requirement cannot regress.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 95c03838-c07a-44e2-bd82-cb3d2131015c
📒 Files selected for processing (2)
platform-api/src/internal/service/llm_deployment.goplatform-api/src/internal/service/llm_deployment_test.go
| "keyExtraction": []map[string]interface{}{ | ||
| {"type": "routename"}, | ||
| {"type": "metadata", "key": "x-wso2-application-id"}, | ||
| }, |
There was a problem hiding this comment.
Add the missing metadata fallback in consumer request key extraction.
On Line 936 and Line 1024, the metadata extractor omits fallback: default, so the generated policy does not match the intended consumer request-limit shape.
Suggested patch
"keyExtraction": []map[string]interface{}{
{"type": "routename"},
- {"type": "metadata", "key": "x-wso2-application-id"},
+ {"type": "metadata", "key": "x-wso2-application-id", "fallback": "default"},
},Also applies to: 1022-1025
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@platform-api/src/internal/service/llm_deployment.go` around lines 934 - 937,
The metadata extractor entries inside the "keyExtraction" maps for consumer
request key generation are missing the fallback field; update the metadata
extractor objects that currently read {"type": "metadata", "key":
"x-wso2-application-id"} to include the fallback property (e.g., "fallback":
"default") so the generated policy matches the intended consumer request-limit
shape—apply this change to both occurrences where "keyExtraction" contains the
routename and metadata extractor entries.
3 participants
Purpose
advanced-ratelimitwithx-wso2-application-idin key extraction (fallback: default) and quota nameconsumer-request-limittoken-based-ratelimitwithconsumerBased: truellm-cost-based-ratelimitwithconsumerBased: true;hasPolicyguard preventsllm-costappearing twice when both backend and consumer cost limits are configuredaddOrAppendPolicyPathto be scope-aware so backend and consumer entries with the same policy name are not merged