Add metric to count the number of rate-limited transactions #839
Conversation
m-Peter
requested review from
janezpodhostnik,
peterargue and
zhangchiqing
as code owners
|
""" WalkthroughA new Prometheus counter metric was added to track the total number of rate-limited transactions. The Collector interface and its implementations were updated to support this metric. The requester service now records rate-limited transactions using the new method without EOA-specific labeling. Changes
Assessment against linked issues
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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: 0
🧹 Nitpick comments (1)
metrics/collector.go (1)
80-83: Consider the cardinality implications of using EOA addresses as labels.The metric definition is correct, but using EOA addresses as labels could potentially create a high-cardinality metric if there are many unique addresses that get rate-limited. This might impact Prometheus performance and storage requirements.
Consider implementing a cardinality limit or monitoring the number of unique labels to ensure it stays within acceptable bounds.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
metrics/collector.go(6 hunks)metrics/nop.go(1 hunks)services/requester/requester.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Test
🔇 Additional comments (7)
metrics/collector.go (5)
99-99: LGTM: Metric properly registered.The new metric is correctly added to the metrics slice for registration.
115-115: LGTM: Interface extension is well-defined.The method signature is clear and follows the existing naming convention.
135-135: LGTM: Struct field addition is consistent.The field follows the same naming pattern as other metric fields in the struct.
158-158: LGTM: Constructor initialization is correct.The new metric field is properly initialized in the constructor.
233-237: LGTM: Method implementation is correct.The implementation correctly uses prometheus.Labels with the address parameter and increments the counter. The pattern matches other similar methods in the file.
metrics/nop.go (1)
27-27: LGTM: No-op implementation is correct.The empty implementation correctly satisfies the interface requirement for scenarios where metrics collection is disabled.
services/requester/requester.go (1)
222-222: LGTM: Improved metric granularity for rate limiting.The change from generic method-based rate limiting metrics to EOA-specific tracking provides better observability. Using
from.Hex()correctly identifies the rate-limited account.
metrics/collector.go
Outdated
| @@ -77,6 +77,11 @@ var requestRateLimitedCounters = prometheus.NewCounterVec(prometheus.CounterOpts | |||
| Help: "Total number of rate limits by JSON-RPC method", | |||
| }, []string{"method"}) | |||
|
|
|||
| var eoaRateLimitedTransactions = prometheus.NewCounterVec(prometheus.CounterOpts{ | |||
There was a problem hiding this comment.
I am using a CounterVec here, not sure if a GaugeVec would be preferable though.
There was a problem hiding this comment.
no, counter is the right type for this
metrics/collector.go
Outdated
| @@ -77,6 +77,11 @@ var requestRateLimitedCounters = prometheus.NewCounterVec(prometheus.CounterOpts | |||
| Help: "Total number of rate limits by JSON-RPC method", | |||
| }, []string{"method"}) | |||
|
|
|||
| var eoaRateLimitedTransactions = prometheus.NewCounterVec(prometheus.CounterOpts{ | |||
There was a problem hiding this comment.
no, counter is the right type for this
metrics/collector.go
Outdated
| @@ -221,6 +230,12 @@ func (c *DefaultCollector) RequestRateLimited(method string) { | |||
| ).Inc() | |||
| } | |||
|
|
|||
| func (c *DefaultCollector) EOARateLimited(address string) { | |||
| c.eoaRateLimitedTransactions. | |||
| With(prometheus.Labels{"address": address}). | |||
There was a problem hiding this comment.
let's not include the address as a label. I think it's fine to just have a regular counter.
as a best practice, don't include arbitrary fields as labels otherwise prometheus will a separate metric for each address. On a high volume server, this can create a huge number of metrics.
There was a problem hiding this comment.
Good point 👍 Updated in a74c713 .
I used a plain prometheus.Counter instead of a prometheus.CounterVec, since we don't need any labels.
m-Peter
changed the title
m-Peter
force-pushed
the
mpeter/add-eoa-tx-rate-limited-counter
branch
from
a74c713 to
fa4c0bd
Compare
m-Peter
force-pushed
the
mpeter/add-eoa-tx-rate-limited-counter
branch
from
fa4c0bd to
add299d
Compare
Closes: #838
Description
For contributor use:
masterbranchFiles changedin the Github PR explorerSummary by CodeRabbit
New Features
Bug Fixes
Chores