Closed
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughPolicy version bumps and addition of Changes
Sequence DiagramsequenceDiagram
participant Consumer as Consumer
participant Gateway as Gateway
participant Auth as api-key-auth
participant RateLimit as RateLimiter
participant Backend as SharedBackend
Consumer->>Gateway: Request (may include API Key)
activate Gateway
Gateway->>Auth: Validate/extract API key -> x-wso2-application-id
activate Auth
Auth-->>Gateway: app-id or none
deactivate Auth
Gateway->>RateLimit: Evaluate policy (consumerBased true/false, limits)
activate RateLimit
alt consumerBased == true
RateLimit->>RateLimit: Lookup counter for app-id (or "default")
RateLimit->>RateLimit: Increment & compare vs consumer limit
alt within consumer limit
RateLimit-->>Gateway: Allow (200)
else
RateLimit-->>Gateway: Reject (429)
end
else consumerBased == false
RateLimit->>Backend: Increment/check shared backend counter
Backend->>Backend: Compare vs backend limit
alt within backend limit
Backend-->>RateLimit: Allow
RateLimit-->>Gateway: Allow (200)
else
Backend-->>RateLimit: Reject
RateLimit-->>Gateway: Reject (429)
end
end
deactivate RateLimit
Gateway-->>Consumer: Response (200 or 429)
deactivate Gateway
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
platform-api/src/internal/service/llm_deployment_test.go (1)
22-22: Unused helper function.
float32Ptris defined but not used in any of the tests. Consider removing it to avoid dead code.🧹 Proposed fix
-func float32Ptr(f float32) *float32 { return &f } -🤖 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` at line 22, The helper function float32Ptr is unused and should be removed to eliminate dead code: delete the float32Ptr(f float32) *float32 { return &f } declaration from the test file (or, if it was intended to be used, replace its callers to use it and ensure tests import it); ensure no references to float32Ptr remain in the test package before committing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@platform-api/src/internal/service/llm_deployment_test.go`:
- Line 22: The helper function float32Ptr is unused and should be removed to
eliminate dead code: delete the float32Ptr(f float32) *float32 { return &f }
declaration from the test file (or, if it was intended to be used, replace its
callers to use it and ensure tests import it); ensure no references to
float32Ptr remain in the test package before committing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9929c116-db65-417d-a5dc-dcf3fdafdc9a
📒 Files selected for processing (10)
gateway/gateway-controller/default-policies/advanced-ratelimit.yamlgateway/gateway-controller/default-policies/api-key-auth.yamlgateway/gateway-controller/default-policies/llm-cost-based-ratelimit.yamlgateway/gateway-controller/default-policies/token-based-ratelimit.yamlgateway/it/features/consumer-cost-based-ratelimit.featuregateway/it/features/consumer-request-based-ratelimit.featuregateway/it/features/consumer-token-based-ratelimit.featuregateway/it/suite_test.goplatform-api/src/internal/service/llm_deployment.goplatform-api/src/internal/service/llm_deployment_test.go
Contributor
Author
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Purpose
All LLM gateway rate limits are currently applied at the provider level with a single shared counter. If App A exhausts the token or cost budget, App B gets blocked too. This PR adds consumer-based rate limiting so each GenAI application has its own independent counter.
Goals
x-wso2-application-id)"default"counter rather than colliding with the backend countergateway/gateway-controller/default-policies/with upstreamApproach
platform-api/src/internal/service/llm_deployment.goadvanced-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 mergedgateway/gateway-controller/default-policies/token-based-ratelimit.yaml,llm-cost-based-ratelimit.yaml,advanced-ratelimit.yaml, andapi-key-auth.yamlto match upstream policy definitionsgateway/it/consumer-token-based-ratelimit.feature,consumer-request-based-ratelimit.feature,consumer-cost-based-ratelimit.feature(registered insuite_test.go)Automation tests
Unit tests —
llm_deployment_test.go(11 tests)Covers
generateLLMProviderDeploymentYAMLfor: backend-only limits, consumer-only limits, both limits together, all three consumer limit types combined, and disabled limits being skipped.Integration tests — 9 scenarios across 3 feature files
consumer-token-based-ratelimitdefaultcounter when no app IDconsumer-request-based-ratelimitconsumer-cost-based-ratelimitRelated PRs
Feat/consumer-based-rl— policy-level changes (api-key-auth,token-based-ratelimit,llm-cost-based-ratelimit,advanced-ratelimit)fix/consumer-ratelimit-ui— removes the "Coming Soon" chip from the Per Consumer card in the UIFixes: #2307
Summary by CodeRabbit
New Features
Updates
Tests