chore(metrics): use libevm metrics package and delete local metrics #1422
Conversation
6cb3c96 to
b63a56c
Compare
- Bring over refactoring and fixes done in ava-labs/libevm#103 - Bring over test refactoring done in ava-labs/libevm#103
34e9afd to
8c8514c
Compare
| @@ -32,8 +33,7 @@ import ( | |||
| "github.com/ava-labs/subnet-evm/core/types" | |||
| "github.com/ava-labs/subnet-evm/eth" | |||
| "github.com/ava-labs/subnet-evm/eth/ethconfig" | |||
| "github.com/ava-labs/subnet-evm/metrics" | |||
| subnetEVMPrometheus "github.com/ava-labs/subnet-evm/metrics/prometheus" | |||
| subnetevmprometheus "github.com/ava-labs/subnet-evm/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!)
| @@ -555,7 +555,7 @@ func (vm *VM) initializeMetrics() error { | |||
| return nil | |||
| } | |||
|
|
|||
| gatherer := subnetEVMPrometheus.Gatherer(metrics.DefaultRegistry) | |||
| gatherer := subnetevmprometheus.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
| metricsEnabled := metrics.Enabled | ||
| if !metricsEnabled { | ||
| metrics.Enabled = true | ||
| t.Cleanup(func() { | ||
| metrics.Enabled = false | ||
| }) | ||
| } |
There was a problem hiding this comment.
This might not be needed given the init() block setting the metrics to Enabled, but I preferred to keep it here if we one day revert the default Metrics Enabled to false (as it is in geth)
There was a problem hiding this comment.
Could we put this code in a different location such as plugin/evm/metricstest
then have metricstest.WithMetrics(t) for example?
I'm assuming this code was added only to tests that were failing when metrics was disabled?
There was a problem hiding this comment.
Done in plugin/evm/testutils/metrics.go with a function WithMetrics(t). I went a bit crazy on the use-testutils-only-for-tests-outside-plugin/evm (inspired by your import test + arran's check for test files), since I think it would be good to only have a testutils to reduce changes in geth code, not for our own code (prefer copy pasting and defining local code for readability + reduce package dependencies)
metrics/prometheus/prometheus.go
Outdated
| 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.
| ps := snapshot.Percentiles(pvShortPercent) | ||
| qs := make([]*dto.Quantile, len(pv)) | ||
| for i := range pvShort { | ||
| v := pv[i] | ||
| s := ps[i] | ||
| qs[i] = &dto.Quantile{ | ||
| Quantile: &v, | ||
| Value: &s, | ||
| } |
There was a problem hiding this comment.
Note this was some really sketchy copy and paste code; the test is refactored to cover those lines which were not previously covered.
Signed-off-by: Quentin McGaw <quentin.mcgaw@gmail.com>
Signed-off-by: Quentin McGaw <quentin.mcgaw@gmail.com>
| metricsEnabled := metrics.Enabled | ||
| if !metricsEnabled { | ||
| metrics.Enabled = true | ||
| t.Cleanup(func() { | ||
| metrics.Enabled = false | ||
| }) | ||
| } |
There was a problem hiding this comment.
Could we put this code in a different location such as plugin/evm/metricstest
then have metricstest.WithMetrics(t) for example?
I'm assuming this code was added only to tests that were failing when metrics was disabled?
- add exported comments - rename `g` to `gatherer` in test - Format copyright as it as ™️ - Remove TODO
7c08815 to
15866c8
Compare
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.gometrics/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 🙏