chore(metrics): use geth metrics package and delete local metrics #745
+314
−5,856
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
2 times, most recently
from
2164d72 to
caf4d61
Compare
qdm12
commented
Jan 13, 2025
qdm12
force-pushed
the
qdm12/libevm/metrics
branch
3 times, most recently
from
d7b1c12 to
3e3d44c
Compare
|
some review suggestions: #747 |
darioush
previously approved these changes
Jan 14, 2025
darioush
previously approved these changes
Jan 15, 2025
ceyonur
reviewed
Jan 22, 2025
qdm12
force-pushed
the
qdm12/libevm/metrics
branch
from
0de8a89 to
00f7ec7
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
Signed-off-by: Quentin McGaw <[email protected]>
Signed-off-by: Quentin McGaw <[email protected]>
qdm12
force-pushed
the
qdm12/libevm/metrics
branch
from
00f7ec7 to
97bdf9e
Compare
darioush
previously approved these changes
Jan 23, 2025
|
I think you may have to fix the regex on the linter. |
qdm12
force-pushed
the
qdm12/libevm/metrics
branch
3 times, most recently
from
1354b31 to
bbce8d0
Compare
qdm12
force-pushed
the
qdm12/libevm/metrics
branch
from
bbce8d0 to
5e17fb5
Compare
darioush
approved these changes
Jan 24, 2025
qdm12
changed the title
maru-ava
approved these changes
Jan 27, 2025
qdm12
commented
Jan 28, 2025
| // If the [metrics.Enabled] is already true, nothing is done. | ||
| // Otherwise, it is set to true and is reverted to false when the test finishes. | ||
| func WithMetrics(t *testing.T) { | ||
| if metrics.Enabled { |
There was a problem hiding this comment.
Waiting to see if metrics were shown before on e2e tests or not. Ideally we want to keep the same behavior as before, even if we want to change it, it should be in another PR.
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 🙏