Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore(metrics/prometheus): add files needed for coreth and subnet-evm #103

Closed
wants to merge 1 commit into from

Conversation

qdm12
Copy link
Collaborator

@qdm12 qdm12 commented Jan 13, 2025

Why this should be merged

So that coreth and subnet-evm can use the metrics package from libevm instead of defining/depending on their own local metrics package.

➡️ coreth PR using this branch
➡️ subnet-evm PR using this branch

Also fixes and refactoring.

How this works

  • Add metrics/prometheus/prometheus.libevm.go containing extra (non conflicting) code needed for coreth and subnet-evm
    • Code based on https://github.com/ava-labs/coreth/blob/b6b4dfbc4bfc7f17e4cfe5f3c0fb44944176c884/metrics/prometheus/prometheus.go
    • Function Gatherer renamed to NewGatherer to export gatherer and return a concrete *Gatherer type ("accept interfaces, return concrete types")
    • Fix slice out of range panic in metrics.ResettingTimer switch case
    • Change behavior to return an error if metric type is not supported, instead of ignoring it
    • Define local narrower Registry interface in metrics/prometheus/interfaces.libevm.go
    • Rework switch for metric families to be simpler, notably using ptrTo generic function

How this was tested

CI passing here, in coreth's PR and subnet-evm's PR

- replace `Gatherer` function with `NewGatherer` to export `gatherer` and return a concrete `*Gatherer` type
- fix slice out of range bug in metrics.ResettingTimer case
- change behavior to return an error if metric type is not supported
- define local narrower registry interface
- rework switch for metric families to be simpler
@ARR4N
Copy link
Collaborator

ARR4N commented Jan 13, 2025

Why does this need to be in libevm at all? It doesn't use anything from the existing metrics/prometheus package (i.e. it still compiles if moving all of the new files to a different directory). coreth- / subnet-evm-specific functionality must not leak into libevm.

I see that it's still in draft so I may have checked too early, but I also want to save you doing unnecessary work.

qdm12 added a commit to ava-labs/subnet-evm that referenced this pull request Jan 13, 2025
- Bring over refactoring and fixes done in ava-labs/libevm#103
- Bring over test refactoring done in ava-labs/libevm#103
qdm12 added a commit to ava-labs/coreth that referenced this pull request Jan 13, 2025
- Bring over refactoring and fixes done in ava-labs/libevm#103
- Bring over test refactoring done in ava-labs/libevm#103
qdm12 added a commit to ava-labs/subnet-evm that referenced this pull request Jan 13, 2025
- Bring over refactoring and fixes done in ava-labs/libevm#103
- Bring over test refactoring done in ava-labs/libevm#103
qdm12 added a commit to ava-labs/coreth that referenced this pull request Jan 13, 2025
- Bring over refactoring and fixes done in ava-labs/libevm#103
- Bring over test refactoring done in ava-labs/libevm#103
qdm12 added a commit to ava-labs/coreth that referenced this pull request Jan 13, 2025
- Bring over refactoring and fixes done in ava-labs/libevm#103
- Bring over test refactoring done in ava-labs/libevm#103
@qdm12
Copy link
Collaborator Author

qdm12 commented Jan 13, 2025

Closed since not needed - coreth and subnet-evm do import libevm, but define their own metrics/prometheus files for their own custom code (even if it's the same for now)

@qdm12 qdm12 closed this Jan 13, 2025
@qdm12 qdm12 deleted the qdm12/metrics branch January 13, 2025 16:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants