imp(rpc) Add support for EVM RPC metrics #1303
Conversation
|
After digging deep into go-ethereum code and investigating the different alternatives, here are the different approaches to support metrics on the EVM rpc: Approach 1:go-ethereum already has support for metrics including setting up a separate metrics server and exposing metrics. To use that, ethermint can call: Pros: Cons: Approach 2:Create an http middleware that gets called after the request has been processed by go-ethereum which needs to:
Pros: Cons: Approach 3:Copy the implementation of ServeHTTP from go-ethereum to ethermint which includes the metrics that are only relevant for the RPC specified here Pros: Cons: What do you think? |
fedekunze
changed the title
v-homsi
force-pushed
the
feature/expose_evm_http_server_metrics
branch
from
47b3303
to
77dae19
Compare
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #1303 +/- ##
==========================================
+ Coverage 54.78% 55.65% +0.86%
==========================================
Files 108 124 +16
Lines 10015 11342 +1327
==========================================
+ Hits 5487 6312 +825
- Misses 4255 4705 +450
- Partials 273 325 +52
|
v-homsi
requested review from
0a1c and
ramacarlucho
and removed request for
a team
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Found 48 potential problems in the proposed changes. Check the Files changed tab for more details.
v-homsi
changed the title
There was a problem hiding this comment.
@v-homsi thanks! Let's add the new files into client/, server/, and the other existing folders. We also need to add a new API breaking change entry in the Changelog.
We should address the linter warnings and gosec errors as well
@v-homsi |
go-ethereum expects the Another drawback of this approach is that we would be exposing metrics that are not relevant and would never change (at the mercy of what go-ethereum exposes). |
There was a problem hiding this comment.
Found 31 potential problems in the proposed changes. Check the Files changed tab for more details.
v-homsi
force-pushed
the
feature/expose_evm_http_server_metrics
branch
from
2e46694
to
d757936
Compare
There was a problem hiding this comment.
Found 31 potential problems in the proposed changes. Check the Files changed tab for more details.
v-homsi
force-pushed
the
feature/expose_evm_http_server_metrics
branch
from
d757936
to
3d6dc8d
Compare
There was a problem hiding this comment.
Found 31 potential problems in the proposed changes. Check the Files changed tab for more details.
rpc/websocket.go
Outdated
| return nil, err | ||
| } | ||
| } | ||
| conn, resp, err := dialer.DialContext(ctx, dialURL, header) //nolint:bodyclose // not fixed as imported from go-ethereum |
There was a problem hiding this comment.
Linter complained about this line that conn needs to be closed like:
defer conn.Close()
However, as it's a websocket connection, I think adding the defer conn.Close() might close the connection while websocket is still open. What do you think?
v-homsi
changed the title
The folder I migrated the files to ( rpc ) already exists and I think it might be better to separate by feature rather than functionality (All RPC stuff goes under rpc folder). What do you think? |
| msg := &rpcCodec.JsonrpcMessage{Version: types.Vsn, ID: c.nextID(), Method: method} | ||
| if paramsIn != nil { // prevent sending "params":null | ||
| var err error | ||
| if msg.Params, err = json.Marshal(paramsIn); err != nil { |
Check warning
Code scanning / Semgrep
Should `$X` be modified when an error could be returned?
There was a problem hiding this comment.
gosec found more than 10 potential problems in the proposed changes. Check the Files changed tab for more details.
v-homsi
force-pushed
the
feature/expose_evm_http_server_metrics
branch
3 times, most recently
from
073bf0e
to
3aa8d61
Compare
- Import http and handlers from go-ethereum RPC to ethermint RPC - Support enabling/disabling RPC metrics and metric server
- Fix linter issues - Add entry to CHANGELOG - Update gomod2nix.toml
- Refactor rpc code into different packages - Update test timeout in github action
v-homsi
force-pushed
the
feature/expose_evm_http_server_metrics
branch
2 times, most recently
from
80d6aba
to
d1a820b
Compare
v-homsi
force-pushed
the
feature/expose_evm_http_server_metrics
branch
from
d1a820b
to
3491e5a
Compare
|
Converting this PR to draft PR again. I came across https://github.com/ledgerwatch/erigon and I would like to investigate potentially replacing |
| rpcServer.WithMetrics(registry) | ||
| metricsErrChan := setupMetricsServer(ctx, config, registry) | ||
|
|
||
| select { |
There was a problem hiding this comment.
A comment here explaining what you are waiting for would be helpful
| @@ -43,14 +41,23 @@ const ( | |||
| apiVersion = "1.0" | |||
| ) | |||
|
|
|||
| // API describes the set of methods offered over the RPC interface | |||
| type API struct { | |||
There was a problem hiding this comment.
I think I would rather this API struct to be an extension of the go-ethereum one given that making sure that we stay align with their API is key for different integrations.
| EthPendingBlockNumber = BlockNumber(-2) | ||
| EthLatestBlockNumber = BlockNumber(-1) | ||
| EthEarliestBlockNumber = BlockNumber(0) | ||
| EthSafeBlockNumber = BlockNumber(-4) |
There was a problem hiding this comment.
This is not relevant to our chain as we have instant finality. So Safe Finalized and Latest should be BlockNumber(-1).
| default: | ||
| // check if the input is a block hash | ||
| if len(input) == 66 { |
There was a problem hiding this comment.
Why were the comments deleted?
|
|
||
| func (bnh *BlockNumberOrHash) String() string { | ||
| if bnh.BlockNumber != nil { | ||
| return strconv.Itoa(int(*bnh.BlockNumber)) |
There was a problem hiding this comment.
I know it might be redundant as it is check in the decoding but it might be better to play it safe here and check that blockNumber does not overflow the integer type
the |
Thanks for taking a look! The problem with this is that there's no guarantee that setting Interestingly, this metric works fine because it gets registered after the I created a PR in my fork with the change you mentioned. Feel free to try it out yourself. |
|
After discussions with @facs95 regarding this PR, he suggested to go with approach 1 and try to upstream changes to go-ethereum suitable for our use cases instead of migrating the entire RPC code base. I created this PR for it |
|
Replaced by - #1378 |
Closes: #XXX
Description
This PR exposes EVM RPC metrics similar to the ones exposed by
go-ethereum.The metrics correspond to:
This PR creates a separate metrics server to serve the metrics for RPC and will be used to expose other EVM/ application metrics in the future.
Based on this discussion, three approaches are identified to exposing the metrics:
go-ethereumby passing a hardcoded flag in the CLI--metrics. This option will expose metrics that are not relevant to Ethermint and does not allow for a config file option.Sample metrics:
For contributor use:
docs/) or specification (x/<module>/spec/)godoccomments.Unreleasedsection inCHANGELOG.mdFiles changedin the Github PR explorerFor admin use:
WIP,R4R,docs, etc)