Add metric to count the rate-limited requests #763
Conversation
m-Peter
requested review from
janezpodhostnik,
peterargue and
zhangchiqing
as code owners
WalkthroughThe pull request refactors the rate limiting mechanism across several API components. The changes replace the old Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant API as API Endpoint
participant RL as RateLimiter
Client->>API: Send request
API->>RL: Apply(ctx)
alt Token available
RL-->>API: nil
API-->>Client: Response OK
else Token not available
RL-->>API: ErrRateLimit
API-->>Client: Error response
end
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (8)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (4)
⏰ Context from checks skipped due to timeout of 90000ms (1)
🔇 Additional comments (7)
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
api/api.go(21 hunks)api/debug.go(8 hunks)api/pull.go(8 hunks)api/rate_limiter.go(1 hunks)api/ratelimiter.go(0 hunks)bootstrap/bootstrap.go(4 hunks)metrics/collector.go(4 hunks)metrics/nop.go(1 hunks)
💤 Files with no reviewable changes (1)
- api/ratelimiter.go
🔇 Additional comments (9)
metrics/nop.go (1)
26-26: LGTM!The no-op implementation of
RequestRateLimitedcorrectly follows the established pattern.api/rate_limiter.go (1)
16-18: LGTM! Clean interface design.The
RateLimiterinterface is well-defined and focused on a single responsibility.metrics/collector.go (1)
24-24: LGTM! Well-structured metrics implementation.The rate limit metrics implementation follows Prometheus best practices:
- Counter vector uses appropriate labels
- Clear help text
- Follows metric naming conventions
Also applies to: 43-43, 110-113, 211-217
api/debug.go (1)
54-54: LGTM! Consistent rate limit implementation.The rate limiting has been properly integrated:
- All methods consistently check rate limits
- Error handling is appropriate
- Rate limit checks are placed at the start of each method
Also applies to: 66-66, 77-77, 87-89, 99-101, 116-118, 134-136, 257-259
api/pull.go (1)
137-137: LGTM! The rate limiting changes look good.The changes consistently replace the old rate limiting implementation with the new
RateLimitertype across all API methods. The implementation:
- Properly initializes the rate limiter in the struct and constructor
- Consistently applies rate limiting using
rateLimiter.Apply(ctx)before processing requests- Correctly handles error responses from rate limiting
Also applies to: 146-146, 155-155, 172-174, 200-202, 249-251, 293-295, 343-345
bootstrap/bootstrap.go (3)
274-279: LGTM! The rate limiter configuration is more readable.The multi-line configuration structure improves readability while maintaining the same functionality.
281-282: LGTM! The error message is more descriptive.The error message now specifically mentions "in-memory limiter store" which better describes the component that failed.
283-283: LGTM! The rate limiter is consistently used across API components.The new
RateLimiteris properly initialized and passed to all API components that require rate limiting.Also applies to: 299-299, 321-321, 333-333
api/api.go (1)
89-89: LGTM! The rate limiting implementation is consistent.The changes properly integrate the new
RateLimitertype across all API methods:
- Correctly initializes the rate limiter in the struct and constructor
- Consistently applies rate limiting using
rateLimiter.Apply(ctx)before processing requests- Properly handles error responses from rate limiting
Also applies to: 100-100, 112-112, 119-119, 178-180, 203-205, 230-232, 259-261, 294-296, 334-336, 368-370, 402-404, 439-441, 485-487, 509-511, 546-548, 594-596, 683-685, 715-717, 765-767, 881-883
m-Peter
force-pushed
the
mpeter/rate-limited-requests-metric
branch
from
33fd540 to
c4304ac
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
api/rate_limiter.go (1)
38-64:Applymethod logic
Well-structured approach to rate limiting, with clear debug logs and a call to increment the new metric. Consider verifying the reliability ofremoteif rate-limiting is critical for security.metrics/collector.go (1)
31-43: Add fieldrequestRateLimitedCounterstoDefaultCollector
The new counter vector is well-defined. Keep an eye on unbounded label cardinality ifmethodis untrusted.api/pull.go (1)
172-174: LGTM! Consistent rate limiter implementation across methods.The rate limiter is consistently applied across all API methods, with proper error handling and method name tracking.
Consider using constants for method names.
To prevent typos and ensure consistency, consider defining the method names as constants.
+const ( + MethodNewPendingTransactionFilter = "NewPendingTransactionFilter" + MethodNewBlockFilter = "NewBlockFilter" + MethodNewFilter = "NewFilter" + MethodGetFilterLogs = "GetFilterLogs" + MethodGetFilterChanges = "GetFilterChanges" +) func (api *PullAPI) NewPendingTransactionFilter(ctx context.Context, fullTx *bool) (rpc.ID, error) { - if err := api.rateLimiter.Apply(ctx, "NewPendingTransactionFilter"); err != nil { + if err := api.rateLimiter.Apply(ctx, MethodNewPendingTransactionFilter); err != nil {Also applies to: 200-202, 249-251, 293-295, 343-345
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
api/api.go(21 hunks)api/debug.go(8 hunks)api/pull.go(8 hunks)api/rate_limiter.go(1 hunks)api/ratelimiter.go(0 hunks)bootstrap/bootstrap.go(4 hunks)metrics/collector.go(4 hunks)metrics/nop.go(1 hunks)
💤 Files with no reviewable changes (1)
- api/ratelimiter.go
🚧 Files skipped from review as they are similar to previous changes (3)
- metrics/nop.go
- bootstrap/bootstrap.go
- api/debug.go
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Test
🔇 Additional comments (30)
api/api.go (20)
89-113: IntroducerateLimiterfield and constructor parameter
The addition of therateLimiterfield and its constructor parameter looks cleanly integrated into theBlockChainAPI. It aligns well with the new rate limiting strategy.
119-119: Enforce rate limit inBlockNumber
No issues here. Ensuring rate limits on this endpoint seems appropriate.
138-138: Enforce rate limit inSyncing
Properly applies rate limiting for the sync status check.
178-178: Enforce rate limit inSendRawTransaction
Good practice to rate-limit transaction submissions.
203-203: Enforce rate limit inGetBalance
Looks good. This prevents excessively frequent balance queries.
230-230: Enforce rate limit inGetTransactionByHash
No concerns.
259-259: Enforce rate limit inGetTransactionByBlockHashAndIndex
Consistent usage of rate limiting.
294-294: Enforce rate limit inGetTransactionByBlockNumberAndIndex
Implementation remains straightforward.
334-334: Enforce rate limit inGetTransactionReceipt
No issues noted.
368-368: Enforce rate limit inGetBlockByHash
Approach is consistent with other functions.
402-402: Enforce rate limit inGetBlockByNumber
Looks correct.
439-439: Enforce rate limit inGetBlockReceipts
No issues.
485-485: Enforce rate limit inGetBlockTransactionCountByHash
Implementation follows the pattern.
509-509: Enforce rate limit inGetBlockTransactionCountByNumber
No concerns here.
546-546: Enforce rate limit inCall
This is a sensible point to apply rate limiting.
594-594: Enforce rate limit inGetLogs
All good.
683-683: Enforce rate limit inGetTransactionCount
Implementation appears consistent.
716-716: Enforce rate limit inEstimateGas
No issues found.
765-765: Enforce rate limit inGetCode
Matches the pattern in other calls.
881-881: Enforce rate limit inGetStorageAt
Implementation looks straightforward.api/rate_limiter.go (3)
16-19: DefineRateLimiterinterface
The interface is concise and clearly expresses the contract for rate limiting.
20-24: DeclareDefaultRateLimiterstruct
Struct fields align well with the intended limiting behavior and metrics collection.
26-36:NewRateLimiterconstructor
Constructs the default rate limiter with the expected dependencies. No issues found.metrics/collector.go (5)
24-24: AddRequestRateLimitedtoCollector
This new method appropriately extends the interface for rate limit metrics.
110-113: InitializerequestRateLimitedCounters
The name and help text are descriptive, which is good for metric clarity.
128-128: RegisterrequestRateLimitedCounters
Ensures the new counter is properly registered.
136-148: UpdateDefaultCollectorconstructor
Initialization logic is consistent, and the code is readable.
211-218: ImplementRequestRateLimitedmethod
Incrementing the counter with the method label completes the metrics flow for rate-limiting events.api/pull.go (2)
137-137: LGTM! Field name follows Go conventions.The change from
ratelimiter limiter.StoretorateLimiter RateLimiterfollows Go naming conventions and suggests a more focused, custom implementation.
146-156: LGTM! Constructor changes are consistent.The constructor parameter and field initialization changes align with the struct field changes, maintaining a consistent order.
m-Peter
force-pushed
the
mpeter/rate-limited-requests-metric
branch
from
c4304ac to
8b90e95
Compare
Closes: #762
Description
Add a new type,
RateLimiter, which also encapsulates ametrics.Collectorobject.Add a metric to count the number of rate-limited requests.
For contributor use:
masterbranchFiles changedin the Github PR explorerSummary by CodeRabbit
New Features
Refactor
Chores