Skip to content

Conversation

@dancing-ui
Copy link

@dancing-ui dancing-ui commented Sep 14, 2025

@CLAassistant
Copy link

CLAassistant commented Sep 14, 2025

CLA assistant check
All committers have signed the CLA.

@LearningGp LearningGp added kind/feature Category issues or PRs related to feature request to-review PRs to review area/flow-control Issues or PRs related to flow control labels Oct 23, 2025
@LearningGp LearningGp requested a review from Copilot October 23, 2025 01:48
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds LLM (Large Language Model) Token Rate Limiting functionality to sentinel-golang, implementing two rate limiting strategies: Fixed Window and PETA (Predictive Estimated Token Allowance). The feature enables token-based rate limiting for LLM API calls with support for multiple token counting strategies and Redis-based distributed rate limiting.

Key Changes:

  • Implements Fixed Window and PETA token rate limiting strategies with Redis backend
  • Adds token encoding support (currently OpenAI) for estimating token usage
  • Provides adapters for Eino framework integration
  • Includes comprehensive example implementation with Gin middleware

Reviewed Changes

Copilot reviewed 69 out of 102 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
go.mod Updates dependencies to support token rate limiting (Redis, tiktoken, copier, etc.)
pkg/adapters/eino/wrapper.go Implements LLM wrapper for Eino framework with token rate limiting
pkg/adapters/eino/options.go Defines configuration options for Eino adapter
core/llm_token_ratelimit/*.go Core implementation of token rate limiting logic, rule management, and strategies
core/llm_token_ratelimit/script/*.lua Redis Lua scripts for atomic rate limiting operations
example/llm_token_ratelimit/* Complete example with Gin server and LLM client integration

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Copy link
Collaborator

@LearningGp LearningGp left a comment

Choose a reason for hiding this comment

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

Overall, the feature functionality is solid with no critical issues (as previously discussed). The rule loading mechanism still needs full alignment with Sentinel's existing datasource architecture. The proposed adjustments can be prioritized in future releases based on your schedule, but could you confirm your plan to proceed or defer them? This implementation is acceptable for the current preview/experimental release phase.

sc.AddRuleCheckSlot(isolation.DefaultSlot)
sc.AddRuleCheckSlot(hotspot.DefaultSlot)
sc.AddRuleCheckSlot(circuitbreaker.DefaultSlot)
sc.AddRuleCheckSlot(llmtokenratelimit.DefaultSlot)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I suggest placing the slots related to rate limiting before those related to circuit breaking.

Copy link
Author

Choose a reason for hiding this comment

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

Thank you for your reply. I will place the llmtokenratelimit component right after the flow component and before the circuitbreaker component.


type Config struct {
Enabled bool `json:"enabled" yaml:"enabled"`
Rules []*Rule `json:"rules" yaml:"rules"`
Copy link
Collaborator

Choose a reason for hiding this comment

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

In Sentinel's design, configurations and rules are typically decoupled. Configurations are usually defined through YAML configuration files, specifying general parameters for features, while rules are dynamically updated via the datasource module, fully defining the behavior of the functionalities.

Copy link
Author

Choose a reason for hiding this comment

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

Thank you for your reply. I will adjust this part of the content and remove the rules from the configuration file.

// limitations under the License.

// Package config provides llm token rate limit mechanism.
package llm_token_ratelimit
Copy link
Collaborator

Choose a reason for hiding this comment

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

The package documentation comment for llm_token_ratelimit needs to be added to doc.go.

Copy link
Author

Choose a reason for hiding this comment

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

Thank you for your reply. I will add the documentation description for the llm_token_ratelimit component.

values = []string{pattern}
}
for _, v := range values {
if util.RegexMatch(identifier.Value, key) && util.RegexMatch(pattern, v) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This logic seems to iterate through all headers, which could become a performance bottleneck. Considering actual requirements, key values likely don't require regex pattern matching. This would allow direct value retrieval from the map , completely avoiding the full iteration process.

Copy link
Author

Choose a reason for hiding this comment

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

Thank you for your reply, but iterating through all headers for regex matching is a necessary step.
Suppose users have configured some key-value pairs with regular expressions for request matching. When a request arrives, Sentinel does not know whether the request matches any rule. Therefore, it must retrieve all headers of the request and perform regex matching against all configured key-value pairs—this is an unavoidable process.

)

var (
ruleMap = make(map[string][]*Rule)
Copy link
Collaborator

Choose a reason for hiding this comment

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

The rule loading mechanism isn't fully adapted for Sentinel's datasource module - consider referencing existing rule implementations for compatibility.

Copy link
Author

@dancing-ui dancing-ui Oct 24, 2025

Choose a reason for hiding this comment

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

Thank you for your reply. I will supplement the ext/datasource/helper.go content for the datasource module.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/flow-control Issues or PRs related to flow control kind/feature Category issues or PRs related to feature request to-review PRs to review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants