Add metric to count iterations taken for eth_estimateGas
#725
Conversation
m-Peter
requested review from
janezpodhostnik,
peterargue and
zhangchiqing
as code owners
WalkthroughThe pull request introduces a new metric to track the number of iterations required during gas estimation for EVM transactions. This is implemented by adding a Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 (
|
m-Peter
force-pushed
the
mpeter/remove-relic-trace-download-metric
branch
from
9df2e9c to
bc88364
Compare
| defer func() { | ||
| e.collector.GasEstimationIterations(iterations) | ||
| }() | ||
|
|
There was a problem hiding this comment.
Maybe might be easier to read if you introduce a closure here:
dryRun := func(gasLimit ??) (??, error) {
tx.Gas = gasLimit
result, err := e.dryRunTx(tx, from, height, stateOverrides, nil)
iterations += 1
return result, err
}There was a problem hiding this comment.
That's neat 💯 💯 💯 Updated in c9cb612.
services/requester/requester.go
Outdated
| @@ -310,6 +310,11 @@ func (e *EVM) EstimateGas( | |||
| height uint64, | |||
| stateOverrides *ethTypes.StateOverride, | |||
| ) (uint64, error) { | |||
| iterations := 0 | |||
| defer func() { | |||
| e.collector.GasEstimationIterations(iterations) | |||
There was a problem hiding this comment.
Are there any concerns about reporting the iterations for the case where this function errors?
There was a problem hiding this comment.
Good point 👍 I guess we should not report iterations for calls/transactions that we know would error out / or revert. It will skew our metrics, as we know that we need 2+ iterations to estimate the gas, with the currently defined error ratio of 1.5%.
Updated in c9cb612#diff-dead54ca0eb128b19b453911c9bc13c64e01b2c50553a574820820564552e39bR350-R355
m-Peter
force-pushed
the
mpeter/eth-estimate-gas-track-iterations
branch
from
280fcd8 to
eec6a89
Compare
![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)
services/requester/requester.go (1)
335-335: Consider refactoring using a closure for better readability.The iteration counting is spread across multiple dryRun calls. Consider using a closure to encapsulate this logic:
func (e *EVM) EstimateGas(...) (uint64, error) { iterations := 0 defer func() { e.collector.GasEstimationIterations(iterations) }() + dryRun := func(gasLimit uint64) (*evmTypes.Result, error) { + tx.Gas = gasLimit + result, err := e.dryRunTx(tx, from, height, stateOverrides, nil) + iterations++ + return result, err + } + // ... rest of the code using dryRun instead of direct calls ... - result, err := e.dryRunTx(tx, from, height, stateOverrides, nil) - iterations += 1 + result, err := dryRun(passingGasLimit)Also applies to: 361-361, 392-392
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
metrics/collector.go(4 hunks)metrics/nop.go(1 hunks)services/requester/requester.go(4 hunks)
🔇 Additional comments (6)
metrics/nop.go (1)
24-24: LGTM!The implementation follows the established no-op collector pattern.
metrics/collector.go (4)
22-22: LGTM!The interface addition is properly placed with other metric collection methods.
39-39: LGTM!The field addition follows the established pattern for metric fields.
95-98: LGTM!The metric initialization is well-structured with a clear name and descriptive help text.
184-186: LGTM!The implementation correctly sets the gauge value.
services/requester/requester.go (1)
313-316: LGTM!The iteration tracking is well-implemented with proper initialization and deferred collection. The defer ensures iterations are reported even when errors occur.
Closes: #684
Description
For contributor use:
masterbranchFiles changedin the Github PR explorerSummary by CodeRabbit
New Features
Improvements