Skip to content

Conversation

qdm12
Copy link
Contributor

@qdm12 qdm12 commented Mar 21, 2025

Why this should be merged

Use libevm/metrics instead of local subnet-evm/metrics.

This is required to be able to continue work on libevm.

(Formerly #1422 which got merged by error, big time apologies for that, and I have no idea how it happened! 🤷 😢 )

How this works

⚠️ This is blocked by a libevm upgrade (in both avalanchego and here) to have the metrics package global variable Enabled = true in order to have global scope metrics registered as non-no-op-metrics, see ava-labs/libevm#165 libevm bumped with the Enabled global variable defaulting to true.

Comparing the following:

Therefore:

  • file kept and refactored metrics/prometheus/prometheus.go with the Gatherer implementation we use
  • file kept and refactored metrics/prometheus/prometheus_test.go
  • new file metrics/prometheus/interfaces.go added for refactoring
  • geth global variable metrics.Enabled is set to true in plugin/evm.VM.initializeMetrics

Note for the two files refactored (with fixes as well), this was done in some intermediary step in ava-labs/libevm#103, so I decided to bring this over here so it doesn't get trashed.

How this was tested

CI passing

Need to be documented?

No

Need to update RELEASES.md?

No

Base automatically changed from qdm12/libevm/master-merge-1 to libevm March 24, 2025 10:01
qdm12 and others added 26 commits March 28, 2025 11:37
- delete content of `metrics` package except `prometheus` package
- only keep subnet-evm specifics in `metrics/prometheus`
- Bring over refactoring and fixes done in ava-labs/libevm#103
- Bring over test refactoring done in ava-labs/libevm#103
- add exported comments
- rename `g` to `gatherer` in test
- Format copyright as it should be ™️
- Remove TODO
- remove non linkable prometheus.Registry
Co-authored-by: Arran Schlosberg <[email protected]>
Signed-off-by: Quentin McGaw <[email protected]>
ARR4N and others added 3 commits March 28, 2025 11:38
- Metrics enabled corrrectly
- Remove config argument from BackendConstructor since it's no longer needed
- Export newTimestampCompatError with wrapping function NewTimestampCompatErr
- rlpgen supports type aliases
- Better logging

See original PR ava-labs/coreth#889
@qdm12 qdm12 force-pushed the qdm12/libevm/metrics branch from 279bbe1 to a6dabd3 Compare March 28, 2025 10:39
@qdm12 qdm12 marked this pull request as ready for review March 28, 2025 10:41
@qdm12 qdm12 requested review from a team, ceyonur and darioush as code owners March 28, 2025 10:41
@qdm12 qdm12 changed the title chore(metrics): use geth metrics package and delete local metrics chore(metrics): use libevm metrics package Mar 28, 2025
qdm12 added 2 commits March 28, 2025 14:45
`geth` (and hence `libevm`) global metrics tend to be created with `NewRegistered*()`, which assumes that there are no name conflicts on the same `Registry`. If there are then the second call fails silently to register the new metric.

Except for in `warp` (where there won't be any conflicts) metric registration is changed globally to use `GetOrRegister*()` instead of `NewRegistered*()`. With GNU tools (not the Mac budget replacements):

```shell
find -iname '*.go' -not -iwholename '*/warp/*' | xargs sed -i 's|metrics.NewRegistered|metrics.GetOrRegister|g'
```

All modified files that don't already import their `libevm` counterparts now `_` import them to force the initialisation order. The `libevm` construction with `NewRegistered*()` therefore always comes before the respective `GetOrRegister*()` so both call sites have the same metric instance.

Metrics that we expect to be counters instead of timers are unregistered and newly created with the correct type, borrowing from ava-labs/libevm#159.

See original PR ava-labs/coreth#860
@qdm12 qdm12 merged commit 1366b3b into libevm Apr 1, 2025
11 checks passed
@qdm12 qdm12 deleted the qdm12/libevm/metrics branch April 1, 2025 08:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants