chore(metrics): use libevm metrics package and delete local metrics #745
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
qdm12
force-pushed
the
qdm12/libevm/metrics
branch
3 times, most recently
from
f8dadc2 to
dd1a167
Compare
qdm12
force-pushed
the
qdm12/libevm/metrics
branch
from
e6c2e5b to
2164d72
Compare
- Enable metrics for specific tests
- Bring over refactoring and fixes done in ava-labs/libevm#103 - Bring over test refactoring done in ava-labs/libevm#103
qdm12
force-pushed
the
qdm12/libevm/metrics
branch
from
2164d72 to
caf4d61
Compare
qdm12
commented
Jan 13, 2025
| @@ -34,8 +34,7 @@ import ( | |||
| "github.com/ava-labs/coreth/core/types" | |||
| "github.com/ava-labs/coreth/eth" | |||
| "github.com/ava-labs/coreth/eth/ethconfig" | |||
| "github.com/ava-labs/coreth/metrics" | |||
| corethPrometheus "github.com/ava-labs/coreth/metrics/prometheus" | |||
| corethprometheus "github.com/ava-labs/coreth/metrics/prometheus" | |||
There was a problem hiding this comment.
nit I took the liberty to have this all lowercase, I believe this is Go convention for package name aliases (correct me if I'm wrong!)
| @@ -636,7 +636,7 @@ func (vm *VM) initializeMetrics() error { | |||
| return nil | |||
| } | |||
|
|
|||
| gatherer := corethPrometheus.Gatherer(metrics.DefaultRegistry) | |||
| gatherer := corethprometheus.NewGatherer(metrics.DefaultRegistry) | |||
There was a problem hiding this comment.
nit I took the liberty (again) to rename Gatherer to NewGatherer in order to export gatherer as Gatherer and return a *Gatherer from NewGatherer to satisfy the "return concrete types" concept
metrics/prometheus/prometheus.go
Outdated
Comment on lines
194
to
195
| default: | ||
| return nil, fmt.Errorf("metric type is not supported: %T", metric) |
There was a problem hiding this comment.
I took the liberty to change this to return an error (instead of silently ignoring it) on an unsupported type to prevent in the future to define new types and forget to add the implementation in the switch here.
Signed-off-by: Quentin McGaw <[email protected]>
Signed-off-by: Quentin McGaw <[email protected]>
qdm12
force-pushed
the
qdm12/libevm/metrics
branch
from
2d6a89e to
624eafc
Compare
qdm12
force-pushed
the
qdm12/libevm/metrics
branch
from
624eafc to
d7b1c12
Compare
qdm12
force-pushed
the
qdm12/libevm/metrics
branch
from
d7b1c12 to
3e3d44c
Compare
|
some review suggestions: #747 |
darioush
previously approved these changes
Jan 14, 2025
darioush
approved these changes
Jan 15, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
ℹ️ pretty much copy pasta of ava-labs/subnet-evm#1422
Why this should be merged
Contribution to the libevm effort.
Note there are the same changes for both coreth and subnet-evm, but they should not be part of libevm since these are extra features specific to subnet-evm/coreth, even if they're the same for both.
How this works
Comparing the following:
mastervs corethmaster: subnet-evm is missing the extra file https://github.com/ava-labs/coreth/blob/b6b4dfbc4bfc7f17e4cfe5f3c0fb44944176c884/metrics/init_test.go (which is present in geth as well)mastervs gethv1.13.14: subnet-evm has many deletions, very few minor unneeded changes and the new non-conflicting filemetrics/prometheus/prometheus.goTherefore:
metricsare deleted EXCEPT:metrics/prometheus/prometheus.go+initblock added to set the metrics enabled by defaultmetrics/prometheus/prometheus_test.gometrics/prometheus/interfaces.go(new file added whilst refactoring)metrics.Enabledis set to true ininitializeMetricsHow this was tested
CI passing
Need to be documented?
No
Need to update RELEASES.md?
Not really, since it should not change anything 🙏